drush icon indicating copy to clipboard operation
drush copied to clipboard

Need to run config import several times

Open mikkeschiren opened this issue 8 years ago • 25 comments

I get some random behaviors of drush cim - usually I need to run it like three, four times, and usually the "batches" of config is different on each run. When I am importing config inside Drupal it works on the first try. Number of config files is around 300-400. I am running drush cim after a site install.

mikkeschiren avatar Nov 17 '16 14:11 mikkeschiren

Can you pastebin the output of all config-export runs with --debug flag? Then link to that pastebin here.

weitzman avatar Nov 17 '16 14:11 weitzman

I have seen similar behavior and it seems others have too:

  • https://www.drupal.org/project/config_split/issues/2901394#comment-12216877
  • https://www.drupal.org/project/config_split/issues/2962511#comment-12595892
  • https://github.com/acquia/blt/pull/1398

However, in my experience, the only relation to a large number of config changes is correlation, there is no causation that I can detect.


In the past I have solved this by simply including drush config:import twice in the deploy scripts as @mikkeschiren noted above. However, we recently switched to using drush deploy (thank you for this standardization!) in an attempt to follow best practices.

I wanted to note that while we are seeing this issue with drush, this is really an issue with the config import process. Config has to be imported twice regardless of if drush or the config UI is used (I confirmed this for my use case explained below). The factor that makes this a drush issue at all, is that drush deploy is now available as a codified best practice that makes it clear config should only need to be imported once. Perhaps I'm reading into that too much though. 🤷

The issue of whether config:import should be ran twice on deployments is something that I've been "debating" co-workers about for years and I want to try to gain a better understanding of the problem and how others solve it. The scenarios where the second config import is needed are limited, so we switched to using the single config import offered by drush deploy recently and figured we would see if the issue popped up again.

tl;dr it happened again 🙁

The specifics of the modules used might not be important, as I've seen this happen with config split (issues linked above) and now the layout_builder_restrictions module as well. It's possible that the outcome of this issue is that the modules need to manage their own configuration better. So it is entirely possible that this issue doesn't even belong here, and should be a Drupal core or contrib issue, but I wanted to start here since drush deploy was the first time I saw config import being only run once as a documented best practice.


Modules:

drupal/core-recommended: 8.9.7
drush/drush: 10.3.5
drupal/layout_builder_restrictions: 2.7.0

Steps to reproduce:

  1. Run drush config:import and confirm it imports everything.
  2. Run drush config:export and confirm no changes are present.
  3. Enable layout builder for a content type and save the form (/admin/structure/types/manage/article/display).
  4. Enable per content type block restrictions and save the form (/admin/structure/types/manage/article/display).
  5. Run drush config:export and commit the config diff. config-issue.txt
  6. Grab a fresh database locally or attempt to deploy this to a new database in a different environment.
  7. Run drush config:impoort
  8. Run drush config:export and see a diff. config-additional.txt

However, as noted above, if the config is imported twice back to back, then there is no diff on the export. A single config import isn't sufficient under some limited circumstances.


Line 6: Assuming the first config import completes successfully, we need to run the config import a second time. The first config import may include changes to the behavior of config import, such as config ignore or config split. We must run config import a second time to ensure those changes take effect.

  • https://www.bounteous.com/insights/2020/03/11/automate-drupal-deployments/

I'm very interested in other's take on this and what the best practice is here.

Hopefully the short novel was helpful. I was trying to answer all of the questions I would ask I got a similar issue report!

adamzimmermann avatar Oct 14 '20 20:10 adamzimmermann

Thanks for the great report. Hopefully someone will follow your reproduce steps and suggest a fix. I am pretty sure that double config:import is never part of system's design.

weitzman avatar Oct 24 '20 12:10 weitzman

While I was doing some unrelated research on deployments, I stumbled upon this article about deployment best practices. The following sections seem to apply here.

drush sset system.maintenance_mode TRUE
# Create a restore point by taking backups of anything that is not in the code repository: database, media, cache
# Checkout the code you are deploying
drush updb
drush cim sync -y || drush cim sync -y
drush cim sync -y
drush sset system.maintenance_mode FALSE
drush cr

Line 5: Immediately after running the update process, you’ll want to import your configuration changes. You’ll notice the line has two pipe characters. There is an issue with the config import process where if a module is installed and configuration entities are being created for that module it may fail to create the config entities because of missing dependencies. This is a cache issue and we don’t want to fail the entire deployment if that is why it is failing. To achieve this, we run config import again if the first time it fails — that is what the double pipe does. See the bash reference for how the double pipe works.

Line 6: Assuming the first config import completes successfully, we need to run the config import a second time. The first config import may include changes to the behavior of config import, such as config ignore or config split. We must run config import a second time to ensure those changes take effect.

@josephdpurcell is the author of that article. I'm hoping he can chime in here too.


Perhaps the outcome of this is still that this was not the intention of core and drush deploy should stay as is, but hopefully it will help the community better understand what a practical best practice deployment script looks like. As I'm guessing many people are using the aforementioned modules (config ignore or config split), so drush deploy doesn't quite cut it for them.


I included some of this in an update to my comment above too.

adamzimmermann avatar Jan 21 '21 15:01 adamzimmermann

I'm happy to chime in. Everything I'm about to share is more from experience and applied theory about Drupal's internals, rather than rigorously proving certain issues/bugs exist, e.g. with an automated test.

The need for importing config twice is very simple: things like config ignore and config split are config themselves. The first config import updates Drupal with these configuration changes. However, it is not until the second config import that those changes are reflected.

How can this be fixed? I'm not sure -- to my knowledge, all of my projects and peers are using the double config import to address this issue. Can / should this be fixed in Drush or Drupal? This seems like a Drupal core/contrib issue. I've not done research to find a drupal.org ticket that identifies this issue, but perhaps one exists already and perhaps there are great ideas for resolving it. I can point to the fact that BLT does a double config import as well, which implies that BLT developers ran into this same issue: https://github.com/acquia/blt/blob/main/src/Robo/Commands/Drupal/ConfigCommand.php#L147-L150

I am aligned with declaring that the drush deploy feature has a bug, specifically that it runs updb, cim, deploy:hook in immediate sequence, but by the time deploy:hook is run the second cim would not have been run, therefore the state of config cannot be trusted at this point. But, again, whether this is declared a bug in Drupal vs Drush I don't have a strong opinion on. I think for convenience it would be nice if some accommodation was made in Drush? BLT solved it in BLT.

This is a very quick reply, so please push back on anything I've said here if it's not thoughtful or correct; I very well may have missed something.

NOT MENTIONED was this sneaky bit drush cim sync -y || drush cim sync -y. The first config import may fail, for example if a module is installed on config import and configuration entities are being created for that module, it may fail to create the config entities because of missing dependencies. I've not dug around for a drupal.org ticket for this, I have to suspect one exists. I noticed this issue more when we were adding new modules to sites with the exported config in the sync directory.

josephdpurcell avatar Jan 21 '21 17:01 josephdpurcell

A typical Drupal deployment sequence has three critical steps:

  1. Repair the database (updb).
  2. Synchronize configuration (cim).
  3. Rebuild the caches.

A configuration sync should not occur until the database is considered to be in a repaired state.

That means that anything that needs to happen to get a configuration synchronization to run once, and be complete, needs to occur during the repair phase. If you've updated config_ignore or config_split configuration, using what Drupal has at its disposal currently, then you need to add an update hook which updates those configuration items first. You may also want to nuke some caches as part of that update hook. I think everyone understands that, but doesn't do it because its silly to write the same update hook 100000 times which updates the same configuration items over and over and over again.

It seems that some modules that are meant to manipulate the configuration synchronization/transformation, have their behavior determined by configuration themselves (which itself is susceptible to the transformation API). Any such configuration should obviously be imported first. There's a couple ways you could accomplish that:

  1. Add an option to config:import which takes a list of prioritized configuration IDs, and imports that configuration before running through the full sync (it may need to cache bust as well).
  2. Add a separate command config:import:critical which does more or less the same thing as number 1, but is a dedicated command with a purpose of importing configuration flagged as "critical" in some way.
  3. Start a Drupal core issue for formalizing the need for prioritized import of configuration (IMHO, solving this in core is the way to go, as there is clearly a need).

So 3, 2, 1 is probably the order which you'd want to tackle these things. That said, nothing happens fast in Drupal, so there would likely need to be some kind of interim stopgap to make this less of a pain. IMHO, there isn't really a bug here, just a lack of features.

lpeabody avatar Jan 21 '21 18:01 lpeabody

Excellent discussion on this thread. Perhaps @bircher or @alexpott could lend their wisdom here.

weitzman avatar Jan 21 '21 18:01 weitzman

This is all great. Thank you! I've been keeping an eye on this thread the past week or so and thinking it over more. I'll continue to do that and give others a chance to chime in.

I will revisit this in a week or so and draw up a core issue with what we have discussed so far.


As I write this, I'm wondering if we could continue to just have one command, but in the background it handles the issue for the user. This is riffing off of what @lpeabody suggested above in #1. However, this would be a bit simpler for the user, as deploy scripts or drush deploy for that matter wouldn't need to be changed.

  1. Search for all config tagged in some way.
    • Modules such as config_split or config_ignore would need to tag their config to support this.
  2. Import the tagged config.
  3. Run the existing config import/sync process.

I'm thinking the logic would go right around here.

Thoughts?

adamzimmermann avatar Feb 05 '21 13:02 adamzimmermann

This can indeed happen. In my experience there are two categories.

  1. Site building and then exporting config, then on a fresh install from config there are some changes that don't get reverted because they should have been the way they are on the new site but were not exported correctly. This is probably a core bug or a bug with a module which doesn't save the config according to the schema. But I never had the time to track this down because it is just a minor annoyance usually and quite easily fixed by exporting the config. Ie the workaround is so much faster than trying to reproduce it, I have not had the luxury to debug it and find the root. But I know that it is possible to have the config import "stuck" and attempting to import the same thing over and over.
  2. The change is due to something "messing" with what is imported, for example with config split a split could become active (available/created) during a config import and then the next import would also add the config from this split. Before the deploy command one could just run drush cim a couple of times until there are no changes any longer or until the change set is the same. But of course with the config storage transformation API, one can create all sorts of interesting edge cases. For example set the site slogan to the current time stamp or cycle the color of bartik between three colors at every config import. Maybe these example cases are a bit contrived, but this is essentially what happens if you "move" config to a split and turn it on at the same time. (we solved this by creating the split in a post update hook when we did that)

But I don't really know what the best solution could be though. Maybe we could do a "re-try" option that imports config x times if there are still changes to import?

bircher avatar Apr 03 '21 19:04 bircher

  1. I have also seen this too. Sometimes when you save a form that hasn't been saved for a while new config keys are added or 0 is changed to false. I think that is a separate conversation, but it might be related to this. I need to set some time aside to think about this more in-depth at some point.
  2. That all sounds correct. The time stamp idea is the same issue from a technical perspective, but that is almost a separate issue as it would never be able to be in sync. So ya probably a bit contrived, but I think it illustrates the main issue well.

(we solved this by creating the split in a post update hook when we did that)

I'm hoping to avoid the need for this, but it is an option in this scenario.


But I don't really know what the best solution could be though. Maybe we could do a "re-try" option that imports config x times if there are still changes to import?

I think I like this idea. Perhaps drush deploy --cim=2 or something like that? 🤔

adamzimmermann avatar Apr 05 '21 13:04 adamzimmermann

I shouldn't have to import configuration twice in order to get a complete import. If I say import, it should just import everything and have it be what you expect.

lpeabody avatar Apr 05 '21 13:04 lpeabody

Based on the conversation happening in this issue and some feedback from @weitzman and @lpeabody during a DrupalCon BoF, it seems like there is some momentum around this either becoming an issue in the Config Split or Drupal core issue queues. Does anyone have strong arguments for one over the other?

markdorison avatar Apr 13 '21 18:04 markdorison

Based upon the example I shared above related to Layout Builder config I think this is more of a core issue. Not because Layout Builder is in core, but simply because there seems to be areas where this is a problem unrelated to the use of Config Split.

adamzimmermann avatar Apr 13 '21 20:04 adamzimmermann

Has anyone tried the layout_builder_restrictions flow mentioned above in the Drupal import UI? Does it import in one try?

weitzman avatar Apr 14 '21 13:04 weitzman

Drupal core issue created. ✅

  • https://www.drupal.org/project/drupal/issues/3241439

Please add any additional information there and hopefully we can get some traction on this. 🤞🏼

Thank you everyone for your contribution so far.


I suppose this can be closed then? 🤷🏼‍♂️

adamzimmermann avatar Oct 07 '21 20:10 adamzimmermann

Yeah, lets converse there and reopen here later if needed. Thanks @adamzimmermann

weitzman avatar Oct 08 '21 03:10 weitzman

I posted a bit more info at https://www.drupal.org/project/drupal/issues/3241439. Hopefully someone can dig further.

weitzman avatar Jan 18 '23 20:01 weitzman

@weitzman thank you!

adamzimmermann avatar Jan 18 '23 20:01 adamzimmermann

@weitzman I'm hopeful we could reconsider this issue at this point in time. Right now the only issue preventing us from fully embracing drush deploy as part of our recommended build workflow is that we've realized more often than not we do need to import config twice. Apart from the edge-cases mentioned above and in the core issue, a pragmatic scenario we see very often is when using config_split to manage sets of configurations (some projects can't even choose another mechanism, for example sites hosted on Acquia Site Factory must use config_split to manage per-site config differences).

Would you be willing to consider an opt-in mechanism that would execute config import a second time, if requested? (for example a flag such as drush deploy --import-config-twice or something along those lines)

I have bandwidth to work on a patch, but wanted to check if the idea has buy-in first.

Thanks!

marcoscano avatar Jul 26 '23 13:07 marcoscano

The other common situation we run into is when importing migration configuration for sites on Pantheon. Many of their tiers limit memory to 256MB including drush and their recommended workflow is to run config import multiple times.

https://docs.pantheon.io/guides/account-mgmt/plans/resources

drush deploy --import-config-twice

I think this could be made more flexible by either specifying the number of times to run (--repeat-import=3), or keeping track of the config differences after each attempted import and looping until you get the same result twice.

(some projects can't even choose another mechanism, for example sites hosted on Acquia Site Factory must use config_split to manage per-site config differences).

Total aside, but I was discussing this with an Acquia engineer, and they said that config_split isn't actually required by ASF. Nothing stops sites from using settings.php overrides instead. However, they default to assuming sites will use it so I think it's safe to assume the majority of ASF hosted sites will.

deviantintegral avatar Aug 24 '23 13:08 deviantintegral

How does having multiple config:import specifically benefit folks on low memory plans? Is it because they fatal error during import and then a rerun succeeds? The fatal left the site in an unknown state, and I'm not sure its fully healed by a re-run.

In general I'm still -1 to fixing this in Drush. Drupal core made a wise architecture system to make Config be a declarative system. That is, at the end of an import you have the configuration thats been declared in your export directory. The guarantee does not say "after multiple imports you have the configuration of the export directory". This is fundamentally why I am waiting for fix in core, and not in Drush.

The Drupal issue above has steps to reproduce in it. Seemingly nobody is available to work on that issue. Perhaps @marcoscano can dedicate some time in that direction (we acknowledge that this approach takes more time than adding an option to deploy).

weitzman avatar Aug 24 '23 16:08 weitzman

How does having multiple config:import specifically benefit folks on low memory plans?

Since configuration imports aren't atomic, each subsequent import gets more configuration items imported. When PHP is killed, whatever has successfully imported is left as is and not rolled back.

deviantintegral avatar Aug 24 '23 19:08 deviantintegral

@weitzman thanks for the feedback. In my mind the ability to re-run config import would help 3 categories of issues:

a) config that isn't imported the first time "by design". This is point 2 of Fabian's comment here. A split definition can be modified and have its "splitted" config objects modified in the same deploy, so it's understandable it would be hard to do both imports with a single execution

b) OOM errors, as Andrew points out above

c) Mis-declared dependencies, and other edge cases, including the layout builder scenario described in the drupal issue. (which I understand corresponds to Fabian's point 1 above)

If I understand things correctly, even if we solved the core bug, we'd still have people in a) and b) that would need more than one config import to finish the syncrhonization. In my experience, a) and b) are actually more frequent than c), and it seems we are not the only ones that just take this from a pragmatic standpoint.

Even if the opt-in ability to re-run the import it doesn't solve the above issues at their root, it seemed to me to be harmless enough to be worth pondering its pros/cons.

Thanks!

marcoscano avatar Aug 25 '23 14:08 marcoscano

a) multiple config imports might be 'by design' for sites using config split, but i don't think it is by design for the config api in general. Input from @alexpott and @davidstrauss would be illuminating here.

A split definition can be modified and have its "splitted" config objects modified in the same deploy, so it's understandable it would be hard to do both imports with a single execution

Sure. The answer might be "dont do that". This sounds analogous to the wisdom to not uninstall a module and remove it from composer in the same deployment. I'm not a config split user, so I cant speak too deeply whether it could be changed to be more robust (i.e. splits could be code defined and not themselves be config).

b) Sounds like these customers have hosting that is insufficient to cleanly maintain a modern Drupal site.

c) Is this really misdeclared dependencies? The issue hasnt mentioned that.

weitzman avatar Aug 25 '23 16:08 weitzman

c) Is this really misdeclared dependencies? The issue hasnt mentioned that.

Sorry I didn't mean to imply that particular issue on d.org is due to misdeclared dependencies. We have however experienced in past projects scenarios where misdeclared dependencies required more than one config import.

marcoscano avatar Aug 28 '23 08:08 marcoscano