I had a brief discussion with David about some of the reload issues this week at the d.o sprint here in Portland. Our talk was centered around the whole issue of 'what happens when something on prod has been overridden since last reload' but it is possible that it might help in the other issues as well.
Basically the idea is that whenever we reload config, we take that a snapshot of that config and save it away somewhere else. What this gives us at any point in time is the following:
1) The config in the files
2) The config in the active store
3) The config that was last reloaded
This means we can actually determine not only that the active store has drifted since last reload, but also in what way it has drifted. It allows us a little more detail too. If I change a setting on prod, then decide to revert it, the active store is still the same as the last reload even though it was at some point modified. We couldn't know this with the simple flag solution. This offers the opportunity for contrib to do some really cool 3-way diff stuff to determine how and where config has changed. While this does add some complexity to our solution, I really like it a lot.
Comment | File | Size | Author |
---|---|---|---|
#87 | 83-87-interdiff.txt | 8.04 KB | alexpott |
#87 | 1515312-config-import-snapshots-87.patch | 12.44 KB | alexpott |
#83 | 81-83-interdiff.txt | 5.06 KB | alexpott |
#83 | 1515312-config-import-snapshots-83.patch | 11.87 KB | alexpott |
#81 | 79-81-interdiff.txt | 5.75 KB | alexpott |
Comments
Comment #1
Anonymous (not verified) CreditAttribution: Anonymous commented.
Comment #2
gddA solution for this was proposed in #1447686: Allow importing and synchronizing configuration when files are updated that I think makes a lot more sense. Updated issue summary with such.
Comment #3
sunThis issue/discussion seems to duplicate #1703168: [Meta] Ensure that configuration system functionality matches expected workflows for users and devs now.
The essential conclusion thus far is that configuration staging is more complex than diffing two or three snapshots (although that depends on the "type" of configuration), and that we even have huge troubles to determine the proper storage/location for the "second" snapshot in a two-snapshot diff.
Let's centralize the discussion in that issue.
Comment #4
gddNow that the architecture has settled down I'd like to reopen this issue. The Views team is in particular quite interested in this functionality, and if it doesn't get in then it is a feature regression for them.
Comment #5
tim.plunkettI'm postponing #1620140: Allow synchronizing config entities from default config when modules are updated and #1497268: Add revert functionality to Config entities on this issue.
And tests written for #1776830: [META-1] Installation and uninstallation of configuration provided by a module that belongs to another module's API might eventually bump this to critical.
Comment #6
sunGiven that we have #1741804: Implement a config import/staging directory and #1702080: Introduce canonical FileStorage + (regular) cache now,
I don't really get what a third snapshot would make possible that isn't possible today already?
There also seems to be quite some overlap with #1642062: Add TempStore for persistent, limited-term storage of non-cache data
(In light of this, let's demote this back to a feature; there's no reason to block other patches from landing)
Comment #7
Anonymous (not verified) CreditAttribution: Anonymous commentedhere's a first cut at a patch to get the discussion going.
Comment #9
Anonymous (not verified) CreditAttribution: Anonymous commentedsame patch as #7, but make sure to create a snapshot dir for tests.
Comment #10
Anonymous (not verified) CreditAttribution: Anonymous commentedrerolled to chase HEAD.
Comment #11
realityloop#10: 1515312-10-snapshot.patch queued for re-testing.
Comment #13
realitylooprerolled patch, untested
Comment #14
realityloop#13: 1515312-13-snapshot.patch queued for re-testing.
Comment #16
Anonymous (not verified) CreditAttribution: Anonymous commentedrerolled to keep up with head, lets see what the bot thinks.
Comment #17
gddI am assuming this would have to go in before feature freeze, tagging as such.
I'm curious why we use FileStorage for the snapshot instead of a database table. Seems like the latter would make more sense. This data never needs to be staged, and having a third directory in the config area could be confusing for users. This isn't a critical performance path or anything.
With #1821172: Hash the config directory name, unhash the active and staging directory names in place, this should be changed to be the hashed config directory . '/snapshot'
Comment #18
gddComment #19
mitchell CreditAttribution: mitchell commentedDoes this issue include adding revision support to FileStorage?
#1821548: Add a "diff" of some kind to the CMI UI could add merge support to #1697256: Create a UI for importing new configuration.
More descriptions:
* #1703168-24: [Meta] Ensure that configuration system functionality matches expected workflows for users and devs
* #1825344: 3-way Diff View
Comment #20
gddI think I can safely say we're not going to be adding revisions to FileStorage anytime soon.
Comment #21
mitchell CreditAttribution: mitchell commentedWould it be that hard to do, or is a
GitStorage
Controller, as described in #1703168-24, possible?Will revisioning configs be a contrib add-on? Does this affect how diff will be used in the Config UI?
Comment #22
gddIt will be available in contrib, and actually there is already one contrib sandbox which implements it.
Comment #23
gddNo longer applies, working on a reroll
Comment #24
gddOK here's a reroll to HEAD. I talked to beejeebus about moving this to database storage and he said he's not against it, so I'll probably make that change. I was pondering adding some API functions around it (config_is_config_overridden($object) or something) however we might not need that at all because we can just use config_sync_get_changes() with $active and $snapshot storage. If it turns out we want something more as things get implemented we can add them in followups.
Comment #25
gddComment #26
mkadin CreditAttribution: mkadin commented+1 for this feature, I'd love to see some fancy diffing of views and such.
Re-queuing as this patch is breaking the installer for me. Let's see...
Comment #27
mkadin CreditAttribution: mkadin commented#24: 1515312-config-import-snapshots-24.patch queued for re-testing.
Comment #28
gddHere's a new version which runs on database storage and has a custom table for storing the snapshots. I started playing around with the idea of storing a hash of the data (for comparisons) in addition to the actual data (for diffs) but it wasn't worth the trouble. Once this is in we should figure out how/if we're going to surface it in the import UI.
Comment #30
gddThis is passing for me locally, however it appears the snapshot table isn't getting installed because system.module isn't getting enabled. Lets try this.
Note that I thought about putting table creation into config.install, but we need this table to exist even if someone uninstalls the Config UI (they can still do imports via drush and the like.)
Comment #32
xjmDoes this work instead?
Comment #34
xjm#32: config-1515312-32.patch queued for re-testing.
Comment #35
xjmI'm a dork.
Comment #38
gddI realized we should be creating an initial snapshot at install time, so this patch adds that.
Other than that this is all tests. I modified the tests in ConfigImportTest to check that the snapshot and active match at initial install as well as after a successful import.
I also created ConfigSnapshotTest which tests the following:
- We have a matching snapshot at install time
- That it doesn't match when we modify the active config
- That it doesn't match staging but still matches active when we change the staging config
- That active and snapshot match again after an import.
Comment #40
mkadin CreditAttribution: mkadin commented#38: 1515312-config-import-snapshots-36.patch queued for re-testing.
Comment #41
webchickSeems like this is pretty key to a well-functioning configuration system, so escalating to major.
Comment #42
alexpottI wonder whether we should be using the state system to store the snapshots instead of a new table?
Comment #43
moshe weitzman CreditAttribution: moshe weitzman commentedIf we are going to stick with just saving one snapshot, then I could see us moving this to state system instead of DB storage. State designed for name+blob storage like this. However, I expected to see yet another directory for this. That is, I expected FileStorage. That lets sysadmins use standard diff tools to look for changes. I'm curious why we went for DB storage.
I'd love to see core start saving multiple snapshots. And we use the upcoming diff feature to compare arbitrary snapshots. I'm OK with that being in Contrib for D8.
Personally, I would remove the word 'import' here.
The code comment suggests to me that I would have seen two FALSE and one TRUE
Comment #44
gddMy concern about adding a third directory is that it is going to cause confusion when people are staging their config. Having a staging and active directory is already more confusing than I'd like. However the state system does potentially make sense. We would need to write a StateStorage controller. I'm trying to think if there are scenarios when we would want snapshot to be a bit more permanent. Moving your site to a new server? As long as the state API is non-ephemeral it might be a better option, yes. However if we decided to save multiple snapshots, state probably wouldn't be appropriate anymore.
config_sync_get_changes() actually returns FALSE when the two storages are the same, and an array of changes when they are different. I was confused at first too. I clarified that comment and the other one in this new patch.
Comment #45
sunI agree that this should use the key/value storage.
The primary reason for that is that it would be wrong/invalid to stage a snapshot.
Furthermore, I cannot see why that storage would have to be tied to config in any way. It's just a new key_value $bin, nothing else.
In turn, that also eliminates the new database table.
Most storage controllers will be able to handle this in a more efficient way. We should add a
deleteAll()
method that has the same signature aslistAll()
.EDIT: The
deleteAll()
suggestion is obviously obsolete by switching to key/value.I'm not sure whether the addition of this API function makes sense. I can't really see in which case you'd want to revert all configuration to a previous state.
In any case, dfac() should not be contained in this API function.
The call to dfac() is the reason for why you had to change the test to a WebTest, so we can revert that change, too.
I do not understand what this comment tries to say, and why we need to manually create a snapshot.
This test should use DrupalUnitTestBase.
Comment #46
xjmSee #30 through #35. If you can get it working using a unit test, then by all means, but otherwise, I see no reason not to use a web test. Slower? Sure. But it works.
Comment #47
gddI do now as well, although there is still some dissent around whether it should use k/v or be stored in a third subdirectory in the file system. I'm not a fan of that option but I'd like to hear some more voices.
Comment #48
gddAgreed, #1851180: Add deleteAll() function to Drupal\Core\Config\StorageInterface opened.
I spent some time attempting to implement this as keyvalue storage and I'm not sure it works properly as things currently stand. In order for this to make sense we really need to create a KeyValueStorage implementation of StorageInterface, so that it can be used in config_sync_changes() and config_sync_get_changes(). However implementing this is not a no-brainer. The biggest problem is that there is currently no way to implement listAll() for keyvalue storage, and the functionality doesn't really make sense for keyvalue anyways. We could just have it throw an exception, but then we have to rework the way ConfigTestBase works (it assumes a valid implementation of listAll() for all storage implementations.) A similar problem exists for rename(), which we could work around by just doing a delete/add operation, but it feels wrong.
Adding deleteAll() would remove the dependence on listAll() for this patch, but the greater questions are still worth pondering.
Comment #49
xjmSo I had @heyrocker explain #48 to me. If we create a k/v bin for each snapshot, and want to look for a particular configuration object or set or objects, then we have to load all the snapshots to look for them. That's a misuse of the k/v store I think.
Comment #50
gddYes, to clarify, getting one object or all objects from keyvalue is easy. Its the querying by prefix that listAll() does which is the problem.
Comment #51
gddGiven the above, I'd like to leave the table. I'm still against a file-based snapshot implementation. Anyone else have any thoughts here? I'd like to push this through.
Comment #52
Anonymous (not verified) CreditAttribution: Anonymous commentedhere's a reroll to get this going again.
Comment #53
gddI agree and changed this API function to revert a single piece of configuration.
This actually ended up getting reverted in another patch.
I made this more explicit in the tests.
Other changes:
- Changed lock functions to use new API
- Now using new deleteAll() functionality for config storage
- Added restore functionality to tests
Having heard no more discussion about where to store the snapshots can we consider that topic shut?
I'm getting an odd test fail on this locally so I'm throwing it at the bot.
Comment #55
Anonymous (not verified) CreditAttribution: Anonymous commentedlooked at this, found that we're doing this in config_restore_from_snapshot():
fixed that so it uses CONFIG_IMPORT_LOCK, but the test still fails. still digging...
Comment #56
Anonymous (not verified) CreditAttribution: Anonymous commentedturns out that calling drupal_static_resset() (which we do via drupal_flush_all_caches()) is not safe during a lock.
because lock():
will go ahead and get a new lock backend, so we end up with a different $lockId, and lock()->release() will always fail. because the test takes a lock for config_restore_from_snapshot(), but doesn't release it, config_import() never actually runs.
the quick fix in the attached patch is to move the drupal_flush_all_caches() call after the lock()->release(). i think we probably need another issue to deal with the lock stuff.
Comment #57
gddThanks for tracking that down! I personally think this is ready but someone else should pull the switch.
Comment #58
tim.plunkettNot that it really matters, but
if (!isset($config_changes[$op])) {
does the same thing with no function calls.Couldn't this be moved to the top, before even the drupal_container() calls?
For readability, I'd have $success = FALSE at the top, then $success = TRUE in the try{}. This way, it seems like we're saying it succeeded before even trying.
:( Why did this need to be done?
Contains \Drupal\...
Missing a blank line before the method
---
Except the switch away from a Unit test, this is great.
Comment #59
xjmSee #30 through #35 and #45 / #46. The dfac() is still there, I think. Someone could try reverting the class change and seeing if it works.
Comment #60
gddActually the call to drupal_flush_all_caches() that was causing problems is gone, so we can in theory change to DrupalUnitTestBase. However, when I just tried that, I discovered that the initial config snapshot never gets created, because that happens in install_finished(), which never gets called in unit tests. Note that this snapshot has to get created *after* all other modules are installed, otherwise it will be incomplete. If someone knows a better place to put this, let me know.
I think this is a bad idea, since we want the code that executes after the lock is acquired to be as small as absolutely necessary. Talked to timplunkett about this in IRC and got his agreement.
All other issues addressed in attached patch. No interdiff since I also merged head, sorry.
Comment #61
sunReverted ConfigImportTest back to DUTB. The schema of the {config_snapshot} table just had to be installed.
That said, I'm really not sure whether the snapshot assertions actually belong in there. Remember, these are supposed to be unit tests — mixing arbitrary other functionality into an existing unit test is not really good.
Also, I was not able to understand the test plan in the new ConfigSnapshotTest. I think the code comments, but also the variable names being used need some clarification on what is actually going on in there.
Lastly, I wasn't really able to make sense of the {config_snapshot} table/schema... it only has name/data columns — no snapshot identifier? So there can only be a single snapshot, and whenever you do one, and whenever you import configuration, you overwrite a possibly existing snapshot?
Comment #62
gddSince successfully creating a snapshot during import operations is an important functionality, I thought they made sense to be added to those tests. On the other hand, the main snapshot tests cover an import operation already, so I'd be OK removing them from ConfigImportTest.
That is correct, it can only be used to compare for changes since the last import. I think that for core this is fine, contrib can do tracking of multiple snapshots if necessary, although that would require them to maintain their own tables or storage. So hrm. I can see adding a snapshot identifier for future growth purposes, but I still don't think core should maintain more than one snapshot out of the box.
Will improve comments in the test in next roll, hopefully by tomorrow morning. Would be nice to get this in.
Comment #63
gddNew patch removes snapshot tests from ConfigImportTest and attempts to clarify ConfigSnapshotTest. I held off on the change to the schema of {config_snapshot} pending further comment.
Comment #64
moshe weitzman CreditAttribution: moshe weitzman commentedI've reviewed this before and all suggestions including mine have abeed addressed. Lets go RTBC on this. I think we could have aimed higher here (preserve multiple snapshots), but this will do for now.
Comment #65
moshe weitzman CreditAttribution: moshe weitzman commented#63: 1515312-config-import-snapshots-63.patch queued for re-testing.
Comment #66
tim.plunkett#63: 1515312-config-import-snapshots-63.patch queued for re-testing.
Comment #68
gddReroll, just moving code around. Should go back to RTBC if the bot comes back green.
Comment #69
gddComment #70
alexpottI don't want to be a party pooper but think this patch needs more work because
config_restore_from_snapshot()
function.I would like to see #1808248: Add a separate module install/uninstall step to the config import process done before this... and therefore should get done before to #1890784: Refactor configuration import and sync functions which would significantly change this code as snapshotting should be done on firing on the synchronise event. And the restore from snapshot code would be a lot simpler too.
Comment #71
gddTalked this over with alexpott in IRC. I had also been having some misgivings about the restore process, as I'm not sure we can get single-config-restore to work consistently at all. There are some functions in Drupal that are inherently not restorable (think field changes) and the interactions between config changes that may or may not have happened makes this area inherently complex. We both agreed that it is best to simply punt on restore entirely for this iteration of the patch, and if we do that then this is fine to move on. In the next followup we can expose this in the import ui as a warning (HEY YOU MIGHT BE OVERWRITING DATA DANGER DANGER) and link to a diff of the changes so you can sort it out if you need to.
He also pointed out that the tests are missing in the last patch, which I can fix easily enough.
I'll do that this afternoon.
Comment #72
moshe weitzman CreditAttribution: moshe weitzman commentedIf we had validation, then a field would reject such a restore. See http://api.drupal.org/api/drupal/core%21modules%21field%21field.crud.inc...
Comment #73
gddI'm happy to integrate validation into this conversation when it exists. I think a lot of this stuff can be iterated going forward, but I really want to get the basics in place first in case none of the dependencies end up going forward.
Attached patch re-adds the test and removes config_restore_from_snapshot() and hopefully we're done here then.
Comment #75
gddThe upgrade path failures are random bot fails, the other is me being stupid.
Comment #77
gddNote to self: Actually run tests after changing them. (CKEditor test fail is unrelated and is happening all over the place today.)
Comment #78
alexpottReal minor nit...
Missing a new line at end of file
Also the test should probably be a DrupalUnitTestBase but this is non trivial as we'd need to create the services on the container or just use the classes to create objects. Happy to do this as a follow up.
Comment #79
gddBased on the nit this is RTBC if the bot comes back green (which may not be until Sunday at this rate.)
Comment #81
alexpottThe reason for the test failures in #79 is the simpletest overrides to system.file is leaking into the test. To mitigate this I've changed the test to DrupalUnitTestBase and I'm using the config provided by the config_test module not config provided by the system module.
In changing the tests to DrupalUnitTestBase I think I might have uncovered an issue with snapshots and module enables / disables / uninstalls as these activities do not touch the snapshot table. See the code from the test below:
what is concerning is that if we moved
$this->assertIdentical($active->listAll(), $snapshot->listAll());
it would fail because the snapshot would be empty but active would contain a number of config objects... andconfig_sync_get_changes($snapshot, $active))
is reporting there is nothing to sync...Comment #82
alexpottTest please...
Comment #83
alexpottAfter discussing this with heyrocker on IRC we decided that is was a bug that after importing config calling
config_sync_get_changes()
with the snapshot and active storages returned FALSE (as in no changes). This is a result of howconfig_sync_get_changes()
assumes that basic config files can only be changed and config entities as managed using the manifest files.One possible solution is to use a function similar to
config_sync_get_changes()
before the addition of manifests. The patched attached implementsconfig_compare_storage_contents()
to do exactly that.Comment #85
alexpottThe following test failure does not occur locally...
Sending for a re-test
Comment #86
alexpott#83: 1515312-config-import-snapshots-83.patch queued for re-testing.
Comment #87
alexpottBorrowed code from #1889620: config_install_default_config() overwrites existing configuration that adds an argument to
config_sync_get_changes()
as to whether to use manifests. If set to false then the function compares everything rather than using the manifests present in the source storage.Comment #88
effulgentsia CreditAttribution: effulgentsia commented#87 looks great to me. Given that the premise of this issue was RTBC already back in #69, and the implementation fixes since then look good, I think it's safe to re-RTBC this. Committers: please give this one at least 24 hours in case others more involved with this issue than I've been know of something still wrong with it.
Comment #89
Anonymous (not verified) CreditAttribution: Anonymous commentedthis looks ready to fly.
Comment #90
sunAlso happy to move forward here. I guess this facility will undergo larger revamps anyway later on.
Comment #91
YesCT CreditAttribution: YesCT commentedwhen this goes in, this might help #1790398: Re introduce revert functionality for views using the config system
Comment #92
webchickOk, since it's the 11th hour of feature freeze here, I did a review of this patch. There's nothing to see since this doesn't introduce any revert capabilities or whatever in the UI to interact with these snapshots; it only creates the table to store them in and adds an API/test coverage. I was told by Tim that this is required functionality for #1790398: Re introduce revert functionality for views using the config system, and since #1790398: Re introduce revert functionality for views using the config system is basically a regression in Views, I'm personally comfortable moving this to a task rather than a feature request. The other issue will have to see whether this becomes a core or contrib thing, but in any case we need this stuff for the functionality at all, it sounds like.
Committed and pushed to 8.x. Thanks!
Comment #93.0
(not verified) CreditAttribution: commentedUpdated issue summary.