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

Host container migrations #294

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from
69 changes: 60 additions & 9 deletions sources/api/storewolf/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ use std::{env, fs, process};
use bottlerocket_modeled_types::SingleLineString;
use datastore::key::{Key, KeyType};
use datastore::serialization::{to_pairs, to_pairs_with_prefix};
use datastore::{self, DataStore, FilesystemDataStore, ScalarError};
use datastore::{self, Committed, DataStore, FilesystemDataStore, ScalarError};

// The default path to defaults.toml.
const DEFAULTS_TOML: &str = "/etc/storewolf/defaults.toml";
Expand Down Expand Up @@ -186,11 +186,50 @@ fn parse_metadata_toml(md_toml_val: toml::Value) -> Result<Vec<model::Metadata>>
// A table means there is more processing to do. Add the current
// key and value to the Vec to be processed further.
toml::Value::Table(table) => {
for (key, val) in table {
trace!("Found table for key '{}'", &key);
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
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

// "command": "command",
// "strength": "weak",
// "skip-if-populated": true
if table.contains_key("command")
&& table.contains_key("strength")
&& table.contains_key("skip-if-populated")
Comment on lines +189 to +195
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@cbgbt cbgbt Dec 9, 2024

Choose a reason for hiding this comment

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

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 😆

{
ensure!(
!path.is_empty(),
error::InternalSnafu {
msg: "Cannot create empty metadata data key - is root not a Table?"
.to_string()
}
);

// Get the metadata key from the end of the path
let md_key = path.pop().context(error::InternalSnafu {
msg: "parse_metadata_toml found empty 'path' in the to_process vec - is 'metadata' not a Table?",
})?;
let data_key = path.join(".");

trace!(
"Found metadata key '{}' for data key '{}'",
&md_key,
&data_key
);

// Ensure the metadata/data keys don't contain newline chars
let md =
SingleLineString::try_from(md_key).context(error::SingleLineStringSnafu)?;
let key = SingleLineString::try_from(data_key)
.context(error::SingleLineStringSnafu)?;
let val = toml::Value::Table(table);

// Create the Metadata struct
def_metadatas.push(model::Metadata { key, md, val })
} else {
for (key, val) in table {
trace!("Found table for key '{}'", &key);
let mut path = path.clone();
path.push(key.to_string());
to_process.push((path, val));
}
}
}

Expand Down Expand Up @@ -259,10 +298,10 @@ fn populate_default_datastore<P: AsRef<Path>>(
if live_path.exists() {
debug!("Gathering existing data from the datastore");
existing_metadata = datastore
.list_populated_metadata("", &None as &Option<&str>)
.list_populated_metadata("", &Committed::Live, &None as &Option<&str>)
.context(error::QueryMetadataSnafu)?;
existing_data = datastore
.list_populated_keys("", &datastore::Committed::Live)
.list_populated_keys("", &Committed::Live)
.context(error::QueryDataSnafu)?;
} else {
info!("Creating datastore at: {}", &live_path.display());
Expand Down Expand Up @@ -333,6 +372,18 @@ fn populate_default_datastore<P: AsRef<Path>>(
}

// If we have metadata, write it out to the datastore in Live state
// 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'"
Comment on lines +375 to +377
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

// strength = "weak"
// skip-if-populated = true
// We will have to write this in datastore as file with name source.settings-generator
// contents {
// "command" : "schnauzer-v2 render --requires 'aws@v1(helpers=[ecr-prefix])' --template '{{ ecr-prefix settings.aws.region }}/bottlerocket-admin:v0.11.13'",
// "strength" : "weak",
// "skip-if-populated" : "true"
// }
// we will parse such metadata differently in parse_metadata_toml function
if let Some(def_metadata_val) = maybe_metadata_val {
debug!("Serializing metadata and writing new keys to datastore");
// Create a Vec<Metadata> from the metadata toml::Value
Expand Down Expand Up @@ -385,7 +436,7 @@ fn populate_default_datastore<P: AsRef<Path>>(
for metadata in metadata_to_write {
let (md, key, val) = metadata;
datastore
.set_metadata(&md, &key, val)
.set_metadata(&md, &key, val, &Committed::Live)
.context(error::WriteMetadataSnafu)?;
}
}
Expand Down