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 host container migration #4324

Merged

Conversation

vyaghras
Copy link
Contributor

@vyaghras vyaghras commented Dec 6, 2024

Issue number:

Closes #

Description of changes:

migrations: remove all the weak setting on downgrade 
migrations: Change public control host-container source metadata to object 
migrations: Change aws admin host-container source metadata to object 
migrations: Change public control host-container source metadata to object 
migrations: Change public admin host-container source metadata to object 
common_migrations: add new migrations to handle settings-generator as struct 
models: Add Strength enum and Setting-Generator struct 
apiclient: update README to document version 2 of /tx API 
shared-defaults: change public host container source setting-generator to table 
shared-defaults: change aws host container source setting-generator to table 
datastore: changes to match changes in Core kit datastore

Testing done:
Refer to testing in bottlerocket-os/bottlerocket-core-kit#294

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.

@vyaghras vyaghras force-pushed the remove_host_container_migration branch from f3531f7 to b77efc3 Compare December 6, 2024 19:02
@vyaghras
Copy link
Contributor Author

vyaghras commented Dec 6, 2024

☝️ Changed the Version in Twoliter.toml

@vyaghras vyaghras requested review from bcressey and cbgbt December 6, 2024 20:57
@vyaghras vyaghras mentioned this pull request Jan 2, 2025
7 tasks
@vyaghras vyaghras force-pushed the remove_host_container_migration branch 10 times, most recently from e9f7de6 to d1a7c76 Compare January 27, 2025 17:58
@vyaghras vyaghras requested review from cbgbt and bcressey January 27, 2025 18:09
@vyaghras vyaghras force-pushed the remove_host_container_migration branch from d1a7c76 to afdc836 Compare January 28, 2025 17:11
Copy link
Contributor

@bcressey bcressey left a comment

Choose a reason for hiding this comment

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

Nice!

Copy link
Contributor

@cbgbt cbgbt left a comment

Choose a reason for hiding this comment

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

nit: I'd spend a moment to clean up the commit messages if possible -- misspelling of metadata in a few commit messages titles, and the line wrapping is inconsistent in the GitHub UI for some of the commit message details (though this is sometimes just the GitHub UI being strange.).

@vyaghras vyaghras force-pushed the remove_host_container_migration branch from afdc836 to 573de3a Compare January 29, 2025 17:16
@vyaghras vyaghras force-pushed the remove_host_container_migration branch 5 times, most recently from 9a5a389 to cbe0f74 Compare January 31, 2025 22:21
…o table

Update the source from string like:
schnauzer-v2 render --requires 'aws@v1(helpers=[ecr-prefix])' --template
'{{ ecr-prefix settings.aws.region }}/bottlerocket-admin:v0.11.14'

To setting generator as object, that contains:
- command
- strength
- skip-if-populated
@vyaghras vyaghras force-pushed the remove_host_container_migration branch from cbe0f74 to 6b7301e Compare February 5, 2025 19:49
@vyaghras vyaghras requested review from cbgbt and bcressey February 5, 2025 23:23
Copy link
Contributor

@bcressey bcressey left a comment

Choose a reason for hiding this comment

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

Looks good. Please double-check the commit message for this one as it refers to an earlier version of the changes:

common_migrations: add new migrations to handle settings-generator as struct

If we don't strictly need them, consider backing out the changes to ReplaceSchnauzerMigration, or else splitting them into a separate commit that explains why.

@vyaghras vyaghras force-pushed the remove_host_container_migration branch from 6b7301e to 0d0b88c Compare February 6, 2025 04:47
@vyaghras
Copy link
Contributor Author

vyaghras commented Feb 6, 2025

☝️ Reverted the changes in ReplaceSchnauzerMigration and updated the commit.

@vyaghras vyaghras force-pushed the remove_host_container_migration branch from 0d0b88c to 0344c32 Compare February 7, 2025 00:53
Copy link
Contributor

@cbgbt cbgbt left a comment

Choose a reason for hiding this comment

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

I separately sent a few nits for the commit messages, but the code looks good to me. Nice work!

Updated the `commit_transaction` function to enable committing
metadata from pending transactions. In commit transaction we will first
commit metadata and then pending keys to correctly perform the check to
identify if key exists or not.

The strength handling among pending and committed transaction is as:
If pending metadata is strong and committed metadata is weak, commit
the pending setting.

If pending metadata is weak, committed metadata is strong and
the setting is already available, do not downgrade from strong to
weak. Otherwise add the strength file with value as weak.

If pending and committed metadata are the same, no action is
performed.

Additionally, made minor changes to metadata functions for improved
access and flexibility:
- Introduced a `committed` field to dynamically access metadata in
 pending transactions.
- Replaced the hardcoded use of live committed metadata with this
 committed variable ans pass Committed::Live from previous usages.

Refer commit:
bottlerocket-os/bottlerocket-core-kit@20a435e
Refer PR:
bottlerocket-os/bottlerocket-core-kit#294
… struct

- RemoveMetadataAndWeakSettingsMigration: When we downgrade multiple
  version to a version where migrator is not aware of deleting the
  setting-generator as struct or the strength file. This migration
  will help us to delete the weak settings and all metadata.

- RemoveSchnauzerMigration: This will remove metadata(always) and data
  if the value matches with the value in existing datastore.

- RemoveMatchingString: This will remove the data when value is same
  as the old value for public host containers.
Add a setting-generator that will populate the host-containers
source and add strength=weak metadata if the setting is not
populated.
In this migration we will remove the data and metadata if it matches
with existing ones in datastore.We will rely on storewolf and sundog
to populate correct metadata and data.
In this migration we will remove setting-generator metadata and source
data if it matches with existing ones in datastore. We will rely on
storewolf and sundog to populate correct metadata and data.
In this migration we will remove the data if it matches
with existing ones in datastore. We will rely on storewolf
and sundog to populate correct data from defaults.
In this migration we will remove the data if it matches
with existing ones in datastore. We will rely on
Storewolf and Sundog to populate correct data.
On downgrade we will remove all the weak metadata and settings,
so that it can be populated from defaults.
@vyaghras vyaghras force-pushed the remove_host_container_migration branch from 0344c32 to 77f8444 Compare February 7, 2025 19:07
@vyaghras vyaghras merged commit 133e1e2 into bottlerocket-os:develop Feb 7, 2025
2 checks passed
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