Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(MR): [MR-649] Support incremental rollout of best-effort calls #3688

Merged

Conversation

alin-at-dfinity
Copy link
Contributor

@alin-at-dfinity alin-at-dfinity commented Jan 30, 2025

Replace the on-off flag with an enum of 4 progressive rollout stages:

  • Stage 1a: Silently ignore ic0_call_with_best_effort_response() calls, falling back to guaranteed response calls; and reject best-effort requests when routing.
  • Stage 1b: On selected subnets, silently ignore ic0_call_with_best_effort_response() calls, falling back to guaranteed response calls; and reject best-effort requests that should be routed to subnets other than the selected ones.
  • Stage 2: On system subnets, silently ignore ic0_call_with_best_effort_response() calls, falling back to guaranteed response calls; and reject best-effort requests that should be routed to system subnets.
  • Stage 3: Fully enable API calls; always route best-effort requests.

Rollbacks (incremental or direct) are also supported, including all the way to Stage 1a. In case of a rollback, not-yet-routed best-effort requests will be rejected; already existing best-effort callbacks will expire when appropriate, as per their deadlines; and best-effort responses will be delivered (unless they expire on the way).

Replace the on-off flag with an enum of 4 progressive rollout stages:

 * Stage 0: Trap on related API calls; and reject best-effort requests when routing (status quo).
 * Stage 1: Silently ignore `ic0_call_with_best_effort_response()` calls, falling back to guaranteed response calls; and reject best-effort requests when routing.
 * Stage 2: On system subnets, silently ignore `ic0_call_with_best_effort_response()` calls, falling back to guaranteed response calls; and reject best-effort requests that should be routed to system subnets.
 * Stage 3: Fully enable API calls; always route best-effort requests.

Rollbacks (incremental or direct) are also supported, but it's probably a bad idea to roll back to stage 0, as any canisters already using best-effort calls would break. Badly.
Copy link
Contributor

@stiegerc stiegerc left a comment

Choose a reason for hiding this comment

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

LGTM at first glance, but I wasn't present when this was discussed.

rs/config/src/embedders.rs Outdated Show resolved Hide resolved
Copy link

@crusso crusso left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@derlerd-dfinity derlerd-dfinity left a comment

Choose a reason for hiding this comment

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

Thanks a lot. Left some initial comments.

rs/config/src/embedders.rs Show resolved Hide resolved
rs/config/src/embedders.rs Outdated Show resolved Hide resolved
rs/system_api/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@derlerd-dfinity derlerd-dfinity left a comment

Choose a reason for hiding this comment

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

Thank you. LGTM.

rs/config/src/embedders.rs Show resolved Hide resolved
rs/messaging/src/routing/stream_builder.rs Show resolved Hide resolved
rs/messaging/src/routing/stream_builder.rs Show resolved Hide resolved
rs/messaging/src/routing/stream_builder/tests.rs Outdated Show resolved Hide resolved
rs/system_api/tests/system_api.rs Outdated Show resolved Hide resolved
@alin-at-dfinity alin-at-dfinity added this pull request to the merge queue Feb 7, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 7, 2025
Copy link
Contributor

@stiegerc stiegerc left a comment

Choose a reason for hiding this comment

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

Did another pass, LGTM.

@alin-at-dfinity alin-at-dfinity added this pull request to the merge queue Feb 7, 2025
Merged via the queue into master with commit 513258a Feb 7, 2025
25 checks passed
@alin-at-dfinity alin-at-dfinity deleted the alin/MR-649-no-best-effort-calls-to-system-subnets branch February 7, 2025 08:36
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.

6 participants