CARL -> Fix Ethernet Bridges and Executors being duplicated in PeerCo…

…nfiguration every time the cluster got redeployed.
eclipse-opendut · Dec 20, 2024 · 6b1af81 · 6b1af81
1 parent bc7098c
commit 6b1af81
Show file tree

Hide file tree

Showing 5 changed files with 75 additions and 17 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/opendut-carl/src/actions/peers/assign_cluster.rs b/opendut-carl/src/actions/peers/assign_cluster.rs
@@ -57,15 +57,15 @@ pub async fn assign_cluster(params: AssignClusterParams) -> Result<(), AssignClu
                 .unwrap_or_default();
 
             for executor in Clone::clone(&peer_descriptor.executors).executors.into_iter() {
-                peer_configuration.insert(executor, ParameterTarget::Present); //TODO not always Present
+                peer_configuration.set(executor, ParameterTarget::Present); //TODO not always Present
             }
 
             {
                 let bridge = peer_descriptor.clone().network.bridge_name
                     .unwrap_or(options.bridge_name_default);
                 let bridge = EthernetBridge { name: bridge };
 
-                peer_configuration.insert(bridge, ParameterTarget::Present); //TODO not always Present
+                peer_configuration.set(bridge, ParameterTarget::Present); //TODO not always Present
             }
 
             peer_configuration
@@ -189,7 +189,7 @@ mod tests {
             executors: vec![],
             ethernet_bridges: vec![],
         };
-        peer_configuration.insert(EthernetBridge { name: NetworkInterfaceName::try_from("br-opendut-1")? }, ParameterTarget::Present);
+        peer_configuration.set(EthernetBridge { name: NetworkInterfaceName::try_from("br-opendut-1")? }, ParameterTarget::Present);
 
         let received = receiver.recv().await.unwrap()
             .message.unwrap();

diff --git a/opendut-types/Cargo.toml b/opendut-types/Cargo.toml
@@ -29,6 +29,7 @@ prost = { workspace = true }
 strum_macros = "0.25.3"
 
 [dev-dependencies]
+anyhow = { workspace = true }
 googletest = { workspace = true }
 indoc = { workspace = true }
 serde_yaml = { workspace = true }

diff --git a/opendut-types/src/peer/configuration/mod.rs b/opendut-types/src/peer/configuration/mod.rs
@@ -18,15 +18,80 @@ pub struct PeerConfiguration {
     //TODO migrate more parameters
 }
 impl PeerConfiguration {
-    pub fn insert<T: ParameterValue>(&mut self, value: T, target: ParameterTarget) {
+    pub fn set<T: ParameterValue>(&mut self, value: T, target: ParameterTarget) {
         let parameter = Parameter {
             id: value.parameter_identifier(),
             dependencies: vec![], //TODO
             target,
             value,
         };
 
-        T::peer_configuration_field(self)
-            .push(parameter); //TODO don't insert, if already contained? Or maybe set the previous element absent?
+        let parameters = T::peer_configuration_field(self);
+
+        parameters.retain(|existing_parameter| {
+            existing_parameter.id != parameter.id
+        });
+
+        parameters.push(parameter);
+    }
+}
+
+
+#[cfg(test)]
+mod tests {
+    use crate::peer::executor::{ExecutorId, ExecutorKind, ResultsUrl};
+    use crate::util::net::NetworkInterfaceName;
+    use super::*;
+
+    #[test]
+    fn should_replace_a_previous_parameter_when_it_is_set_another_time() -> anyhow::Result<()> {
+
+        let parameter_value = EthernetBridge { name: NetworkInterfaceName::try_from("br-opendut")? };
+
+        let mut testee = PeerConfiguration {
+            executors: vec![],
+            ethernet_bridges: vec![],
+        };
+        testee.set(parameter_value.clone(), ParameterTarget::Present);
+
+
+        testee.set(parameter_value.clone(), ParameterTarget::Present);
+        assert_eq!(testee.ethernet_bridges.len(), 1);
+
+        testee.set(parameter_value.clone(), ParameterTarget::Absent);
+        assert_eq!(testee.ethernet_bridges.len(), 1);
+        assert_eq!(testee.ethernet_bridges[0].target, ParameterTarget::Absent);
+
+        Ok(())
+    }
+
+
+    #[test]
+    fn should_update_the_value_of_a_parameter() -> anyhow::Result<()> {
+
+        let parameter_value = ExecutorDescriptor {
+            id: ExecutorId::random(),
+            kind: ExecutorKind::Executable,
+            results_url: Some(ResultsUrl::try_from("https://example.com")?),
+        };
+
+        let mut testee = PeerConfiguration {
+            executors: vec![],
+            ethernet_bridges: vec![],
+        };
+        testee.set(parameter_value.clone(), ParameterTarget::Present);
+
+
+        let expected = None;
+        let parameter_value = ExecutorDescriptor {
+            results_url: expected.clone(),
+            ..parameter_value
+        };
+
+        testee.set(parameter_value, ParameterTarget::Present);
+        assert_eq!(testee.executors.len(), 1);
+        assert_eq!(testee.executors[0].value.results_url, expected);
+
+        Ok(())
     }
 }
diff --git a/opendut-types/src/peer/configuration/parameter/value.rs b/opendut-types/src/peer/configuration/parameter/value.rs
@@ -40,16 +40,7 @@ pub trait ParameterValue: Any + Hash + Sized {
 
 impl ParameterValue for ExecutorDescriptor {
     fn parameter_identifier(&self) -> ParameterId {
-        let mut hasher = DefaultHasher::new(); //ID not stable across Rust releases
-        match &self.kind {
-            ExecutorKind::Executable => self.kind.hash(&mut hasher),
-            ExecutorKind::Container { name, .. } => name.hash(&mut hasher),
-        }
-        self.results_url.hash(&mut hasher);
-        let id = hasher.finish();
-
-        let id = Uuid::new_v5(&OPENDUT_UUID_NAMESPACE, &id.to_le_bytes());
-        ParameterId(id)
+        ParameterId(self.id.uuid)
     }
     fn peer_configuration_field(peer_configuration: &mut PeerConfiguration) -> &mut Vec<Parameter<Self>>  {
         &mut peer_configuration.executors
@@ -90,7 +81,7 @@ mod tests {
             results_url: None
         };
         let target = ParameterTarget::Present;
-        peer_configuration.insert(value.clone(), target);
+        peer_configuration.set(value.clone(), target);
 
         assert_eq!(peer_configuration.executors.len(), 1);