migrator: allow removing settings in migrations #644
Conversation
tjkirch
force-pushed
the
migrator-remove-setting
branch
from
db84b08
to
324d91e
Compare
|
Note: I updated the commit message, removing the following paragraph: This was a bit misleading. The problem is old migrations being run through the new migrator. This (#643) is the only migration since our last release, so the problem would be users upgrading through more than one release. For example, someone updating from 0.1.5 to 0.2.1 (without any changes on our part) would attempt to run the migrations for 0.1.5 to 0.1.6 and 0.1.6 to 0.2.0, which were compiled without this new syntax, and then this migration from 0.2.0 to 0.2.1, which was compiled with this new syntax. That wouldn't work, because the older migrations would fail and the upgrade would halt. We can:
|
This is the approach we chose as a team. I'll be working on modifying our existing migrations to support both migrator interfaces, as a workaround for the interface change. (Future migrations won't require this.) I'd still like to get this approved so we're otherwise ready to go. |
tjkirch
force-pushed
the
migrator-remove-setting
branch
from
324d91e
to
e641511
Compare
|
This push adds commits that allow migrations that remove settings to work even when downgrading to OS versions with the old migration interface, as long as we compile the migrations with the updated migration-helpers library. It's best to look at the individual commits to understand the changes - see the updated description with the commit messages. (The only change to the single existing commit was a missed |
tjkirch
dismissed
etungsten’s stale review
Major changes, requesting re-review
tjkirch
force-pushed
the
parameterize-region
branch
from
929b5eb
to
d9b7706
Compare
tjkirch
force-pushed
the
migrator-remove-setting
branch
from
e641511
to
d174748
Compare
|
This push just rebases on the changes from #638. |
tjkirch
force-pushed
the
parameterize-region
branch
from
d9b7706
to
67a403b
Compare
tjkirch
force-pushed
the
migrator-remove-setting
branch
from
d174748
to
d164c6d
Compare
|
This push is just a rebase onto the changes from #637. |
tjkirch
force-pushed
the
parameterize-region
branch
from
67a403b
to
a67331d
Compare
tjkirch
force-pushed
the
migrator-remove-setting
branch
from
d164c6d
to
c1e9877
Compare
|
This push just rebases on a typo fix from #636. |
tjkirch
force-pushed
the
parameterize-region
branch
from
a67331d
to
e20a6b4
Compare
tjkirch
force-pushed
the
migrator-remove-setting
branch
from
c1e9877
to
08136e9
Compare
|
This push is just a rebase onto changes from #637. |
tjkirch
force-pushed
the
migrator-remove-setting
branch
from
08136e9
to
6f4691b
Compare
|
This push addresses @bcressey's concerns by (1) removing a comment and (2) deleting empty directories after removing keys. (2) was a bit hard to test because it can't happen in our system, but I pulled that function out into a separate binary and confirmed it works as documented. |
tjkirch
force-pushed
the
migrator-remove-setting
branch
from
6f4691b
to
3d91ae2
Compare
|
This push just fixes a typo in a comment that bcressey mentioned offline. |
tjkirch
force-pushed
the
parameterize-region
branch
from
e20a6b4
to
f0133b3
Compare
Migrations weren't able to remove settings because we started with a copy of the existing data store and wrote out any changes key-by-key, meaning a "removal" would just leave the old copied key in place on disk. With this change, we no longer copy the data store, we instead give migrations the source data store and the target data store, and (via migration-helpers) the input data is read from the source instead of a copy. This is more efficient because it avoids a needless copy, and allows the migration to determine exactly what's left after it runs.
This allows for DataStore-based testing in other crates.
We've gotten by without this because settings are effectively all required, but there are now some advanced cases (migrator interface change) that require removing keys from an existing data store. This probably shouldn't be exposed in a higher-level API, though, unless we account for optional settings.
…ation The old migrator interface used a single data store as source and target. After a migration changes the MigrationData, the resulting values are written out to the data store. Since this data store also served as the source, it had all of the old keys on disk already, and nothing explicitly removed them; we'd only see new keys overwrite old. To allow downgrading to versions with the old migrator interface, we need migrations to explicitly remove keys from the data store. This change to migration-helpers detects keys that were removed from MigrationData by a migration and tells the data store to explicitly remove them. As long as migrations are rebuilt against this library, they'll work in the old or new migrator. This workaround can be removed when we no longer support versions with the old migrator interface.
tjkirch
force-pushed
the
migrator-remove-setting
branch
from
3d91ae2
to
4fed84a
Compare
|
Just a rebase on develop to get the fix from #637, no changes. |
We found confusion between data store versions and OS versions, so this simplifies the system to only use OS version. If there's been no change in the data store, which is now determined by there being no migrations, then the new data store will simply be a link to the last version that did have changes. These are the changes at a high level: * Remove the data_store_version library and its type, instead using semver::Version * Cascade this change through migrator, storewolf, update_metadata, and updog * Remove OS version -> data store version mapping in updog * Remove the data-store-version file, instead using OS version from bottlerocket-release * In migrator, just symlink to previous data store if there are no migrations * In migrator, flip symlinks for current, major, minor, and patch, since any could change * In storewolf, also create the new patch-level symlink One change deserves more description. It's an extension of the problem from #644 that only showed up while testing this change, and was made easier to fix by these changes, so it was done together. Background: In migrator, we were giving each migration the same source and target data store. Migrations make whatever changes they want to a MigrationData, each key of which is then written to the data store. Problem: Let's say the system has settings A and B. One migration adds setting C. This is written to the target data store. Another migration tries to remove setting B. It does so in memory, but B has already been written to the target by the first migration, so it's to no avail. Solution: Chain the input and output of migrations in order. The first migration receives the source data store and adds setting C, writing out to a *new* data store path it was given. The second migration is given that new data store as a source; it removes setting B and writes out to another new data store path it was given. At the end of the chain, the final data store is kept and the intermediate ones are removed.
Builds on #638.
Fixes #641.
Testing done:
See #643 for testing of the first commit.
The following three commits include unit tests, which pass. In addition:
Migrate up to 0.2
Other, normal migration worked:
Add region, which would be generated by #638:
Migrate, which should remove it:
Other, normal migration still worked:
Setting is now gone, which (I double checked) doesn't work with a migration built with older migration-helpers: