Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent cache collisions in multisite db-update tasks #3166

Merged
merged 6 commits into from
Oct 12, 2018

Conversation

lcatlett
Copy link
Contributor

Fixes #3158 subtasks

Changes proposed:

  • Adds utility script for generating unique drush cache directory per site / environment
  • Creates and sets unique drush cache directory per site process for db update tasks on ACSF
  • Removes redundant cache clears resetting site context
  • Adds BLT update hook

Steps to replicate the issue:

the original issue can be replicated by running a site update on ACSF. When the db-update Factory hook is executed, updates and config impport will fail because the site context is being reset by cache race conditions:

[ded-000|10677] - The db-update.sh script has completed: Completed in 27 seconds. Exit code: 0. Standard out: Found installation profile profile1.
sandboxacsf.01live: Running BLT deploy tasks on commercialus.sandboxacsf.acsitefactory.com domain in 01live environemnt on the sandboxacsf subscription.
> drupal:config:import
Executing pre-config-import target hook...
No pre-config-import configured.
 �[37;46;1m[info]�[39;49;22m Executing: mysql --defaults-file=/mnt/tmp/sandboxacsf01live/drush_mmFoqe --database=sandboxacsdb216364 --host=dbmaster-1028 --port=3306 --silent  < /mnt/tmp/sandboxacsf01live/drush_8GA1Hh > /dev/null
 �[37;46;1m[info]�[39;49;22m Executing: mysql --defaults-file=/mnt/tmp/sandboxacsf01live/drush_zNbUU7 --database=sandboxacsdb216364 --host=dbmaster-1028 --port=3306 --silent  < /mnt/tmp/sandboxacsf01live/drush_0NVDab > /dev/null
 �[37;46;1m[success]�[39;49;22m 'drush' cache was cleared.

�[33mIn EntityTypeManager.php line 133:�[39m
�[37;41m                                                               �[39;49m
�[37;41m  [Drupal\Component\Plugin\Exception\PluginNotFoundException]  �[39;49m
�[37;41m  The "" entity type does not exist.                           �[39;49m
�[37;41m                        ...nt/www/html/sandboxacsf01live/docroot
[ExecStack] Done in 0.002s
[Acquia\Blt\Robo\Tasks\DrushTask] Running /mnt/www/html/sandboxacsf01live/vendor/bin/drush @self cc drush --uri=commercialus-site.sandboxdigital.net --no-interaction -v --ansi in /mnt/www/html/sandboxacsf01live/docroot
[Acquia\Blt\Robo\Tasks\DrushTask] Done in 0.767s
[Acquia\Blt\Robo\Tasks\DrushTask] Running /mnt/www/html/sandboxacsf01live/vendor/bin/drush @self cache-rebuild --uri=commercialus-site.sandboxdigital.net --no-interaction -v --ansi in /mnt/www/html/sandboxacsf01live/docroot
[Acquia\Blt\Robo\Tasks\DrushTask]  Exit code 1  Time 8.993s
[error]  Failed to import configuration! 
[error]  Command `drupal:config:import ` exited with code 1. 
 [error]  Drupal\Core\Entity\EntityStorageException: Exception thrown while performing a schema update. SQLSTATE[22004]: Null value not allowed: 1138 Invalid use of NULL value: ALTER TABLE {taxonomy_term_field_data} CHANGE `status` `status` TINYINT NOT NULL; Array
(
)
 in Drupal\Core\Entity\Sql\SqlContentEntityStorage->wrapSchemaException() (line 1481 of /mnt/www/html/sandboxacsf01live/docroot/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php). 
 [success] Cache rebuild complete.
 [success] Finished performing updates.

Steps to verify the solution:

  1. Deploy PR to ACSF environment
  2. Run site update at admin/gardens/site-update/update, selecting the branch or tag of this PR
  3. Validate that db-update hook in site update executes without errors. To test via the command line as if the task was run by the factory, run /mnt/www/html/sandbox.01live/factory-hooks/db-update/db-update.sh' 'sandbox' '01live' '[db_role]' [uri]', substituting your site, environment, database role, and uri as needed.

@lcatlett
Copy link
Contributor Author

lcatlett commented Oct 11, 2018

@lpeabody this is the first refactored PR to replace #3006 and you can see the status of the remaining tasks in #3158. I will have the additional PRs available to test as soon as possible.

@lcatlett lcatlett changed the title Prevent cache collisions in ACSF db-update tasks Prevent cache collisions in multisite db-update tasks Oct 11, 2018
@seanhamlin
Copy link

seanhamlin commented Oct 11, 2018

I have a PR up for our shared ACSF customer with @lpeabody. Will hopefully get a test release out today to test the BLT 9.2.0 upgrade + this PR patch.

EDIT

The PR appears to work for this customer. I had to merge the script db-update.sh into our existing db-update script (as we need several customizations to it). The 17 sites on ACSF DEV appeared to work fine.

WIP log (customer name redacted):

[staging-1032|26500] - The 000-updates.sh script has completed: Completed in 29 seconds. Exit code: 0. Standard out: Found installation profile lightning.
Generated temporary drush cache directory: /mnt/tmp/customer.01dev/drush_tmp_cache/ce2a12cd556c6e8af6fcb73451eafd1d.
customer.01dev: Running BLT deploy tasks on professionalau.dev-customer.acsitefactory.com domain in 01dev environment on the customer subscription.
> drupal:config:import
 �[37;46;1m[info]�[39;49;22m Executing: mysql --defaults-file=/mnt/tmp/customer01dev/drush_IkWW9U --database=customeracsdb123106 --host=staging-1032 --port=3306 --silent  < /mnt/tmp/customeracsf01dev/drush_9idUR9 > /dev/null
 �[37;46;1m[info]�[39;49;22m Executing: mysql --defaults-file=/mnt/tmp/customer01dev/drush_JaAuNr --database=customeracsdb123106 --host=staging-1032 --port=3306 --silent  < /mnt/tmp/customer01dev/drush_tR1HtG > /dev/null
 �[37;46;1m[success]�[39;49;22m 'drush' cache was cleared.
 �[37;46;1m[success]�[39;49;22m Cache rebuild complete.
 �[37;46;1m[info]�[39;49;22m Executing: mysql --defaults-file=/mnt/tmp/customer01dev/drush_c8mLQV --database=customeracsdb123106 --host=staging-1032 --port=3306 --silent  < /mnt/tmp/customer01dev/drush_Zc49lA > /dev/null
 �[37;4...guration import strategy defined, falling back to running the database updates only.
[Acquia\Blt\Robo\Tasks\DrushTask] Running /mnt/www/html/customer01dev/vendor/bin/drush @self cc drush --uri=professionalau-dev-site.customer.com --no-interaction -v --ansi in /mnt/www/html/customeracsf01dev/docroot
[Acquia\Blt\Robo\Tasks\DrushTask] Done in 1.21s
[Acquia\Blt\Robo\Tasks\DrushTask] Running /mnt/www/html/customeracsf01dev/vendor/bin/drush @self cache-rebuild --uri=professionalau-dev-site.customer.com --no-interaction -v --ansi in /mnt/www/html/customeracsf01dev/docroot
[Acquia\Blt\Robo\Tasks\DrushTask] Done in 11.863s
[Acquia\Blt\Robo\Tasks\DrushTask] Running /mnt/www/html/customeracsf01dev/vendor/bin/drush @self updb --uri=professionalau-dev-site.customer.com --no-interaction -v --ansi in /mnt/www/html/customer01dev/docroot
[Acquia\Blt\Robo\Tasks\DrushTask] Done in 17.206s
[Acquia\Blt\Robo\Tasks\DrushTask] Running /mnt/www/html/customer01dev/vendor/bin/drush @self cache-rebuild --uri=professionalau-dev-site.customer.com --no-interaction -v --ansi in /mnt/www/html/customer01dev/docroot
[Acquia\Blt\Robo\Tasks\DrushTask] Done in 27.49s
[info] modules.01dev.enable is not set.
[info] modules.01dev.uninstall is not set.

@ba66e77
Copy link
Contributor

ba66e77 commented Oct 12, 2018

@lcatlett, I filed lcatlett#11 against your branch with a couple minor tweaks. Those aside, I'm not seeing areas of concern with the code here, but I'm still testing things out.

A couple notes:

  1. I do not have a setup which will allow me to fully test this out on ACSF. I'm taking on faith that you and @seanhamlin have done that testing.
  2. This does not address non-ACSF multisite installations.
  3. I'm not sure about the changes to src/Robo/Commands/Setup/ConfigCommand.php. We've beat that question back and forth a few times now and even though they should not be needed we've seen replicable cases where removing them triggered problems. That's what I'm testing at the moment.

Style tweaks and setting update version number to 9.2.
Copy link
Contributor

@ba66e77 ba66e77 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have not been able to trigger issues stemming from the removal of the drush cr's so, with the other caveats noted in my previous comment, I'll give this a thumbs up.

Now that the 10.x branch is the default, we should be creating PRs against it and backporting to earlier versions. However, given we don't have anyone able to do a full live test on 10.x and the resolution of this and related PRs is a priority, I think it's reasonable to make an exception to process in this case.

@ba66e77 ba66e77 merged commit 2d73091 into acquia:9.2.x Oct 12, 2018
@lcatlett
Copy link
Contributor Author

@ba66e77 thanks for the PR - I just merged. The approach in this specific PR was not intended to work on standard multisite since it uses Factory Hooks and ACE uses Cloud Hooks, so I can create an issue to implemement similar functionality on ACE regarding the DRUSH_PATHS_CACHE_DIRECTORY config. The alternative would be to configure this value for every BLT drush command, but that seems like overkill.

Regarding the ConfigCommand.php changes, I realize those changes may seem substantial, however at the least, the drush cc drush is redundant and what is causing a lot of challenges since it is already run by the parent db-update Factory Hook process prior to the custom dp-update.sh firing, though I am not sure if this is the case with Cloud Hooks on ACE. Additionally, drush cc drush does not pass the site, root, and uri contexts which negates the benefits derived from setting DRUSH_PATHS_CACHE_DIRECTORY in the parent process.

Regarding the drush cr, this was likely added to ConfigCommand when we didn't know a lot about the internals of deploys in D8. A heavy-handed cache flush or cc all was a common step in D7 deploys because we did not yet have the ability to cache the service container or plugin discovery mechanisms, and were forced to use config management such as Features which bypassed core caching. In looking at past issues related to deploying BLT config and code updates, it looks like most would not actually be resolved using current drupal core or current BLT config.

I would argue that we should actually be encouraging developers to enable caching locally since even when caching is turned off, cache flushes are still required to refresh plugins, config, and classes. Using a full cr to accomplish this requires a full bootstrap of Drupal locally which makes development unnecessarily slow without a lot of benefit. Additionally, on the remote upstream environment where caching is presumaly enabled, a cr does not actually refresh those particular cache objects in most cases unless you are using an alternative cache backend.

At any rate, the drush cc drush is an absolute must to remove, and if the cr is not removed completely, then BLT should only invalidate the cache objects required for refreshing configuration in ConfigCommand, which in this case I believe is the service container, plugin cache, and config bins, but that should likely be in a dedicated PR and discussed separately due to the urgency of this one.

lcatlett added a commit to lcatlett/blt that referenced this pull request Oct 20, 2018
* Add drush cache dir utility script.

* Enhanced Factory Hook to isolate db-update tasks to tempoary drush cache.

* Update Factory Hooks from BLT template.

* Remove redundant cache clear and rebuild to preserve drush context of current request.

* Style tweaks and setting update version number to 9.2.
@lcatlett lcatlett mentioned this pull request Oct 20, 2018
lcatlett added a commit to lcatlett/blt that referenced this pull request Nov 10, 2018
* Add drush cache dir utility script.

* Enhanced Factory Hook to isolate db-update tasks to tempoary drush cache.

* Update Factory Hooks from BLT template.

* Remove redundant cache clear and rebuild to preserve drush context of current request.

* Style tweaks and setting update version number to 9.2.
mikemadison13 pushed a commit that referenced this pull request Dec 14, 2018
* Test against Drupal 8.6.x-dev. (#2937)

* Updating versions of Drupal core and Lightning. (#3061)

* Updating CHANGELOG.md and setting version for 9.2.0-alpha1.

* Fixes #3055: Update memcache settings to match changes in Drupal memcache module. (#3058) (#3067)

* Require drupa/memcache equal or greater than 2.0-alpha7.
* Updating memcache settings based on change in drupal/memcache module.

* Allow PHPUnit to bootstrap from core. (#3071)

* Clarified Git hook documentation. (#3073)

* Finished updating memcache config for alpha7. (#3076)

* Finished updating memcache config for alpha7.
* Locking drupal/memcache to 2.0-alpha7 until memcache stabilizes.

* Updating CHANGELOG.md and setting version for 9.2.0-alpha2.

* Travis CI Memcache Service and Settings (#3082)

* Start memcached service and add php extension.
* Check status of memcached service.
* Use blt memcache config on CI.

* Run simplesaml config command on all composer calls. (#3051)

* Update local-development.md (#3087)

Correcting links to PHPStorm documentation on Acquia's documentation service.

* Remove memcache.yml and references to it. (#3093) (#3094)

* Cherry-pick in lost/abandoned commits from 9.x (#3072)

* VM setup tips (#3036)

* Update local-development.md to reference committing DrupalVM files.

* New guidelines for contribution. (#3045)

* Adding replication and verification step sections and clean up formatting.

* Adding new guidelines for PRs to the CONTRIBUTING docs.

* Fixes #2964: When manually deploying using a tag also push that tag to source repo. (#2992)

* Adding a cutSourceTag method and invoking when deploying tag.

* Set default deploy.tag_source config setting and check it when deploying.

* Refactoring to use a single cutTag method.

* Remove line about expectation you already have a tag on your source before you tag a release.

* Warn if user is creating a tag but tag_source option is false.

* Updating deploy documentation.

* Fixes #3053: Update requirement to drush/drush:^9.4.0. (#3078)

* Fixes #3065: Use webflo/drupal-core-require-dev instead of tracking core dev dependencies. (#3068)

* Use core's PHPUnit bootstrap file. (#3092)

* amotic -> atomic (#3098)

Fixed minor typo in code review docs.

* Support memcache on ACSF via flags. (#3096)

* Remove support for PHPUnit 5. (#3102)

* Run validation without interaction on CI. (#3070) (#3097)

* Fixes #3046: Cleaned up Pipelines docs. (#3104)

* Exclude the 'sites/settings' dir for the default list of sites. (#2994) (#3105)

Force-approving this based on the same code's approval on the 9.x branch.

* Updating CHANGELOG.md and setting version for 9.2.0-alpha3.

* Update config_split.config_split.ci.yml (#3129)

* Update README.md (#3113)

* Minor variable / comment refactor. (#3111)

* Fix the link of Memcache documentation page. (#3135)

* Remove PHP 5.6 from the list of tested versions. (#3137)

* Fixes #2945 by removing cloud hook exception for ACSF. (#2946)

* Fixes #2945: removing exception for cloud hooks on ACSF.
* Cleaning up variable use in db scrub hook.
* Adding ACSF check to post deploy and makes db scrub consistent with others.

* updating blt docs to cover ACSF memcache use. (#3136)

* Fix INSTALL.md installation directions for Drush Launcher. (#3127)

* Fixes #3118: Update theme path to more standardized D8 defaults. (#3119)

* Resolves #3106: Add 'skip_permissions_hardening' setting to local settings file template (#3107)

* Update support policies (#3147)

* Update support policies

* Update README.md

* Fix PHP syntax error in acquia simplesamlphp config. (#3141)

* 3149: Minor grammar fix (#3150)

* Fixed travis output. (#3151)

* Travis shouldn't duplicate Drupal 8.6 tests (#3152)

* Travis should test Drupal 8.7.

* Disable 8.7 tests.

* Updating CHANGELOG.md and setting version for 9.2.0.

* Prevent cache collisions in multisite db-update tasks (#3166)

* Add drush cache dir utility script.

* Enhanced Factory Hook to isolate db-update tasks to tempoary drush cache.

* Update Factory Hooks from BLT template.

* Remove redundant cache clear and rebuild to preserve drush context of current request.

* Style tweaks and setting update version number to 9.2.

* Prevent cache collisions in multisite install tasks (#3171)

* Refactor post-install hook to use temp cache dir per site, use requests instead of ENV.

* Refresh Factory Hooks from template for 9002001.

* Update version number for hook.

* Remove redundant factory hook update.

* Removed unused use statements.

* Style and phpcs updates.

* Update BLT settings for site and http/proxy detection (#3172)

* Fix auto detection of sites dir due to core changes.

* Update proxy and host detection for core updates and Acquia VCL changes.

* PHPCS fixes.

* Update deploy.md with correct Cloud Hooks links. (#3188)

The Acquia docs link pointed to a stale page and the BLT one pointed to the 8.x branch, both throwing 404's. I've updated both to point to the latest iterations of each page.

* Update to Drupal Coder 8.3.x. (#3132)

Fixes #2289 
--------

Changes proposed:
---------
(What are you proposing we change?)

- Update composer contraint on Drupal Coder newest release (8.3.1 as of this date)
- Update composer constraint on squizlabs/php_codesniffer to 3.0.1 (matching Coder)
- Remove the use of a bootstrap file with PHPCS. File paths that are not allowed by filesets won't be sniffed by default.
- Add a tests:phpcs:sniff:modified command to sniff unstaged modified or added files in repo for convenience.

* Updating CHANGELOG.md and setting version for 9.2.1.

* Merge tag '9.1.7' of github.com:acquia/blt

* PHP CS fixes.

* Restrict lightning version constraint to 8.5x.

* Revert coder to older version.

* Revert required and suggested composer merge plugin config.

* Update RoboFile.php

* Fix default drush local multisite alias bug causing test failures.

* Fix multisite test alias bug.

* Revert "Update to Drupal Coder 8.3.x. (#3132)"

fccd0d0

* Update hooks and phpcs config.

* BLT Backport of 9.2 to 9x (#3280)

* Fix comment formatting in PhpUnitCommand. (#3211)

* Fixes #3063 by updating faq and install docs to improve install profile instructions. (#3064)

* [9.2.x] Fix undefined trusted_reverse_proxy_ips #3214 (#3215)

Fixes #3214 
--------

Changes proposed:
---------
(What are you proposing we change?)

- check if trusted_reverse_proxy_ips is defined as an array
- if not, set an empty array

Steps to replicate the issue:
----------
(If this PR is not fixing a defect/bug, you can remove this section.)

1. blt setup
2. bootstrap drupal

Steps to verify the solution:
-----------
(How does someone verify that this does what you think does?)

1. apply the patch
2. blt setup / bootstrap Drupal
3. check to ensure following warning does not appear:

```
Warning: in_array() expects parameter 2 to be array, null given in require() (line 27 of /app/vendor/acquia/blt/settings/blt.settings.php).
require('/app/vendor/acquia/blt/settings/blt.settings.php') (Line: 806)
require('/app/docroot/sites/naswa_main/settings.php') (Line: 125)
Drupal\Core\Site\Settings::initialize('/app/docroot', 'sites/naswa_main', Object) (Line: 1056)
Drupal\Core\DrupalKernel->initializeSettings(Object) (Line: 655)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)
```

* Fixes #3185 to bring install and local docs inline. (#3186)

* Fixing updates 9002000 message. (#3205)

* Fixing missing semi-colon at end of line. (#3208)

* Fixes #3243 to clean up dry-run in deploy command. (#3263)

* Multisite bugfixes (#3231)

* Fix multisite test alias bug.

* Fix default drush local multisite alias bug causing test failures.

* Fix global varible scope and phpcs syntax.

* Update variable names and fix phpcs validation errors..

* Add missing phpunit-bridge package.

* Add BLT update hook for 9.1.9 release.

* Update version for 9.1.9 release.

* Revert "Lock core in 9.x branch to 8.5.6. (#3062)"

b2e0bcf

* Use assertions to resolve build failures due to phpunit 'risky tests' notice.

* Revert travis backports to debug build.

* Ensure minimum of dmore/chrome-mink-driver 2.5.0.

* Remove duplicate update hook.

* Removing changes from composer files.
grasmash added a commit to grasmash/bolt that referenced this pull request Dec 15, 2018
grasmash added a commit to grasmash/bolt that referenced this pull request Dec 17, 2018
mikemadison13 pushed a commit that referenced this pull request Dec 17, 2018
* Prevent cache collisions in multisite install tasks (#3171)

* Refactor post-install hook to use temp cache dir per site, use requests instead of ENV.

* Refresh Factory Hooks from template for 9002001.

* Update version number for hook.

* Remove redundant factory hook update.

* Removed unused use statements.

* Style and phpcs updates.

* Update BLT settings for site and http/proxy detection (#3172)

* Fix auto detection of sites dir due to core changes.

* Update proxy and host detection for core updates and Acquia VCL changes.

* PHPCS fixes.

* [9.2.x] Fix undefined trusted_reverse_proxy_ips #3214 (#3215)

Fixes #3214 
--------

Changes proposed:
---------
(What are you proposing we change?)

- check if trusted_reverse_proxy_ips is defined as an array
- if not, set an empty array

Steps to replicate the issue:
----------
(If this PR is not fixing a defect/bug, you can remove this section.)

1. blt setup
2. bootstrap drupal

Steps to verify the solution:
-----------
(How does someone verify that this does what you think does?)

1. apply the patch
2. blt setup / bootstrap Drupal
3. check to ensure following warning does not appear:

```
Warning: in_array() expects parameter 2 to be array, null given in require() (line 27 of /app/vendor/acquia/blt/settings/blt.settings.php).
require('/app/vendor/acquia/blt/settings/blt.settings.php') (Line: 806)
require('/app/docroot/sites/naswa_main/settings.php') (Line: 125)
Drupal\Core\Site\Settings::initialize('/app/docroot', 'sites/naswa_main', Object) (Line: 1056)
Drupal\Core\DrupalKernel->initializeSettings(Object) (Line: 655)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)
```

* Fixing missing semi-colon at end of line. (#3208)

* Multisite bugfixes (#3231)

* Fix multisite test alias bug.

* Fix default drush local multisite alias bug causing test failures.

* Fix global varible scope and phpcs syntax.

* Update variable names and fix phpcs validation errors..

* Prevent cache collisions in multisite db-update tasks (#3166).

* Check if argv is set before using it (#3282)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants