Remove host container migration #4324

vyaghras · 2024-12-06T17:51:43Z

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.

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

…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

…r to table Update the source from string like: public.ecr.aws/bottlerocket/bottlerocket-admin:v0.11.14 To setting generator as object, that contains: - command - strength - skip-if-populated

We need to add these to match changes in Bottlerocket-core-kit Refer commit: bottlerocket-os/bottlerocket-core-kit@a72f6bd PR: bottlerocket-os/bottlerocket-core-kit#294

… struct - RemoveWeakSettingsMigration: 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 strength and setting generator metadata. - MetadataStringToStructMigration: migration to change host-containers source metadata to struct from string. - ReplaceSettingWithSettingGeneratorMigration: Migration to replace a setting with setting generator. This migration will be used to convert a strong setting to a weak setting.

…bject This migration will change the public control host-container source to setting-generator metadata(that will generate source using sundog) on forward migration. In backward migration, it will reset the value of the source as string.

This migration will change the aws admin host-container source setting-generator from string to object setting-generator metadata (that will generate source using sundog) on forward migration. We will also add a strength file with value "weak". In backward migration, it will reset the value of the source setting generator as string and delete the strength metadata.

…bject This migration will change the public control host-container source to setting-generator metadata(that will generate source using sundog) on forward migration. In backward migration, it will reset the value of the source as string.

We need this migration that will delete all the weak settings and setting-generators.

This migration will change the public admin host-container source to setting-generator metadata(that will generate source using sundog) on forward migration. In backward migration, it will reset the value of the source as string.

vyaghras · 2024-12-06T19:02:53Z

☝️ Changed the Version in Twoliter.toml

bcressey · 2024-12-18T21:23:35Z

sources/shared-defaults/aws-host-containers.toml

+command = "schnauzer-v2 render --requires 'aws@v1(helpers=[ecr-prefix])' --template '{{ ecr-prefix settings.aws.region }}/bottlerocket-control:v0.7.18'"
+strength = "weak"
+skip-if-populated = true
+


Suggested change

bcressey · 2024-12-18T21:24:49Z

sources/shared-defaults/aws-host-containers.toml

+[metadata.settings.host-containers.admin.source.setting-generator]
+command = "schnauzer-v2 render --requires 'aws@v1(helpers=[ecr-prefix])' --template '{{ ecr-prefix settings.aws.region }}/bottlerocket-admin:v0.11.14'"
+strength = "weak"
+skip-if-populated = true


Shouldn't skip-if-populated default to true? Can we omit this?

bcressey · 2024-12-18T21:25:43Z

sources/api/datastore/src/error.rs

+    #[snafu(display("Unable to de-serialize data: {}", source))]
+    DeSerialize { source: serde_json::Error },


Suggested change

#[snafu(display("Unable to de-serialize data: {}", source))]

DeSerialize { source: serde_json::Error },

#[snafu(display("Unable to deserialize data: {}", source))]

Deserialize { source: serde_json::Error },

bcressey · 2024-12-18T21:32:26Z

sources/settings-migrations/v1.29.0/change-public-control-container-to-set-gen/src/main.rs

+        setting: "settings.host-containers.control.source",
+        old_val: OLD_CONTROL_CTR,
+        new_val: NEW_CONTROL_CTR,
+        setting_gen_command: "emitter public.ecr.aws/bottlerocket/bottlerocket-control:v0.7.17",


I don't understand where emitter comes from.

bcressey · 2024-12-18T21:41:18Z

sources/settings-migrations/v1.29.0/change-public-control-container-to-set-gen/src/main.rs

+    migrate(ReplaceSettingWithSettingGeneratorMigration {
+        setting: "settings.host-containers.control.source",
+        old_val: OLD_CONTROL_CTR,
+        new_val: NEW_CONTROL_CTR,


It's confusing to me to mix settings changes with metadata changes.

I would model this with a pair of migration types:

RemoveSchnauzerMigration - like ReplaceSchnauzerMigration, but removes the setting on upgrade if it matches. No-op on downgrade, like RemoveMetadataMigration.

Maybe ResetMetadataMigration where the upgrade and downgrade action is the same, and we delete the metadata both ways, so that storewolf fixes it up? In effect this would combine the behaviors of AddMetadataMigration and RemoveMetadataMigration.

Edit: having seen the RemoveWeakSettingsMigration - I think you want a similar ResetMetadataMigration that clears out all metadata for all settings. Otherwise it'll be easy to lose track of which settings might have metadata that persists on downgrade.

bcressey · 2024-12-18T21:53:38Z

sources/api/migration/migration-helpers/src/common_migrations.rs

+        Ok(input)
+    }
+
+    /// No work to do on backward migrations, copy the same datastore


Comment seems incorrect?

bcressey · 2024-12-18T21:57:59Z

sources/api/migration/migration-helpers/src/common_migrations.rs

+                inner_map.remove("strength");
+                input.metadata.insert(key.clone(), inner_map);
+            }
+            input.data.remove(&key);


nit: add a comment that we're also removing the setting, not just the "strength" metadata

bcressey · 2024-12-18T21:59:00Z

sources/api/migration/migration-helpers/src/common_migrations.rs

+        // Remove all the setting generators with weak strength
+        for (key, inner_map) in input.metadata.clone() {
+            if let Some(Value::Object(_)) = inner_map.get("setting-generator") {
+                // We need to remove the setting-generators that are an object
+                // because the API in destination version is not aware about the
+                // object setting generator.
+                let mut inner_map = inner_map.clone();
+                inner_map.remove("setting-generator");
+                input.metadata.insert(key.clone(), inner_map);
+            }
+        }


I don't think we should touch settings generators here

Suggested change

// Remove all the setting generators with weak strength

for (key, inner_map) in input.metadata.clone() {

if let Some(Value::Object(_)) = inner_map.get("setting-generator") {

// We need to remove the setting-generators that are an object

// because the API in destination version is not aware about the

// object setting generator.

let mut inner_map = inner_map.clone();

inner_map.remove("setting-generator");

input.metadata.insert(key.clone(), inner_map);

}

}

cbgbt · 2025-01-16T19:28:49Z

sources/shared-defaults/public-host-containers.toml

 [settings.host-containers.admin]
 enabled = false
 superpowered = true
-source = "public.ecr.aws/bottlerocket/bottlerocket-admin:v0.11.14"
+
+[metadata.settings.host-containers.admin.source.setting-generator]
+command = "schnauzer-v2 render --template 'public.ecr.aws/bottlerocket/bottlerocket-admin:v0.11.14'"
+strength = "weak"
+skip-if-populated = true


Since these are static strings and don't require templates, can we disregard the setting generator and accomplish the same thing like this?

Suggested change

[settings.host-containers.admin]

enabled = false

superpowered = true

source = "public.ecr.aws/bottlerocket/bottlerocket-admin:v0.11.14"

[metadata.settings.host-containers.admin.source.setting-generator]

command = "schnauzer-v2 render --template 'public.ecr.aws/bottlerocket/bottlerocket-admin:v0.11.14'"

strength = "weak"

skip-if-populated = true

[settings.host-containers.admin]

enabled = false

superpowered = true

source = "public.ecr.aws/bottlerocket/bottlerocket-admin:v0.11.14"

[metadata.settings.host-containers.admin.source]

strength = "weak"

cbgbt · 2025-01-16T19:29:25Z

sources/shared-defaults/public-host-containers.toml

+
+[metadata.settings.host-containers.control.source.setting-generator]
+command = "schnauzer-v2 render --template 'public.ecr.aws/bottlerocket/bottlerocket-control:v0.7.18'"
+strength = "weak"
+skip-if-populated = true


Same feedback as the admin container source for public ECR.

cbgbt · 2025-01-16T19:40:30Z

sources/api/migration/migration-helpers/src/common_migrations.rs

+// We use this migration to change the aws host-containers setting-generator to struct from string.
+// A metadata strength as `weak`, shows that associated setting is a weak setting.


nit: use docstring comments

Less nit: A brief amount of context on string vs struct is useful here, since we're adding this to common_migrations and theoretically someone will need to read this comment and code again in the future.

e.g.

Suggested change

// We use this migration to change the aws host-containers setting-generator to struct from string.

// A metadata strength as `weak`, shows that associated setting is a weak setting.

/// We use this migration to change the aws host-containers setting-generator to struct from string.

/// A metadata strength as `weak`, shows that associated setting is a weak setting.

///

/// String `setting-generators` only provide the command to run, using the default

/// values for `strength` and `skip-if-populated` ("weak", and "true").

/// Struct generators allow customization of those fields.

cbgbt · 2025-01-16T19:42:54Z

sources/api/migration/migration-helpers/src/common_migrations.rs

+pub struct MetadataStringToStructMigration {
+    pub setting: &'static str,
+    pub old_cmdline: &'static str,
+    pub new_cmdline: &'static str,
+    pub metadata_type: &'static str,
+    pub binary_for_generator: &'static str,
+    pub skip_if_populated: bool,
+}


This seems highly specific to setting-generators, I doubt we'd use it elsewhere. I'd suggest renaming it to be more specific and dropping metadata_type as an argument.

We're already using Shlex ("shell lexer") in this migrator. Do we need binary_for_generator if we can just parse it out? The binary should always be the first element of the lexed command line.

Finally, I wonder if the settings for the "new" generator should just be a SettingsGenerator struct?

vyaghras added 12 commits December 5, 2024 22:04

shared-defaults: change public host container source setting-generato…

b9fbb93

…r to table Update the source from string like: public.ecr.aws/bottlerocket/bottlerocket-admin:v0.11.14 To setting generator as object, that contains: - command - strength - skip-if-populated

apiclient: update README to document version 2 of /tx API

5aa44d3

models: Add Strength enum and Setting-Generator struct

52fa106

We need to add these to match changes in Bottlerocket-core-kit Refer commit: bottlerocket-os/bottlerocket-core-kit@a72f6bd PR: bottlerocket-os/bottlerocket-core-kit#294

migrations: remove all the weak setting on downgrade

e787092

We need this migration that will delete all the weak settings and setting-generators.

release: update Bottlerocket release version to 1.29.0

b77efc3

vyaghras force-pushed the remove_host_container_migration branch from f3531f7 to b77efc3 Compare December 6, 2024 19:02

vyaghras requested review from bcressey and cbgbt December 6, 2024 20:57

bcressey reviewed Dec 19, 2024

View reviewed changes

vyaghras mentioned this pull request Jan 2, 2025

Add default bootstrap container #4349

Draft

7 tasks

cbgbt reviewed Jan 16, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove host container migration #4324

Remove host container migration #4324

vyaghras commented Dec 6, 2024

vyaghras commented Dec 6, 2024 •

edited

Loading

bcressey Dec 18, 2024

bcressey Dec 18, 2024

bcressey Dec 18, 2024

bcressey Dec 18, 2024

bcressey Dec 18, 2024

bcressey Dec 18, 2024

bcressey Dec 18, 2024

bcressey Dec 18, 2024

cbgbt Jan 16, 2025

cbgbt Jan 16, 2025

cbgbt Jan 16, 2025 •

edited

Loading

cbgbt Jan 16, 2025

		#[snafu(display("Unable to de-serialize data: {}", source))]
		DeSerialize { source: serde_json::Error },

		// We use this migration to change the aws host-containers setting-generator to struct from string.
		// A metadata strength as `weak`, shows that associated setting is a weak setting.

-// We use this migration to change the aws host-containers setting-generator to struct from string.
-// A metadata strength as `weak`, shows that associated setting is a weak setting.
+/// We use this migration to change the aws host-containers setting-generator to struct from string.
+/// A metadata strength as `weak`, shows that associated setting is a weak setting.
+///
+/// String `setting-generators` only provide the command to run, using the default
+/// values for `strength` and `skip-if-populated` ("weak", and "true").
+/// Struct generators allow customization of those fields.

Remove host container migration #4324

Are you sure you want to change the base?

Remove host container migration #4324

Conversation

vyaghras commented Dec 6, 2024

vyaghras commented Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbgbt Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vyaghras commented Dec 6, 2024 •

edited

Loading

cbgbt Jan 16, 2025 •

edited

Loading