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

migration-helpers: AddSettingsMigration, RemoveSettingsMigration for migrating multiple settings at once #958

Merged

Conversation

etungsten
Copy link
Contributor

@etungsten etungsten commented Jun 25, 2020

Issue number:
N/A

Description of changes:

Author: Erikson Tung <etung@amazon.com>
Date:   Thu Jun 25 17:05:41 2020 -0700

    migration-helpers: `RemoveSettingsMigration` to remove multiple settings at once
    
    Adds new migration helper `RemoveSettingsMigration` to remove multiple
    settings at once.
    
    Original `RemoveSettingMigration` now wraps `RemoveSettingsMigration`
    and is now deprecated.
Author: Erikson Tung <etung@amazon.com>
Date:   Thu Jun 25 13:42:16 2020 -0700

    migration-helpers: AddSettingsMigration handle more than one setting
    
    Adds new migration helper `AddSettingsMigration`.
    `AddSettingsMigration` handles migrating more than one setting.
    
    Original `AddSettingMigration` now wraps `AddSettingsMigration` and is
    now deprecated

Testing done:
Created my own datastore locally with storewolf.
Running apiserver locally, committed initial bottlerocket-launch transaction with settings-commiter.
The source datastore has the following content for update settings:

$ ls -al ds/current/live/settings/updates
total 32
drwxr-xr-x 2 etung domain^users 4096 Jun 25 14:05 .
drwxr-xr-x 6 etung domain^users 4096 Jun 25 14:05 ..
-rw-r--r-- 1 etung domain^users    5 Jun 25 14:05 ignore-waves
-rw-r--r-- 1 etung domain^users   46 Jun 25 14:01 metadata-base-url.setting-generator
-rw-r--r-- 1 etung domain^users   80 Jun 25 14:01 metadata-base-url.template
-rw-r--r-- 1 etung domain^users   11 Jun 25 14:01 seed.setting-generator
-rw-r--r-- 1 etung domain^users   43 Jun 25 14:05 targets-base-url
-rw-r--r-- 1 etung domain^users    8 Jun 25 14:05 version-lock

Tried the following with a migration using this helper:
Forward migration:

Running `add-version-lock-ignore-waves --source-datastore ~/thar/testing/ds/current --target-datastore ~/thar/testing/ds/next --forward`
AddSettingsMigration(["settings.updates.version-lock", "settings.updates.ignore-waves"]) has no work to do on upgrade.

Observed that the temporary datastore created matches the source datastore. Which is expected

Backward migration:

     Running `add-version-lock-ignore-waves --source-datastore ~/thar/testing/ds/current --target-datastore ~/thar/testing/ds/next --backward`
Removed settings.updates.version-lock, which was set to '"latest"'
Removed settings.updates.ignore-waves, which was set to 'false'

Observed that the temporary datastore no longer has the version-lock and ignore-waves settings.

$ ls -al ds/next/live/settings/updates
total 24
drwxr-xr-x 2 etung domain^users 4096 Jun 25 15:03 .
drwxr-xr-x 6 etung domain^users 4096 Jun 25 15:03 ..
-rw-r--r-- 1 etung domain^users   46 Jun 25 15:03 metadata-base-url.setting-generator
-rw-r--r-- 1 etung domain^users   80 Jun 25 15:03 metadata-base-url.template
-rw-r--r-- 1 etung domain^users   11 Jun 25 15:03 seed.setting-generator
-rw-r--r-- 1 etung domain^users   43 Jun 25 15:03 targets-base-url

Testing the original AddSettingMigration (singular setting)

Backwards migration to remove a single version-lock setting:

Running `add-version-lock-ignore-waves --source-datastore ~/thar/testing/ds/current --target-datastore ~/thar/testing/ds/next --backward`
Removed settings.updates.version-lock, which was set to '"latest"'

Resultant temporary datastore where version-lock no longer exists.

$ ls -al ds/next/live/settings/updates
total 28
drwxr-xr-x 2 etung domain^users 4096 Jun 25 15:07 .
drwxr-xr-x 6 etung domain^users 4096 Jun 25 15:07 ..
-rw-r--r-- 1 etung domain^users    5 Jun 25 15:07 ignore-waves
-rw-r--r-- 1 etung domain^users   46 Jun 25 15:07 metadata-base-url.setting-generator
-rw-r--r-- 1 etung domain^users   80 Jun 25 15:07 metadata-base-url.template
-rw-r--r-- 1 etung domain^users   11 Jun 25 15:07 seed.setting-generator
-rw-r--r-- 1 etung domain^users   43 Jun 25 15:07 targets-base-url

For RemoveSettingsMigration, I inverted the forward/backward testing from the steps above and they work as expected.

Terms of contribution:

By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.

@etungsten etungsten requested review from tjkirch, zmrow and webern June 25, 2020 22:12
Copy link
Contributor

@tjkirch tjkirch left a comment

Choose a reason for hiding this comment

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

I think we should do the same for removing settings with a RemoveSettingsMigration; the same problem would happen there, in reverse.

Adds new migration helper `AddSettingsMigration`.
`AddSettingsMigration` handles migrating more than one setting.

Original `AddSettingMigration` now wraps `AddSettingsMigration` and is
now deprecated
…ngs at once

Adds new migration helper `RemoveSettingsMigration` to remove multiple
settings at once.

Original `RemoveSettingMigration` now wraps `RemoveSettingsMigration`
and is now deprecated.
@etungsten etungsten force-pushed the migration-helper-add-settings branch from 6a907f2 to cff7869 Compare June 26, 2020 00:13
@etungsten etungsten changed the title migration-helpers: AddSettingsMigration to handle more than one setting migration-helpers: AddSettingsMigration, RemoveSettingsMigration for migrating multiple settings at once Jun 26, 2020
@etungsten
Copy link
Contributor Author

Push above addresses @tjkirch 's comments.

  • Adds a new RemoveSettingsMIgration that remove multiple settings at once.
  • Deprecated RemoveSettingMigration

Tested the migrations and they still work as expected.

@etungsten etungsten requested a review from tjkirch June 26, 2020 00:17
@etungsten etungsten merged commit 36a335d into bottlerocket-os:develop Jun 29, 2020
@etungsten etungsten deleted the migration-helper-add-settings branch June 29, 2020 18:17
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.

3 participants