chore(bors): merge pull request #1012

1012: Refactor Scheduling Sorting r=tiagolobocastro a=tiagolobocastro

We're trying to find spots where total order is not implemented, leading to panics by the rust std library.
This PR doesn't solve anything, just some minor refactor and a couple of fixes where sort order was incorrect.
We do switch to the unstable sorter which might "help", since in our simulated testing we could not reproduce
the panics with the unstable sort.

Further we've identified the dual weight scoring as the main cause behind the total order issue, given that by 
nature, it's not a total order!!

Co-authored-by: Tiago Castro <[email protected]>

Author: mayastor-bors

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

@@ -82,15 +82,15 @@ pub(crate) trait ResourceFilter: Sized {
     }
     fn sort<F: FnMut(&Self::Item, &Self::Item) -> std::cmp::Ordering>(mut self, sort: F) -> Self {
         let data = self.data();
-        data.list.sort_by(sort);
+        data.list.sort_unstable_by(sort);
         self
     }
     fn sort_ctx<F: FnMut(&Self::Request, &Self::Item, &Self::Item) -> std::cmp::Ordering>(
         mut self,
         mut sort: F,
     ) -> Self {
         let data = self.data();
-        data.list.sort_by(|a, b| sort(&data.context, a, b));
+        data.list.sort_unstable_by(|a, b| sort(&data.context, a, b));
         self
     }
     fn collect(self) -> Vec<Self::Item>;
@@ -195,16 +195,12 @@ impl ChildSorters {
 
                     let childa_is_local = !a.spec().share.shared();
                     let childb_is_local = !b.spec().share.shared();
-                    if childa_is_local == childb_is_local {
-                        match a.ag_replicas_on_pool().cmp(&b.ag_replicas_on_pool()) {
-                            Ordering::Less => Ordering::Greater,
-                            Ordering::Equal => Ordering::Equal,
-                            Ordering::Greater => Ordering::Less,
+                    match (childa_is_local, childb_is_local) {
+                        (true, true) | (false, false) => {
+                            b.ag_replicas_on_pool().cmp(&a.ag_replicas_on_pool())
                         }
-                    } else if childa_is_local {
-                        std::cmp::Ordering::Greater
-                    } else {
-                        std::cmp::Ordering::Less
+                        (true, false) => Ordering::Greater,
+                        (false, true) => Ordering::Less,
                     }
                 }
                 ord => ord,
@@ -214,53 +210,30 @@ impl ChildSorters {
     }
     // sort replicas by their health: prefer healthy replicas over unhealthy
     fn sort_by_health(a: &ReplicaItem, b: &ReplicaItem) -> std::cmp::Ordering {
-        match a.child_info() {
-            None => {
-                match b.child_info() {
-                    Some(b_info) if b_info.healthy => {
-                        // sort replicas by their health: prefer healthy replicas over unhealthy
-                        std::cmp::Ordering::Less
-                    }
-                    _ => std::cmp::Ordering::Equal,
-                }
-            }
-            Some(a_info) => match b.child_info() {
-                Some(b_info) if a_info.healthy && !b_info.healthy => std::cmp::Ordering::Greater,
-                Some(b_info) if !a_info.healthy && b_info.healthy => std::cmp::Ordering::Less,
-                _ => std::cmp::Ordering::Equal,
-            },
+        match (a.child_info(), b.child_info()) {
+            (None, None) => Ordering::Equal,
+            (None, Some(b)) if b.healthy => Ordering::Less,
+            (None, Some(_)) => Ordering::Equal,
+            (Some(a), None) if a.healthy => Ordering::Greater,
+            (Some(_), None) => Ordering::Equal,
+            (Some(a), Some(b)) => a.healthy.cmp(&b.healthy),
         }
     }
     // remove unused replicas first
     fn sort_by_child(a: &ReplicaItem, b: &ReplicaItem) -> std::cmp::Ordering {
-        match a.child_spec() {
-            None => {
-                match b.child_spec() {
-                    None => std::cmp::Ordering::Equal,
-                    Some(_) => {
-                        // prefer the replica that is not part of a nexus
-                        std::cmp::Ordering::Greater
-                    }
-                }
-            }
-            Some(_) => {
-                match b.child_spec() {
-                    // prefer the replica that is not part of a nexus
-                    None => std::cmp::Ordering::Less,
-                    // compare the child states, and then the rebuild progress
-                    Some(_) => match (a.child_state(), b.child_state()) {
-                        (Some(a_state), Some(b_state)) => {
-                            match a_state.state.partial_cmp(&b_state.state) {
-                                None => a_state.rebuild_progress.cmp(&b_state.rebuild_progress),
-                                Some(ord) => ord,
-                            }
-                        }
-                        (Some(_), None) => std::cmp::Ordering::Less,
-                        (None, Some(_)) => std::cmp::Ordering::Greater,
-                        (None, None) => std::cmp::Ordering::Equal,
-                    },
-                }
-            }
+        match (a.child_spec(), b.child_spec()) {
+            (None, None) => Ordering::Equal,
+            (None, Some(_)) => Ordering::Less,
+            (Some(_), None) => Ordering::Greater,
+            (Some(_), Some(_)) => match (a.child_state(), b.child_state()) {
+                (Some(a_state), Some(b_state)) => match a_state.state.cmp(&b_state.state) {
+                    Ordering::Equal => a_state.rebuild_progress.cmp(&b_state.rebuild_progress),
+                    ord => ord,
+                },
+                (Some(_), None) => std::cmp::Ordering::Greater,
+                (None, Some(_)) => std::cmp::Ordering::Less,
+                (None, None) => std::cmp::Ordering::Equal,
+            },
         }
     }
 }
@@ -363,6 +336,8 @@ impl AddReplicaSorters {
         a: &ChildItem,
         b: &ChildItem,
     ) -> std::cmp::Ordering {
+        // todo: preferring local to healthy children seems strange, though also why would we
+        //  have healthy replicas not part of the nexus?
         let a_is_local = a.state().node == request.nexus_spec().node;
         let b_is_local = b.state().node == request.nexus_spec().node;
         match (a_is_local, b_is_local) {
@@ -374,7 +349,7 @@ impl AddReplicaSorters {
                 match (a_healthy, b_healthy) {
                     (true, false) => std::cmp::Ordering::Less,
                     (false, true) => std::cmp::Ordering::Greater,
-                    (_, _) => a.pool().free_space().cmp(&b.pool().free_space()),
+                    (_, _) => b.pool().free_space().cmp(&a.pool().free_space()),
                 }
             }
         }

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

@@ -138,10 +138,10 @@ impl SimplePolicy {
         b: &PoolItem,
     ) -> std::cmp::Ordering {
         match request.registry().encryption_preference_soft() {
-            true => match b.pool.encrypted().partial_cmp(&a.pool.encrypted()) {
-                Some(Ordering::Greater) => Ordering::Greater,
-                Some(Ordering::Less) => Ordering::Less,
-                None | Some(Ordering::Equal) => {
+            true => match b.pool.encrypted().cmp(&a.pool.encrypted()) {
+                Ordering::Greater => Ordering::Greater,
+                Ordering::Less => Ordering::Less,
+                Ordering::Equal => {
                     // sort pools in order of total weight of certain field values.
                     SimplePolicy::sort_by_weights(request, a, b)
                 }
@@ -152,7 +152,7 @@ impl SimplePolicy {
     }
 
     /// Sort pools using weights between:
-    /// 1. number of replicas or number of replicas of a ag (N_REPL_WEIGHT %)
+    /// 1. number of replicas or number of replicas of an ag (N_REPL_WEIGHT %)
     /// 2. free space         (FREE_SPACE_WEIGHT %)
     /// 3. overcommitment     (OVER_COMMIT_WEIGHT %)
     pub(crate) fn sort_by_weights(

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

@@ -92,10 +92,10 @@ impl ThickPolicy {
         b: &PoolItem,
     ) -> std::cmp::Ordering {
         match request.registry().encryption_preference_soft() {
-            true => match b.pool.encrypted().partial_cmp(&a.pool.encrypted()) {
-                Some(Ordering::Greater) => Ordering::Greater,
-                Some(Ordering::Less) => Ordering::Less,
-                None | Some(Ordering::Equal) => {
+            true => match b.pool.encrypted().cmp(&a.pool.encrypted()) {
+                Ordering::Greater => Ordering::Greater,
+                Ordering::Less => Ordering::Less,
+                Ordering::Equal => {
                     // sort pools in order of preference (from least to most number of replicas)
                     ThickPolicy::sort_by_weights(request, a, b)
                 }

control-plane/stor-port/src/types/v0/transport/child.rs

@@ -99,30 +99,35 @@ impl ChildState {
 }
 impl PartialOrd for ChildState {
     fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
+        Some(self.cmp(other))
+    }
+}
+impl Ord for ChildState {
+    fn cmp(&self, other: &Self) -> Ordering {
         match &self {
             ChildState::Unknown => match &other {
-                ChildState::Unknown => Some(Ordering::Equal),
-                ChildState::Online => Some(Ordering::Less),
-                ChildState::Degraded => Some(Ordering::Less),
-                ChildState::Faulted => Some(Ordering::Greater),
+                ChildState::Unknown => Ordering::Equal,
+                ChildState::Online => Ordering::Less,
+                ChildState::Degraded => Ordering::Less,
+                ChildState::Faulted => Ordering::Greater,
             },
             ChildState::Online => match &other {
-                ChildState::Unknown => Some(Ordering::Greater),
-                ChildState::Online => Some(Ordering::Equal),
-                ChildState::Degraded => Some(Ordering::Greater),
-                ChildState::Faulted => Some(Ordering::Greater),
+                ChildState::Unknown => Ordering::Greater,
+                ChildState::Online => Ordering::Equal,
+                ChildState::Degraded => Ordering::Greater,
+                ChildState::Faulted => Ordering::Greater,
             },
             ChildState::Degraded => match &other {
-                ChildState::Unknown => Some(Ordering::Greater),
-                ChildState::Online => Some(Ordering::Less),
-                ChildState::Degraded => Some(Ordering::Equal),
-                ChildState::Faulted => Some(Ordering::Greater),
+                ChildState::Unknown => Ordering::Greater,
+                ChildState::Online => Ordering::Less,
+                ChildState::Degraded => Ordering::Equal,
+                ChildState::Faulted => Ordering::Greater,
             },
             ChildState::Faulted => match &other {
-                ChildState::Unknown => Some(Ordering::Less),
-                ChildState::Online => Some(Ordering::Less),
-                ChildState::Degraded => Some(Ordering::Less),
-                ChildState::Faulted => Some(Ordering::Equal),
+                ChildState::Unknown => Ordering::Less,
+                ChildState::Online => Ordering::Less,
+                ChildState::Degraded => Ordering::Less,
+                ChildState::Faulted => Ordering::Equal,
             },
         }
     }