From ed1e15578ac7f74a4cece721c951ad8c0bde45bd Mon Sep 17 00:00:00 2001 From: David Gilligan-Cook Date: Fri, 8 Dec 2023 14:13:07 -0800 Subject: [PATCH] Updates for rebase of runtime stack change Signed-off-by: David Gilligan-Cook --- crates/spfs-cli/main/src/cmd_run.rs | 25 +++++++++++++++--------- crates/spfs-cli/main/src/cmd_run_test.rs | 2 ++ crates/spfs/src/runtime/storage.rs | 23 ++++++++++------------ crates/spfs/src/runtime/storage_test.rs | 20 ++++++++++--------- 4 files changed, 39 insertions(+), 31 deletions(-) diff --git a/crates/spfs-cli/main/src/cmd_run.rs b/crates/spfs-cli/main/src/cmd_run.rs index 6eed8e117b..6f63d9bd7e 100644 --- a/crates/spfs-cli/main/src/cmd_run.rs +++ b/crates/spfs-cli/main/src/cmd_run.rs @@ -31,10 +31,10 @@ pub struct ExternalData { /// `spfs info` with the `--get ` or `--get-all` arguments /// /// External data is key/value string pairs separated by either an - /// equals sign or colon (--external-data name=value --external-data - /// other:value). Additionally, many pair of external data can be - /// specified at once in yaml or json format (--external-data '{name: - /// value, other: value}'). + /// equals sign or colon (--external-data name=value + /// --external-data other:value). Additionally, multiple pairs of + /// external data can be specified at once in yaml or json format + /// (--external-data '{name: value, other: value}'). /// /// External data can also be given in a json or yaml file via the /// `--external-data-file ` argument. If given, `--external-data` @@ -52,10 +52,10 @@ pub struct ExternalData { } impl ExternalData { - // Returns a list of external data key-value pairs gathered from all - // the external data related command line arguments. The same keys, - // and values, can appear multiple times in the list if specified - // multiple times in various command line arguments. + // Returns a list of external data key-value pairs gathered from + // all the external data related command line arguments. The same + // keys, and values, can appear multiple times in the list if + // specified multiple times in various command line arguments. pub fn get_data(&self) -> Result> { let mut data: Vec = Vec::new(); @@ -98,9 +98,14 @@ impl ExternalData { ) })?; + // Using insert allows data specified later on the command + // line to override earlier data. data.push((name.to_string(), value.to_string())); } + // Reversed so that the values from the later arguments take + // precedence over the earlier ones when they are added to the + // runtime. Ok(data) } } @@ -246,7 +251,9 @@ impl CmdRun { let data = self.external_data.get_data()?; if !data.is_empty() { tracing::debug!("with extra external data: {data:?}"); - for (key, value) in data { + // These are added in reverse order so that the ones + // specified later on the command line will take precedence. + for (key, value) in data.into_iter().rev() { runtime .add_external_data(key, value, config.filesystem.external_data_size_limit) .await?; diff --git a/crates/spfs-cli/main/src/cmd_run_test.rs b/crates/spfs-cli/main/src/cmd_run_test.rs index 52b60af3a1..f9dd3f671f 100644 --- a/crates/spfs-cli/main/src/cmd_run_test.rs +++ b/crates/spfs-cli/main/src/cmd_run_test.rs @@ -40,7 +40,9 @@ fn test_cmd_run_create_external_data(#[case] values: Vec) { .expect("unable to parse cmd_run --external-data values"); assert!(!result.is_empty()); assert!(result.len() == 2); + println!("r0: {:?}", result[0]); assert!(result[0] == (field1, value1)); + println!("r1: {:?}", result[1]); assert!(result[1] == (field2, value2)); } diff --git a/crates/spfs/src/runtime/storage.rs b/crates/spfs/src/runtime/storage.rs index a58a87048c..13e9408a00 100644 --- a/crates/spfs/src/runtime/storage.rs +++ b/crates/spfs/src/runtime/storage.rs @@ -729,15 +729,15 @@ impl Runtime { .save_external_data_layer(external_data_layer) .await?; - // The new external data is added to the top of the runtime's stack + // The new external data is added to the bottom of the runtime's stack self.push_digest(external_data_layer_digest); Ok(()) } /// Get the string data stored as extra data under the given key. pub async fn external_data(&self, key: &str) -> Result> { - for digest in self.status.stack.iter() { - if let Some(s) = self.storage.get_external_data(digest, key).await? { + for digest in self.status.stack.iter_bottom_up() { + if let Some(s) = self.storage.get_external_data(&digest, key).await? { return Ok(Some(s)); } } @@ -747,11 +747,8 @@ impl Runtime { /// Get the string data stored as extra data under the given key. pub async fn all_external_data(&self) -> Result> { let mut data: BTreeMap = BTreeMap::new(); - // Reversing the stack order ensures that external data layers - // using the same key override each other correctly (highest - // layer, i.e. last one added, takes overrides the others). - for digest in self.status.stack.iter().rev() { - let pairs = self.storage.get_external_key_value_pairs(digest).await?; + for digest in self.status.stack.iter_bottom_up() { + let pairs = self.storage.get_external_key_value_pairs(&digest).await?; for (key, value) in pairs { data.insert(key, value); } @@ -947,7 +944,7 @@ impl Runtime { /// and change the overlayfs options, but not save the runtime or /// update any currently running environment. Returns false /// if adding the digest had no change to the runtime stack. - pub fn push_digest(&mut self, digest: encoding::Digest) -> bool { + pub fn push_digest(&mut self, digest: Digest) -> bool { self.status.stack.push(digest) } @@ -1251,8 +1248,8 @@ impl Storage { for digest in digests_to_process.iter() { match self.inner.read_ref(digest.to_string().as_str()).await? { graph::Object::Platform(platform) => { - for reference in platform.stack.iter() { - next_iter_digests.push(*reference); + for reference in platform.stack.iter_bottom_up() { + next_iter_digests.push(reference); } } graph::Object::ExternalDataLayer(external_data_layer) => { @@ -1286,8 +1283,8 @@ impl Storage { for digest in digests_to_process.iter() { match self.inner.read_ref(digest.to_string().as_str()).await? { graph::Object::Platform(platform) => { - for reference in platform.stack.iter() { - next_iter_digests.push(*reference); + for reference in platform.stack.iter_bottom_up() { + next_iter_digests.push(reference); } } graph::Object::ExternalDataLayer(external_data_layer) => { diff --git a/crates/spfs/src/runtime/storage_test.rs b/crates/spfs/src/runtime/storage_test.rs index d252a7ac74..96c3d1bbf6 100644 --- a/crates/spfs/src/runtime/storage_test.rs +++ b/crates/spfs/src/runtime/storage_test.rs @@ -193,26 +193,28 @@ async fn test_storage_runtime_with_external_data(tmpdir: tempfile::TempDir) { .await .expect("failed to create runtime in storage"); - // Test - insert new data + // Test - insert data let key = "somefield".to_string(); let value = "somevalue".to_string(); assert!(runtime - .add_external_data(key.clone(), value, limit) + .add_external_data(key.clone(), value.clone(), limit) .await .is_ok()); - // Test - insert over existing data + // Test - insert some more data let value2 = "someothervalue".to_string(); assert!(runtime - .add_external_data(key.clone(), value2.clone(), limit) + .add_external_data(key.clone(), value2, limit) .await .is_ok()); - // Test - retrieve data + // Test - retrieve data - the first inserted data should be the + // what is retrieved because of how adding to the runtime stack + // works. let result = runtime.external_data(&key).await.unwrap(); assert!(result.is_some()); - assert!(value2 == *result.unwrap()); + assert!(value == *result.unwrap()); } #[rstest] @@ -239,7 +241,7 @@ async fn test_storage_runtime_with_nested_external_data(tmpdir: tempfile::TempDi // make a platform that contains the external data layer let layers: Vec = vec![external_data_layer.digest().unwrap()]; - let platform = Platform::new(layers.iter()).unwrap(); + let platform = Platform::from_iter(layers); repo.write_object(&Object::Platform(platform.clone())) .await .unwrap(); @@ -344,7 +346,7 @@ async fn test_storage_runtime_with_nested_external_data_all(tmpdir: tempfile::Te // make a platform with one external data layer let layers: Vec = vec![external_data_layer.digest().unwrap()]; - let platform = Platform::new(layers.iter()).unwrap(); + let platform = Platform::from_iter(layers); repo.write_object(&Object::Platform(platform.clone())) .await .unwrap(); @@ -355,7 +357,7 @@ async fn test_storage_runtime_with_nested_external_data_all(tmpdir: tempfile::Te platform.digest().unwrap(), external_data_layer2.digest().unwrap(), ]; - let platform2 = Platform::new(layers2.iter()).unwrap(); + let platform2 = Platform::from_iter(layers2); repo.write_object(&Object::Platform(platform2.clone())) .await .unwrap();