From 66e9b6b37ac8e3e8a272afe3c642613023b46339 Mon Sep 17 00:00:00 2001 From: sinhaashish Date: Sun, 21 Jul 2024 18:36:29 +0000 Subject: [PATCH 1/5] feat(topology): code to add label to pool in core and storport Signed-off-by: sinhaashish --- .../agents/src/bin/core/node/specs.rs | 6 +- .../src/bin/core/pool/pool_operations.rs | 53 ++++++++++- .../agents/src/bin/core/pool/service.rs | 63 ++++++++++++- .../agents/src/bin/core/pool/specs.rs | 65 +++++++++++-- control-plane/agents/src/common/errors.rs | 23 +++-- .../stor-port/src/types/v0/store/pool.rs | 92 ++++++++++++++++++- .../stor-port/src/types/v0/transport/mod.rs | 4 + .../stor-port/src/types/v0/transport/pool.rs | 48 +++++++++- 8 files changed, 326 insertions(+), 28 deletions(-) diff --git a/control-plane/agents/src/bin/core/node/specs.rs b/control-plane/agents/src/bin/core/node/specs.rs index 9dbcd1c66..301addaba 100644 --- a/control-plane/agents/src/bin/core/node/specs.rs +++ b/control-plane/agents/src/bin/core/node/specs.rs @@ -196,7 +196,8 @@ impl SpecOperationsHelper for NodeSpec { let (existing, conflict) = self.label_collisions(labels); if !*overwrite && !existing.is_empty() { Err(SvcError::LabelsExists { - node_id: self.id().to_string(), + resource: ResourceKind::Node, + id: self.id().to_string(), labels: format!("{existing:?}"), conflict, }) @@ -209,7 +210,8 @@ impl SpecOperationsHelper for NodeSpec { // Check that the label is present. if !self.has_labels_key(label_key) { Err(SvcError::LabelNotFound { - node_id: self.id().to_string(), + resource: ResourceKind::Node, + id: self.id().to_string(), label_key: label_key.to_string(), }) } else { diff --git a/control-plane/agents/src/bin/core/pool/pool_operations.rs b/control-plane/agents/src/bin/core/pool/pool_operations.rs index cacfd764a..ccf9a2b26 100644 --- a/control-plane/agents/src/bin/core/pool/pool_operations.rs +++ b/control-plane/agents/src/bin/core/pool/pool_operations.rs @@ -2,16 +2,17 @@ use crate::controller::{ io_engine::PoolApi, registry::Registry, resources::{ - operations::ResourceLifecycle, + operations::{ResourceLabel, ResourceLifecycle}, operations_helper::{GuardedOperationsHelper, OnCreateFail, OperationSequenceGuard}, OperationGuardArc, }, }; use agents::errors::{SvcError, SvcError::CordonedNode}; +use std::collections::HashMap; use stor_port::{ transport_api::ResourceKind, types::v0::{ - store::pool::PoolSpec, + store::pool::{PoolOperation, PoolSpec}, transport::{CreatePool, CtrlPoolState, DestroyPool, Pool}, }, }; @@ -67,7 +68,7 @@ impl ResourceLifecycle for OperationGuardArc { let state = pool.complete_create(result, registry, on_fail).await?; let spec = pool.lock().clone(); - Ok(Pool::new(spec, CtrlPoolState::new(state))) + Ok(Pool::new(spec, Some(CtrlPoolState::new(state)))) } async fn destroy( @@ -135,3 +136,49 @@ impl ResourceLifecycle for Option> { } } } + +/// Resource Label Operations. +#[async_trait::async_trait] +impl ResourceLabel for OperationGuardArc { + type LabelOutput = PoolSpec; + type UnlabelOutput = PoolSpec; + + /// Label a node via operation guard functions. + async fn label( + &mut self, + registry: &Registry, + labels: HashMap, + overwrite: bool, + ) -> Result { + let cloned_pool_spec = self.lock().clone(); + let spec_clone = self + .start_update( + registry, + &cloned_pool_spec, + PoolOperation::Label((labels, overwrite).into()), + ) + .await?; + + self.complete_update(registry, Ok(()), spec_clone).await?; + Ok(self.as_ref().clone()) + } + + /// Unlabel a node via operation guard functions. + async fn unlabel( + &mut self, + registry: &Registry, + label_key: String, + ) -> Result { + let cloned_pool_spec = self.lock().clone(); + let spec_clone = self + .start_update( + registry, + &cloned_pool_spec, + PoolOperation::Unlabel(label_key.into()), + ) + .await?; + + self.complete_update(registry, Ok(()), spec_clone).await?; + Ok(self.as_ref().clone()) + } +} diff --git a/control-plane/agents/src/bin/core/pool/service.rs b/control-plane/agents/src/bin/core/pool/service.rs index 92608f3d0..4f39b2e4d 100644 --- a/control-plane/agents/src/bin/core/pool/service.rs +++ b/control-plane/agents/src/bin/core/pool/service.rs @@ -1,7 +1,7 @@ use crate::controller::{ registry::Registry, resources::{ - operations::{ResourceLifecycle, ResourceResize, ResourceSharing}, + operations::{ResourceLabel, ResourceLifecycle, ResourceResize, ResourceSharing}, operations_helper::{OperationSequenceGuard, ResourceSpecsLocked}, OperationGuardArc, ResourceMutex, }, @@ -11,7 +11,9 @@ use agents::errors::{PoolNotFound, ReplicaNotFound, SvcError}; use grpc::{ context::Context, operations::{ - pool::traits::{CreatePoolInfo, DestroyPoolInfo, PoolOperations}, + pool::traits::{ + CreatePoolInfo, DestroyPoolInfo, LabelPoolInfo, PoolOperations, UnlabelPoolInfo, + }, replica::traits::{ CreateReplicaInfo, DestroyReplicaInfo, ReplicaOperations, ResizeReplicaInfo, ShareReplicaInfo, UnshareReplicaInfo, @@ -21,13 +23,14 @@ use grpc::{ use stor_port::{ transport_api::{ v0::{Pools, Replicas}, - ReplyError, + ReplyError, ResourceKind, }, types::v0::{ store::{pool::PoolSpec, replica::ReplicaSpec}, transport::{ CreatePool, CreateReplica, DestroyPool, DestroyReplica, Filter, GetPools, GetReplicas, - NodeId, Pool, PoolId, Replica, ResizeReplica, ShareReplica, UnshareReplica, VolumeId, + LabelPool, NodeId, Pool, PoolId, Replica, ResizeReplica, ShareReplica, UnlabelPool, + UnshareReplica, VolumeId, }, }, }; @@ -68,6 +71,36 @@ impl PoolOperations for Service { let pools = self.get_pools(&req).await?; Ok(pools) } + + async fn label( + &self, + pool: &dyn LabelPoolInfo, + _ctx: Option, + ) -> Result { + let req = pool.into(); + let service = self.clone(); + let pool = Context::spawn(async move { service.label(&req).await }).await??; + Ok(pool) + } + + /// Remove the specified label key from the pool. + async fn unlabel( + &self, + pool: &dyn UnlabelPoolInfo, + _ctx: Option, + ) -> Result { + let req: UnlabelPool = pool.into(); + let service = self.clone(); + if req.label_key().is_empty() { + return Err(SvcError::InvalidLabel { + labels: req.label_key(), + resource_kind: ResourceKind::Pool, + } + .into()); + } + let pool = Context::spawn(async move { service.unlabel(&req).await }).await??; + Ok(pool) + } } #[tonic::async_trait] @@ -334,4 +367,26 @@ impl Service { let mut replica = self.specs().replica(&request.uuid).await?; replica.resize(&self.registry, request).await } + + /// Label the specified pool. + #[tracing::instrument(level = "info", skip(self), err, fields(pool.id = %request.pool_id))] + async fn label(&self, request: &LabelPool) -> Result { + let mut guarded_pool = self.specs().guarded_pool(&request.pool_id).await?; + let spec = guarded_pool + .label(&self.registry, request.labels(), request.overwrite()) + .await?; + let state = self.registry.ctrl_pool_state(&request.pool_id).await.ok(); + Ok(Pool::new(spec, state)) + } + + /// Remove the specified label from the specified pool. + #[tracing::instrument(level = "info", skip(self), err, fields(pool.id = %request.pool_id))] + async fn unlabel(&self, request: &UnlabelPool) -> Result { + let mut guarded_pool = self.specs().guarded_pool(&request.pool_id).await?; + let spec = guarded_pool + .unlabel(&self.registry, request.label_key()) + .await?; + let state = self.registry.ctrl_pool_state(&request.pool_id).await.ok(); + Ok(Pool::new(spec, state)) + } } diff --git a/control-plane/agents/src/bin/core/pool/specs.rs b/control-plane/agents/src/bin/core/pool/specs.rs index 03961ba28..2ce080600 100644 --- a/control-plane/agents/src/bin/core/pool/specs.rs +++ b/control-plane/agents/src/bin/core/pool/specs.rs @@ -13,12 +13,12 @@ use stor_port::{ transport_api::ResourceKind, types::v0::{ store::{ - pool::{PoolOperation, PoolSpec}, + pool::{PoolLabelOp, PoolOperation, PoolSpec, PoolUnLabelOp}, replica::{ReplicaOperation, ReplicaSpec}, SpecStatus, SpecTransaction, }, transport::{ - CreatePool, CreateReplica, NodeId, PoolId, PoolState, PoolStatus, Replica, ReplicaId, + CreatePool, CreateReplica, NodeId, PoolId, PoolStatus, Replica, ReplicaId, ReplicaOwners, ReplicaStatus, }, }, @@ -29,8 +29,8 @@ impl GuardedOperationsHelper for OperationGuardArc { type Create = CreatePool; type Owners = (); type Status = PoolStatus; - type State = PoolState; - type UpdateOp = (); + type State = PoolSpec; + type UpdateOp = PoolOperation; type Inner = PoolSpec; fn validate_destroy(&self, registry: &Registry) -> Result<(), SvcError> { @@ -59,8 +59,8 @@ impl SpecOperationsHelper for PoolSpec { type Create = CreatePool; type Owners = (); type Status = PoolStatus; - type State = PoolState; - type UpdateOp = (); + type State = PoolSpec; + type UpdateOp = PoolOperation; fn start_create_op(&mut self, _request: &Self::Create) { self.start_op(PoolOperation::Create); @@ -68,6 +68,46 @@ impl SpecOperationsHelper for PoolSpec { fn start_destroy_op(&mut self) { self.start_op(PoolOperation::Destroy); } + async fn start_update_op( + &mut self, + _: &Registry, + _state: &Self::State, + op: Self::UpdateOp, + ) -> Result<(), SvcError> { + match &op { + PoolOperation::Label(PoolLabelOp { labels, overwrite }) => { + let (existing, conflict) = self.label_collisions(labels); + if !*overwrite && !existing.is_empty() { + Err(SvcError::LabelsExists { + resource: ResourceKind::Pool, + id: self.id().to_string(), + labels: format!("{existing:?}"), + conflict, + }) + } else { + self.start_op(op); + Ok(()) + } + } + PoolOperation::Unlabel(PoolUnLabelOp { label_key }) => { + // Check that the label is present. + if !self.has_labels_key(label_key) { + Err(SvcError::LabelNotFound { + resource: ResourceKind::Pool, + id: self.id().to_string(), + label_key: label_key.to_string(), + }) + } else { + self.start_op(op); + Ok(()) + } + } + _ => { + self.start_op(op); + Ok(()) + } + } + } fn dirty(&self) -> bool { // The pool spec can be dirty if a pool create operation fails to complete because it cannot @@ -411,4 +451,17 @@ impl ResourceSpecsLocked { } pending_ops } + + /// Get guarded pool spec by its `PoolId`. + pub async fn guarded_pool( + &self, + pool_id: &PoolId, + ) -> Result, SvcError> { + match self.pool_rsc(pool_id) { + Some(pool) => pool.operation_guard_wait().await, + None => Err(PoolNotFound { + pool_id: pool_id.to_owned(), + }), + } + } } diff --git a/control-plane/agents/src/common/errors.rs b/control-plane/agents/src/common/errors.rs index 9613bb4b2..0637a5ec4 100644 --- a/control-plane/agents/src/common/errors.rs +++ b/control-plane/agents/src/common/errors.rs @@ -31,14 +31,19 @@ pub enum SvcError { NoNodes {}, #[snafu(display("Node {} is cordoned", node_id))] CordonedNode { node_id: String }, - #[snafu(display("Node {node_id} is already labelled with labels '{labels}'"))] + #[snafu(display("{resource} {id} is already labelled with labels '{labels}'"))] LabelsExists { - node_id: String, + resource: ResourceKind, + 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("{resource} {id} doesn't have the label key '{label_key}'"))] + LabelNotFound { + resource: ResourceKind, + 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 {node_id} does not have a cordon label '{label}'"))] @@ -598,20 +603,22 @@ impl From for ReplyError { extra, }, - SvcError::LabelsExists { conflict, .. } => ReplyError { + SvcError::LabelsExists { + resource, conflict, .. + } => ReplyError { kind: if conflict { ReplyErrorKind::FailedPrecondition } else { ReplyErrorKind::AlreadyExists }, - resource: ResourceKind::Node, + resource, source, extra, }, - SvcError::LabelNotFound { .. } => ReplyError { + SvcError::LabelNotFound { resource, .. } => ReplyError { kind: ReplyErrorKind::FailedPrecondition, - resource: ResourceKind::Node, + resource, source, extra, }, diff --git a/control-plane/stor-port/src/types/v0/store/pool.rs b/control-plane/stor-port/src/types/v0/store/pool.rs index 1fff5edc2..f6a344051 100644 --- a/control-plane/stor-port/src/types/v0/store/pool.rs +++ b/control-plane/stor-port/src/types/v0/store/pool.rs @@ -15,7 +15,7 @@ pub type PoolLabel = std::collections::HashMap; use crate::types::v0::transport::ImportPool; use pstor::ApiVersion; use serde::{Deserialize, Serialize}; -use std::{convert::From, fmt::Debug}; +use std::{collections::HashMap, convert::From, fmt::Debug}; /// Pool data structure used by the persistent store. #[derive(Serialize, Deserialize, Debug, PartialEq)] @@ -85,6 +85,62 @@ pub struct PoolSpec { pub operation: Option, } +impl PoolSpec { + /// Pool identification. + pub fn id(&self) -> &PoolId { + &self.id + } + + /// Label pool by applying the labels. + pub fn label(&mut self, labels: HashMap) { + match &mut self.labels { + Some(existing_labels) => { + existing_labels.extend(labels); + } + None => { + self.labels = Some(labels); + } + } + } + + /// Check if the pool has the given topology label key. + pub fn has_labels_key(&self, key: &str) -> bool { + if let Some(labels) = &self.labels { + return labels.contains_key(key); + } + false + } + + /// Remove label from pool. + pub fn unlabel(&mut self, label_key: &str) { + if let Some(labels) = &mut self.labels { + labels.remove(label_key); + } + } + + /// Check if there are key collisions between current topology labels and the given labels. + pub fn label_collisions<'a>( + &'a self, + labels: &'a HashMap, + ) -> (HashMap<&'a String, &'a String>, bool) { + let mut conflict = false; + let mut existing_conflicts = HashMap::new(); + + if let Some(existing_labels) = &self.labels { + for (key, value) in labels { + if let Some(existing_value) = existing_labels.get(key) { + if existing_value != value { + conflict = true; + existing_conflicts.insert(key, existing_value); + } + } + } + } + + (existing_conflicts, conflict) + } +} + impl From<&PoolSpec> for ImportPool { fn from(value: &PoolSpec) -> Self { Self { @@ -134,6 +190,12 @@ impl SpecTransaction for PoolSpec { PoolOperation::Create => { self.status = SpecStatus::Created(transport::PoolStatus::Online); } + PoolOperation::Label(PoolLabelOp { labels, .. }) => { + self.label(labels); + } + PoolOperation::Unlabel(PoolUnLabelOp { label_key }) => { + self.unlabel(&label_key); + } } } self.clear_op(); @@ -159,6 +221,10 @@ impl SpecTransaction for PoolSpec { fn pending_op(&self) -> Option<&PoolOperation> { self.operation.as_ref().map(|o| &o.operation) } + + fn log_op(&self, _operation: &PoolOperation) -> (bool, bool) { + (false, true) + } } /// Available Pool Operations @@ -166,6 +232,30 @@ impl SpecTransaction for PoolSpec { pub enum PoolOperation { Create, Destroy, + Label(PoolLabelOp), + Unlabel(PoolUnLabelOp), +} + +/// Parameter for adding pool labels. +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq)] +pub struct PoolLabelOp { + pub labels: HashMap, + pub overwrite: bool, +} +impl From<(HashMap, bool)> for PoolLabelOp { + fn from((labels, overwrite): (HashMap, bool)) -> Self { + Self { labels, overwrite } + } +} +/// Parameter for removing pool labels. +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq)] +pub struct PoolUnLabelOp { + pub label_key: String, +} +impl From for PoolUnLabelOp { + fn from(label_key: String) -> Self { + Self { label_key } + } } impl PartialEq for PoolSpec { diff --git a/control-plane/stor-port/src/types/v0/transport/mod.rs b/control-plane/stor-port/src/types/v0/transport/mod.rs index bac0fe898..06409457b 100644 --- a/control-plane/stor-port/src/types/v0/transport/mod.rs +++ b/control-plane/stor-port/src/types/v0/transport/mod.rs @@ -77,6 +77,10 @@ pub enum MessageIdVs { CreatePool, /// Destroy Pool. DestroyPool, + /// Label Pool. + LabelPool, + /// Unlabel Pool. + UnlabelPool, /// Import Pool. ImportPool, /// Get replicas with filter. diff --git a/control-plane/stor-port/src/types/v0/transport/pool.rs b/control-plane/stor-port/src/types/v0/transport/pool.rs index ee251896f..be5d211c9 100644 --- a/control-plane/stor-port/src/types/v0/transport/pool.rs +++ b/control-plane/stor-port/src/types/v0/transport/pool.rs @@ -5,7 +5,7 @@ use crate::{ IntoOption, }; use serde::{Deserialize, Serialize}; -use std::{cmp::Ordering, fmt::Debug, ops::Deref}; +use std::{cmp::Ordering, collections::HashMap, fmt::Debug, ops::Deref}; use strum_macros::{Display, EnumString}; /// Pool Service @@ -166,11 +166,11 @@ pub struct Pool { impl Pool { /// Construct a new pool with spec and state. - pub fn new(spec: PoolSpec, state: CtrlPoolState) -> Self { + pub fn new(spec: PoolSpec, state: Option) -> Self { Self { id: spec.id.clone(), spec: Some(spec), - state: Some(state), + state, } } /// Construct a new pool with spec but no state. @@ -192,7 +192,7 @@ impl Pool { /// Try to construct a new pool from spec and state. pub fn try_new(spec: Option, state: Option) -> Option { match (spec, state) { - (Some(spec), Some(state)) => Some(Self::new(spec, state)), + (Some(spec), Some(state)) => Some(Self::new(spec, Some(state))), (Some(spec), None) => Some(Self::from_spec(spec)), (None, Some(state)) => Some(Self::from_state(state, None)), _ => None, @@ -346,3 +346,43 @@ impl DestroyPool { Self { node, id } } } + +/// Label Pool Request. +#[derive(Serialize, Deserialize, Default, Debug, Clone, Eq, PartialEq)] +#[serde(rename_all = "camelCase")] +pub struct LabelPool { + /// Id of the pool. + pub pool_id: PoolId, + /// Labels to be set on the pool + pub labels: HashMap, + /// Overwrite the existing labels + pub overwrite: bool, +} + +impl LabelPool { + /// Create new `Self` from the given parameters. + pub fn new(pool_id: PoolId, labels: HashMap, overwrite: bool) -> Self { + Self { + pool_id, + labels, + overwrite, + } + } +} + +/// Un-Label Pool Request. +#[derive(Serialize, Deserialize, Default, Debug, Clone, Eq, PartialEq)] +#[serde(rename_all = "camelCase")] +pub struct UnlabelPool { + /// Id of the pool. + pub pool_id: PoolId, + /// Label key to be removed from the pool. + pub label_key: String, +} + +impl UnlabelPool { + /// Create new `Self` from the given parameters. + pub fn new(pool_id: PoolId, label_key: String) -> Self { + Self { pool_id, label_key } + } +} From f00dd4df07e45dbeb25b459ee01c9a00e15e8d90 Mon Sep 17 00:00:00 2001 From: sinhaashish Date: Sun, 21 Jul 2024 18:37:13 +0000 Subject: [PATCH 2/5] feat(topology): code to add label to pool in grpc Signed-off-by: sinhaashish --- control-plane/grpc/proto/v1/pool/pool.proto | 35 +++++ .../grpc/src/operations/pool/client.rs | 42 +++++- control-plane/grpc/src/operations/pool/mod.rs | 18 ++- .../grpc/src/operations/pool/server.rs | 37 ++++- .../grpc/src/operations/pool/traits.rs | 128 +++++++++++++++++- 5 files changed, 249 insertions(+), 11 deletions(-) diff --git a/control-plane/grpc/proto/v1/pool/pool.proto b/control-plane/grpc/proto/v1/pool/pool.proto index b25960963..2a47a420d 100644 --- a/control-plane/grpc/proto/v1/pool/pool.proto +++ b/control-plane/grpc/proto/v1/pool/pool.proto @@ -129,9 +129,44 @@ message GetPoolsReply { } } +// Label Pool request +message LabelPoolRequest { + // Pool identification + string pool_id = 1; + // Pool label map + map labels = 2; + // Overwrite an existing key + bool overwrite = 3; +} + +// Reply type for a LabelPool request +message LabelPoolReply { + oneof reply { + Pool pool = 1; + common.ReplyError error = 2; + } +} + +message UnlabelPoolRequest { + // Pool identification + string pool_id = 1; + // Pool label key to remove + string label_key = 2; +} + +message UnlabelPoolReply { + oneof reply { + Pool pool = 1; + common.ReplyError error = 2; + } +} + + // Service for managing storage pools service PoolGrpc { rpc CreatePool (CreatePoolRequest) returns (CreatePoolReply) {} rpc DestroyPool (DestroyPoolRequest) returns (DestroyPoolReply) {} rpc GetPools (GetPoolsRequest) returns (GetPoolsReply) {} + rpc LabelPool (LabelPoolRequest) returns (LabelPoolReply) {} + rpc UnlabelPool (UnlabelPoolRequest) returns (UnlabelPoolReply) {} } diff --git a/control-plane/grpc/src/operations/pool/client.rs b/control-plane/grpc/src/operations/pool/client.rs index 0fb206021..88b4b4ee6 100644 --- a/control-plane/grpc/src/operations/pool/client.rs +++ b/control-plane/grpc/src/operations/pool/client.rs @@ -1,10 +1,10 @@ use crate::{ common::{CommonFilter, NodeFilter, NodePoolFilter, PoolFilter}, context::{Client, Context, TracedChannel}, - operations::pool::traits::{CreatePoolInfo, DestroyPoolInfo, PoolOperations}, + operations::pool::traits::{CreatePoolInfo, DestroyPoolInfo, LabelPoolInfo, PoolOperations}, pool::{ - create_pool_reply, get_pools_reply, get_pools_request, pool_grpc_client::PoolGrpcClient, - GetPoolsRequest, + create_pool_reply, get_pools_reply, get_pools_request, label_pool_reply, + pool_grpc_client::PoolGrpcClient, unlabel_pool_reply, GetPoolsRequest, }, }; use std::{convert::TryFrom, ops::Deref}; @@ -14,6 +14,8 @@ use stor_port::{ }; use tonic::transport::Uri; +use super::traits::UnlabelPoolInfo; + /// RPC Pool Client #[derive(Clone)] pub struct PoolClient { @@ -105,4 +107,38 @@ impl PoolOperations for PoolClient { None => Err(ReplyError::invalid_response(ResourceKind::Pool)), } } + + #[tracing::instrument(name = "PoolClient::label", level = "debug", skip(self), err)] + async fn label( + &self, + request: &dyn LabelPoolInfo, + ctx: Option, + ) -> Result { + let req = self.request(request, ctx, MessageIdVs::LabelPool); + let response = self.client().label_pool(req).await?.into_inner(); + match response.reply { + Some(label_pool_reply) => match label_pool_reply { + label_pool_reply::Reply::Pool(pool) => Ok(Pool::try_from(pool)?), + label_pool_reply::Reply::Error(err) => Err(err.into()), + }, + None => Err(ReplyError::invalid_response(ResourceKind::Pool)), + } + } + + #[tracing::instrument(name = "PoolClient::unlabel", level = "debug", skip(self), err)] + async fn unlabel( + &self, + request: &dyn UnlabelPoolInfo, + ctx: Option, + ) -> Result { + let req = self.request(request, ctx, MessageIdVs::UnlabelPool); + let response = self.client().unlabel_pool(req).await?.into_inner(); + match response.reply { + Some(unlabel_pool_reply) => match unlabel_pool_reply { + unlabel_pool_reply::Reply::Pool(pool) => Ok(Pool::try_from(pool)?), + unlabel_pool_reply::Reply::Error(err) => Err(err.into()), + }, + None => Err(ReplyError::invalid_response(ResourceKind::Pool)), + } + } } diff --git a/control-plane/grpc/src/operations/pool/mod.rs b/control-plane/grpc/src/operations/pool/mod.rs index 21cb85a51..bdc8beb4a 100644 --- a/control-plane/grpc/src/operations/pool/mod.rs +++ b/control-plane/grpc/src/operations/pool/mod.rs @@ -100,7 +100,9 @@ mod test { context::Context, operations::pool::{ test::TimeoutTester, - traits::{CreatePoolInfo, DestroyPoolInfo, PoolOperations}, + traits::{ + CreatePoolInfo, DestroyPoolInfo, LabelPoolInfo, PoolOperations, UnlabelPoolInfo, + }, }, }; use std::time::Duration; @@ -136,6 +138,20 @@ mod test { tester.complete(); Ok(Pools(vec![])) } + async fn label( + &self, + _pool: &dyn LabelPoolInfo, + _ctx: Option, + ) -> Result { + todo!() + } + async fn unlabel( + &self, + _pool: &dyn UnlabelPoolInfo, + _ctx: Option, + ) -> Result { + todo!() + } } } } diff --git a/control-plane/grpc/src/operations/pool/server.rs b/control-plane/grpc/src/operations/pool/server.rs index ef9cb8a2f..0df74a58a 100644 --- a/control-plane/grpc/src/operations/pool/server.rs +++ b/control-plane/grpc/src/operations/pool/server.rs @@ -2,10 +2,11 @@ use crate::{ operations::pool::traits::PoolOperations, pool, pool::{ - create_pool_reply, get_pools_reply, + create_pool_reply, get_pools_reply, label_pool_reply, pool_grpc_server::{PoolGrpc, PoolGrpcServer}, - CreatePoolReply, CreatePoolRequest, DestroyPoolReply, DestroyPoolRequest, GetPoolsReply, - GetPoolsRequest, + unlabel_pool_reply, CreatePoolReply, CreatePoolRequest, DestroyPoolReply, + DestroyPoolRequest, GetPoolsReply, GetPoolsRequest, LabelPoolReply, LabelPoolRequest, + UnlabelPoolReply, UnlabelPoolRequest, }, }; use stor_port::types::v0::transport::Filter; @@ -88,4 +89,34 @@ impl PoolGrpc for PoolServer { })), } } + + async fn label_pool( + &self, + request: tonic::Request, + ) -> Result, tonic::Status> { + let req: LabelPoolRequest = request.into_inner(); + match self.service.label(&req, None).await { + Ok(pool) => Ok(Response::new(LabelPoolReply { + reply: Some(label_pool_reply::Reply::Pool(pool.into())), + })), + Err(err) => Ok(Response::new(LabelPoolReply { + reply: Some(label_pool_reply::Reply::Error(err.into())), + })), + } + } + + async fn unlabel_pool( + &self, + request: tonic::Request, + ) -> Result, tonic::Status> { + let req: UnlabelPoolRequest = request.into_inner(); + match self.service.unlabel(&req, None).await { + Ok(pool) => Ok(Response::new(UnlabelPoolReply { + reply: Some(unlabel_pool_reply::Reply::Pool(pool.into())), + })), + Err(err) => Ok(Response::new(UnlabelPoolReply { + reply: Some(unlabel_pool_reply::Reply::Error(err.into())), + })), + } + } } diff --git a/control-plane/grpc/src/operations/pool/traits.rs b/control-plane/grpc/src/operations/pool/traits.rs index 46b35f983..1c0c653c3 100644 --- a/control-plane/grpc/src/operations/pool/traits.rs +++ b/control-plane/grpc/src/operations/pool/traits.rs @@ -3,22 +3,26 @@ use crate::{ context::Context, misc::traits::StringValue, pool, - pool::{get_pools_request, CreatePoolRequest, DestroyPoolRequest}, + pool::{ + get_pools_request, CreatePoolRequest, DestroyPoolRequest, LabelPoolRequest, + UnlabelPoolRequest, + }, }; -use std::convert::TryFrom; use stor_port::{ transport_api::{v0::Pools, ReplyError, ResourceKind}, types::v0::{ store::pool::{PoolLabel, PoolSpec, PoolSpecStatus}, transport, transport::{ - CreatePool, CtrlPoolState, DestroyPool, Filter, NodeId, Pool, PoolDeviceUri, PoolId, - PoolState, VolumeId, + CreatePool, CtrlPoolState, DestroyPool, Filter, LabelPool, NodeId, Pool, PoolDeviceUri, + PoolId, PoolState, UnlabelPool, VolumeId, }, }, IntoOption, }; +use std::{collections::HashMap, convert::TryFrom}; + /// Trait implemented by services which support pool operations. #[tonic::async_trait] pub trait PoolOperations: Send + Sync { @@ -36,6 +40,18 @@ pub trait PoolOperations: Send + Sync { ) -> Result<(), ReplyError>; /// Get pools based on the filters async fn get(&self, filter: Filter, ctx: Option) -> Result; + /// Associate the labels with the given pool. + async fn label( + &self, + pool: &dyn LabelPoolInfo, + ctx: Option, + ) -> Result; + /// Remove label from the a given pool. + async fn unlabel( + &self, + pool: &dyn UnlabelPoolInfo, + ctx: Option, + ) -> Result; } impl TryFrom for PoolSpec { @@ -390,3 +406,107 @@ impl From for common::SpecStatus { } } } + +/// LabelPoolInfo trait for the pool labeling to be implemented by entities which want +/// to avail this operation +pub trait LabelPoolInfo: Send + Sync + std::fmt::Debug { + /// Id of the pool. + fn pool_id(&self) -> PoolId; + /// Labels to be set on the pool. + fn labels(&self) -> HashMap; + /// Overwrite the existing labels. + fn overwrite(&self) -> bool; +} + +impl LabelPoolInfo for LabelPool { + fn pool_id(&self) -> PoolId { + self.pool_id.clone() + } + + fn labels(&self) -> HashMap { + self.labels.clone() + } + + fn overwrite(&self) -> bool { + self.overwrite + } +} + +impl LabelPoolInfo for LabelPoolRequest { + fn pool_id(&self) -> PoolId { + self.pool_id.clone().into() + } + + fn labels(&self) -> HashMap { + self.labels.clone() + } + + fn overwrite(&self) -> bool { + self.overwrite + } +} + +impl From<&dyn LabelPoolInfo> for LabelPoolRequest { + fn from(data: &dyn LabelPoolInfo) -> Self { + Self { + pool_id: data.pool_id().to_string(), + labels: data.labels().clone(), + overwrite: data.overwrite(), + } + } +} + +impl From<&dyn LabelPoolInfo> for LabelPool { + fn from(data: &dyn LabelPoolInfo) -> Self { + Self { + pool_id: data.pool_id(), + labels: data.labels(), + overwrite: data.overwrite(), + } + } +} + +/// UnlabelPoolInfo trait for the pool unlabeling to be implemented by entities which want to avail +/// this operation +pub trait UnlabelPoolInfo: Send + Sync + std::fmt::Debug { + /// Id of the pool. + fn pool_id(&self) -> PoolId; + /// Key of the label to be removed. + fn label_key(&self) -> String; +} + +impl UnlabelPoolInfo for UnlabelPool { + fn pool_id(&self) -> PoolId { + self.pool_id.clone() + } + fn label_key(&self) -> String { + self.label_key.clone() + } +} + +impl UnlabelPoolInfo for UnlabelPoolRequest { + fn pool_id(&self) -> PoolId { + self.pool_id.clone().into() + } + fn label_key(&self) -> String { + self.label_key.clone() + } +} + +impl From<&dyn UnlabelPoolInfo> for UnlabelPoolRequest { + fn from(data: &dyn UnlabelPoolInfo) -> Self { + Self { + pool_id: data.pool_id().to_string(), + label_key: data.label_key().clone(), + } + } +} + +impl From<&dyn UnlabelPoolInfo> for UnlabelPool { + fn from(data: &dyn UnlabelPoolInfo) -> Self { + Self { + pool_id: data.pool_id(), + label_key: data.label_key(), + } + } +} From f87c28cad46bbbf099a2163b23fbd2453c85ca0c Mon Sep 17 00:00:00 2001 From: sinhaashish Date: Sun, 21 Jul 2024 18:37:35 +0000 Subject: [PATCH 3/5] feat(topology): code to add label to pool in rest Signed-off-by: sinhaashish --- .../rest/openapi-specs/v0_api_spec.yaml | 79 +++++++++++++++++++ control-plane/rest/service/src/v0/pools.rs | 30 ++++++- control-plane/rest/src/versions/v0.rs | 18 ++++- 3 files changed, 123 insertions(+), 4 deletions(-) diff --git a/control-plane/rest/openapi-specs/v0_api_spec.yaml b/control-plane/rest/openapi-specs/v0_api_spec.yaml index 663df79cc..6d42b70a2 100644 --- a/control-plane/rest/openapi-specs/v0_api_spec.yaml +++ b/control-plane/rest/openapi-specs/v0_api_spec.yaml @@ -1320,6 +1320,85 @@ paths: $ref: '#/components/responses/ServerError' security: - JWT: [] + '/pools/{pool_id}/label/{key}={value}': + put: + tags: + - Pools + operationId: put_pool_label + description: |- + Add labels to pool. + parameters: + - in: path + name: pool_id + required: true + schema: + $ref: '#/components/schemas/PoolId' + - in: path + name: key + required: true + schema: + type: string + description: |- + The key of the label to be added. + - in: path + name: value + required: true + schema: + type: string + description: |- + The value of the label to be added. + - in: query + name: overwrite + description: |- + Overwrite existing label if the label key exists. + required: false + schema: + type: boolean + default: false + responses: + '200': + description: OK + content: + application/json: + schema: + $ref: '#/components/schemas/Pool' + '4XX': + $ref: '#/components/responses/ClientError' + '5XX': + $ref: '#/components/responses/ServerError' + security: + - JWT: [] + '/pools/{id}/label/{key}': + delete: + tags: + - Pools + operationId: del_pool_label + parameters: + - in: path + name: id + required: true + schema: + $ref: '#/components/schemas/PoolId' + - in: path + name: key + required: true + schema: + type: string + description: |- + The key of the label to be removed. + responses: + '200': + description: OK + content: + application/json: + schema: + $ref: '#/components/schemas/Pool' + '4XX': + $ref: '#/components/responses/ClientError' + '5XX': + $ref: '#/components/responses/ServerError' + security: + - JWT: [] /replicas: get: tags: diff --git a/control-plane/rest/service/src/v0/pools.rs b/control-plane/rest/service/src/v0/pools.rs index 66ded21fa..851bb3120 100644 --- a/control-plane/rest/service/src/v0/pools.rs +++ b/control-plane/rest/service/src/v0/pools.rs @@ -1,7 +1,8 @@ use super::*; use grpc::operations::pool::traits::PoolOperations; use rest_client::versions::v0::apis::Uuid; -use stor_port::types::v0::transport::{DestroyPool, Filter}; +use std::collections::HashMap; +use stor_port::types::v0::transport::{DestroyPool, Filter, UnlabelPool}; use transport_api::{ReplyError, ReplyErrorKind, ResourceKind}; fn client() -> impl PoolOperations { @@ -103,6 +104,33 @@ impl apis::actix_server::Pools for RestApi { let pool = client().create(&create, None).await?; Ok(pool.into()) } + + async fn put_pool_label( + Path((pool_id, key, value)): Path<(String, String, String)>, + Query(overwrite): Query>, + ) -> Result> { + let labels = HashMap::from([(key, value)]); + let label_pool_request = LabelPool { + pool_id: pool_id.into(), + labels, + overwrite: overwrite.unwrap_or(false), + }; + + let pool = client().label(&label_pool_request, None).await?; + Ok(pool.into()) + } + + async fn del_pool_label( + Path((pool_id, label_key)): Path<(String, String)>, + ) -> Result> { + let unlabel_pool_request = UnlabelPool { + pool_id: pool_id.into(), + label_key, + }; + + let pool = client().unlabel(&unlabel_pool_request, None).await?; + Ok(pool.into()) + } } /// returns pool from pool option and returns an error on non existence diff --git a/control-plane/rest/src/versions/v0.rs b/control-plane/rest/src/versions/v0.rs index 56de372f1..20c5a3183 100644 --- a/control-plane/rest/src/versions/v0.rs +++ b/control-plane/rest/src/versions/v0.rs @@ -1,7 +1,10 @@ #![allow(clippy::field_reassign_with_default)] use super::super::RestClient; -use std::convert::{TryFrom, TryInto}; +use std::{ + collections::HashMap, + convert::{TryFrom, TryInto}, +}; pub use stor_port::{ transport_api, @@ -11,8 +14,8 @@ pub use stor_port::{ transport::{ AddNexusChild, BlockDevice, Child, ChildUri, CreateNexus, CreatePool, CreateReplica, CreateVolume, DestroyNexus, DestroyPool, DestroyReplica, DestroyVolume, Filter, - GetBlockDevices, JsonGrpcRequest, Nexus, NexusId, NexusShareProtocol, Node, NodeId, - Pool, PoolDeviceUri, PoolId, Protocol, RemoveNexusChild, Replica, ReplicaId, + GetBlockDevices, JsonGrpcRequest, LabelPool, Nexus, NexusId, NexusShareProtocol, Node, + NodeId, Pool, PoolDeviceUri, PoolId, Protocol, RemoveNexusChild, Replica, ReplicaId, ReplicaShareProtocol, ShareNexus, ShareReplica, Specs, Topology, UnshareNexus, UnshareReplica, VolumeId, VolumeLabels, VolumePolicy, VolumeProperty, Watch, WatchCallback, WatchResourceId, @@ -273,3 +276,12 @@ impl From for SetVolumePropertyBody { } } } + +/// Convert into rpc request type. +pub fn to_request(pool_id: PoolId, labels: HashMap, overwrite: bool) -> LabelPool { + LabelPool { + pool_id, + labels, + overwrite, + } +} From 9521a2bde44a7fc89f8f579541222e9bcbe29a88 Mon Sep 17 00:00:00 2001 From: sinhaashish Date: Mon, 22 Jul 2024 08:06:59 +0000 Subject: [PATCH 4/5] feat(topology): add python test for pool labels Signed-off-by: sinhaashish --- .../node/label/test_label_unlabel_node.py | 2 +- .../features/pool/label/pool-label.feature | 30 ++ .../pool/label/test_label_unlabel_pool.py | 288 ++++++++++++++++++ 3 files changed, 319 insertions(+), 1 deletion(-) create mode 100644 tests/bdd/features/pool/label/pool-label.feature create mode 100644 tests/bdd/features/pool/label/test_label_unlabel_pool.py diff --git a/tests/bdd/features/node/label/test_label_unlabel_node.py b/tests/bdd/features/node/label/test_label_unlabel_node.py index 86a48baeb..6a3b2f5bd 100644 --- a/tests/bdd/features/node/label/test_label_unlabel_node.py +++ b/tests/bdd/features/node/label/test_label_unlabel_node.py @@ -28,7 +28,7 @@ # Fixtures @pytest.fixture(scope="module") def init(): - Deployer.start(NUM_IO_ENGINES, io_engine_env="MAYASTOR_HB_INTERVAL_SEC=0") + Deployer.start(NUM_IO_ENGINES) yield Deployer.stop() diff --git a/tests/bdd/features/pool/label/pool-label.feature b/tests/bdd/features/pool/label/pool-label.feature new file mode 100644 index 000000000..de75d0e41 --- /dev/null +++ b/tests/bdd/features/pool/label/pool-label.feature @@ -0,0 +1,30 @@ +Feature: Label a pool, this will be used while scheduling replica of volume considering the + pool topology + + Background: + Given a control plane, two Io-Engine instances, two pools + + Scenario: Label a pool + Given an unlabeled pool + When the user issues a label command with a label to the pool + Then the given pool should be labeled with the given label + + Scenario: Label a pool when label key already exist and overwrite is false + Given a labeled pool + When the user attempts to label the same pool with the same label key with overwrite as false + Then the pool label should fail with error "PRECONDITION_FAILED" + + Scenario: Label a pool when label key already exist and overwrite is true + Given a labeled pool + When the user attempts to label the same pool with the same label key and overwrite as true + Then the given pool should be labeled with the new given label + + Scenario: Unlabel a pool + Given a labeled pool + When the user issues a unlabel command with a label key present as label for the pool + Then the given pool should remove the label with the given key + + Scenario: Unlabel a pool when the label key is not present + Given a labeled pool + When the user issues an unlabel command with a label key that is not currently associated with the pool + Then the unlabel operation for the pool should fail with error PRECONDITION_FAILED \ No newline at end of file diff --git a/tests/bdd/features/pool/label/test_label_unlabel_pool.py b/tests/bdd/features/pool/label/test_label_unlabel_pool.py new file mode 100644 index 000000000..4d87ea765 --- /dev/null +++ b/tests/bdd/features/pool/label/test_label_unlabel_pool.py @@ -0,0 +1,288 @@ +"""Label a pool, this will be used while scheduling replica of volume considering the feature tests.""" + +from pytest_bdd import ( + given, + scenario, + then, + when, +) + + +import docker +import pytest +import sys +import http + + +from pytest_bdd import given, scenario, then, when +from common.deployer import Deployer +from common.apiclient import ApiClient +from common.docker import Docker +from common.etcd import Etcd +from common.operations import Cluster, wait_node_online +from openapi.model.node import Node +from openapi.exceptions import ApiException +from openapi.model.create_pool_body import CreatePoolBody +from openapi.model.pool import Pool + + +NUM_IO_ENGINES = 2 +LABEL1 = "KEY1=VALUE1" +LABEL1_NEW = "KEY1=NEW_LABEL" +LABEL2_NEW = "KEY2=NEW_LABEL" +LABEL1_MAP = "{'KEY1': 'VALUE1'}" +LABEL2 = "KEY2=VALUE2" +LABEL_1_NEW = "KEY1=NEW_LABEL" +LABEL_KEY_TO_DELETE = "KEY2" +LABEL_KEY_TO_DELETE_ABSENT = "ABSENT_KEY" + + +VOLUME_SIZE = 10485761 +NUM_VOLUME_REPLICAS = 1 +CREATE_REQUEST_KEY = "create_request" +POOL_1_UUID = "4cc6ee64-7232-497d-a26f-38284a444980" +NODE_1_NAME = "io-engine-1" +POOL_2_UUID = "24d36c1a-3e6c-4e05-893d-917ec9f4c1bb" +NODE_2_NAME = "io-engine-2" +REPLICA_CONTEXT_KEY = "replica" +REPLICA_ERROR = "replica_error" + + +@pytest.fixture(scope="module") +def init(): + Deployer.start(NUM_IO_ENGINES, io_engine_env="MAYASTOR_HB_INTERVAL_SEC=0") + ApiClient.pools_api().put_node_pool( + NODE_1_NAME, + POOL_1_UUID, + CreatePoolBody(["malloc:///disk?size_mb=50"]), + ) + ApiClient.pools_api().put_node_pool( + NODE_2_NAME, + POOL_2_UUID, + CreatePoolBody( + ["malloc:///disk?size_mb=50"], + labels={ + "KEY2": "VALUE2", + }, + ), + ) + yield + Deployer.stop() + + +@pytest.fixture(scope="function") +def context(): + return {} + + +@pytest.fixture(scope="function") +def pool(context): + yield context["pool"] + + +@scenario("pool-label.feature", "Label a pool") +def test_label_a_pool(): + """Label a pool.""" + + +@scenario( + "pool-label.feature", + "Label a pool when label key already exist and overwrite is false", +) +def test_label_a_pool_when_label_key_already_exist_and_overwrite_is_false(): + """Label a pool when label key already exist and overwrite is false.""" + + +@scenario( + "pool-label.feature", + "Label a pool when label key already exist and overwrite is true", +) +def test_label_a_pool_when_label_key_already_exist_and_overwrite_is_true(): + """Label a pool when label key already exist and overwrite is true.""" + + +@scenario("pool-label.feature", "Unlabel a pool") +def test_unlabel_a_pool(): + """Unlabel a pool.""" + + +@scenario("pool-label.feature", "Unlabel a pool when the label key is not present") +def test_unlabel_a_pool_when_the_label_key_is_not_present(): + """Unlabel a pool when the label key is not present.""" + + +@given("a control plane, two Io-Engine instances, two pools") +def a_control_plane_two_ioengine_instances_two_pools(init): + """a control plane, two Io-Engine instances, two pools.""" + docker_client = docker.from_env() + + # The control plane comprises the core agents, rest server and etcd instance. + for component in ["core", "rest", "etcd"]: + Docker.check_container_running(component) + + # Check all Io-Engine instances are running + try: + io_engines = docker_client.containers.list( + all=True, filters={"name": "io-engine"} + ) + + except docker.errors.NotFound: + raise Exception("No Io-Engine instances") + + for io_engine in io_engines: + Docker.check_container_running(io_engine.attrs["Name"]) + + # Check for a pools + pools = ApiClient.pools_api().get_pools() + assert len(pools) == 2 + Cluster.cleanup(pools=False) + + +@given("an unlabeled pool") +def an_unlabeled_pool(): + """an unlabeled pool.""" + pool = ApiClient.pools_api().get_pool(POOL_1_UUID) + assert not "labels" in pool.spec + + +@given("a labeled pool") +def a_labeled_pool(context): + """a labeled pool.""" + pool = ApiClient.pools_api().get_pool(POOL_2_UUID) + assert "labels" in pool.spec + context["pool"] = pool + + +@when("the user issues a label command with a label to the pool") +def the_user_issues_a_label_command_with_a_label_to_the_pool(attempt_add_label_pool): + """the user issues a label command with a label to the pool.""" + + +@when( + "the user attempts to label the same pool with the same label key with overwrite as false" +) +def the_user_attempts_to_label_the_same_pool_with_the_same_label_key_with_overwrite_as_false( + context, pool +): + """the user attempts to label the same pool with the same label key with overwrite as false.""" + attempt_add_label(pool.id, LABEL2_NEW, False, context) + + +@when( + "the user attempts to label the same pool with the same label key and overwrite as true" +) +def the_user_attempts_to_label_the_same_pool_with_the_same_label_key_and_overwrite_as_true( + context, pool +): + """the user attempts to label the same pool with the same label key and overwrite as true.""" + attempt_add_label(pool.id, LABEL2_NEW, True, context) + + +@when( + "the user issues a unlabel command with a label key present as label for the pool" +) +def the_user_issues_a_unlabel_command_with_a_label_key_present_as_label_for_the_pool( + context, pool +): + """the user issues a unlabel command with a label key present as label for the pool.""" + + +@when( + "the user issues an unlabel command with a label key that is not currently associated with the pool" +) +def the_user_issues_an_unlabel_command_with_a_label_key_that_is_not_currently_associated_with_the_pool( + context, pool +): + """the user issues an unlabel command with a label key that is not currently associated with the pool.""" + attempt_delete_label(pool.id, LABEL_KEY_TO_DELETE_ABSENT, context) + + +@then("the given pool should be labeled with the given label") +def the_given_pool_should_be_labeled_with_the_given_label( + attempt_add_label_pool, context +): + """the given pool should be labeled with the given label.""" + labelling_succeeds(attempt_add_label_pool, context) + + +@then('the pool label should fail with error "PRECONDITION_FAILED"') +def the_pool_label_should_fail_with_error_precondition_failed(pool_attempt): + """the pool label should fail with error "PRECONDITION_FAILED".""" + assert pool_attempt.status == http.HTTPStatus.PRECONDITION_FAILED + assert ApiClient.exception_to_error(pool_attempt).kind == "FailedPrecondition" + + +@then("the given pool should be labeled with the new given label") +def the_given_pool_should_be_labeled_with_the_new_given_label( + attempt_add_label_one_with_overwrite, context +): + """the given pool should be labeled with the new given label.""" + labelling_succeeds(attempt_add_label_one_with_overwrite, context) + + +@then("the given pool should remove the label with the given key") +def the_given_pool_should_remove_the_label_with_the_given_key( + attempt_delete_label_of_pool, context +): + """the given pool should remove the label with the given key.""" + labelling_succeeds(attempt_delete_label_of_pool, context) + + +@then("the unlabel operation for the pool should fail with error PRECONDITION_FAILED") +def the_unlabel_operation_for_the_pool_should_fail_with_error_precondition_failed( + pool_attempt, +): + """the unlabel operation for the pool should fail with error PRECONDITION_FAILED.""" + assert pool_attempt.status == http.HTTPStatus.PRECONDITION_FAILED + assert ApiClient.exception_to_error(pool_attempt).kind == "FailedPrecondition" + + +@pytest.fixture(scope="function") +def pool_attempt(context): + return context["attempt_result"] + + +@pytest.fixture +def attempt_add_label_pool(context): + yield attempt_add_label(POOL_1_UUID, LABEL1, False, context) + + +@pytest.fixture +def attempt_add_label_one_with_overwrite(context): + yield attempt_add_label(POOL_2_UUID, LABEL2_NEW, True, context) + + +@pytest.fixture +def attempt_delete_label_of_pool(context): + yield attempt_delete_label(POOL_2_UUID, LABEL_KEY_TO_DELETE, context) + + +def attempt_add_label(pool_name, label, overwrite, context): + try: + [key, value] = label.split("=") + overwrite = "true" if overwrite else "false" + pool = ApiClient.pools_api().put_pool_label( + pool_name, key, value, overwrite=overwrite + ) + context["pool"] = pool + return pool + except ApiException as exception: + context["attempt_result"] = exception + return exception + + +def attempt_delete_label(pool_name, label, context): + try: + pool = ApiClient.pools_api().del_pool_label(pool_name, label) + context["pool"] = pool + return pool + except ApiException as exception: + context["attempt_result"] = exception + return exception + + +def labelling_succeeds(result, context): + # raise result for exception information + assert isinstance(result, Pool) + ApiClient.pools_api().get_pool(result.id) + context["pool"] = result From b3614c6a51e18fe5c9a6887745cf12b41cd9ac2e Mon Sep 17 00:00:00 2001 From: sinhaashish Date: Mon, 22 Jul 2024 09:12:27 +0000 Subject: [PATCH 5/5] feat(topology): add label pool code in plugin Signed-off-by: sinhaashish --- control-plane/plugin/src/lib.rs | 5 + control-plane/plugin/src/resources/error.rs | 77 ++++++++++- control-plane/plugin/src/resources/mod.rs | 18 +++ control-plane/plugin/src/resources/node.rs | 135 ++++---------------- control-plane/plugin/src/resources/pool.rs | 108 +++++++++++++++- control-plane/plugin/src/resources/utils.rs | 99 ++++++++++++++ 6 files changed, 325 insertions(+), 117 deletions(-) diff --git a/control-plane/plugin/src/lib.rs b/control-plane/plugin/src/lib.rs index 29500a2d1..89f559ec1 100644 --- a/control-plane/plugin/src/lib.rs +++ b/control-plane/plugin/src/lib.rs @@ -244,6 +244,11 @@ impl ExecuteOperation for LabelResources { label, overwrite, } => node::Node::label(id, label.to_string(), *overwrite, &cli_args.output).await, + LabelResources::Pool { + id, + label, + overwrite, + } => pool::Pool::label(id, label.to_string(), *overwrite, &cli_args.output).await, } } } diff --git a/control-plane/plugin/src/resources/error.rs b/control-plane/plugin/src/resources/error.rs index 173064735..a9c4876a0 100644 --- a/control-plane/plugin/src/resources/error.rs +++ b/control-plane/plugin/src/resources/error.rs @@ -1,4 +1,3 @@ -use crate::resources::node; use snafu::Snafu; /// All errors returned when resources command fails. @@ -25,9 +24,14 @@ pub enum Error { source: openapi::tower::client::Error, }, #[snafu(display("Invalid label format: {source}"))] - NodeLabelFormat { source: node::TopologyError }, + NodeLabelFormat { source: TopologyError }, #[snafu(display("{source}"))] - NodeLabel { source: node::OpError }, + NodeLabel { source: OpError }, + #[snafu(display("Invalid label format: {source}"))] + PoolLabelFormat { source: TopologyError }, + #[snafu(display("{source}"))] + PoolLabel { source: OpError }, + /// Error when node uncordon request fails. #[snafu(display("Failed to uncordon node {id}. Error {source}"))] NodeUncordonError { @@ -99,3 +103,70 @@ pub enum Error { ))] LabelNodeFilter { labels: String }, } + +/// Errors related to label topology formats. +#[derive(Debug, Snafu)] +#[snafu(visibility(pub))] +pub enum TopologyError { + #[snafu(display("key must not be an empty string"))] + KeyIsEmpty {}, + #[snafu(display("value must not be an empty string"))] + ValueIsEmpty {}, + #[snafu(display("key part must not be more than 63 characters"))] + KeyTooLong {}, + #[snafu(display("value part must not be more than 63 characters"))] + ValueTooLong {}, + #[snafu(display("both key and value parts must start with an ascii alphanumeric character"))] + EdgesNotAlphaNum {}, + #[snafu(display("key can contain at most one `/` character"))] + KeySlashCount {}, + #[snafu(display( + "only ascii alphanumeric characters and (`/`,` - `, `_`,`.`) are allowed for the key part" + ))] + KeyIsNotAlphaNumericPlus {}, + #[snafu(display( + "only ascii alphanumeric characters and (`-`,` _ `, `.`) are allowed for the label part" + ))] + ValueIsNotAlphaNumericPlus {}, + #[snafu(display("only a single assignment key=value is allowed"))] + LabelMultiAssign {}, + #[snafu(display( + "the supported formats are: \ + key=value for adding (example: group=a) \ + and key- for removing (example: group-)" + ))] + LabelAssign {}, +} + +/// Errors related to node label topology operation execution. +#[derive(Debug, snafu::Snafu)] +#[snafu(visibility(pub))] +pub enum OpError { + #[snafu(display("{resource} {id} not unlabelled as it did not contain the label"))] + LabelNotFound { resource: String, id: String }, + #[snafu(display("{resource} {id} not labelled as the same label already exists"))] + LabelExists { resource: String, id: String }, + #[snafu(display("{resource} {id} not found"))] + ResourceNotFound { resource: String, id: String }, + #[snafu(display( + "{resource} {id} not labelled as the label key already exists, but with a different value and --overwrite is false" + ))] + LabelConflict { resource: String, id: String }, + #[snafu(display("Failed to label {resource} {id}. Error {source}"))] + Generic { + resource: String, + id: String, + source: openapi::tower::client::Error, + }, +} + +impl From for Error { + fn from(source: TopologyError) -> Self { + Self::NodeLabelFormat { source } + } +} +impl From for Error { + fn from(source: OpError) -> Self { + Self::NodeLabel { source } + } +} diff --git a/control-plane/plugin/src/resources/mod.rs b/control-plane/plugin/src/resources/mod.rs index 30d40dc2e..6b2185a60 100644 --- a/control-plane/plugin/src/resources/mod.rs +++ b/control-plane/plugin/src/resources/mod.rs @@ -146,6 +146,24 @@ pub enum LabelResources { #[clap(long)] overwrite: bool, }, + /// Adds or removes a label to or from the specified pool. + Pool { + /// The id of the pool to label/unlabel. + id: PoolId, + /// The label to be added or removed from the pool. + /// 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, + }, } #[derive(clap::Subcommand, Debug)] diff --git a/control-plane/plugin/src/resources/node.rs b/control-plane/plugin/src/resources/node.rs index 2e82875bc..bf935c1ea 100644 --- a/control-plane/plugin/src/resources/node.rs +++ b/control-plane/plugin/src/resources/node.rs @@ -1,9 +1,10 @@ use crate::{ operations::{Cordoning, Drain, GetWithArgs, Label, ListWithArgs, PluginResult}, resources::{ - error::Error, + error::{Error, LabelAssignSnafu, OpError, TopologyError}, utils::{ - self, optional_cell, print_table, CreateRow, CreateRows, GetHeaderRow, OutputFormat, + self, optional_cell, print_table, validate_topology_key, validate_topology_value, + CreateRow, CreateRows, GetHeaderRow, OutputFormat, }, NodeId, }, @@ -677,109 +678,6 @@ impl Drain for Node { } } -/// Errors related to node label topology formats. -#[derive(Debug, snafu::Snafu)] -pub enum TopologyError { - #[snafu(display("key must not be an empty string"))] - KeyIsEmpty {}, - #[snafu(display("value must not be an empty string"))] - ValueIsEmpty {}, - #[snafu(display("key part must no more than 63 characters"))] - KeyTooLong {}, - #[snafu(display("value part must no more than 63 characters"))] - ValueTooLong {}, - #[snafu(display("both key and value parts must start with an ascii alphanumeric character"))] - EdgesNotAlphaNum {}, - #[snafu(display("key can contain at most one / character"))] - KeySlashCount {}, - #[snafu(display( - "only ascii alphanumeric characters and (/ - _ .) are allowed for the key part" - ))] - KeyIsNotAlphaNumericPlus {}, - #[snafu(display( - "only ascii alphanumeric characters and (- _ .) are allowed for the label part" - ))] - ValueIsNotAlphaNumericPlus {}, - #[snafu(display("only a single assignment key=value is allowed"))] - LabelMultiAssign {}, - #[snafu(display( - "the supported formats are: \ - key=value for adding (example: group=a) \ - and key- for removing (example: group-)" - ))] - LabelAssign {}, -} - -/// Errors related to node label topology operation execution. -#[derive(Debug, snafu::Snafu)] -pub enum OpError { - #[snafu(display("Node {id} not unlabelled as it did not contain the label"))] - LabelNotFound { id: String }, - #[snafu(display("Node {id} not labelled as the same label already exists"))] - LabelExists { id: String }, - #[snafu(display("Node {id} not found"))] - NodeNotFound { id: String }, - #[snafu(display( - "Node {id} not labelled as the label key already exists, but with a different value and --overwrite is false" - ))] - LabelConflict { id: String }, - #[snafu(display("Failed to label node {id}. Error {source}"))] - Generic { - id: String, - source: openapi::tower::client::Error, - }, -} - -impl From for Error { - fn from(source: TopologyError) -> Self { - Self::NodeLabelFormat { source } - } -} -impl From for Error { - fn from(source: OpError) -> Self { - Self::NodeLabel { source } - } -} - -fn allowed_topology_chars(key: char) -> bool { - key.is_ascii_alphanumeric() || matches!(key, '_' | '-' | '.') -} -fn allowed_topology_tips(label: &str) -> bool { - fn allowed_topology_tips_chars(char: Option) -> bool { - char.map(|c| c.is_ascii_alphanumeric()).unwrap_or(true) - } - - allowed_topology_tips_chars(label.chars().next()) - && allowed_topology_tips_chars(label.chars().last()) -} -fn validate_topology_key(key: &str) -> Result<(), TopologyError> { - snafu::ensure!(!key.is_empty(), KeyIsEmptySnafu); - snafu::ensure!(key.len() <= 63, KeyTooLongSnafu); - snafu::ensure!(allowed_topology_tips(key), EdgesNotAlphaNumSnafu); - - snafu::ensure!( - key.chars().filter(|c| c == &'/').count() <= 1, - KeySlashCountSnafu - ); - - snafu::ensure!( - key.chars().all(|c| allowed_topology_chars(c) || c == '/'), - KeyIsNotAlphaNumericPlusSnafu - ); - - Ok(()) -} -fn validate_topology_value(value: &str) -> Result<(), TopologyError> { - snafu::ensure!(!value.is_empty(), ValueIsEmptySnafu); - snafu::ensure!(value.len() <= 63, ValueTooLongSnafu); - snafu::ensure!(allowed_topology_tips(value), EdgesNotAlphaNumSnafu); - snafu::ensure!( - value.chars().all(allowed_topology_chars), - ValueIsNotAlphaNumericPlusSnafu - ); - Ok(()) -} - #[async_trait(?Send)] impl Label for Node { type ID = NodeId; @@ -803,15 +701,25 @@ impl Label for Node { { Err(source) => match source.status() { Some(StatusCode::UNPROCESSABLE_ENTITY) if output.none() => { - Err(OpError::LabelExists { id: id.to_string() }) + Err(OpError::LabelExists { + resource: "Node".to_string(), + id: id.to_string(), + }) } Some(StatusCode::PRECONDITION_FAILED) if output.none() => { - Err(OpError::LabelConflict { id: id.to_string() }) + Err(OpError::LabelConflict { + resource: "Node".to_string(), + id: id.to_string(), + }) } Some(StatusCode::NOT_FOUND) if output.none() => { - Err(OpError::NodeNotFound { id: id.to_string() }) + Err(OpError::ResourceNotFound { + resource: "Node".to_string(), + id: id.to_string(), + }) } _ => Err(OpError::Generic { + resource: "Node".to_string(), id: id.to_string(), source, }), @@ -829,12 +737,19 @@ impl Label for Node { { Err(source) => match source.status() { Some(StatusCode::PRECONDITION_FAILED) if output.none() => { - Err(OpError::LabelNotFound { id: id.to_string() }) + Err(OpError::LabelNotFound { + resource: "Node".to_string(), + id: id.to_string(), + }) } Some(StatusCode::NOT_FOUND) if output.none() => { - Err(OpError::NodeNotFound { id: id.to_string() }) + Err(OpError::ResourceNotFound { + resource: "Node".to_string(), + id: id.to_string(), + }) } _ => Err(OpError::Generic { + resource: "Node".to_string(), id: id.to_string(), source, }), diff --git a/control-plane/plugin/src/resources/pool.rs b/control-plane/plugin/src/resources/pool.rs index 037888870..d63e57d70 100644 --- a/control-plane/plugin/src/resources/pool.rs +++ b/control-plane/plugin/src/resources/pool.rs @@ -1,15 +1,20 @@ use crate::{ - operations::{Get, ListWithArgs, PluginResult}, + operations::{Get, Label, ListWithArgs, PluginResult}, resources::{ - error::Error, + error::{Error, LabelAssignSnafu, OpError, TopologyError}, utils, - utils::{CreateRow, GetHeaderRow}, + utils::{ + optional_cell, print_table, validate_topology_key, validate_topology_value, CreateRow, + GetHeaderRow, OutputFormat, + }, NodeId, PoolId, }, rest_wrapper::RestClient, }; use async_trait::async_trait; +use openapi::apis::StatusCode; use prettytable::Row; +use snafu::ResultExt; use std::collections::HashMap; use super::VolumeId; @@ -50,7 +55,7 @@ impl CreateRow for openapi::models::Pool { ::utils::bytes::into_human(state.capacity), ::utils::bytes::into_human(state.used), ::utils::bytes::into_human(free), - utils::optional_cell(state.committed.map(::utils::bytes::into_human)), + optional_cell(state.committed.map(::utils::bytes::into_human)), ] } } @@ -179,3 +184,98 @@ pub(crate) fn labels_matched( } Ok(true) } + +#[async_trait(?Send)] +impl Label for Pool { + type ID = PoolId; + async fn label( + id: &Self::ID, + label: String, + overwrite: bool, + output: &utils::OutputFormat, + ) -> PluginResult { + let result = if label.contains('=') { + let [key, value] = label.split('=').collect::>()[..] else { + return Err(TopologyError::LabelMultiAssign {}.into()); + }; + + validate_topology_key(key).context(super::error::PoolLabelFormatSnafu)?; + validate_topology_value(value).context(super::error::PoolLabelFormatSnafu)?; + match RestClient::client() + .pools_api() + .put_pool_label(id, key, value, Some(overwrite)) + .await + { + Err(source) => match source.status() { + Some(StatusCode::UNPROCESSABLE_ENTITY) if output.none() => { + Err(OpError::LabelExists { + resource: "Pool".to_string(), + id: id.to_string(), + }) + } + Some(StatusCode::PRECONDITION_FAILED) if output.none() => { + Err(OpError::LabelConflict { + resource: "Pool".to_string(), + id: id.to_string(), + }) + } + Some(StatusCode::NOT_FOUND) if output.none() => { + Err(OpError::ResourceNotFound { + resource: "Pool".to_string(), + id: id.to_string(), + }) + } + _ => Err(OpError::Generic { + resource: "Pool".to_string(), + id: id.to_string(), + source, + }), + }, + Ok(pool) => Ok(pool), + } + } else { + snafu::ensure!(label.len() >= 2 && label.ends_with('-'), LabelAssignSnafu); + let key = &label[.. label.len() - 1]; + validate_topology_key(key)?; + match RestClient::client() + .pools_api() + .del_pool_label(id, key) + .await + { + Err(source) => match source.status() { + Some(StatusCode::PRECONDITION_FAILED) if output.none() => { + Err(OpError::LabelNotFound { + resource: "Pool".to_string(), + id: id.to_string(), + }) + } + Some(StatusCode::NOT_FOUND) if output.none() => { + Err(OpError::ResourceNotFound { + resource: "Pool".to_string(), + id: id.to_string(), + }) + } + _ => Err(OpError::Generic { + resource: "Pool".to_string(), + id: id.to_string(), + source, + }), + }, + Ok(pool) => Ok(pool), + } + }?; + let pool = result.into_body(); + match output { + OutputFormat::Yaml | OutputFormat::Json => { + // Print json or yaml based on output format. + print_table(output, pool); + } + OutputFormat::None => { + // In case the output format is not specified, show a success message. + let labels = pool.spec.unwrap().labels.unwrap_or_default(); + println!("Pool {id} labelled successfully. Current labels: {labels:?}"); + } + } + Ok(()) + } +} diff --git a/control-plane/plugin/src/resources/utils.rs b/control-plane/plugin/src/resources/utils.rs index aaa165d27..de1d43836 100644 --- a/control-plane/plugin/src/resources/utils.rs +++ b/control-plane/plugin/src/resources/utils.rs @@ -1,3 +1,8 @@ +use crate::resources::error::{ + EdgesNotAlphaNumSnafu, KeyIsEmptySnafu, KeyIsNotAlphaNumericPlusSnafu, KeySlashCountSnafu, + KeyTooLongSnafu, TopologyError, ValueIsEmptySnafu, ValueIsNotAlphaNumericPlusSnafu, + ValueTooLongSnafu, +}; use prettytable::{format, Row, Table}; use serde::ser; @@ -202,3 +207,97 @@ where } } } + +/// Checks if a given character is allowed in topology keys. +/// +/// # Description +/// This function determines if a provided character is permissible for use in topology keys. +/// The allowed characters are: +/// - ASCII alphanumeric characters (letters and digits) +/// - Special characters: underscore (`_`), hyphen (`-`), and period (`.`). +/// +/// # Parameters +/// - `key`: A `char` representing the character to check. +/// +/// # Returns +/// Returns `true` if the character is allowed in topology keys; otherwise, returns `false`. +/// +/// # Examples +/// ``` +/// assert_eq!(allowed_topology_chars('a'), true); // ASCII letter +/// assert_eq!(allowed_topology_chars('1'), true); // ASCII digit +/// assert_eq!(allowed_topology_chars('_'), true); // Underscore +/// assert_eq!(allowed_topology_chars('-'), true); // Hyphen +/// assert_eq!(allowed_topology_chars('.'), true); // Period +/// assert_eq!(allowed_topology_chars('!'), false); // Not allowed +/// assert_eq!(allowed_topology_chars(' '), false); // Space is not allowed +/// assert_eq!(allowed_topology_chars('@'), false); // Special character not allowed +/// ``` +pub fn allowed_topology_chars(key: char) -> bool { + key.is_ascii_alphanumeric() || matches!(key, '_' | '-' | '.') +} + +/// Determines if the provided label string has allowed topology tips. +/// +/// # Description +/// This function checks whether the first and last characters of a given string (`label`) are valid +/// for use as topology tips. A valid character for the tips of the string is an ASCII alphanumeric +/// character. If the string is empty, it will return `true`. +/// +/// Internally, this function uses a helper function to verify the first and last characters +/// by mapping them to their alphanumeric status or defaulting to `true` if the character is `None`. +pub fn allowed_topology_tips(label: &str) -> bool { + fn allowed_topology_tips_chars(char: Option) -> bool { + char.map(|c| c.is_ascii_alphanumeric()).unwrap_or(true) + } + + allowed_topology_tips_chars(label.chars().next()) + && allowed_topology_tips_chars(label.chars().last()) +} + +/// Validates a topology key based on specific criteria. +/// +/// # Description +/// This function validates a given topology key string to ensure it adheres to a set of predefined +/// rules. These rules include checks for the key's length, content, and structure. The key is +/// considered valid if: +/// - It is not empty. +/// - It does not exceed 63 characters in length. +/// - The first and last characters are ASCII alphanumeric. +/// - It contains at most one slash ('/') character. +/// - All characters are ASCII alphanumeric, underscore ('_'), hyphen ('-'), period ('.'), or a +/// slash ('/'). +/// +/// If any of these conditions are not met, the function returns an appropriate `TopologyError`. +/// +/// # Parameters +/// - `key`: A `&str` reference representing the topology key to be validated. +pub fn validate_topology_key(key: &str) -> Result<(), TopologyError> { + snafu::ensure!(!key.is_empty(), KeyIsEmptySnafu); + snafu::ensure!(key.len() <= 63, KeyTooLongSnafu); + snafu::ensure!(allowed_topology_tips(key), EdgesNotAlphaNumSnafu); + + snafu::ensure!( + key.chars().filter(|c| c == &'/').count() <= 1, + KeySlashCountSnafu + ); + + snafu::ensure!( + key.chars().all(|c| allowed_topology_chars(c) || c == '/'), + KeyIsNotAlphaNumericPlusSnafu + ); + + Ok(()) +} + +/// Validates a topology value based on specific criteria. +pub fn validate_topology_value(value: &str) -> Result<(), TopologyError> { + snafu::ensure!(!value.is_empty(), ValueIsEmptySnafu); + snafu::ensure!(value.len() <= 63, ValueTooLongSnafu); + snafu::ensure!(allowed_topology_tips(value), EdgesNotAlphaNumSnafu); + snafu::ensure!( + value.chars().all(allowed_topology_chars), + ValueIsNotAlphaNumericPlusSnafu + ); + Ok(()) +}