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

Admin container v0.5.0 migration #903

Merged
merged 1 commit into from
Apr 16, 2020

Conversation

etungsten
Copy link
Contributor

@etungsten etungsten commented Apr 14, 2020

commit 8c0aadfbb39461bf41da9e2a8511d1f084f355ba
Author: Erikson Tung <etung@amazon.com>
Date:   Tue Apr 14 14:52:31 2020 -0700

    migrations: add migration to migrate admin container version

    Adds a new migration to migrate the default admin host-container version
    from v0.4.0 to v0.5.0.

Issue number:
Addresses #890

Testing done:
Created a datastore locally using storewolf and manually populated settings.aws.region with "us-west-2"
Forward migration:

     Running `migrate-admin-container --source-datastore /tmp/ds/next --target-datastore /tmp/ds/current --forward`
Updating template and value of 'settings.host-containers.admin.source' on upgrade
Changing template of 'settings.host-containers.admin.source' from '328549459982.dkr.ecr.{{ settings.aws.region }}.amazonaws.com/bottlerocket-admin:v0.4.0' to '328549459982.dkr.ecr.{{ settings.aws.region }}.amazonaws.com/bottlerocket-admin:v0.5.0'
Changing value of 'settings.host-containers.admin.source' from '328549459982.dkr.ecr.us-west-2.amazonaws.com/bottlerocket-admin:v0.4.0' to '328549459982.dkr.ecr.us-west-2.amazonaws.com/bottlerocket-admin:v0.5.0'

$ cat /tmp/ds/current/live/settings/host-containers/admin/source.template
"328549459982.dkr.ecr.{{ settings.aws.region }}.amazonaws.com/bottlerocket-admin:v0.5.0"
$ cat /tmp/ds/current/live/settings/host-containers/admin/source
"328549459982.dkr.ecr.us-west-2.amazonaws.com/bottlerocket-admin:v0.5.0"

Backwards migration:

     Running `migrate-admin-container --source-datastore /tmp/ds/current --target-datastore /tmp/ds/next --backward`
Updating template and value of 'settings.host-containers.admin.source' on downgrade
Changing template of 'settings.host-containers.admin.source' from '328549459982.dkr.ecr.{{ settings.aws.region }}.amazonaws.com/bottlerocket-admin:v0.5.0' to '328549459982.dkr.ecr.{{ settings.aws.region }}.amazonaws.com/bottlerocket-admin:v0.4.0'
Changing value of 'settings.host-containers.admin.source' from '328549459982.dkr.ecr.us-west-2.amazonaws.com/bottlerocket-admin:v0.5.0' to '328549459982.dkr.ecr.us-west-2.amazonaws.com/bottlerocket-admin:v0.4.0'

$ cat /tmp/ds/next/live/settings/host-containers/admin/source.template 
"328549459982.dkr.ecr.{{ settings.aws.region }}.amazonaws.com/bottlerocket-admin:v0.4.0"
$ cat /tmp/ds/next/live/settings/host-containers/admin/source
"328549459982.dkr.ecr.us-west-2.amazonaws.com/bottlerocket-admin:v0.4.0"

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 a review from tjkirch April 14, 2020 22:07
@etungsten etungsten requested a review from zmrow April 14, 2020 22:09
@tjkirch tjkirch linked an issue Apr 14, 2020 that may be closed by this pull request
@tjkirch tjkirch changed the title Admin ctr migration Admin container v0.5.0 migration Apr 14, 2020
@etungsten
Copy link
Contributor Author

Force push above adds missing license information.

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.

Sorry about needing the revert :(

@etungsten
Copy link
Contributor Author

etungsten commented Apr 14, 2020

Addresses @tjkirch 's comments.

Checking why github actions is failing...

@etungsten
Copy link
Contributor Author

The build check is failing because rpm2migrations is trying to install bottlerocket-x86_64-migrations-debuginfo-0.0-0.x86_64.rpm which has a dependency on bottlerocket-x86_64-os-debuginfo(x86-64) which it was not able to find and install...

rpm -iv --root "${ROOT_TEMP}" "${PACKAGE_DIR}"/*.rpm

In any case, rpm2migrations shouldn't be installing every single rpm that's built. It should really only need to install the ones needed by the main migrations rpm (currently has no dependencies, so just the migrations rpm).

We could also fix this by updating os.spec to strip the binaries with install -s (currently it's not stripping the migrations binaries which results in an migrations-debuginfo package):

built_path="${HOME}/.cache/%{__cargo_target_static}/release/${crate_name}"
target_path="%{buildroot}%{_cross_datadir}/migrations/${migration_binary_name}"
install -m 0555 "${built_path}" "${target_path}"

@tjkirch
Copy link
Contributor

tjkirch commented Apr 15, 2020

@etungsten The Dockerfile sets it up so that only the migration rpm is in the packages directory when rpm2migrations is run - see the repobuild section. I think the problem is that what's copying the RPM into /local/migrations, namely the migrationbuild section, is using a "*" after "migration" so that it doesn't have to worry about every single component of the RPM's filename, and that unfortunately captures the debuginfo RPM too. I think we need to make the filename pattern in the cp more exact.

I'm not sure why this would have changed...

@zmrow
Copy link
Contributor

zmrow commented Apr 15, 2020

Hm... this change in behavior seems... odd. I don't recall seeing this issue when putting together rpm2migrations.

@etungsten
Copy link
Contributor Author

Push above removes the direct dependency listing on schnauzer.

I'll fix the build issue in a separate PR.

@tjkirch
Copy link
Contributor

tjkirch commented Apr 15, 2020

LGTM, but we shouldn't approve/merge until we know we can build migrations safely again.

Perhaps the revert of the WrappedSettings change should also go into the other PR related to fixing the build?

@etungsten etungsten mentioned this pull request Apr 15, 2020
Adds a new migration to migrate the default admin host-container version
from v0.4.0 to v0.5.0.
@etungsten
Copy link
Contributor Author

Rebased on develop to get migration build fix.

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.

🌺

@etungsten etungsten merged commit c79260a into bottlerocket-os:develop Apr 16, 2020
@etungsten etungsten deleted the admin-ctr-migration branch April 16, 2020 16:55
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.

Migration for new upcoming admin container version
3 participants