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

Remove data store version #760

Merged
merged 3 commits into from
Feb 21, 2020
Merged

Remove data store version #760

merged 3 commits into from
Feb 21, 2020

Conversation

tjkirch
Copy link
Contributor

@tjkirch tjkirch commented Feb 20, 2020

commit 42dbf9a1a45d12bf9fdcf57a9a499c9fcf193029
Author: Tom Kirchner <tjk@amazon.com>
Date:   Fri Feb 14 14:15:21 2020 -0800

Remove data store version, use OS version instead

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.
commit 3f5c6980f16b3270d4e51effee7c89ccad923eb8
Author: Tom Kirchner <tjk@amazon.com>
Date:   Wed Feb 19 16:38:05 2020 -0800

apiserver: don't fail list_transactions if '/pending' doesn't exist
commit b7ea75daeafbac7df70174a83108391d8322e0dd
Author: Tom Kirchner <tjk@amazon.com>
Date:   Wed Feb 19 10:39:03 2020 -0800

Fix type in RemoveSettingMigration so we don't need to copy strings

Testing done:

  • I made an AMI (0.2.2), confirmed health, and checked settings through the API.
  • I made two migrations, one that adds a setting and one that removes a setting, and updated the model to match.
  • I made new images (0.2.3) and put them into a repo.
  • I updated the instance and saw that the migrations worked; the API showed the one added and the other removed.
  • Instance still healthy, pod still runs.
  • I updated /etc/release to say 0.2.4 and ran the migrator again, and saw it symlinks to 0.2.3 correctly, because there were no migrations.
  • Data store links appear as expected:
lrwxrwxrwx 1 root root    2 Feb 20 22:14 current -> v0
lrwxrwxrwx 1 root root    4 Feb 20 22:14 v0 -> v0.2
lrwxrwxrwx 1 root root    6 Feb 20 22:14 v0.2 -> v0.2.4
lrwxrwxrwx 1 root root   23 Feb 20 22:05 v0.2.2 -> v0.2.2_IejSOH6K1zpvpmM9
drwxr-xr-x 4 root root 4096 Feb 20 22:05 v0.2.2_IejSOH6K1zpvpmM9
lrwxrwxrwx 1 root root   23 Feb 20 22:09 v0.2.3 -> v0.2.3_UAgMOzFx6MKgAFnq
drwxr-xr-x 3 root root 4096 Feb 20 22:14 v0.2.3_UAgMOzFx6MKgAFnq
lrwxrwxrwx 1 root root   23 Feb 20 22:14 v0.2.4 -> v0.2.3_UAgMOzFx6MKgAFnq
  • I confirmed a downgrade back to 0.2.2 worked too.
  • I also did a lot of playing with migrator locally - changing major versions and minor versions, same-version no-op runs, no-migration just-symlink runs, etc.

@tjkirch
Copy link
Contributor Author

tjkirch commented Feb 20, 2020

Rebase on develop to fix conflict; I looked through the changes and nothing relates to this PR.

workspaces/api/migration/migrator/src/lib.rs Show resolved Hide resolved
@@ -142,8 +179,8 @@ where
/// Returns the sublist of the given migrations that should be run, in the returned order, to move
/// from the 'from' version to the 'to' version.
fn select_migrations<P: AsRef<Path>>(
Copy link
Contributor

Choose a reason for hiding this comment

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

Not necessarily something to fix for this review, but I was thinking that it might make sense for migrator and updata to share the logic that determines which migrations are applicable.

Thoughts?

Copy link
Contributor Author

@tjkirch tjkirch Feb 21, 2020

Choose a reason for hiding this comment

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

I'm not sure what you mean since I don't think updata has a list that includes irrelevant migrations - let's talk separately.

workspaces/api/migration/migrator/src/main.rs Show resolved Hide resolved
workspaces/api/migration/migrator/src/main.rs Show resolved Hide resolved
workspaces/api/migration/migrator/src/main.rs Outdated Show resolved Hide resolved
workspaces/updater/updog/src/bin/updata.rs Outdated Show resolved Hide resolved
@tjkirch
Copy link
Contributor Author

tjkirch commented Feb 21, 2020

This push rebases on #756, and fixes the "mirations" typo, which didn't seem big enough to put in its own push.

@jhaynes jhaynes added this to the v0.3.0 milestone Feb 21, 2020
Copy link
Contributor

@zmrow zmrow left a comment

Choose a reason for hiding this comment

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

🍀

workspaces/updater/updog/src/main.rs Outdated Show resolved Hide resolved
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.
@tjkirch
Copy link
Contributor Author

tjkirch commented Feb 21, 2020

This push fixes the "muliple" typo.

@tjkirch tjkirch merged commit 41ab462 into develop Feb 21, 2020
@tjkirch tjkirch deleted the no-data-store-ver branch February 21, 2020 21:39
tjkirch added a commit to tjkirch/bottlerocket that referenced this pull request Mar 11, 2020
Missed this in bottlerocket-os#760 that removed data store version.
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

5 participants