Create a phpstorage backend that works with a local disk but coordinates across multiple webheads

Do we expect that a problem would already occur so early that we wouldn't be able to initialize the container and execute the first event (REQUEST I think)?

If we can say that this is going to work in 99% of the cases, maybe it's enough to just have a request event listener that checks for some flag/state, if that doesn't match, force a container rebuild and reload the page or something like that?

We discussed this with msonnabaum (at least) at ... BADcamp? I think. Simple solution:

> thus, any Drupal event that should lead to a new container

That's a module enable / disable. You are code deploying for that. Deploy a container. Do not allow module enable/disable on multiple webheads. Add a feature to disable the module screen. Or write a module to do it. I have NFI how to do that, in D7 it was a hook_menu_alter with 1 line changing an access callback to FALSE. Or form_alter the submit button out :P

So I presume you are patching PhpStorageFactory::get to include a counter in the path.

Since this already uses Settings, may I have a settings override please for the method determining the counter? Thanks! BootstrapConfigStorageFactory::get is an excellent example of what I would like to see.

Re #6 , there's already a class override in the factory so we just need to keep that alive.

Edit: http://privatepaste.com/6526b897db

I think we are not running INSERT/UPDATE statements? I would put the relevant db_insert / db_update (or just a single db_merge ? ) in methods and then use a variable calling method $callable = $this->update; $callable() and provide ways from settings.php to override that callable so that for eg redis or similar can be used.

Needs work for chx's suggestion to use db_insert/db_update or db_merge

1. +++ b/core/lib/Drupal/Component/PhpStorage/CoordinatedMTimeProtectedFileStorage.php
   @@ -0,0 +1,134 @@
   + * Definition of Drupal\Component\PhpStorage\CoordinatedMTimeProtectedFileStorage.
   

   "Contains \..."

2. +++ b/core/lib/Drupal/Component/PhpStorage/CoordinatedMTimeProtectedFileStorage.php
   @@ -0,0 +1,134 @@
   +namespace Drupal\Component\PhpStorage;
   +
   +use Drupal\Core\Database\Database;
   

   Our current policy is that code in Drupal\Component must not depend on code of Drupal\Core, so this implementation needs to be moved into Drupal\Core\PhpStorage (which contains the Drupal-specific factory already)

3. +++ b/core/lib/Drupal/Component/PhpStorage/CoordinatedMTimeProtectedFileStorage.php
   @@ -0,0 +1,134 @@
   +  /**
   +   * A custom callable to use to fetch the counter.
   +   */
   +  protected $fetchCallable;
   +
   +  /**
   +   * A custom callable to use to increment the counter.
   +   */
   +  protected $incrementCallable;
   

   Not sure I can get behind these — wouldn't it be much more natural to simply subclass this implementation and override desired methods?

4. +++ b/core/lib/Drupal/Component/PhpStorage/CoordinatedMTimeProtectedFileStorage.php
   @@ -0,0 +1,134 @@
   +   * @param int
   +   *   The current counter to be used in the service container path.
   +   */
   +  protected function fetchCounter() {
   

   Non-existing @param.

5. +++ b/core/lib/Drupal/Component/PhpStorage/CoordinatedMTimeProtectedFileStorage.php
   @@ -0,0 +1,134 @@
   +    $sql = "SELECT value
   +            FROM {key_value}
   ...
   +        Database::getConnection()
   +          ->insert('key_value')
   ...
   +        Database::getConnection()
   +          ->update('key_value')
   

   This won't work in the early installer, in case the settings.php override has been set already, because the key_value table does not exist yet.

   We can either adjust the docs in settings.php to explicitly state that this implementation cannot be used for installing Drupal, or we'd have to catch some more database exceptions and silently default the counter to 0.

I've been thinking about a low-budget version of this, that's using a request listener or middleware-thingy to check something after we bootstrapped (and assume that we can get at least that far), then check something there and try rebuild there then.

I had a discussion today with David Strauss and we can't find a better way to do this in core -- of course if you have a nice queue system you can do more -- but using the files directory doesn't seem to make sense. files is supposed to be sync'd among webheads, right? So perhaps let's default this driver to the tmp directory -- quite probably not even overrideable and not the Drupal tmp directory because that's not yet available, but sys_get_temp_dir() which is certainly local.

Given that many sites will need to run on multiple webheads, I think this is at least major. I'm working on a reroll of #31 as a starting point for reviving discussion.

Heh, no reroll required. I haven't seen that on an 9-month old patch in some time :)

There were some tweaks needed however.

1. +++ b/core/lib/Drupal/Core/PhpStorage/CoordinatedMTimeProtectedFileStorage.php
   @@ -0,0 +1,132 @@
   +    if (!empty($configuration['fetch_callable']) && is_callable($configuration['fetch_callable'])) {
   +      $this->fetchCallable = $configuration['fetch_callable'];
   +    }
   

   Instead of individual callables, we want an interface and a default class, then just instantiate the class here and provide it with configuration, too.

2. +++ b/core/lib/Drupal/Core/PhpStorage/CoordinatedMTimeProtectedFileStorage.php
   @@ -0,0 +1,132 @@
   +    $sql = "SELECT value
   +            FROM {key_value}
   +            WHERE collection = 'coordinated_mtime_storage_counter'
   +            AND name = 'service_container_counter'";
   

   This should all be in the default implementation of the MySQLFileStorageCoordinator class.

   That also makes it way easier to re-use with a different backend (e.g. just FileStorage).

Here's an initial attempt at moving the custom callables to a proper interface (forgive the naming, it feels awkward at the moment).

Unsure about making it dependent on the database driver by default, I think just falling back to Database is enough - as we always have a database available at this point.

The rest however looks and feels really good.

What about having the interface define the semantics instead of the implementation:

->getRevision() / ->getInvalidationsCounter() / ->getUniqueCounter() / ->getChecksum()
->flushCache()

Thanks for your work on that!

@msonnabaum will be very happy :).

This should address the first part. For moving more to the interface, I'm not sure I follow the methods mentioned. I can see moving more logic from getFullPath(), and perhaps deleteAll(), but don't see an immediate mapping to the methods listed in #40

+++ b/core/lib/Drupal/Core/PhpStorage/Coordinator/DatabaseCoordinator.php
@@ -0,0 +1,52 @@
+      Database::getConnection()
+        ->update('key_value')
+        ->condition('collection', 'coordinated_mtime_storage_counter')
+        ->condition('name', 'service_container_counter')
+        ->fields(array('value' => 1))
+        ->execute();

I don't think this is actually incrementing anything.

Please do NOT rely on the database directly. There are trial implementations of D8 running on 100% MongoDB, and if that gets used in production it's likely that those will all be multi-head. Since we're reading/writing the k/v table already, why not just use the keyvalue service, or state service? Those seems like the appropriate tool (and could be backed by SQL, Redis, Mongo, or whatever the site chooses).

Because this is about checking if the container is still valid. Which has to happen *before* the container is loaded. No container, no key value service. Just like php storage also isn't using the container/services.

It is pluggable with it's own mechanism.

To be fair the K/V class could be used, just would need to inject the database directly.

As far as I know K/V does not depend on any further services we need to inject than the database - we got it running with D7.

In that case just need a EarlyDatabaseKeyValueStore instance and then could just use key/value directly.

The class would still be pluggable, but instead just overwrite __construct to statically inject the Database (or Mongo) or whatever is needed. (or have a ::create() method)

--

That would also get rid of the interface and the incrementCounter / getCounter methods :).

You'd need something like the bootstrap storage in DrupalKernel if you want with pluggability provided by settings.

I'm fine with the idea of a bootstrap storage service, and it defaulting to the db. If we do that, it should be independent of any existing services (kv, state, etc).

I dont think the "site running 100% on mongo" example is worthwhile to cater to, but I can see the use case for a custom configuration service that could handle this.

Here's an attempt to add a bootstrap storage service. This issue will need tests once the direction is stable.

Here's a version of #41, but with an incrementCounter method that actually increments the value.

This makes it clear that we actually need more than just a dumb k/v for a bootstrap storage interface, since we'll want the increment to be atomic. It may need to be an interface purely for this task, or maybe a counter service or something.

I'm not sure that there is a way to atomically increment that works on mysql and postgres, but the version I used I think may work, I didnt test it.

It also adds an increment during module install so I could test the basic functionality. Implementation is super ugly, but we dont seem to have any reasonable API for "something changed that requires a rebuild".

That's good standard SQL why it wouldn't? http://www.postgresql.org/docs/8.3/static/sql-update.html

An expression to assign to the column. The expression can use the old values of this and other columns in the table.

Same page has example queries like UPDATE weather SET temp_lo = temp_lo+1... UPDATE employees SET sales_count = sales_count + 1

Core has

core/lib/Drupal/Core/Menu/MenuTreeStorage.php:534:    $query->expression('depth', 'depth + :depth', array(':depth' => $shift));
core/modules/system/src/Tests/Database/UpdateComplexTest.php:101:      ->expression('age', 'age + :age', array(':age' => 4))
core/modules/system/src/Tests/Database/UpdateComplexTest.php:122:      ->expression('age', 'age + :age', array(':age' => 4))

so why are we not using this and a raw query instead...? Performance? But this is not on a performance critical path...?

I did not know about expression.

Related, but not dependent issue I started to provide a more reasonable interface for invalidation: #2493665: Add centralized container invalidation method

The approach of a 'coordinator' class, or a bootstrap storage service still hasn't been fully decided. The patch in #55 is the 'coordinator' approach, and the patch in #48 was an initial attempt at the new bootstrap service.

Since either approach is overridable by contrib that doesn't want to use the database, I don't have a preference one way or the other.

I'm not clear on the vision for the "bootstrap storage" version. Replacing the service at the level that we're using PhpStorage seems like the wrong place to me.

An interface with get/set isn't enough to express what's going on here. We need something with an increment method so that a backend can perform the increment atomically.

When I mentioned I was fine with the idea of a bootstrap service, I was thinking we'd have a service that maybe only handled the counter itself.

Please elaborate on the other approach if I got any of that wrong.

I think if we call it the AtomicCounter component, we are fine with the Coordinator approach, just before when it was just get/set I did not see why we should not use KeyValue directly.

If I were writing this (thanks god i am not) I would add an IncrementableKeyValueStore interface (we planned another such interface at #2161643: Add a KeyValueStore\IterableKeysInterface (allowing to retrieve keys with a given prefix) ) and just have the normal k-v class implement it.

After talking on IRC with msonnabaum and Fabianx, I'm currently pursuing the following:

• Add an AtomicCounterInterface. It shouldn't conceptually be tied to k/v since some backends such as Redis have dedicated operations for counters
• The core/default implementation will be by the existing k/v class

Using the DatabaseStorage k/v class seems a little messy:

    if (!isset($configuration['atomic_counter_class']) || !is_callable($configuration['atomic_counter_class'])) {
      $this->counter = new DatabaseStorage('coordinated_mtime_storage_counter', new PhpSerialize(), Database::getConnection());
    }

but aside from that, this seems to be working. Interdiff is from #55.

Didn't review yet, can we maybe use something like this also for a rebuid-lock to help with #2497243: Replace Symfony container with a Drupal one, stored in cache?

1. +++ b/core/lib/Drupal/Core/AtomicCounter/AtomicCounterInterface.php
   @@ -0,0 +1,34 @@
   +/**
   + * Defines the interface for atomic counter operations.
   + */
   

   Can we specify what an atomic counter is? I'm assuming it's unrelated to the resonant frequency of cesium...

2. +++ b/core/lib/Drupal/Core/KeyValueStore/DatabaseStorage.php
   @@ -169,4 +170,36 @@ public function deleteAll() {
   +    try {
   +      $this->connection
   +        ->insert($this->table)
   +        ->fields([
   +          'collection' => $this->collection,
   +            'name' => $name,
   +            'value' => 1,
   +        ])
   +        ->execute();
   +    } catch (\Exception $e) {
   +      $this->connection->update($this->table)
   +        ->expression('value', 'value + 1')
   +        ->condition('name', $name)
   +        ->execute();
   +    }
   

   Why can't this just be a merge query, which does the either/or logic for us? Doing the update in a catch block is very slow, because that's presumably the typical case. This would need to hit the DB, get a DB fault, create an exception (non-cheap), and then do a second DB connection. Totally wasteful.

   Or, better still:
   https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Database%...

   (Even if we can't use nextId directly, there was a ton of discussion around how to do that performantly that is relevant here.)

This switches the try/catch to use a merge instead of an update (and that is attempted first). We still need to catch the exception and use an insert for the initialization of the row in the db, since merge doesn't appear to work with expressions when a row isn't found. I added a very simple test as well.

One thing this patch doesn't account for is cleaning up the invalidated directories that will be left in place.

i'm interested in my merge() fails. jhedstrom - do you think that's a bug in the underlying db code? or is this a case of 'works as designed, don't do that'?

The failure is actually an SQL exception directly from MySQL, so I'd guess it is the underlying implementation, and also backend-specific.

Do folks have thoughts on how we can go about cleaning out the stranded container directories (and twig caches if it is used for that)?

This is the actual error:

SQLSTATE[HY000]: General error: 1364 Field 'value' doesn't have a default value: INSERT INTO {key_value} (collection, name) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1); Array ( [:db_insert_placeholder_0] => test_counter [:db_insert_placeholder_1] => test_atomic_counter ) i

Manually doing an insert with an expression on MySQL works fine:

MariaDB [d8]> insert into key_value (name, value, collection) values ('foo', value + 1, 'bar');
Query OK, 1 row affected (0.00 sec)

MariaDB [d8]> select * from key_value where name = 'foo' and collection = 'bar';
+------------+------+-------+
| collection | name | value |
+------------+------+-------+
| bar        | foo  | 1     |
+------------+------+-------+
1 row in set (0.00 sec)

MariaDB [d8]> update key_value set value = value + 1 where name = 'foo' and collection = 'bar';
Query OK, 1 row affected (0.00 sec)
Rows matched: 1  Changed: 1  Warnings: 0

MariaDB [d8]> select * from key_value where name = 'foo' and collection = 'bar';
+------------+------+-------+
| collection | name | value |
+------------+------+-------+
| bar        | foo  | 2     |
+------------+------+-------+
1 row in set (0.00 sec)

However, I don't think addressing the internal implementations of merge queries should block this issue.

That being said, here's the switch back to using update instead of merge.

This adds a unit test similar to the other phpstorage classes. Note I had to do the goofy test counter class rather than using a proper mock object for testing.

This test can be updated to ensure that leftover container directories are properly removed (eg, simulating invalidation across multiple webheads where only one directory is removed from a single webhead).

We also might want to change

+++ b/core/lib/Drupal/Core/PhpStorage/CoordinatedMTimeProtectedFileStorage.php
@@ -0,0 +1,82 @@
+    $counter = $this->counter->fetchCounter('service_container_counter');

the hard-coded service_container_counter so this can be used for other storage (eg, Twig templates).

Is everyone here aware of #2497243: Replace Symfony container with a Drupal one, stored in cache? AFAIK, the currently proposed solution there would solve that problem on it's own, because container would be stored in a synchronized storage, so this problem would simply no longer exist...

That issue definitely needs testing/feedback, so it would be great if people could review/comment/test the patches there...

Re: #75

I think this issue may still be necessary for other shared invalidations (I don't know enough about twig caching offhand to know if it is safe currently to store those locally on separate webheads w/o centralized invalidations).

Twig should just use a chained fast cache backend with fast == phpstorage 'twig' backend, then it gets cache tag and coordinated invalidation for free. We can also properly invalidate twig templates then (that still is very buggy sometimes ...) and ensure once we have them cached in the DB we just need to put them to disk on other web heads => faster startup times.

=> Overall win (And as I am author of the original twig implementation, this is what I would use _today_ if I were to integrate twig now.)

Edit: Hmm, maybe I would not store the PHP code in consistent backend (PHP Code in DB? What have I been thinking ...), but just using cache tags and invalidate on any write should work and probably the default implementation for PHP Storage Cache backend anyway ...

See #2497243-81: Replace Symfony container with a Drupal one, stored in cache for a writeup + code. I think that'd solve this neatly. Perhaps switch the K-V store to cache?

So, going 5.5 gives us a very small window to actually read the container from cache and yet run it through opcache. To see my code actually does get the container into opcache, add in CacheStorage::load just after the include but before the stream_wrapper_restore print_r(opcache_get_status(TRUE)['scripts']["phar://$name"]);. It is impossible to test this from CLI since opcache.enable_cli is not enabled and doesn't look runtime changeable either. But, the dump will show you it lives happily in cache and the hit counter grows on every reload. Huzzah.

Yes, the patch makes baby jesus cry but please file your complaints at those who hardwired the opcacheable wrappers. As I said: the window is very small.

Regarding cache expire, I checked everything under the sun and they all set to microtime like http://cgit.drupalcode.org/memcache/tree/src/MemcacheBackend.php?h=8.x-2... memcache does and all core backends do. So we are good there.

Although this patch uses the SQL cache backend it's waaaaaay easier than the patches above to just swap in memcached at will.

After discussing with beejeebus I am separating this out to #2513326: Performance: create a PHP storage backend directly backed by cache .

Should this issue be postponed until #2513326: Performance: create a PHP storage backend directly backed by cache gets in?

@anavarre - No.

I am not even sure what this patch solves since it extends mtime storage which uses the files directory which will typically be shared disk and not local disk? it should just work off the base class IMO.

Re #84 -- I get it -- if you point the directory to a local dir then it works.

Per https://www.drupal.org/node/2547827#comment-10213679 this is unlikely to be committed for now because core is being changed not to require coordination across webheads. and so I am moving over to 8.1.x-dev .

Been a while since this was touched. Tagging as needing an IS update with what remains to be done.

Reroll

A lot of this got resolved by moving the container out of PHP, and by #2544932: Twig should not rely on loading php from shared file system.

PhpStorage itself doesn't handle multiple web-heads, but core does now. Dropping priority for now.

+++ b/sites/default/default.settings.php
@@ -601,6 +601,27 @@
+ * Multiple webheads and coordinated caching of the service container.
+ *
+ * Drupal's service container cache does no checking for a stale container. If
+ * a cached container exists, and the mtime-hash matching is ok, it is loaded.
+ * Thus, any Drupal event that should lead to a new container must take care
+ * to remove the old one, so a new one will be built.
+ *
+ * The CoordinatedMTimeProtectedFileStorage backend provides a loader
+ * implementation that checks a central location to determine if the locally
+ * cached container is stale.
+ *
+ * A custom location of the service container storage can be specified in
+ * $settings['php_storage']['service_container']['directory'].
+ *
+ * To use this backend, uncomment the following:
+ */
+#$settings['php_storage']['service_container'] = array(
+#  'class' => 'Drupal\Core\PhpStorage\CoordinatedMTimeProtectedFileStorage',
+#);
+
+/**

Given that core is storing the container in the database by default, do we want to update this settings.php comment to explain that, and why you might want to move the container to file storage?

#90 is right, but there's one big caveat.

#90 is right in that:

1. this is not a problem for the container anymore, since the container now lives in the DB: #2497243: Replace Symfony container with a Drupal one, stored in cache
2. this is not a problem for Twig templates, since #2544932: Twig should not rely on loading php from shared file system ensures that if Twig extensions are modified (or new ones are installed), existing compiled Twig templates are ignored

BUT! It doesn't yet take Twig template modifications are detected. And rightly so, because it'd mean requesting the Twig template's mtime or reading its contents on every request. That doesn't work. We specifically rely on compiled Twig templates, i.e. PHP classes, to avoid much of the work there.

This means that whenever you modify Twig templates (including when you deploy code), you need to clear Twig's PHP storage directory! This is also what drupal_flush_all_caches() does:

  // Wipe the Twig PHP Storage cache.
  PhpStorageFactory::get('twig')->deleteAll();

So if you forget to clear Twig's PHP Storage cache on all webheads, you may end up using old (stale) compiled Twig templates! We should document this more clearly.

19:00:20 <WimLeers> catch: https://www.drupal.org/node/2301163#comment-11275693
19:00:23 <Druplicon> https://www.drupal.org/node/2301163 => Create a phpstorage backend that works with a local disk but coordinates across multiple webheads #2301163: Create a phpstorage backend that works with a local disk but coordinates across multiple webheads => 94 comments, 26 IRC mentions
19:02:00 <catch> WimLeers: could we use $deployment_identifier?
19:03:09 <WimLeers> catch: Yes.
19:16:59 <WimLeers> catch: shouldn't we use deployment_identifier for _all_ PhpStorageCache things?

I had thought we had included the deployment identifier in the twig hash, but if not we indeed should certainly add it.

I had a quick look at it and it doesn't seem to be that we use the deployment identified explicitly for twig nor for all php storage things. For twig we argued that its enough to vary by twig extensions.

For twig we argued that its enough to vary by twig extensions.

It's not, unfortunately. When Twig is stored in PhpStorage, and PhpStorage is specific to each webhead, then you need to wipe each web head's PHP storage after each deployment. Because each deployment may modify Twig templates, but if a prior version of the Twig template is already compiled, then the class_exists() logic in \Twig_Environment::loadTemplate() will see that the template is already compiled, and just use it.
Including the deployment identifier in the Twig template class name would definitely fix that.

Varying only by Twig extensions means that only new Twig extensions or updated Twig extensions cause Twig templates to be recompiled. And updating Twig extensions is of course orders of magnitude more rare than updating Twig templates.

OH yeah no question, I was just saying what we used to think.

Btw. I'm not actually sure we need our own custom logic to deal with twig extensions. There is \Twig_Environment::getTemplateClass as well, which seems to deal with it already.

Including the deployment identifier in the Twig template class name would definitely fix that.

One tricky bit is: How do you know as part of your deployment that any twig file changed. Otherwise we don't want to recompile those templates.
Is there anything we can help with?

Otherwise we don't want to recompile those templates.

Well, that is the downside. We'd always recompile those templates. But only for those sites that actually use deployment identifiers. Would that really be a problem? It's a bigger problem when stale compiled templates are used, which is the problem we have today.
Correctness first, performance second.

The only alternative I see is to provide an official "build step script" for Drupal 8, which allows you to compile all Twig templates ahead of time, commit them, and deploy them, as code. That seems like a much bigger leap.

Is there anything we can help with?

What do you mean?

For that compiles script, see https://github.com/drush-ops/drush/blob/7f59c15d18daff5552f00c2c6ea9c57f.... Would need a little more work for core.

Is there anything we can help with?
What do you mean?

Well at least store the timestamps of every template and in people writing build scripts, provide them these timestamps, for example.

So in theory we could have cake and eat it, but it is a little tricky implementation and I am not sure the current structure of FileSystemLoader allows it:

We have the timestamp to check if the PHP template matches.

Currently we only do check the timestamp if $this->autoRefresh == TRUE.

However we could check the timestamp if we have a cache miss with the rebuild identifier.

e.g. we store the invalidation key inside of the class and do cache re-validation instead of invalidation.

Tricky, but would take care of the concern.

On the other hand: Correctness and KISS first ...

@msonnabaum opened #2752961: No reliable method exists for clearing the Twig cache to focus on the most important subset of this issue: the aspect that #94 describes, i.e. Twig.

Since #2752961: No reliable method exists for clearing the Twig cache was fixed one year ago, maybe it's time to revisit this ?

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.