From b3427876f50853e551464849b46e7f8c596702d5 Mon Sep 17 00:00:00 2001 From: nikolamilosa Date: Wed, 16 Jul 2025 23:14:45 +0200 Subject: [PATCH 1/4] feat: implementing force replace command to replace nodes in a subnet --- rs/cli/src/commands/subnet/force_replace.rs | 62 +++++++++++++++++++++ rs/cli/src/commands/subnet/mod.rs | 4 +- 2 files changed, 65 insertions(+), 1 deletion(-) create mode 100644 rs/cli/src/commands/subnet/force_replace.rs diff --git a/rs/cli/src/commands/subnet/force_replace.rs b/rs/cli/src/commands/subnet/force_replace.rs new file mode 100644 index 000000000..f4d60c872 --- /dev/null +++ b/rs/cli/src/commands/subnet/force_replace.rs @@ -0,0 +1,62 @@ +use std::collections::BTreeSet; + +use clap::Args; +use ic_types::PrincipalId; +use itertools::Itertools; + +use crate::{exe::ExecutableCommand, submitter::SubmissionParameters}; + +#[derive(Args, Debug)] +pub struct ForceReplace { + /// Subnet id to perform force replacement from + #[clap(long)] + subnet_id: PrincipalId, + + /// Nodes to remove from the given subnet + #[clap(long, num_args = 1..)] + from: Vec, + + /// Nodes to include into a given subnet + #[clap(long, num_args = 1..)] + to: Vec, + + #[clap(flatten)] + pub submission_parameters: SubmissionParameters, +} + +impl ExecutableCommand for ForceReplace { + fn require_auth(&self) -> crate::auth::AuthRequirement { + crate::auth::AuthRequirement::Neuron + } + + fn validate(&self, _args: &crate::exe::args::GlobalArgs, cmd: &mut clap::Command) { + let from: BTreeSet = self.from.iter().cloned().collect(); + let to: BTreeSet = self.to.iter().cloned().collect(); + + if from.len() != to.len() { + cmd.error( + clap::error::ErrorKind::InvalidValue, + format!("`from` and `to` have to contain the same number of elements"), + ) + .exit(); + } + + let duplicates = from.intersection(&to).collect_vec(); + + if duplicates.is_empty() { + return; + } + + let duplicates = duplicates.iter().map(|p| p.to_string().split_once("-").unwrap().0.to_string()).join(", "); + + cmd.error( + clap::error::ErrorKind::ValueValidation, + format!("`from` and `to` contain the following duplicates: [{duplicates}]"), + ) + .exit() + } + + async fn execute(&self, ctx: crate::ctx::DreContext) -> anyhow::Result<()> { + Ok(()) + } +} diff --git a/rs/cli/src/commands/subnet/mod.rs b/rs/cli/src/commands/subnet/mod.rs index f338cabb0..242ea4310 100644 --- a/rs/cli/src/commands/subnet/mod.rs +++ b/rs/cli/src/commands/subnet/mod.rs @@ -1,6 +1,7 @@ use clap::Parser; use create::Create; use deploy::Deploy; +use force_replace::ForceReplace; use replace::Replace; use rescue::Rescue; use resize::Resize; @@ -11,6 +12,7 @@ use crate::exe::impl_executable_command_for_enums; mod create; mod deploy; +mod force_replace; mod replace; mod rescue; mod resize; @@ -23,4 +25,4 @@ pub struct Subnet { pub subcommands: Subcommands, } -impl_executable_command_for_enums! { Subnet, WhatifDecentralization, Deploy, Replace, Resize, Create, Rescue, SetAuthorization } +impl_executable_command_for_enums! { Subnet, WhatifDecentralization, Deploy, Replace, Resize, Create, Rescue, SetAuthorization, ForceReplace } From b10c1d9093cdd655fc02b50d48f9b42f9c4ff034 Mon Sep 17 00:00:00 2001 From: nikolamilosa Date: Thu, 17 Jul 2025 00:12:24 +0200 Subject: [PATCH 2/4] implementing the functionality --- rs/cli/src/commands/subnet/force_replace.rs | 101 +++++++++++++++++++- rs/cli/src/ctx/mod.rs | 4 + 2 files changed, 102 insertions(+), 3 deletions(-) diff --git a/rs/cli/src/commands/subnet/force_replace.rs b/rs/cli/src/commands/subnet/force_replace.rs index f4d60c872..07ca17415 100644 --- a/rs/cli/src/commands/subnet/force_replace.rs +++ b/rs/cli/src/commands/subnet/force_replace.rs @@ -1,10 +1,20 @@ use std::collections::BTreeSet; use clap::Args; +use decentralization::{ + network::{DecentralizedSubnet, SubnetChangeRequest}, + SubnetChangeResponse, +}; +use ic_management_types::Node; use ic_types::PrincipalId; +use indexmap::IndexMap; use itertools::Itertools; -use crate::{exe::ExecutableCommand, submitter::SubmissionParameters}; +use crate::{ + exe::ExecutableCommand, + forum::ForumPostKind, + submitter::{SubmissionParameters, Submitter}, +}; #[derive(Args, Debug)] pub struct ForceReplace { @@ -20,6 +30,10 @@ pub struct ForceReplace { #[clap(long, num_args = 1..)] to: Vec, + /// Additional motivation + #[clap(long)] + motivation: Option, + #[clap(flatten)] pub submission_parameters: SubmissionParameters, } @@ -36,7 +50,7 @@ impl ExecutableCommand for ForceReplace { if from.len() != to.len() { cmd.error( clap::error::ErrorKind::InvalidValue, - format!("`from` and `to` have to contain the same number of elements"), + "`from` and `to` have to contain the same number of elements".to_string(), ) .exit(); } @@ -57,6 +71,87 @@ impl ExecutableCommand for ForceReplace { } async fn execute(&self, ctx: crate::ctx::DreContext) -> anyhow::Result<()> { - Ok(()) + let registry = ctx.registry().await; + + let subnets = registry.subnets().await?; + let subnet = subnets + .get(&self.subnet_id) + .ok_or_else(|| anyhow::anyhow!("Subnet {} is not present in the registry.", self.subnet_id))?; + + // Ensure that the `from` nodes are in the subnet + let wrong_from_nodes: Vec<&PrincipalId> = self + .from + .iter() + .filter(|p| !subnet.nodes.iter().any(|node| node.principal == **p)) + .collect(); + + if !wrong_from_nodes.is_empty() { + return Err(anyhow::anyhow!( + "The following nodes are not memebers of subnet {}: [{}]", + self.subnet_id, + wrong_from_nodes.iter().map(|p| p.to_string()).join(", ") + )); + } + + // Ensure that the `to` nodes are not in any subnet + let nodes = registry.nodes().await?; + let unassigned_nodes: IndexMap = nodes + .iter() + .filter(|(_, n)| n.subnet_id.is_none()) + .map(|(k, v)| (*k, v.clone())) + .collect(); + + let wrong_to_nodes: Vec<&PrincipalId> = self.to.iter().filter(|p| !unassigned_nodes.contains_key(*p)).collect(); + + if !wrong_to_nodes.is_empty() { + return Err(anyhow::anyhow!( + "The following nodes are not found in unassigned nodes: [{}]", + wrong_to_nodes.iter().map(|p| p.to_string()).join(", ") + )); + } + + // Create a request + let nodes_to_remove = subnet.nodes.iter().filter(|n| self.from.contains(&n.principal)).cloned().collect(); + let subnet = DecentralizedSubnet::from(subnet); + + let only_nodes = self.to.iter().map(|p| unassigned_nodes.get(p).unwrap().clone()).collect(); + let request = SubnetChangeRequest::new(subnet, vec![], only_nodes, nodes_to_remove, vec![]); + + let fetcher = ctx.cordoned_features_fetcher(); + let cordoned_features = fetcher.fetch().await?; + let nodes = nodes.values().cloned().collect_vec(); + let health_client = ctx.health_client(); + let health_of_nodes = health_client.nodes().await?; + let response = request.optimize(0, &[], &health_of_nodes, cordoned_features, &nodes)?; + + let run_log = response.after().run_log; + if !run_log.is_empty() { + println!("{}", run_log.iter().join("\n")); + } + + let subnet_change_response = SubnetChangeResponse::new(&response, &health_of_nodes, self.motivation.clone()); + + let runner_proposal = match ctx.runner().await?.propose_subnet_change(&subnet_change_response).await? { + Some(runner_proposal) => runner_proposal, + None => return Ok(()), + }; + + Submitter::from(&self.submission_parameters) + .propose_and_print( + ctx.ic_admin_executor().await?.execution(runner_proposal.clone()), + match subnet_change_response.subnet_id { + Some(id) => ForumPostKind::ReplaceNodes { + subnet_id: id, + body: match (&runner_proposal.options.motivation, &runner_proposal.options.summary) { + (Some(motivation), None) => motivation.to_string(), + (Some(motivation), Some(summary)) => format!("{}\nMotivation:\n{}", summary, motivation), + (None, Some(summary)) => summary.to_string(), + (None, None) => anyhow::bail!("Expected to have `motivation` or `summary` for this proposal"), + }, + }, + None => ForumPostKind::Generic, + }, + ) + .await } } diff --git a/rs/cli/src/ctx/mod.rs b/rs/cli/src/ctx/mod.rs index d51036379..636c758c7 100644 --- a/rs/cli/src/ctx/mod.rs +++ b/rs/cli/src/ctx/mod.rs @@ -259,6 +259,10 @@ impl DreContext { pub fn health_client(&self) -> Arc { self.health_client.clone() } + + pub fn cordoned_features_fetcher(&self) -> Arc { + self.cordoned_features_fetcher.clone() + } } #[cfg(test)] From 260d53482b7bd46e79abc5d382cd13439e4f7da8 Mon Sep 17 00:00:00 2001 From: nikolamilosa Date: Fri, 18 Jul 2025 11:42:21 +0200 Subject: [PATCH 3/4] adding suggestions --- rs/cli/src/commands/proposals/analyze.rs | 7 ++- rs/cli/src/commands/subnet/force_replace.rs | 53 ++++++++++++--------- rs/cli/src/commands/subnet/whatif.rs | 7 ++- rs/cli/src/runner.rs | 5 +- 4 files changed, 43 insertions(+), 29 deletions(-) diff --git a/rs/cli/src/commands/proposals/analyze.rs b/rs/cli/src/commands/proposals/analyze.rs index 3f92a3719..9fcd584fe 100644 --- a/rs/cli/src/commands/proposals/analyze.rs +++ b/rs/cli/src/commands/proposals/analyze.rs @@ -37,7 +37,12 @@ impl ExecutableCommand for Analyze { let runner = ctx.runner().await?; match filter_map_nns_function_proposals::(&[proposal]).first() { - Some((_, change_membership)) => runner.decentralization_change(change_membership, None, proposal_summary).await, + Some((_, change_membership)) => { + let change = runner.decentralization_change(change_membership, None, proposal_summary).await?; + + println!("{}", change); + Ok(()) + } _ => Err(anyhow::anyhow!( "Proposal {} must have {} type", self.proposal_id, diff --git a/rs/cli/src/commands/subnet/force_replace.rs b/rs/cli/src/commands/subnet/force_replace.rs index 07ca17415..6e9ac6682 100644 --- a/rs/cli/src/commands/subnet/force_replace.rs +++ b/rs/cli/src/commands/subnet/force_replace.rs @@ -1,14 +1,11 @@ use std::collections::BTreeSet; use clap::Args; -use decentralization::{ - network::{DecentralizedSubnet, SubnetChangeRequest}, - SubnetChangeResponse, -}; use ic_management_types::Node; use ic_types::PrincipalId; use indexmap::IndexMap; use itertools::Itertools; +use registry_canister::mutations::do_change_subnet_membership::ChangeSubnetMembershipPayload; use crate::{ exe::ExecutableCommand, @@ -87,7 +84,7 @@ impl ExecutableCommand for ForceReplace { if !wrong_from_nodes.is_empty() { return Err(anyhow::anyhow!( - "The following nodes are not memebers of subnet {}: [{}]", + "The following nodes are not members of subnet {}: [{}]", self.subnet_id, wrong_from_nodes.iter().map(|p| p.to_string()).join(", ") )); @@ -110,26 +107,36 @@ impl ExecutableCommand for ForceReplace { )); } - // Create a request - let nodes_to_remove = subnet.nodes.iter().filter(|n| self.from.contains(&n.principal)).cloned().collect(); - let subnet = DecentralizedSubnet::from(subnet); - - let only_nodes = self.to.iter().map(|p| unassigned_nodes.get(p).unwrap().clone()).collect(); - let request = SubnetChangeRequest::new(subnet, vec![], only_nodes, nodes_to_remove, vec![]); - - let fetcher = ctx.cordoned_features_fetcher(); - let cordoned_features = fetcher.fetch().await?; - let nodes = nodes.values().cloned().collect_vec(); - let health_client = ctx.health_client(); - let health_of_nodes = health_client.nodes().await?; - let response = request.optimize(0, &[], &health_of_nodes, cordoned_features, &nodes)?; - - let run_log = response.after().run_log; - if !run_log.is_empty() { - println!("{}", run_log.iter().join("\n")); + // Check that no included nodes have open proposals. + let nodes_with_proposals: Vec = self + .from + .iter() + .chain(self.to.iter()) + .map(|node| nodes.get(node).unwrap()) + .filter(|node| node.proposal.is_some()) + .cloned() + .collect(); + + if !nodes_with_proposals.is_empty() { + return Err(anyhow::anyhow!( + "The following nodes have open proposals:\n{}", + nodes_with_proposals + .iter() + .map(|node| format!(" - {} - {}", node.principal, node.proposal.clone().unwrap().id)) + .join("\n") + )); } - let subnet_change_response = SubnetChangeResponse::new(&response, &health_of_nodes, self.motivation.clone()); + // Create a request + let runner = ctx.runner().await?; + + let change_membership = ChangeSubnetMembershipPayload { + subnet_id: self.subnet_id, + node_ids_add: self.to.iter().map(|id| (*id).into()).collect(), + node_ids_remove: self.from.iter().map(|id| (*id).into()).collect(), + }; + + let subnet_change_response = runner.decentralization_change(&change_membership, None, self.motivation.clone()).await?; let runner_proposal = match ctx.runner().await?.propose_subnet_change(&subnet_change_response).await? { Some(runner_proposal) => runner_proposal, diff --git a/rs/cli/src/commands/subnet/whatif.rs b/rs/cli/src/commands/subnet/whatif.rs index 16cf451cd..392821980 100644 --- a/rs/cli/src/commands/subnet/whatif.rs +++ b/rs/cli/src/commands/subnet/whatif.rs @@ -38,9 +38,12 @@ impl ExecutableCommand for WhatifDecentralization { node_ids_remove: self.remove_nodes.iter().map(|id| (*id).into()).collect(), }; - runner + let change = runner .decentralization_change(&change_membership, self.subnet_nodes_initial.clone(), None) - .await + .await?; + + println!("{}", change); + Ok(()) } fn validate(&self, _args: &GlobalArgs, _cmd: &mut clap::Command) {} diff --git a/rs/cli/src/runner.rs b/rs/cli/src/runner.rs index e5f89c303..54641123a 100644 --- a/rs/cli/src/runner.rs +++ b/rs/cli/src/runner.rs @@ -872,7 +872,7 @@ impl Runner { change: &ChangeSubnetMembershipPayload, override_subnet_nodes: Option>, summary: Option, - ) -> anyhow::Result<()> { + ) -> anyhow::Result { let subnet_before = match override_subnet_nodes { Some(nodes) => { let nodes = self.registry.get_nodes_from_ids(&nodes).await?; @@ -904,8 +904,7 @@ impl Runner { removed_nodes: removed_nodes.clone(), ..Default::default() }; - println!("{}", SubnetChangeResponse::new(&subnet_change, &health_of_nodes, summary)); - Ok(()) + Ok(SubnetChangeResponse::new(&subnet_change, &health_of_nodes, summary)) } pub async fn subnet_rescue(&self, subnet: &PrincipalId, keep_nodes: Option>) -> anyhow::Result> { From 1a5dd3b8d1341b529d06cc2d58e536471d53a437 Mon Sep 17 00:00:00 2001 From: nikolamilosa Date: Fri, 18 Jul 2025 11:58:02 +0200 Subject: [PATCH 4/4] chore: removing area from nakamoto calculation as it is not in target topology spec --- rs/decentralization/src/nakamoto/mod.rs | 2 -- rs/ic-management-types/src/lib.rs | 8 -------- 2 files changed, 10 deletions(-) diff --git a/rs/decentralization/src/nakamoto/mod.rs b/rs/decentralization/src/nakamoto/mod.rs index ba1a3539d..a7711034a 100644 --- a/rs/decentralization/src/nakamoto/mod.rs +++ b/rs/decentralization/src/nakamoto/mod.rs @@ -533,7 +533,6 @@ mod tests { (NodeFeature::NodeOperator, 1.), (NodeFeature::DataCenter, 1.), (NodeFeature::DataCenterOwner, 1.), - (NodeFeature::Area, 1.), (NodeFeature::Country, 1.), ]), value_counts: IndexMap::new(), @@ -542,7 +541,6 @@ mod tests { (NodeFeature::NodeOperator, 1), (NodeFeature::DataCenter, 1), (NodeFeature::DataCenterOwner, 1), - (NodeFeature::Area, 1), (NodeFeature::Country, 1), ]), avg_linear: 1., diff --git a/rs/ic-management-types/src/lib.rs b/rs/ic-management-types/src/lib.rs index 4982ee5e5..b3cf7672a 100644 --- a/rs/ic-management-types/src/lib.rs +++ b/rs/ic-management-types/src/lib.rs @@ -346,15 +346,8 @@ impl Node { .as_ref() .map(|d| d.country.clone()) .unwrap_or_else(|| "unknown".to_string()); - let area = self - .operator - .datacenter - .as_ref() - .map(|d| d.area.clone()) - .unwrap_or_else(|| "unknown".to_string()); NodeFeatures::from_iter([ - (NodeFeature::Area, area), (NodeFeature::Country, country), ( NodeFeature::Continent, @@ -475,7 +468,6 @@ pub enum NodeFeature { NodeProvider, DataCenter, DataCenterOwner, - Area, // Represents smaller geographic entities like cities and states Country, // Covers larger contexts, like countries or broader regions under shared legal jurisdiction Continent, }