Node labelling UX

Jenkinsfile

@@ -117,7 +117,7 @@ pipeline {
         sh 'nix-shell --run "./scripts/rust/generate-openapi-bindings.sh"'
         sh 'nix-shell --run "cargo fmt --all -- --check"'
         sh 'nix-shell --run "cargo clippy --all-targets -- -D warnings"'
-        sh 'nix-shell --run "black tests/bdd"'
+        sh 'nix-shell --run "black --check tests/bdd"'
         sh 'nix-shell --run "./scripts/git/check-submodule-branches.sh"'
       }
     }

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) => {
-                // Check that the label is present.
-                if !overwrite && self.has_labels_key(label.keys().collect()) {
-                    Err(SvcError::LabelExists {
+            NodeOperation::Label(NodeLabelOp { labels, overwrite }) => {
+                let (existing, conflict) = self.label_collisions(labels);
+                if !*overwrite && !existing.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!("{existing:?}"),
+                        conflict,
                     })
                 } 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,17 @@ 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,
+        conflict: bool,
+    },
+    #[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,15 +558,19 @@ impl From<SvcError> for ReplyError {
                 extra,
             },
 
-            SvcError::LabelExists { .. } => ReplyError {
-                kind: ReplyErrorKind::AlreadyExists,
+            SvcError::LabelsExists { conflict, .. } => ReplyError {
+                kind: if conflict {
+                    ReplyErrorKind::FailedPrecondition
+                } else {
+                    ReplyErrorKind::AlreadyExists
+                },
                 resource: ResourceKind::Node,
                 source,
                 extra,
             },
 
             SvcError::LabelNotFound { .. } => ReplyError {
-                kind: ReplyErrorKind::NotFound,
+                kind: ReplyErrorKind::FailedPrecondition,
                 resource: ResourceKind::Node,
                 source,
                 extra,

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/plugin/src/resources/error.rs

@@ -1,3 +1,4 @@
+use crate::resources::node;
 use snafu::Snafu;
 
 /// All errors returned when resources command fails.
@@ -23,14 +24,10 @@ pub enum Error {
         id: String,
         source: openapi::tower::client::Error<openapi::models::RestJsonError>,
     },
-    /// Error when node label request fails.
-    #[snafu(display("Failed to get node {id}. Error {source}"))]
-    NodeLabelError {
-        id: String,
-        source: openapi::tower::client::Error<openapi::models::RestJsonError>,
-    },
-    #[snafu(display("Empty node label {id}."))]
-    EmptyNodeLabelError { id: String },
+    #[snafu(display("Invalid label format: {source}"))]
+    NodeLabelFormat { source: node::TopologyError },
+    #[snafu(display("{source}"))]
+    NodeLabel { source: node::OpError },
     /// Error when node uncordon request fails.
     #[snafu(display("Failed to uncordon node {id}. Error {source}"))]
     NodeUncordonError {

control-plane/plugin/src/resources/mod.rs

@@ -104,10 +104,22 @@ pub enum DrainResources {
 /// The types of resources that support the 'label' operation.
 #[derive(clap::Subcommand, Debug)]
 pub enum LabelResources {
-    /// Label node with the given ID.
+    /// Adds or removes a label to or from the specified node.
     Node {
+        /// The id of the node to label/unlabel.
         id: NodeId,
+        /// The label to be added or removed from the node.
+        /// To add a label, please use the following format:
+        /// ${key}=${value}
+        /// To remove a label, please use the following format:
+        /// ${key}-
+        /// A label key and value must begin with a letter or number, and may contain letters,
+        /// numbers, hyphens, dots, and underscores, up to 63 characters each.
+        /// The key may contain a single slash.
         label: String,
+        /// Allow labels to be overwritten, otherwise reject label updates that overwrite existing
+        /// labels.
+        #[clap(long)]
         overwrite: bool,
     },
 }
@@ -116,6 +128,7 @@ pub enum LabelResources {
 pub enum GetDrainArgs {
     /// Get the drain for the node with the given ID.
     Node {
+        /// The id of the node to get the drain labels from.
         id: NodeId,
     },
     Nodes,