refactor(node-label): use specific types for store operations

PStor operations should use a specific type to allow for better expansion.

Signed-off-by: Tiago Castro <[email protected]>

control-plane/agents/src/bin/core/controller/resources/operations.rs

@@ -36,11 +36,11 @@ pub(crate) trait ResourceLabel {
         label: HashMap<String, String>,
         overwrite: bool,
     ) -> Result<Self::LabelOutput, SvcError>;
-    /// Remove label from the resource.
+    /// Remove the label key from the resource.
     async fn unlabel(
         &mut self,
         registry: &Registry,
-        label: String,
+        label_key: String,
     ) -> Result<Self::UnlabelOutput, SvcError>;
 }
 

control-plane/agents/src/bin/core/node/operations.rs

@@ -67,7 +67,7 @@ impl ResourceLabel for OperationGuardArc<NodeSpec> {
             .start_update(
                 registry,
                 &cloned_node_spec,
-                NodeOperation::Label(label, overwrite),
+                NodeOperation::Label((label, overwrite).into()),
             )
             .await?;
 
@@ -79,11 +79,15 @@ impl ResourceLabel for OperationGuardArc<NodeSpec> {
     async fn unlabel(
         &mut self,
         registry: &Registry,
-        label: String,
+        label_key: String,
     ) -> Result<Self::UnlabelOutput, SvcError> {
         let cloned_node_spec = self.lock().clone();
         let spec_clone = self
-            .start_update(registry, &cloned_node_spec, NodeOperation::Unlabel(label))
+            .start_update(
+                registry,
+                &cloned_node_spec,
+                NodeOperation::Unlabel(label_key.into()),
+            )
             .await?;
 
         self.complete_update(registry, Ok(()), spec_clone).await?;

control-plane/agents/src/bin/core/node/service.rs

@@ -130,16 +130,16 @@ impl NodeOperations for Service {
         let node = self.label(id, label, overwrite).await?;
         Ok(node)
     }
-    /// Remove the specified label from the node.
-    async fn unlabel(&self, id: NodeId, label: String) -> Result<Node, ReplyError> {
-        if label.is_empty() {
+    /// Remove the specified label key from the node.
+    async fn unlabel(&self, id: NodeId, label_key: String) -> Result<Node, ReplyError> {
+        if label_key.is_empty() {
             return Err(SvcError::InvalidLabel {
-                labels: label,
+                labels: label_key,
                 resource_kind: ResourceKind::Node,
             }
             .into());
         }
-        let node = self.unlabel(id, label).await?;
+        let node = self.unlabel(id, label_key).await?;
         Ok(node)
     }
 }

control-plane/agents/src/bin/core/node/specs.rs

@@ -11,7 +11,7 @@ use stor_port::{
     transport_api::ResourceKind,
     types::v0::{
         store::{
-            node::{NodeLabels, NodeOperation, NodeSpec},
+            node::{NodeLabelOp, NodeLabels, NodeOperation, NodeSpec, NodeUnLabelOp},
             SpecStatus, SpecTransaction,
         },
         transport::{NodeId, Register},
@@ -143,13 +143,13 @@ impl SpecOperationsHelper for NodeSpec {
         _state: &Self::State,
         op: Self::UpdateOp,
     ) -> Result<(), SvcError> {
-        match op.clone() {
+        match &op {
             NodeOperation::Cordon(label) | NodeOperation::Drain(label) => {
                 // Do not allow the same label to be applied more than once.
-                if self.has_cordon_label(&label) {
+                if self.has_cordon_label(label) {
                     Err(SvcError::CordonLabel {
                         node_id: self.id().to_string(),
-                        label,
+                        label: label.clone(),
                     })
                 } else {
                     self.start_op(op);
@@ -158,38 +158,35 @@ impl SpecOperationsHelper for NodeSpec {
             }
             NodeOperation::Uncordon(label) => {
                 // Check that the label is present.
-                if !self.has_cordon_label(&label) {
+                if !self.has_cordon_label(label) {
                     Err(SvcError::UncordonLabel {
                         node_id: self.id().to_string(),
-                        label,
+                        label: label.clone(),
                     })
                 } else {
                     self.start_op(op);
                     Ok(())
                 }
             }
-            NodeOperation::Label(label, overwrite) => {
+            NodeOperation::Label(NodeLabelOp { labels, overwrite }) => {
                 // Check that the label is present.
-                if !overwrite && self.has_labels_key(label.keys().collect()) {
-                    Err(SvcError::LabelExists {
+                let collisions = self.label_collisions(labels);
+                if !*overwrite && !collisions.is_empty() {
+                    Err(SvcError::LabelsExists {
                         node_id: self.id().to_string(),
-                        label: label
-                            .into_iter()
-                            .map(|(key, value)| format!("{key}: {value}"))
-                            .collect::<Vec<String>>()
-                            .join(", "),
+                        labels: format!("{collisions:?}"),
                     })
                 } else {
                     self.start_op(op);
                     Ok(())
                 }
             }
-            NodeOperation::Unlabel(label) => {
+            NodeOperation::Unlabel(NodeUnLabelOp { label_key }) => {
                 // Check that the label is present.
-                if !self.has_labels_key(vec![&label]) {
+                if !self.has_labels_key(label_key) {
                     Err(SvcError::LabelNotFound {
                         node_id: self.id().to_string(),
-                        label,
+                        label_key: label_key.to_string(),
                     })
                 } else {
                     self.start_op(op);

control-plane/agents/src/common/errors.rs

@@ -31,13 +31,13 @@ pub enum SvcError {
     NoNodes {},
     #[snafu(display("Node {} is cordoned", node_id))]
     CordonedNode { node_id: String },
-    #[snafu(display("Node {} is already labelled with label '{}'", node_id, label))]
-    LabelExists { node_id: String, label: String },
-    #[snafu(display("Node {} doesn't have the label '{}'", node_id, label))]
-    LabelNotFound { node_id: String, label: String },
-    #[snafu(display("Node {} is already cordoned with label '{}'", node_id, label))]
+    #[snafu(display("Node {node_id} is already labelled with labels '{labels}'"))]
+    LabelsExists { node_id: String, labels: String },
+    #[snafu(display("Node {node_id} doesn't have the label key '{label_key}'"))]
+    LabelNotFound { node_id: String, label_key: String },
+    #[snafu(display("Node {node_id} is already cordoned with label '{label}'"))]
     CordonLabel { node_id: String, label: String },
-    #[snafu(display("Node {} does not have a cordon label '{}'", node_id, label))]
+    #[snafu(display("Node {node_id} does not have a cordon label '{label}'"))]
     UncordonLabel { node_id: String, label: String },
     #[snafu(display(
         "Timed out after '{:?}' attempting to connect to node '{}' via gRPC endpoint '{}'",
@@ -554,7 +554,7 @@ impl From<SvcError> for ReplyError {
                 extra,
             },
 
-            SvcError::LabelExists { .. } => ReplyError {
+            SvcError::LabelsExists { .. } => ReplyError {
                 kind: ReplyErrorKind::AlreadyExists,
                 resource: ResourceKind::Node,
                 source,

control-plane/grpc/proto/v1/node/target_node.proto

@@ -165,8 +165,8 @@ message LabelNodeReply {
 message UnlabelNodeRequest {
   // Node identification
   string node_id = 1;
-  // Node cordon label
-  string label = 2;
+  // Node label key to remove
+  string label_key = 2;
 }
 
 message UnlabelNodeReply {

control-plane/grpc/src/operations/node/client.rs

@@ -166,10 +166,10 @@ impl NodeOperations for NodeClient {
     }
 
     #[tracing::instrument(name = "NodeClient::unlabel", level = "debug", skip(self), err)]
-    async fn unlabel(&self, id: NodeId, label: String) -> Result<Node, ReplyError> {
+    async fn unlabel(&self, id: NodeId, label_key: String) -> Result<Node, ReplyError> {
         let req = UnlabelNodeRequest {
             node_id: id.to_string(),
-            label,
+            label_key,
         };
         let response = self.client().unlabel_node(req).await?.into_inner();
         match response.reply {

control-plane/grpc/src/operations/node/server.rs

@@ -148,7 +148,11 @@ impl NodeGrpc for NodeServer {
         request: tonic::Request<UnlabelNodeRequest>,
     ) -> Result<tonic::Response<UnlabelNodeReply>, tonic::Status> {
         let req: UnlabelNodeRequest = request.into_inner();
-        match self.service.unlabel(req.node_id.into(), req.label).await {
+        match self
+            .service
+            .unlabel(req.node_id.into(), req.label_key)
+            .await
+        {
             Ok(node) => Ok(Response::new(UnlabelNodeReply {
                 reply: Some(unlabel_node_reply::Reply::Node(node.into())),
             })),

control-plane/stor-port/src/types/v0/store/node.rs

@@ -273,8 +273,8 @@ impl NodeSpec {
     }
 
     /// Remove label from node.
-    pub fn unlabel(&mut self, label: String) {
-        self.labels.remove(&label);
+    pub fn unlabel(&mut self, label_key: &str) {
+        self.labels.remove(label_key);
     }
 
     /// Drain node by applying the drain label.
@@ -348,15 +348,19 @@ impl NodeSpec {
         self.has_cordon_only_label(label) || self.has_drain_label(label)
     }
 
-    pub fn has_labels_key(&self, key: Vec<&String>) -> bool {
-        let mut found = false;
-        for k in key {
-            if self.labels.contains_key(k) {
-                found = true;
-                break;
-            }
-        }
-        found
+    /// Check if the node has the given topology label key.
+    pub fn has_labels_key(&self, key: &str) -> bool {
+        self.labels.contains_key(key)
+    }
+    /// Get map of key collisions between current topology labels and the given labels.
+    pub fn label_collisions<'a>(
+        &self,
+        labels: &'a HashMap<String, String>,
+    ) -> HashMap<&'a String, &'a String> {
+        labels
+            .iter()
+            .filter(|(key, _)| self.labels.contains_key(*key))
+            .collect()
     }
 
     /// Returns true if it has the label in the cordon list.
@@ -545,8 +549,30 @@ pub enum NodeOperation {
     RemoveDrainingVolumes(DrainingVolumes),
     RemoveAllDrainingVolumes(),
     SetDrained(),
-    Label(HashMap<String, String>, bool),
-    Unlabel(String),
+    Label(NodeLabelOp),
+    Unlabel(NodeUnLabelOp),
+}
+
+/// Parameter for adding node labels.
+#[derive(Serialize, Deserialize, Debug, Clone, PartialEq)]
+pub struct NodeLabelOp {
+    pub labels: HashMap<String, String>,
+    pub overwrite: bool,
+}
+impl From<(HashMap<String, String>, bool)> for NodeLabelOp {
+    fn from((labels, overwrite): (HashMap<String, String>, bool)) -> Self {
+        Self { labels, overwrite }
+    }
+}
+/// Parameter for removing node labels.
+#[derive(Serialize, Deserialize, Debug, Clone, PartialEq)]
+pub struct NodeUnLabelOp {
+    pub label_key: String,
+}
+impl From<String> for NodeUnLabelOp {
+    fn from(label_key: String) -> Self {
+        Self { label_key }
+    }
 }
 
 /// Operation State for a Node spec resource.
@@ -587,11 +613,11 @@ impl SpecTransaction<NodeOperation> for NodeSpec {
                 NodeOperation::SetDrained() => {
                     self.set_drained();
                 }
-                NodeOperation::Label(label, _) => {
-                    self.label(label);
+                NodeOperation::Label(NodeLabelOp { labels, .. }) => {
+                    self.label(labels);
                 }
-                NodeOperation::Unlabel(label) => {
-                    self.unlabel(label);
+                NodeOperation::Unlabel(NodeUnLabelOp { label_key }) => {
+                    self.unlabel(&label_key);
                 }
             }
         }
@@ -624,7 +650,7 @@ impl SpecTransaction<NodeOperation> for NodeSpec {
             NodeOperation::RemoveDrainingVolumes(_) => (false, true),
             NodeOperation::RemoveAllDrainingVolumes() => (false, true),
             NodeOperation::SetDrained() => (false, true),
-            NodeOperation::Label(_, _) => (false, true),
+            NodeOperation::Label(_) => (false, true),
             NodeOperation::Unlabel(_) => (false, true),
         }
     }