fix: Only consider available nodes for network operator assignment (#1067)

Author: sasa-tomic

deny.toml

@@ -77,6 +77,7 @@ ignore = [
     { id = "RUSTSEC-2021-0145", reason = "the potential security issue only affects Windows, which we do not target" },
     { id = "RUSTSEC-2024-0370", reason = "alternative not yet available" },
     { id = "RUSTSEC-2024-0375", reason = "migration away from atty pending" },
+    { id = "RUSTSEC-2024-0384", reason = "instant is a transitive dependency" },
     #"[email protected]", # you can also ignore yanked crate versions if you wish
     #{ crate = "[email protected]", reason = "you can specify why you are ignoring the yanked crate" },
 ]

rs/cli/src/runner.rs

@@ -451,7 +451,7 @@ impl Runner {
     }
 
     pub async fn remove_nodes(&self, nodes_remover: NodesRemover) -> anyhow::Result<RunnerProposal> {
-        let (healths, nodes_with_proposals) = try_join(self.health_client.nodes(), self.registry.nodes_with_proposals()).await?;
+        let (healths, nodes_with_proposals) = try_join(self.health_client.nodes(), self.registry.nodes_and_proposals()).await?;
         let (mut node_removals, motivation) = nodes_remover.remove_nodes(healths, nodes_with_proposals);
         node_removals.sort_by_key(|nr| nr.reason.message());
 
@@ -528,27 +528,32 @@ impl Runner {
         let node_operators = self.registry.operators().await?;
         // Filter out nodes that are not healthy
         let nodes_all = self.registry.nodes().await?;
-        let healthy_nodes = nodes_all
+        let healthy_nodes = available_nodes
             .iter()
-            .filter(|(node_id, _node)| health_of_nodes.get(*node_id) == Some(&HealthStatus::Healthy))
+            .filter(|node| health_of_nodes.get(&node.id) == Some(&HealthStatus::Healthy))
             .collect::<Vec<_>>();
         // Build a hashmap {node_id -> node} of all nodes that are in some (any) subnet
         let nodes_in_subnets = subnets
             .values()
             .flat_map(|s| s.nodes.iter().map(|n| (n.principal, n)))
             .collect::<AHashMap<_, _>>();
         // Build a hashmap of healthy nodes, grouped by node operator {operator -> Vec<Node>}
-        let healthy_nodes_by_operator = healthy_nodes.iter().into_group_map_by(|(_node_id, node)| node.operator.principal);
+        let healthy_nodes_by_operator = healthy_nodes
+            .iter()
+            .into_group_map_by(|node| nodes_all.get(&node.id).expect("Node should exist").operator.principal);
         // Finally, prepare a list of operators that have at least 1 node, but NONE of their nodes are in a subnet
         let operators_without_nodes_in_subnets = node_operators
             .iter()
             .filter_map(|(operator_id, operator)| {
                 if let Some(operator_nodes) = healthy_nodes_by_operator.get(operator_id) {
-                    if operator_nodes.iter().all(|(node_id, _node)| !nodes_in_subnets.contains_key(node_id)) {
+                    if operator_nodes.iter().all(|node| !nodes_in_subnets.contains_key(&node.id)) {
                         Some((
                             operator_id,
                             operator.datacenter.as_ref().map(|dc| dc.name.clone()).unwrap_or_default(),
-                            operator_nodes.iter().map(|(_, node)| (*node).clone()).collect::<Vec<_>>(),
+                            operator_nodes
+                                .iter()
+                                .map(|node| nodes_all.get(&node.id).expect("Node should exist").clone())
+                                .collect::<Vec<_>>(),
                         ))
                     } else {
                         None

rs/decentralization/src/network.rs

@@ -1140,8 +1140,8 @@ impl SubnetChangeRequest {
             .collect_vec();
 
         info!(
-            "Resizing subnet {} by removing {} healthy and {} unhealthy nodes, and adding {}. Total available {} healthy nodes.",
-            self.subnet.id,
+            "Evaluating change in subnet {} membership: removing ({} healthy + {} unhealthy) and adding {} node. Total available {} healthy nodes.",
+            self.subnet.id.to_string().split_once('-').expect("subnet id is expected to have a -").0,
             how_many_nodes_to_remove,
             how_many_nodes_unhealthy,
             how_many_nodes_to_add + self.include_nodes.len(),

rs/ic-management-backend/src/lazy_registry.rs

@@ -71,7 +71,7 @@ pub trait LazyRegistry:
 
     fn subnets(&self) -> BoxFuture<'_, anyhow::Result<Arc<IndexMap<PrincipalId, Subnet>>>>;
 
-    fn nodes_with_proposals(&self) -> BoxFuture<'_, anyhow::Result<Arc<IndexMap<PrincipalId, Node>>>>;
+    fn nodes_and_proposals(&self) -> BoxFuture<'_, anyhow::Result<Arc<IndexMap<PrincipalId, Node>>>>;
 
     fn nns_replica_version(&self) -> BoxFuture<'_, anyhow::Result<Option<String>>> {
         Box::pin(async {
@@ -683,7 +683,7 @@ impl LazyRegistry for LazyRegistryImpl {
         })
     }
 
-    fn nodes_with_proposals(&self) -> BoxFuture<'_, anyhow::Result<Arc<IndexMap<PrincipalId, Node>>>> {
+    fn nodes_and_proposals(&self) -> BoxFuture<'_, anyhow::Result<Arc<IndexMap<PrincipalId, Node>>>> {
         Box::pin(async {
             let nodes = self.nodes().await?;
             if nodes.iter().any(|(_, n)| n.proposal.is_some()) {
@@ -897,15 +897,15 @@ impl decentralization::network::TopologyManager for LazyRegistryImpl {}
 impl AvailableNodesQuerier for LazyRegistryImpl {
     fn available_nodes(&self) -> BoxFuture<'_, Result<Vec<decentralization::network::Node>, ic_management_types::NetworkError>> {
         Box::pin(async {
-            let (nodes, healths) = try_join!(self.nodes_with_proposals(), self.health_client.nodes())
+            let (nodes_and_proposals, healths) = try_join!(self.nodes_and_proposals(), self.health_client.nodes())
                 .map_err(|e| ic_management_types::NetworkError::DataRequestError(e.to_string()))?;
-            let nodes = nodes
+            let available_nodes = nodes_and_proposals
                 .values()
                 .filter(|n| n.subnet_id.is_none() && n.proposal.is_none() && n.duplicates.is_none() && !n.is_api_boundary_node)
                 .cloned()
                 .collect_vec();
 
-            Ok(nodes
+            Ok(available_nodes
                 .iter()
                 .filter(|n| {
                     // Keep only healthy nodes.
@@ -940,7 +940,7 @@ mock! {
 
         fn subnets(&self) -> BoxFuture<'_, anyhow::Result<Arc<IndexMap<PrincipalId, Subnet>>>>;
 
-        fn nodes_with_proposals(&self) -> BoxFuture<'_, anyhow::Result<Arc<IndexMap<PrincipalId, Node>>>>;
+        fn nodes_and_proposals(&self) -> BoxFuture<'_, anyhow::Result<Arc<IndexMap<PrincipalId, Node>>>>;
 
         fn unassigned_nodes_replica_version(&self) -> BoxFuture<'_, anyhow::Result<Arc<String>>>;