Skip to content

Commit e1f1906

Browse files
mraszykmraszyk
authored
chore(migration-canister): return only a single migration status (#7890)
This PR makes the `migration_status` endpoint of the migration canister return only a single or no migration status for a given pair of canister IDs. This PR also fixes the duration of waiting for a completed migration in the canister migration system test to 7 minutes (since the migration canister now waits for at least 6 minutes before completing canister migration to prevent replay attacks - see [PR](#7787)). To prevent such regressions in the future, this PR adds the system test to `PULL_REQUEST_BAZEL_TARGETS` and tags the test as `long_test` so that it can run on PRs using `PULL_REQUEST_BAZEL_TARGETS`.
1 parent 0d1276c commit e1f1906

File tree

9 files changed

+65
-60
lines changed

9 files changed

+65
-60
lines changed

‎PULL_REQUEST_BAZEL_TARGETS‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -94,3 +94,5 @@ rs/bitcoin/* //rs/tests/ckbtc:btc_adapter_basics_test
9494
//rs/tests/execution:btc_get_balance_test_colocate
9595

9696
rs/boundary_node/rate_limits/* //rs/tests/boundary_nodes:rate_limit_canister_test
97+
98+
rs/migration_canister/* //rs/tests/execution:canister_migration_test_head_nns

‎packages/pocket-ic/tests/icp_features.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -128,14 +128,14 @@ fn test_canister_migration() {
128128
res.0.unwrap();
129129

130130
let status = || {
131-
let res = update_candid::<_, (Vec<MigrationStatus>,)>(
131+
let res = update_candid::<_, (Option<MigrationStatus>,)>(
132132
&pic,
133133
canister_migration_orchestrator,
134134
"migration_status",
135135
(migrate_canister_args.clone(),),
136136
)
137137
.unwrap();
138-
res.0.last().unwrap().clone()
138+
res.0.unwrap()
139139
};
140140
loop {
141141
pic.advance_time(Duration::from_secs(10));

‎rs/migration_canister/migration_canister.did‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,5 @@ service : (MigrationCanisterInitArgs) -> {
3333
disable_api : () -> (MigrationCanisterResult);
3434
enable_api : () -> (MigrationCanisterResult);
3535
migrate_canister : (MigrateCanisterArgs) -> (ValidationResult);
36-
// The same `(canister_id, replace_canister_id)` pair might be processed again after a failure
37-
// and thus we return a vector.
38-
migration_status : (MigrateCanisterArgs) -> (vec MigrationStatus) query;
36+
migration_status : (MigrateCanisterArgs) -> (opt MigrationStatus) query;
3937
}

‎rs/migration_canister/src/canister_state.rs‎

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -94,13 +94,20 @@ pub mod requests {
9494
REQUESTS.with_borrow(|req| req.keys().filter(predicate).collect())
9595
}
9696

97-
pub fn find_request(source: Principal, target: Principal) -> Vec<RequestState> {
98-
// TODO: should do a range scan for efficiency.
99-
REQUESTS.with_borrow(|r| {
97+
pub fn find_request(source: Principal, target: Principal) -> Option<RequestState> {
98+
// We perform a linear scan here which is fine since
99+
// there can only be at most `RATE_LIMIT` (50) requests
100+
// at any time.
101+
let requests: Vec<_> = REQUESTS.with_borrow(|r| {
100102
r.keys()
101103
.filter(|x| x.request().source == source && x.request().target == target)
102104
.collect()
103-
})
105+
});
106+
assert!(
107+
requests.len() <= 1,
108+
"There should only be a single request for a given pair of canister IDs."
109+
);
110+
requests.first().cloned()
104111
}
105112
}
106113

@@ -136,14 +143,12 @@ pub mod events {
136143
})
137144
}
138145

139-
pub fn find_event(source: Principal, target: Principal) -> Vec<Event> {
146+
pub fn find_last_event(source: Principal, target: Principal) -> Option<Event> {
140147
// TODO: should do a range scan for efficiency.
141148
HISTORY.with_borrow(|r| {
142149
r.keys()
143-
.filter(|x| {
144-
x.event.request().source == source && x.event.request().target == target
145-
})
146-
.collect()
150+
.rev()
151+
.find(|x| x.event.request().source == source && x.event.request().target == target)
147152
})
148153
}
149154
}

‎rs/migration_canister/src/migration_canister.rs‎

Lines changed: 14 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ use crate::{
1313
RequestState, ValidationError,
1414
canister_state::{
1515
ValidationGuard, caller_allowed,
16-
events::find_event,
16+
events::find_last_event,
1717
migrations_disabled,
1818
requests::{find_request, insert_request},
1919
set_allowlist,
@@ -100,25 +100,22 @@ pub enum MigrationStatus {
100100
}
101101

102102
#[query]
103-
/// The same (canister_id, replace_canister_id) pair might be present in the `HISTORY`, and valid to process again, so
104-
/// we return a vector.
105-
fn migration_status(args: MigrateCanisterArgs) -> Vec<MigrationStatus> {
106-
let mut active: Vec<MigrationStatus> = find_request(args.canister_id, args.replace_canister_id)
107-
.into_iter()
108-
.map(|r| MigrationStatus::InProgress {
109-
status: r.name().to_string(),
110-
})
111-
.collect();
112-
let events: Vec<MigrationStatus> = find_event(args.canister_id, args.replace_canister_id)
113-
.into_iter()
114-
.map(|event| match event.event {
103+
fn migration_status(args: MigrateCanisterArgs) -> Option<MigrationStatus> {
104+
if let Some(request_status) = find_request(args.canister_id, args.replace_canister_id) {
105+
let migration_status = MigrationStatus::InProgress {
106+
status: request_status.name().to_string(),
107+
};
108+
Some(migration_status)
109+
} else if let Some(event) = find_last_event(args.canister_id, args.replace_canister_id) {
110+
let migration_status = match event.event {
115111
crate::EventType::Succeeded { .. } => MigrationStatus::Succeeded { time: event.time },
116112
crate::EventType::Failed { reason, .. } => MigrationStatus::Failed {
117113
reason,
118114
time: event.time,
119115
},
120-
})
121-
.collect();
122-
active.extend(events);
123-
active
116+
};
117+
Some(migration_status)
118+
} else {
119+
None
120+
}
124121
}

‎rs/migration_canister/src/tests.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ fn test() {
2828
caller,
2929
);
3030
insert_request(RequestState::Accepted { request });
31-
assert!(find_request(source, target).len() == 1);
31+
assert!(find_request(source, target).is_some());
3232
}
3333

3434
#[test]

‎rs/migration_canister/tests/tests.rs‎

Lines changed: 19 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -251,7 +251,7 @@ async fn get_status(
251251
pic: &PocketIc,
252252
sender: Principal,
253253
args: &MigrateCanisterArgs,
254-
) -> Vec<MigrationStatus> {
254+
) -> Option<MigrationStatus> {
255255
let res = pic
256256
.update_call(
257257
MIGRATION_CANISTER_ID.into(),
@@ -261,7 +261,7 @@ async fn get_status(
261261
)
262262
.await
263263
.unwrap();
264-
Decode!(&res, Vec<MigrationStatus>).unwrap()
264+
Decode!(&res, Option<MigrationStatus>).unwrap()
265265
}
266266

267267
/// Advances time by a second and executes enough ticks that the state machine
@@ -593,7 +593,7 @@ async fn replay_call_after_migration() {
593593

594594
loop {
595595
let status = get_status(&pic, sender, &args).await;
596-
if let MigrationStatus::Succeeded { .. } = status[0] {
596+
if let MigrationStatus::Succeeded { .. } = status.unwrap() {
597597
break;
598598
}
599599
// We proceed in small steps here so that
@@ -1156,7 +1156,7 @@ async fn status_correct() {
11561156

11571157
let status = get_status(&pic, sender, &args).await;
11581158
assert_eq!(
1159-
status[0],
1159+
status.unwrap(),
11601160
MigrationStatus::InProgress {
11611161
status: "Accepted".to_string()
11621162
}
@@ -1165,7 +1165,7 @@ async fn status_correct() {
11651165
advance(&pic).await;
11661166
let status = get_status(&pic, sender, &args).await;
11671167
assert_eq!(
1168-
status[0],
1168+
status.unwrap(),
11691169
MigrationStatus::InProgress {
11701170
status: "ControllersChanged".to_string()
11711171
}
@@ -1174,7 +1174,7 @@ async fn status_correct() {
11741174
advance(&pic).await;
11751175
let status = get_status(&pic, sender, &args).await;
11761176
assert_eq!(
1177-
status[0],
1177+
status.unwrap(),
11781178
MigrationStatus::InProgress {
11791179
status: "StoppedAndReady".to_string()
11801180
}
@@ -1183,7 +1183,7 @@ async fn status_correct() {
11831183
advance(&pic).await;
11841184
let status = get_status(&pic, sender, &args).await;
11851185
assert_eq!(
1186-
status[0],
1186+
status.unwrap(),
11871187
MigrationStatus::InProgress {
11881188
status: "RenamedTarget".to_string()
11891189
}
@@ -1192,7 +1192,7 @@ async fn status_correct() {
11921192
advance(&pic).await;
11931193
let status = get_status(&pic, sender, &args).await;
11941194
assert_eq!(
1195-
status[0],
1195+
status.unwrap(),
11961196
MigrationStatus::InProgress {
11971197
status: "UpdatedRoutingTable".to_string()
11981198
}
@@ -1201,7 +1201,7 @@ async fn status_correct() {
12011201
advance(&pic).await;
12021202
let status = get_status(&pic, sender, &args).await;
12031203
assert_eq!(
1204-
status[0],
1204+
status.unwrap(),
12051205
MigrationStatus::InProgress {
12061206
status: "RoutingTableChangeAccepted".to_string()
12071207
}
@@ -1210,7 +1210,7 @@ async fn status_correct() {
12101210
advance(&pic).await;
12111211
let status = get_status(&pic, sender, &args).await;
12121212
assert_eq!(
1213-
status[0],
1213+
status.unwrap(),
12141214
MigrationStatus::InProgress {
12151215
status: "SourceDeleted".to_string()
12161216
}
@@ -1219,7 +1219,7 @@ async fn status_correct() {
12191219
advance(&pic).await;
12201220
let status = get_status(&pic, sender, &args).await;
12211221
assert_eq!(
1222-
status[0],
1222+
status.unwrap(),
12231223
MigrationStatus::InProgress {
12241224
status: "RestoredControllers".to_string()
12251225
}
@@ -1249,7 +1249,7 @@ async fn after_validation_source_not_stopped() {
12491249
advance(&pic).await;
12501250
advance(&pic).await;
12511251
let status = get_status(&pic, sender, &args).await;
1252-
let MigrationStatus::Failed { ref reason, .. } = status[0] else {
1252+
let MigrationStatus::Failed { ref reason, .. } = status.unwrap() else {
12531253
panic!()
12541254
};
12551255
assert_eq!(reason, &"Source is not stopped.".to_string());
@@ -1278,7 +1278,7 @@ async fn after_validation_target_not_stopped() {
12781278
advance(&pic).await;
12791279
advance(&pic).await;
12801280
let status = get_status(&pic, sender, &args).await;
1281-
let MigrationStatus::Failed { ref reason, .. } = status[0] else {
1281+
let MigrationStatus::Failed { ref reason, .. } = status.unwrap() else {
12821282
panic!()
12831283
};
12841284
assert_eq!(reason, &"Target is not stopped.".to_string());
@@ -1319,7 +1319,7 @@ async fn after_validation_target_has_snapshot() {
13191319
advance(&pic).await;
13201320
advance(&pic).await;
13211321
let status = get_status(&pic, sender, &args).await;
1322-
let MigrationStatus::Failed { ref reason, .. } = status[0] else {
1322+
let MigrationStatus::Failed { ref reason, .. } = status.unwrap() else {
13231323
panic!()
13241324
};
13251325
assert_eq!(reason, &"Target has snapshots.".to_string());
@@ -1361,7 +1361,7 @@ async fn after_validation_insufficient_cycles() {
13611361
advance(&pic).await;
13621362
advance(&pic).await;
13631363
let status = get_status(&pic, sender, &args).await;
1364-
let MigrationStatus::Failed { ref reason, .. } = status[0] else {
1364+
let MigrationStatus::Failed { ref reason, .. } = status.unwrap() else {
13651365
panic!()
13661366
};
13671367
assert!(reason.contains("Source does not have sufficient cycles"));
@@ -1391,7 +1391,7 @@ async fn failure_controllers_restored() {
13911391
advance(&pic).await;
13921392
advance(&pic).await;
13931393
let status = get_status(&pic, sender, &args).await;
1394-
let MigrationStatus::Failed { .. } = status[0] else {
1394+
let MigrationStatus::Failed { .. } = status.unwrap() else {
13951395
panic!()
13961396
};
13971397
let mut source_controllers_after = pic.get_controllers(source).await;
@@ -1430,8 +1430,8 @@ async fn success_controllers_restored() {
14301430
advance(&pic).await;
14311431
}
14321432
let status = get_status(&pic, sender, &args).await;
1433-
let MigrationStatus::Succeeded { .. } = status[0] else {
1434-
panic!("status: {:?}", status[0]);
1433+
let MigrationStatus::Succeeded { .. } = status.as_ref().unwrap() else {
1434+
panic!("status: {:?}", status.unwrap());
14351435
};
14361436
let mut source_controllers_after = pic.get_controllers(source).await;
14371437
source_controllers_after.sort();
@@ -1494,7 +1494,7 @@ async fn parallel_migrations() {
14941494
},
14951495
)
14961496
.await;
1497-
let MigrationStatus::InProgress { ref status } = status[0] else {
1497+
let MigrationStatus::InProgress { ref status } = status.unwrap() else {
14981498
panic!()
14991499
};
15001500
assert_eq!(status, "SourceDeleted");

‎rs/tests/execution/BUILD.bazel‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -132,7 +132,9 @@ system_test_nns(
132132
name = "canister_migration_test",
133133
enable_mainnet_nns_variant = False,
134134
env = UNIVERSAL_CANISTER_ENV,
135-
test_timeout = "eternal",
135+
tags = [
136+
"long_test",
137+
],
136138
runtime_deps = UNIVERSAL_CANISTER_RUNTIME_DEPS,
137139
deps = [
138140
"//rs/nns/constants",

‎rs/tests/execution/canister_migration_test.rs‎

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -431,13 +431,13 @@ async fn test_async(env: TestEnv) {
431431

432432
assert_eq!(decoded_result, Ok(()));
433433

434-
// The migration canister has a step where it waits for 5 minutes, so we give it a minute more than that.
435-
println!("Wait over 5 minutes for processing.");
434+
// The migration canister has a step where it waits for 6 minutes, so we give it a minute more than that.
435+
println!("Wait 7 minutes for processing.");
436436

437437
retry_with_msg_async!(
438-
"Wait 5m for migration canister to process",
438+
"Wait 7m for migration canister to process",
439439
&logger,
440-
Duration::from_secs(360),
440+
Duration::from_secs(420),
441441
Duration::from_secs(10),
442442
|| async {
443443
let status = nns_agent
@@ -446,13 +446,14 @@ async fn test_async(env: TestEnv) {
446446
.call_and_wait()
447447
.await
448448
.expect("Failed to call migration_status.");
449-
let decoded_status = Decode!(&status, Vec<MigrationStatus>)
450-
.expect("Failed to decode response from migration_status.");
449+
let decoded_status = Decode!(&status, Option<MigrationStatus>)
450+
.expect("Failed to decode response from migration_status.")
451+
.expect("There should be a migration status available.");
451452

452-
if matches!(decoded_status[0], MigrationStatus::Succeeded { .. }) {
453+
if matches!(decoded_status, MigrationStatus::Succeeded { .. }) {
453454
Ok(())
454455
} else {
455-
bail!("Not ready. Status: {:?}", decoded_status[0])
456+
bail!("Not ready. Status: {:?}", decoded_status)
456457
}
457458
}
458459
)

0 commit comments

Comments
 (0)