Skip to content

test: set a subnet up to be cooled down and check the conditions it violates - #11582

Open
mraszyk wants to merge 6 commits into
masterfrom
mraszyk/subnet-cooling-down-conditions
Open

mraszyk wants to merge 6 commits into
masterfrom
mraszyk/subnet-cooling-down-conditions

Conversation

@mraszyk

@mraszyk mraszyk commented Sep 15, 2026 •

Copy link
Copy Markdown
Contributor

A system test that builds the state a subnet merge has to drain and verifies that a
subnet with outstanding work violates the relevant terms of the "merge readiness"
condition.

This is the first piece of the subnet merging work, split out on its own: no subnet is
labeled "cooling down" here and no proposal is submitted at all. The test covers the
scenario and the terms evaluated on it, not the draining that labeling the subnet sets
off, nor the merge itself.

Readiness conditions and how the test exercises them

The test sets up an NNS subnet with the NNS canisters installed and two Application
subnets: M, the subnet intended to be cooled down and merged away, and T, its
cross-subnet peer. Each subnet has four nodes. The conditions use medians across the
replicas reporting each metric series, with missing data reading as zero.

  1. Every subnet has observed the required registry version. The test evaluates
    this against the registry version of its topology snapshot. It submits no proposal
    to label M as cooling down, so there is no new version for the subnets to catch
    up to. This condition is expected to hold and is only logged.

  2. No other subnet holds messages in its stream to M. A universal canister on
    each Application subnet repeatedly calls the one on the other subnet. The test waits
    for both loops to make progress, then checks that messages in T's stream to M violate this condition.

  3. M holds no messages in any of its streams, including its loopback stream.
    The same call loops send messages out of M. The test checks that the total number
    of messages across all of M's streams is nonzero, violating this condition.

  4. M's ingress history contains only processing entries. Completed setup calls
    leave terminal entries in the ingress history, which violate this condition. The
    test also starts three endless management-call loops: one on M itself, one on M
    called from T, and one on T called from M. These keep calls outstanding and
    leave ingress messages processing, but the condition deliberately permits those
    entries; the assertion checks for entries in other states.

  5. M's subnet input queues are empty. A canister on M issues five install_code
    calls, each installing a universal canister whose initialization burns 295B
    instructions. Only one long-running installation progresses at a time, leaving
    requests queued while it runs. The test waits until all five requests are queued
    or executing before proceeding. The test checks that the subnet input queues contain
    messages, violating this condition.

  6. M's subnet output queues are empty. Management-canister responses occupy these
    queues only until the end of the round they are produced in, when the stream
    builder routes them into the loopback stream, i.e. before the state the metrics
    are observed on is committed. This condition is expected to hold and is only
    logged.

  7. M's subnet call context manager holds no call contexts. The executing
    long-running install_code call retains a subnet call context while initialization
    runs. The test checks that the manager holds call contexts, violating this
    condition.

  8. M's refund pool contains no pending anonymous refunds. Nothing in this workload
    produces an anonymous refund. This condition is expected to hold and is only logged.

The test retries the evaluation until conditions 2, 3, 4, 5, and 7 are all violated in
the same evaluation. Conditions 1, 6, and 8 are logged without assertions.

…iolates

A system test that builds the state a subnet merge has to drain and checks that
the subnet holding it violates the individual terms of the "merge readiness"
condition of the `Subnet merging` dashboard
(`bases/apps/ic-dashboards/core/subnet-merging.json` on branch
`mraszyk/subnet-merging-dashboard` of `dfinity/k8s`), i.e. that the condition a
subnet merging tool waits for before merging a subnet away is not one that a
subnet which never had anything to drain would satisfy anyway.

The IC consists of an NNS subnet (with the NNS canisters installed) and two
Application subnets: `M`, the one that is set up to be cooled down and merged
away, and `T`, of four nodes each, so that the medians the condition is made of
are medians of more than one value. The scenario on them is:

* a universal canister on each subnet, calling the one on the other subnet in a
  loop, where the reply (or reject) callback of every call fires a new call, so
  that messages are flowing between `M` and `T` in both directions;
* a canister on `M` making five `install_code` calls, each installing the
  universal canister module with an `arg` that burns 295B instructions in
  `canister_init`, i.e. five calls that span hundreds of rounds each;
* three endless loops of management canister calls, one of them in a call from
  `T` that `M` can never respond to and one waiting for a response from `T`
  that never arrives, so that `M` also holds ingress messages that stay
  `processing`.

The test then evaluates every term of the readiness condition the way the
dashboard does -- on the median across the replicas reporting the respective
series, with missing data reading as zero -- and checks that the five terms
this scenario exercises do not hold: `T` holds messages in its stream to `M`,
`M` holds messages in its own streams, the ingress history holds terminal
entries, `M`'s subnet input queues hold messages and its subnet call context
manager holds call contexts. The remaining three terms are expected to hold and
are only logged; `VIOLATED_CONDITIONS` says why for each of them.

The evaluation is retried until all five terms are violated at the same
evaluation: the two stream terms come and go with the call loops, as a message
is held in a stream only from the round it is routed in until the receiving
subnet has acknowledged it.

No subnet is labeled "cooling down" here and no proposal is submitted at all:
this covers the scenario and the terms evaluated on it, not the draining that
labeling `M` sets off.

Two helpers the test needs on the way: `UniversalCanister::submit_update`,
which submits an update call as an ingress message without waiting for it to
complete (most of the calls of this test are never meant to complete), and
`install_code(..).with_arg(..)`, which sets the argument the installed
canister's `canister_init` is passed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@mraszyk
mraszyk requested a balanced review from Copilot September 15, 2026 17:25
@github-actions github-actions Bot added the test label Sep 15, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

mraszyk and others added 4 commits September 22, 2026 18:34
* Drop the references to the `Subnet merging` dashboard and several
  explanations that are not needed to follow the test.
* Clarify that the calls waiting for responses that never arrive are not
  drained, but make it possible to assert that a subnet merge populates the
  ingress history properly.
* Rename the universal canister `US` on `M` to `UM`.
* Check the number of `install_code` requests enqueued plus executing directly
  instead of remembering the highest value seen: the retry succeeds as soon as
  a sample reaches the expected number anyway.
* State the actual reason for disabling the mainnet NNS variant: the NNS
  canisters have not been upgraded in production yet.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…und pool

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…down test

* The self-calls of the call loops are inducted directly on the same subnet
  rather than routed through the loopback stream, so call them self-calls.
* The stream builder routes the responses of the management canister out of
  the subnet output queues in the same round they are produced in, i.e. before
  the metrics are observed, not in the next round.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
… produced in

Unless they are produced by shedding a best-effort message, which happens
after the stream builder, in which case they are routed in the next round.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The timing-sensitive, multi-subnet system test requires final human review.

Review effort: Balanced
Findings: None

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The timing-sensitive distributed system test requires final human review.

Review effort: Balanced
Findings: None

Drop the claim that the endless loops keep the subnet input queues busy (their
`canister_status` calls are there only transiently), say that the endless loops
call `canister_status` rather than the looping canister itself, and drop
details from the goal and the refund pool rationale.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The non-monotonic retry condition can make the system test flaky and requires correction or human validation.

Review effort: Balanced
Findings: None

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Metric aggregation must pad missing replica series with zero before computing medians.

Review effort: Balanced
Findings: None

@mraszyk
mraszyk marked this pull request as ready for review September 23, 2026 08:50
@mraszyk
mraszyk requested review from a team as code owners September 23, 2026 08:50
@alin-at-dfinity

Copy link
Copy Markdown
Contributor

The test also starts three endless management-call loops

AFAICT these are regular update calls (inc_global_counter), not management canister calls.

Management-canister responses occupy these queues only until the end of the round they are produced in, when the stream builder routes them into the loopback stream

True, for local management canister calls. Responses to calls (management canister or otherwise) from other subnets may not be routed into the respective stream in that same round, iff the stream is above some limit (10k messages or 10 MB, IIRC). Nothing to do about it (the condition is pretty hard to trigger and even harder to make it hold), so just FYI.

@mraszyk

mraszyk commented Sep 23, 2026

Copy link
Copy Markdown
Contributor Author

The test also starts three endless management-call loops

AFAICT these are regular update calls (inc_global_counter), not management canister calls.

Management canister calls (to the canister_status endpoint) are performed by loop_until_global_data_set. So

  • U3 just spins on canister_status for itself (loopback stream);
  • U4 calls U5 (on another subnet) via regular update call and then U5 spins on canister_status for itself (U5);
  • U6 calls U7 (on another subnet) via regular update call and then U7 spins on canister_status for itself (U7).

Responses to calls (management canister or otherwise) from other subnets may not be routed into the respective stream in that same round, iff the stream is above some limit (10k messages or 10 MB, IIRC). Nothing to do about it (the condition is pretty hard to trigger and even harder to make it hold), so just FYI.

Indeed, that's why I opted out of asserting that we observe stuff in subnet output queues and the refund pool.

@alin-at-dfinity alin-at-dfinity left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment on lines +856 to +858
/// `sum(quantile by (<labels>) (0.5, <metric>{<filter>}))`: the median across
/// the replicas reporting each matching series, summed over those series.
fn sum_of_medians(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: quantile without(ic_node, instance) would be more informative, even though you don't have ic_node or instance labels. Most accurate (but wrong) would be quantile without().

Also, the filter is not named filter, but labels_match.

Comment on lines +869 to +871
/// `quantile(0.5, <metric>{<filter>})`: the median across all replicas
/// reporting any matching series. `None` if there is no such series.
fn median_across_replicas(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Same as above, the filter is not named filter, but labels_match.

It may also be worth mentioning here that this will compute the median across all dimensions (e.g. across all remote or state label values), so it's only really useful for metrics with no labels; or time series filtered down to one single label combination.

Comment on lines +739 to +747
retry_with_msg_async!(
format!(
"waiting until all {} `install_code` requests are inducted on subnet {}",
INSTALL_CODE_TARGETS.len(),
subnet.subnet_id
),
logger,
READY_WAIT_TIMEOUT,
RETRY_BACKOFF,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may be a stupid question, but should we really be waiting for up to 500 seconds for a handful of messages to get inducted? And with a back-off of 5 seconds?

Comment on lines +532 to +534
sum_of_medians(&metrics, METRIC_STREAM_MESSAGES, |labels| {
labels.contains(&remote_label)
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just FYI, you could have used median_across_replicas() just as well. You filtered down the time series to one per replica, so you're only computing one median.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants