Host container migrations #294

vyaghras · 2024-12-04T21:23:25Z

Issue number:

Closes #

Description of changes:
In case of host container upgrade in BoB repo, we need to write migrations. By this PR we are incorperating the concept of weak and strong setting, where a weak setting will be deleted on upgrade and downgrade and default setting will be populated from the settings-defaults.

migrator: remove all the weak setting and settings-generator
apiserver: add version 2 for /tx and /metadata/settings-generator 
apiclient: update README to document version 2 of /tx API 
datastore: support committing metadata from transactions 
models: add strength enum and Settings generator struct 
constants: Use version 2 API to get setting-generators 
openapi: Add version 2 for /tx and /metadata/settings-generator 
settings-committer: change transaction API endpoint to /v2/tx 
sundog: Parse settings generators as a table 
storewolf: enable parsing setting-generator metadata as table

Testing done:
Refer https://gist.github.com/vyaghras/cc7391ade3b276b223d1814a8770eea7

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.

For new setting-generator we process following fields from the defaults TOML file: - command - strength - skip-if-populated This needs to be saved as json in the filesystem.

The get API on route /tx is used to fetch the pending settings in pending transaction. This is used in setting-commiter to just log the pending settings before committing them. The version 2 of this api will also return the pending metadata.

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.

Earlier we used to have setting generator as a string, but we have now changed it to a struct containing the command, strength and skip-if-populated. Hence after requesting the settings generators from api, we need to change these in to Setting generator struct instance. We will use default strength strong and skip_if_populated true if the setting generator is a string and use what has been provided in the response otherwise. Then after running the setting generator, we will send the weak and strong setting settings separately to process them using api.

We will remove the weak settings and settings genrators using the migrator of the destination migrator. The storewolf do not repopulate any metadata or setting, if it is already presnt. As migrator runs before storewolf, if we will delete the weak settings and setting-generators in migrator, storewolf can populate the setting-generator from defaults and ssundog will populate the new source using the new setting generator from default.

- `/v2/tx`: We will also return the pending metadata along with pending settings(that we used to return in version 1). As the return struct is changing, we are doing versioning of the API. - `v2/metadata/settings-generators`: We will also return the settings-generators(that contains strength and are saved as JSON object in datastore). As we just used to return arrays and string earlier as response for this API, returning object may break the existing usage. Hence we need to version this API. - `/settings`(patch and patchkeypair): For both of these we will set strength metadata. The default strength used is strong. - `/tx/commit` and `/tx/commit_and_apply`: We will commit the pending metadata(that just accounts for strength metadata for now) as part of commit. No changes has been done in apply.

vyaghras · 2024-12-05T01:47:13Z

☝️ Fixed failing tests

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

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

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

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

cbgbt · 2024-12-06T21:11:01Z

Do you mind adding more detail to the PR description about the intent of this changeset? Moving the testing done to a gist may also help to make the PR description a bit less cluttered.

cbgbt

Really nice work!

I'm still working through the final two commits, but have run out of time at the moment. Will follow up with any additional feedback.

cbgbt · 2024-12-06T23:45:09Z

sources/api/storewolf/src/main.rs

+                // This is special case to handle metadata as table that contains
+                // "command": "command",
+                // "strength": "weak",
+                // "skip-if-populated": true
+                if table.contains_key("command")
+                    && table.contains_key("strength")
+                    && table.contains_key("skip-if-populated")


We had spoken previously about allowing metadata objects that aren't related to "setting-generators" by e.g. using something like metadata-table to prefix the keys.

It seems like this approach focuses more heavily on only supporting the setting generator case, though.

Is there a reason we shouldn't do the more generic solution?

If we do indeed go with the more specific solution, can we make it more clear here why we've chosen these keys? e.g. the comment should probably explain that these are the setting-generator keys, and we should ensure that the metadata itself is referring to a setting generator.

Spoke with @vyaghras in person about this. CC @bcressey as well so we can reconcile opinions here.

To recap: the problem we are solving is that when storewolf encounters a TOML table, it recursively descends into the settings or metadata within, treating all other TOML value types as "leaves" which will be individually stored as metadata. With the changes to setting-generators, we wish to represent them as objects and so need storewolf to be able to distinguish between "leaf" objects that should be stored, and "branch" objects into which it should recurse.

The proposed solutions that I'm aware of are:

Add a new key type metadata-table for storewolf, indicating that the TOML table should be treated as a leaf.

Pros: Unambiguous, Generic

Cons: Poor clarity: metadata and settings both align exactly with datastore classifications. Adding metadata-table is less clear.

(Implemented here) search through TOML tables for keys that distinguish specific metadata tables as being setting-generators, then store those as objects.

Pros: Preserves datastore semantic clarity

Cons: Unambiguous, but only if you know the rules on what keys are special. Future metadata objects would need special consideration

Some additional ideas I discussed with @vyaghras were:

Mark leaf objects in the config with a special key, like is-object: true. Storewolf can discard this key after using it to identify the object as a leaf.

Pros: Unambiguous, Generic, Maintains datastore semantic clarity

Cons: is-object would steal keyspace that is otherwise reserved for settings data.

Add a postfix or suffix to the TOML table key which is otherwise "illegal" in the keyspace which indicates that a table is a leaf. e.g. [metadata.top-level.sub-level~] or [~metadata.top-level.sub-level].

Pros: Unambiguous, Generic, maintains datastore semantic clarity

Cons: ~ is somewhat opaque.

For what it's worth, I'm partial to the last suggestion because it keeps the keyspace open everywhere else for the data being stored, but curious what ya'll think. I'm also open to a suggestion that I'm overcomplicating this 😆

cbgbt · 2024-12-06T23:47:26Z

sources/api/storewolf/src/main.rs

-                    let mut path = path.clone();
-                    path.push(key.to_string());
-                    to_process.push((path, val));
+                // This is special case to handle metadata as table that contains


Could we add unit tests for the parent function to assert that we're emitting the correct metadata? I think this is especially important now that we are treating a very specific case differently than others.

cbgbt · 2024-12-06T23:52:41Z

sources/api/storewolf/src/main.rs

+    // If we have setting-generator metadata as table like
+    // [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.13'"


nit: I think this comment is a bit distracting here, since it doesn't influence the implementation. Something more generic like "metadata is expressed as arbitrary JSON values" may be better while still getting the point across.

cbgbt · 2024-12-06T23:55:56Z

sources/api/openapi.yaml

@@ -494,6 +515,29 @@ paths:
                  type: string
        500:
          description: "Server error"
+  /v2/metadata/setting-generators:
+    get:
+      summary: "Get programs with strength needed to generate settings"


nit:

Suggested change

summary: "Get programs with strength needed to generate settings"

summary: "Get programs to generate settings, and the strength of the generated settings"

cbgbt · 2024-12-07T00:01:14Z

sources/api/openapi.yaml

+          content:
+            application/json:
+              schema:
+                $ref: #/components/schemas/SettingsResponseWithMetadata"


This component doesn't seem to be defined.

cbgbt · 2024-12-07T00:45:39Z

sources/api/datastore/src/filesystem.rs

+                            warn!("Trying to change the strength from strong to weak for key: {}, Operation ignored", key.name());
+                            continue;


I think that this should cause the transaction to fail.

We would not like to stop the transaction in between, hence we can just log it and keep going with the rest of the transaction.

Left in another comment, but can we make the decision on whether or not to proceed before we start writing anything? That way it would not be possible to commit a partial transaction.

cbgbt · 2024-12-07T00:55:02Z

sources/api/sundog/src/main.rs

@@ -10,6 +10,7 @@ The output is collected and sent to a known Bottlerocket API server endpoint.
 #[macro_use]


Nit for the commit message:

sundog: Parse settings generators as a table

I might change this to something like:

sundog: Parse setting generators using model

cbgbt · 2024-12-07T01:17:37Z

sources/api/sundog/src/main.rs

+    let response: HashMap<String, Value> = serde_json::from_str(&response_body)
        .context(error::ResponseJsonSnafu { method: "GET", uri })?;
+
+    get_setting_generator_from_api_response(response)
+}
+
+fn get_setting_generator_from_api_response(


I'm OK with this as-is, but there's an example in the serde docs for parsing a struct from input which could either be a struct or a string.

If implemented this way, you could leave the details of parsing settings generators to the model, and then just directly parse into HashMap<String, SettingsGenerator>.

cbgbt · 2024-12-07T01:21:23Z

sources/api/sundog/src/main.rs

+        if generator_object.skip_if_populated
+            && populated_settings
+                .iter()
+                .any(|k| k.starts_with_segments(setting.segments()))
        {
            debug!("Setting '{}' is already populated, skipping", setting);


It might be a good idea here to also emit a log message if the value is populated but skip_if_populated is true. One succinct way you could do that would be to match against a tuple of the values:

match (generator_object.skip_if_populated, populated_settings) { (true, true) => { debug!("Setting '{}' is already populated, skipping", setting); continue; }, (false, true) => { debug!("Setting '{}' is already populated, but configured to be overwritten", setting); } _ => (), }

cbgbt · 2024-12-07T01:24:06Z

sources/api/sundog/src/main.rs

-    set_settings(&args.socket_path, settings).await?;
+    set_settings(&args.socket_path, weak_settings, Strength::Weak).await?;
+    set_settings(&args.socket_path, strong_settings, Strength::Strong).await?;


nit: Should we check that the settings aren't empty before submitting them?

It is struct of type model::Settings, hence can not check if it is empty or not?

Is it something we could check prior to creating the model::Settings? Perhaps we could return Option<model::Settings>.

cbgbt · 2024-12-10T19:48:45Z

sources/api/migration/migrator/src/main.rs

+    let datastore = match remove_weak_settings(&args.datastore_path, &args.migrate_to_version).await
+    {
+        Ok(datastore) => datastore,
+        Err(e) => {
+            eprintln!("{}", e);
+            process::exit(1);
+        }
+    };
+
+    // change datastore path in args
+    let mut args = args;
+    args.datastore_path = datastore;


I'd rather see this moved to run() -- It's pretty common for main() to handle argument parsing and logging setup, and then delegate the rest of the programs functionality to some other function.

My specific problem here is that overwriting args could make for very confusing debug logging later on, if the software reports args that are different to the actual command line args for the program.

cbgbt · 2024-12-10T19:50:33Z

sources/api/migration/migrator/src/main.rs

+    Ok(target_datastore)
+}
+
+fn remove_weak_setting_from_datastore(datastore: &mut DataStoreData) -> Result<DataStoreData> {


Do you mind adding docstring comments to remove_weak_settings and remove_weak_settings_from_datastore? The intent (and difference between both) isn't clear without reading the function source.

cbgbt · 2024-12-10T19:54:36Z

sources/api/migration/migrator/src/main.rs

+async fn remove_weak_settings<P>(datastore_path: P, new_version: &Version) -> Result<PathBuf>
+where
+    P: AsRef<Path>,
+{
+    // We start with the given source_datastore, updating this to delete weak settings and setting-generators
+    let source_datastore = datastore_path.as_ref();
+    // We create a new data store (below) to serve as the target for update.  (Start at
+    // source just to have the right type)
+    let target_datastore = new_datastore_location(source_datastore, new_version)?;
+
+    let source = DataStoreImplementation::new(source_datastore);
+    let mut target = DataStoreImplementation::new(&target_datastore);


I'd like to see unit tests of the "weak" removal functionality.

One way to do this would be to delegate the rest of remove_weak_settings (after the selection that I've left my comment on) to another function, something like:

/// Copies the contents of `source` to `target`, excluding any "weak" settings. fn copy_without_weak_settings(source: impl Datastore, target: impl Datastore) { // the rest of the implementation }

Writing this against impl Datastore would let storewolf call it with the source/target that you've created here, but you could add tests which use the in-memory datastore implementation to validate that your logic is correct.

cbgbt · 2024-12-13T22:49:14Z

sources/api/sundog/src/main.rs

-    set_settings(&args.socket_path, settings).await?;
+    set_settings(&args.socket_path, weak_settings, Strength::Weak).await?;
+    set_settings(&args.socket_path, strong_settings, Strength::Strong).await?;


Is it something we could check prior to creating the model::Settings? Perhaps we could return Option<model::Settings>.

cbgbt · 2024-12-14T00:38:05Z

sources/api/datastore/src/filesystem.rs

+        for (key, value) in transaction_metadata {
+            for (metadata_key, metadata_value) in value {
+                // For now we are only processing the strength metadata from pending
+                // transaction to live
+                if metadata_key.name() != "strength" {
+                    continue;
+                }


To elaborate on this one a bit more -- the datastore currently has no concept of "strength", only data and metadata. The concept of strength is a concept in Bottlerocket that happens to be stored in metadata. This is why I think it makes sense to implement the strength checks at the API/controller layer.

I'm not completely opposed to making commits "strength-aware" at the datastore layer. But if we do it, we need to move the implementation "up" one more level of abstraction. To be specific, right now the checks are only implemented for "commits" of the Filesystem implementation, but it should work for both the Filesystem and InMemory implementation.

Some of the shared functionality is implemented at the Trait level as default methods -- perhaps this could be one way to share the functionality?

cbgbt · 2024-12-14T00:57:34Z

sources/api/apiserver/src/server/mod.rs

+fn strength(query: &web::Query<HashMap<String, String>>) -> Strength {
+    match query.get("strength") {
+        Some(strength) => strength.parse::<Strength>().unwrap_or(Strength::Strong),
+        None => Strength::Strong,
+    }
+}
+


If the given query string isn't strong or weak, we should probably throw an error.

Suggested change

fn strength(query: &web::Query<HashMap<String, String>>) -> Strength {

match query.get("strength") {

Some(strength) => strength.parse::<Strength>().unwrap_or(Strength::Strong),

None => Strength::Strong,

}

}

fn strength(query: &web::Query<HashMap<String, String>>) -> Result<Strength> {

Ok(query.get("strength")

.map(str::parse::<Strength>)

.transpose()

.context(SomethingSomethingSomething)?

.unwrap_or_default())

}

cbgbt · 2024-12-14T01:16:43Z

sources/api/apiserver/src/server/mod.rs

+    let mut metadata = HashMap::new();
+    for (key, value) in transaction_metadata {
+        let mut inner_map = HashMap::new();
+        for (inner_key, inner_value) in value {
+            inner_map.insert(inner_key.name().clone(), inner_value);
+        }
+        metadata.insert(key.name().clone(), inner_map);
+    }


Carrying around deeply nested hashmaps and performing "on-the-spot" conversions like this can get messy fast.

Suggestion:

Make a type like SettingsMetadata which implements From<HashMap<Key, HashMap<Key, Value>>>

SettingsMetadata can #[derive(Serialize, Deserialize)] and use the #[serde(transparent)] to expose the inner nested hashmap.

Modify get_transaction_metadata to return a SettingsMetadata struct.

cbgbt · 2024-12-14T01:26:25Z

sources/api/apiserver/src/server/mod.rs

+            // We know it is not possible, as in default we set the setting-generator
+            // as string or object
+            _ => {
+                info!("Invalid value type for key '{}': {:?}", key, value);


We should log this as error!

cbgbt · 2024-12-14T01:28:40Z

sources/api/apiserver/src/server/mod.rs

+    for (key, value) in metadata_for_keys.iter() {
+        match value {


Mentioned in another comment, but if we allowed SettingsGenerator to successfully deserialize from a String or an Object, we could remove a branch and make this much simpler -- just parse the returned metadata into a SettingsGenerator

cbgbt · 2024-12-14T01:37:42Z

sources/api/apiserver/src/server/controller.rs

+
+    info!("Writing Metadata to data store");
+    match strength {
+        Strength::Strong => {
+            // Get keys in the request


The checks against Live happening in this function won't work -- we need to handle checks against strength changes at commit time. If we do it at set, another transaction could come first and change our assumptions, leading to a TOCTOU bug.

vyaghras added 7 commits December 4, 2024 19:56

storewolf: enable parsing setting-generator metadata as table

470ddc3

For new setting-generator we process following fields from the defaults TOML file: - command - strength - skip-if-populated This needs to be saved as json in the filesystem.

openapi: Add version 2 for /tx and /metadata/settings-generator

f7aa372

constants: Use version 2 API to get setting-generators

5c9860b

models: add strength enum and Settings generator struct

a72f6bd

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

d9293d3

vyaghras force-pushed the host_container_migrations branch from 8c3e361 to e997915 Compare December 5, 2024 01:41

vyaghras added 3 commits December 5, 2024 01:46

vyaghras force-pushed the host_container_migrations branch from e997915 to 4d9addf Compare December 5, 2024 01:46

vyaghras mentioned this pull request Dec 6, 2024

Remove host container migration bottlerocket-os/bottlerocket#4324

Open

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

cbgbt reviewed Dec 7, 2024

View reviewed changes

cbgbt reviewed Dec 14, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Host container migrations #294

Host container migrations #294

vyaghras commented Dec 4, 2024 •

edited

Loading

vyaghras commented Dec 5, 2024

cbgbt commented Dec 6, 2024

cbgbt left a comment

cbgbt Dec 6, 2024

cbgbt Dec 9, 2024 •

edited

Loading

cbgbt Dec 6, 2024

cbgbt Dec 6, 2024

cbgbt Dec 6, 2024

cbgbt Dec 7, 2024

cbgbt Dec 7, 2024

vyaghras Dec 12, 2024

cbgbt Dec 14, 2024

cbgbt Dec 7, 2024

cbgbt Dec 7, 2024

cbgbt Dec 7, 2024

cbgbt Dec 7, 2024

vyaghras Dec 12, 2024

cbgbt Dec 13, 2024

cbgbt Dec 10, 2024

cbgbt Dec 10, 2024

cbgbt Dec 10, 2024

cbgbt Dec 13, 2024

cbgbt Dec 14, 2024

cbgbt Dec 14, 2024

cbgbt Dec 14, 2024

cbgbt Dec 14, 2024

cbgbt Dec 14, 2024

cbgbt Dec 14, 2024

	summary: "Get programs with strength needed to generate settings"
	summary: "Get programs to generate settings, and the strength of the generated settings"

		warn!("Trying to change the strength from strong to weak for key: {}, Operation ignored", key.name());
		continue;

		@@ -10,6 +10,7 @@ The output is collected and sent to a known Bottlerocket API server endpoint.
		#[macro_use]

-fn strength(query: &web::Query<HashMap<String, String>>) -> Strength {
-    match query.get("strength") {
-        Some(strength) => strength.parse::<Strength>().unwrap_or(Strength::Strong),
-        None => Strength::Strong,
-    }
-}
+fn strength(query: &web::Query<HashMap<String, String>>) -> Result<Strength> {
+    Ok(query.get("strength")
+        .map(str::parse::<Strength>)
+        .transpose()
+        .context(SomethingSomethingSomething)?
+        .unwrap_or_default())
+}

Host container migrations #294

Are you sure you want to change the base?

Host container migrations #294

Conversation

vyaghras commented Dec 4, 2024 • edited Loading

vyaghras commented Dec 5, 2024

cbgbt commented Dec 6, 2024

cbgbt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbgbt Dec 9, 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

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

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

vyaghras commented Dec 4, 2024 •

edited

Loading

cbgbt Dec 9, 2024 •

edited

Loading