Skip to content

Commit dc7e68f

Browse files
authored
fix(registry): adapt delete_subnet_test to non-System subnet deletion (#10713)
## Problem `//rs/tests/nns:delete_subnet_test` has been failing since 65ad179 (#10683) with: ``` thread 'main' panicked at rs/tests/nns/delete_subnet_test.rs:207:14: called `Result::unwrap_err()` on an `Ok` value: () ``` That commit lifted the CloudEngine-only restriction in `do_delete_subnet`: governance may now delete any non-System subnet, and System subnet deletion is refused with a new error message. The registry unit tests were updated, but the system test still asserted the old behavior. Since the test's universal canister is installed at the governance canister ID, deleting the Application/VerifiedApplication subnets now succeeds, making the test's `unwrap_err()` panic. ## Fix Update the system test to the new semantics: - Governance successfully deletes the CloudEngine, Application, and VerifiedApplication subnets. - Deleting the System (NNS) subnet is refused with `System subnets may not be deleted`. - Assert that only the NNS subnet remains, that the routing table entries of all deleted subnets are gone, and that the nodes of all deleted subnets are unassigned and wiped. - Replace the hardcoded registry version `2` with the actual latest registry version, robust against the additional deletions. The engine-controller-only path is covered by the PocketIC unit tests in `rs/registry/canister/tests/delete_subnet.rs` and is not exercisable from this system test (the UC occupies the governance canister ID). ## Validation - rustfmt + clippy clean - `bazel test //rs/tests/nns:delete_subnet_test` passes on farm: - setup PASSED (69.8s), test PASSED (21.6s), assert_no_metrics_errors PASSED, assert_no_unallowed_log_patterns PASSED
1 parent 26d21ef commit dc7e68f

1 file changed

Lines changed: 53 additions & 29 deletions

File tree

rs/tests/nns/delete_subnet_test.rs

Lines changed: 53 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,8 @@
11
/* tag::catalog[]
22
Title:: Delete Subnet
33
4-
Goal:: Ensure that CloudEngines can be deleted, and that regular App and System subnets cannot be deleted.
4+
Goal:: Ensure that governance can delete non-System subnets (CloudEngine, Application, and
5+
VerifiedApplication subnets), and that System subnets cannot be deleted.
56
67
end::catalog[] */
78

@@ -85,19 +86,26 @@ pub fn test(env: TestEnv) {
8586
.filter(|s| s.subnet_type() == SubnetType::Application)
8687
.collect::<Vec<_>>();
8788
let app_subnet = app_subnet.first().unwrap();
88-
let app_node = app_subnet.nodes().next().unwrap();
89+
let app_nodes: Vec<IcNodeSnapshot> = app_subnet.nodes().collect();
90+
let app_node = app_nodes
91+
.first()
92+
.expect("expected the Application subnet to have at least one node");
93+
let app_node_ids = BTreeSet::from_iter(app_nodes.iter().map(|x| x.node_id));
8994
let vapp_subnet = topology_snapshot
9095
.subnets()
9196
.filter(|s| s.subnet_type() == SubnetType::VerifiedApplication)
9297
.collect::<Vec<_>>();
9398
let vapp_subnet = vapp_subnet.first().unwrap();
99+
let vapp_node_ids = BTreeSet::from_iter(vapp_subnet.nodes().map(|x| x.node_id));
94100
let engine_subnet = topology_snapshot
95101
.subnets()
96102
.filter(|s| s.subnet_type() == SubnetType::CloudEngine)
97103
.collect::<Vec<_>>();
98104
let engine_subnet = engine_subnet.first().unwrap();
99105
let engine_nodes: Vec<IcNodeSnapshot> = engine_subnet.nodes().collect();
100-
let engine_node = &engine_nodes[0];
106+
let engine_node = engine_nodes
107+
.first()
108+
.expect("expected the CloudEngine subnet to have at least one node");
101109
let engine_node_ids = BTreeSet::from_iter(engine_nodes.iter().map(|x| x.node_id));
102110
assert!(
103111
topology_snapshot
@@ -128,53 +136,69 @@ pub fn test(env: TestEnv) {
128136
let _canister_app =
129137
UniversalCanister::new(&app_agent, app_node.effective_canister_id()).await;
130138

131-
// Deleting the engine should work.
139+
// Governance may delete any non-System subnet.
140+
141+
// Deleting the CloudEngine subnet should work.
132142
try_delete_subnet(&engine_subnet.subnet_id, &governance_canister, None).await;
133143

134-
// Deleting the app subnet should not work.
135-
try_delete_subnet(
136-
&app_subnet.subnet_id,
137-
&governance_canister,
138-
Some("Only CloudEngines may be deleted".to_string()),
139-
)
140-
.await;
144+
// Deleting the Application subnet should work.
145+
try_delete_subnet(&app_subnet.subnet_id, &governance_canister, None).await;
141146

142-
// Deleting the verified app subnet should not work.
143-
try_delete_subnet(
144-
&vapp_subnet.subnet_id,
145-
&governance_canister,
146-
Some("Only CloudEngines may be deleted".to_string()),
147-
)
148-
.await;
147+
// Deleting the VerifiedApplication subnet should work.
148+
try_delete_subnet(&vapp_subnet.subnet_id, &governance_canister, None).await;
149149

150-
// Deleting the system subnet should not work.
150+
// Deleting the System subnet should not work.
151151
try_delete_subnet(
152152
&nns_subnet.subnet_id,
153153
&governance_canister,
154-
Some("Only CloudEngines may be deleted".to_string()),
154+
Some("System subnets may not be deleted".to_string()),
155155
)
156156
.await;
157157

158+
// Wait until the local registry reflects all three subnet deletions.
159+
let latest_version = registry_client
160+
.get_latest_version()
161+
.await
162+
.expect("failed to get the latest registry version");
158163
let new_topology_snapshot = topology_snapshot
159-
.block_for_min_registry_version(RegistryVersion::new(2))
164+
.block_for_min_registry_version(RegistryVersion::new(latest_version))
160165
.await
161166
.expect("Could not obtain updated registry.");
162167

163-
// The deleted engine should not be in the subnet list any more.
168+
// The deleted subnets should no longer be in the subnet list; only the
169+
// System (NNS) subnet should remain.
164170
let final_subnets = get_subnet_list_from_registry(&registry_client).await;
165171
assert!(!final_subnets.contains(&engine_subnet.subnet_id));
166-
assert_eq!(final_subnets.len(), 3);
167-
168-
// The subnet record and routing table entries of the engine should be gone.
169-
let routing_table = new_topology_snapshot.subnet_canister_ranges(engine_subnet.subnet_id);
170-
assert!(routing_table.is_empty());
172+
assert!(!final_subnets.contains(&app_subnet.subnet_id));
173+
assert!(!final_subnets.contains(&vapp_subnet.subnet_id));
174+
assert!(final_subnets.contains(&nns_subnet.subnet_id));
175+
assert_eq!(final_subnets.len(), 1);
176+
177+
// The subnet records and routing table entries of the deleted subnets should be gone.
178+
for deleted_subnet_id in [
179+
engine_subnet.subnet_id,
180+
app_subnet.subnet_id,
181+
vapp_subnet.subnet_id,
182+
] {
183+
assert!(
184+
new_topology_snapshot
185+
.subnet_canister_ranges(deleted_subnet_id)
186+
.is_empty()
187+
);
188+
}
171189

172-
// The nodes from the engine should be unassigned.
190+
// The nodes from the deleted subnets should be unassigned.
191+
let expected_unassigned_node_ids: BTreeSet<_> = engine_node_ids
192+
.iter()
193+
.chain(app_node_ids.iter())
194+
.chain(vapp_node_ids.iter())
195+
.copied()
196+
.collect();
173197
let unassigned_node_ids = new_topology_snapshot
174198
.unassigned_nodes()
175199
.map(|x| x.node_id)
176200
.collect::<BTreeSet<_>>();
177-
assert_eq!(unassigned_node_ids, engine_node_ids);
201+
assert_eq!(unassigned_node_ids, expected_unassigned_node_ids);
178202

179203
// The nodes' states should be wiped.
180204
for node in new_topology_snapshot.unassigned_nodes() {

0 commit comments

Comments
 (0)