Updates for rebase of runtime stack change

Signed-off-by: David Gilligan-Cook <dcook@imageworks.com>

crates/spfs-cli/main/src/cmd_run.rs

@@ -31,10 +31,10 @@ pub struct ExternalData {
     /// `spfs info` with the `--get <KEY>` 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 <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<Vec<KeyValuePair>> {
         let mut data: Vec<KeyValuePair> = 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?;

crates/spfs-cli/main/src/cmd_run_test.rs

@@ -40,7 +40,9 @@ fn test_cmd_run_create_external_data(#[case] values: Vec<String>) {
         .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));
 }
 

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<Option<String>> {
-        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<BTreeMap<String, String>> {
         let mut data: BTreeMap<String, String> = 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) => {

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<encoding::Digest> = 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<encoding::Digest> = 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();