chore(bors): merge pull request #968

968: feat(scaling volume): prefer cordoned nodes while removing replicas r=Abhinandan-Purkait a=Abhinandan-Purkait

Currently on scaling down if a replica exists on a node which is cordoned we don't seem to consider that replica explicitly for removal. With this change we will prefer replicas on cordoned nodes first.

Co-authored-by: Abhinandan Purkait <[email protected]>

Author: mayastor-bors

control-plane/agents/src/bin/core/controller/scheduling/mod.rs

@@ -168,6 +168,7 @@ impl ChildSorters {
         match Self::sort_by_health(a, b) {
             Ordering::Equal => match Self::sort_by_child(a, b) {
                 Ordering::Equal => {
+                    // Remove replicas from nodes which are cordoned with most priority.
                     // remove mismatched topology replicas first
                     if let (Some(a), Some(b)) = (a.valid_node_topology(), b.valid_node_topology()) {
                         match a.cmp(b) {
@@ -183,6 +184,15 @@ impl ChildSorters {
                         }
                     }
 
+                    match if let (Some(a), Some(b)) = (a.node_spec(), b.node_spec()) {
+                        b.cordoned().cmp(&a.cordoned())
+                    } else {
+                        a.node_spec().is_some().cmp(&b.node_spec().is_some())
+                    } {
+                        Ordering::Equal => {}
+                        _else => return _else,
+                    }
+
                     let childa_is_local = !a.spec().share.shared();
                     let childb_is_local = !b.spec().share.shared();
                     if childa_is_local == childb_is_local {

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

@@ -7,6 +7,7 @@ use stor_port::types::v0::{
     store::{
         nexus_child::NexusChild,
         nexus_persistence::{ChildInfo, NexusInfo},
+        node::NodeSpec,
         replica::ReplicaSpec,
         snapshots::replica::ReplicaSnapshot,
         volume::VolumeSpec,
@@ -178,10 +179,12 @@ pub(crate) struct ReplicaItem {
     ag_replicas_on_pool: Option<u64>,
     valid_pool_topology: Option<bool>,
     valid_node_topology: Option<bool>,
+    node_spec: Option<NodeSpec>,
 }
 
 impl ReplicaItem {
     /// Create new `Self` from the provided arguments.
+    #[allow(clippy::too_many_arguments)]
     pub(crate) fn new(
         replica: ReplicaSpec,
         replica_state: Option<&Replica>,
@@ -190,6 +193,7 @@ impl ReplicaItem {
         child_spec: Option<NexusChild>,
         child_info: Option<ChildInfo>,
         ag_replicas_on_pool: Option<u64>,
+        node_spec: Option<NodeSpec>,
     ) -> Self {
         Self {
             replica_spec: replica,
@@ -201,6 +205,7 @@ impl ReplicaItem {
             ag_replicas_on_pool,
             valid_pool_topology: None,
             valid_node_topology: None,
+            node_spec,
         }
     }
     /// Set the validity of the replica's node topology.
@@ -221,6 +226,10 @@ impl ReplicaItem {
     pub(crate) fn valid_node_topology(&self) -> &Option<bool> {
         &self.valid_node_topology
     }
+    /// Get a reference to the node topology validity flag.
+    pub(crate) fn node_spec(&self) -> &Option<NodeSpec> {
+        &self.node_spec
+    }
     /// Get a reference to the pool topology validity flag.
     pub(crate) fn valid_pool_topology(&self) -> &Option<bool> {
         &self.valid_pool_topology

control-plane/agents/src/bin/core/controller/scheduling/volume.rs

@@ -297,6 +297,7 @@ impl GetChildForRemovalContext {
         let replica_states = self.registry.replicas().await;
         replicas
             .map(|replica_spec| {
+                let replica_node_spec = self.registry.specs().replica_node_spec(&replica_spec.uuid);
                 let ag_rep_count = pool_ag_rep
                     .as_ref()
                     .and_then(|map| map.get(replica_spec.pool_name()).cloned());
@@ -347,6 +348,7 @@ impl GetChildForRemovalContext {
                             .cloned()
                     }),
                     ag_rep_count,
+                    replica_node_spec,
                 )
                 .with_node_topology({
                     Some(NodeFilters::topology_replica(

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

@@ -13,6 +13,7 @@ use stor_port::{
     transport_api::ResourceKind,
     types::v0::{
         store::{
+            node::NodeSpec,
             pool::{PoolLabelOp, PoolOperation, PoolSpec, PoolUnLabelOp},
             replica::{ReplicaOperation, ReplicaSpec},
             SpecStatus, SpecTransaction,
@@ -344,6 +345,16 @@ impl ResourceSpecsLocked {
         specs.replica_spec_node(replica.immutable_ref())
     }
 
+    /// Get the node where the replica `id` resides on, if it exists.
+    pub(crate) fn replica_node_spec(&self, id: &ReplicaId) -> Option<NodeSpec> {
+        let node_id = self.replica_id_node(id);
+        if let Some(node_id) = node_id {
+            let specs = self.read();
+            return specs.nodes.get(&node_id).map(|n| n.lock().clone());
+        };
+        None
+    }
+
     /// Get or Create the resourced PoolSpec for the given request.
     pub(crate) fn get_or_create_pool(&self, request: &CreatePool) -> ResourceMutex<PoolSpec> {
         let mut specs = self.write();