trident: promote rollback to stable v1; adjust rollback return type to detect no-servicing - #729
Merged
Merged
Conversation
|
Azure Pipelines: Successfully started running 1 pipeline(s). There may be pipelines that require an authorized user to comment /azp run to run. |
This was referenced Aug 5, 2026
bfjelds (bfjelds)
changed the title
bfjelds (bfjelds)
force-pushed
the
user/bfjelds/rollback-grpc-promotion
branch
from
August 5, 2026 20:30
057a7ae to
d27e576
Compare
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new stable v1 rollback() gRPC handler uses a partially-moved request (req.finalize moved out but req.kind() used later), which will fail to compile.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR promotes the core manual rollback RPCs (Rollback/RollbackStage/RollbackFinalize) into the stable trident.v1.RollbackService proto surface, while keeping preview-only rollback query RPCs in trident.v1preview. It also changes Trident’s internal rollback API to return (ExitKind, ServicingType) so gRPC callers can reliably distinguish a true rollback from a no-op via servicing_kind.
Changes:
- Move rollback execution RPCs into a new stable
proto/trident/v1/rollback_service.protoand trimv1previewto query-only rollback RPCs. - Wire up the stable v1 RollbackService implementation in the tridentd server and register it unconditionally.
- Change
Trident::rollback/manual_rollback::execute_rollbackto return(ExitKind, ServicingType)and map no-op rollback toServicingType::NoActiveServicing.
File summaries
| File | Description |
|---|---|
| proto/trident/v1preview/rollback_service.proto | Removes execution RPCs and keeps preview-only rollback queries; references stable v1 ManualRollbackKind. |
| proto/trident/v1/rollback_service.proto | Adds stable v1 rollback execution service/messages. |
| crates/trident/src/server/tridentserver/services/rollback.rs | Implements stable v1 rollback RPCs and preview CheckRollback; maps rollback results to streamed servicing responses. |
| crates/trident/src/server/tridentserver/services/mod.rs | Makes rollback service module always available (not preview-only). |
| crates/trident/src/server/mod.rs | Registers stable v1 RollbackService server; keeps preview rollback server under grpc-preview. |
| crates/trident/src/main.rs | Adapts CLI rollback command to the new (ExitKind, ServicingType) return type. |
| crates/trident/src/lib.rs | Updates Trident::rollback to return (ExitKind, ServicingType) and report no-op rollback explicitly. |
| crates/trident/src/engine/manual_rollback/mod.rs | Updates rollback engine execution to return (ExitKind, ServicingType) and preserve servicing type for stage-only paths. |
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 1
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Contributor
There was a problem hiding this comment.
🟢 Ready to approve
The functional changes appear consistent and fully wired; remaining feedback is limited to minor proto comment accuracy.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (2)
proto/trident/v1preview/rollback_service.proto:11
- The header comment references
CompletedResponse.servicing_kind, but the message intrident/v1/servicing.protoisCompleted(fieldservicing_kind), notCompletedResponse. This can mislead readers trying to locate the field in the proto contract.
// ServicingKind::NoneRequired for a no-op the same way every other
// servicing RPC does (see CompletedResponse.servicing_kind on
// trident.v1.ServicingResponse), trident-acl-agent no longer needs a
// separate precondition query to detect "nothing to roll back" - it reads
// servicing_kind off the RollbackStage response it already makes. This
proto/trident/v1/rollback_service.proto:11
- The comment points readers to
ServicingResponse.servicing_kind, butservicing_kindis actually a field on theCompletedmessage insideServicingResponse(seeservicing.proto). Clarifying this avoids confusion when navigating the proto contract.
// ServicingKind::NoneRequired for a no-op (same as every other servicing
// RPC), which removed trident-acl-agent's only reason to call it - see
// ServicingResponse.servicing_kind in servicing.proto.
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Contributor
There was a problem hiding this comment.
🟢 Ready to approve
The v1 promotion and return-shape changes appear consistently applied across protos, server wiring, and engine code, with no remaining mismatched call sites found in the repo.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
bfjelds (bfjelds)
marked this pull request as ready for review
August 5, 2026 23:41
|
Azure Pipelines: Successfully started running 1 pipeline(s). There may be pipelines that require an authorized user to comment /azp run to run. |
Paco Huelsz (frhuelsz)
previously approved these changes
Aug 7, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (3)
proto/trident/v1preview/rollback_service.proto:10
- The comment references
CompletedResponse.servicing_kindandtrident.v1.ServicingResponse.servicing_kind, but the proto actually definesServicingResponsewith aCompletedmessage that contains theservicing_kindfield (ServicingResponse.completed.servicing_kind, i.e.Completed.servicing_kind). This is misleading for API consumers reading the proto docs.
// once RollbackStage/RollbackFinalize/Rollback started reporting
// ServicingKind::NoneRequired for a no-op the same way every other
// servicing RPC does (see CompletedResponse.servicing_kind on
// trident.v1.ServicingResponse), trident-acl-agent no longer needs a
// separate precondition query to detect "nothing to roll back" - it reads
proto/trident/v1/rollback_service.proto:11
- The comment points readers to
ServicingResponse.servicing_kind, butservicing_kindis on theCompletedmessage insideServicingResponse(seeCompleted.servicing_kindintrident/v1/servicing.proto).
// ServicingKind::NoneRequired for a no-op (same as every other servicing
// RPC), which removed trident-acl-agent's only reason to call it - see
// ServicingResponse.servicing_kind in servicing.proto.
crates/trident/src/engine/manual_rollback/mod.rs:93
- This doc comment references
ServicingResponse.servicing_kind, butservicing_kindis a field on theCompletedmessage nested insideServicingResponse(seeServicingResponse.completed.servicing_kind/Completed.servicing_kindinproto/trident/v1/servicing.proto).
/// Mirrors `engine::update::update()`'s `(ExitKind, ServicingType)` return
/// shape: a no-op ("nothing to roll back") reports
/// `(ExitKind::Done, ServicingType::NoActiveServicing)` rather than a bare
/// `ExitKind::Done` that's indistinguishable from a real rollback at the
/// gRPC layer (see `ServicingResponse.servicing_kind` in servicing.proto).
…type with update/install
Two related API changes to tridentd's manual-rollback gRPC surface:
1. Promotes Rollback, RollbackStage, and RollbackFinalize from
trident.v1preview.RollbackService to the stable trident.v1.RollbackService
(proto/trident/v1/rollback_service.proto, new file). This gives rollback
the same stable, caller-handles-reboot gRPC contract that
UpdateStage/UpdateFinalize already have, instead of CLI-shelling.
CheckRollback, GetRollbackChain, and GetRollbackTarget remain preview-only
(see proto/trident/v1preview/rollback_service.proto).
2. Aligns manual rollback's no-op reporting with update()/install():
- engine::manual_rollback::execute_rollback() and Trident::rollback() now
return (ExitKind, ServicingType) instead of a bare ExitKind. A no-op
(empty rollback chain, or host not in a rollback-eligible servicing
state) now reports (ExitKind::Done, ServicingType::NoActiveServicing)
rather than a bare ExitKind::Done indistinguishable from a real
rollback at the gRPC layer.
- The three gRPC handlers in services/rollback.rs now populate
ServicingResponse.servicing_kind from that value, the same way
update.rs/install.rs already do, instead of hardcoding None. This lets
callers detect a no-op rollback from the response itself rather than
needing a separate precondition query.
- CheckRollback (which existed to answer that same "is a rollback
available" question via a separate round-trip) is demoted back to
trident.v1preview accordingly - its one caller no longer needs it.
try_acquire_read_lock/reading_request/trident_error_to_status (and
their Code/ErrorKind/OwnedRwLockReadGuard imports) are re-gated behind
the grpc-preview feature, since check_rollback was their last
non-preview caller.
- main.rs's CLI `trident rollback` command updated for the new return
shape (matches Install/Update/Commit's existing pattern).
Split out from user/bfjelds/acl-agent-rollback-grpc for isolated API
review: this covers only the tridentd-side proto/engine/server surface,
not trident-acl-agent's consumption of it.
Verified: cargo test -p trident --lib manual_rollback (22 passed), cargo
clippy -p trident --tests -- -D warnings (clean, default features), cargo
clippy -p trident --tests --all-features -- -D warnings (clean, confirms
grpc-preview-off build has no dead code from the CheckRollback demotion),
cargo fmt --check (clean).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 8c06585d-a82d-475f-a802-83fdfa012d86
Mirror the UpdateRequest { stage, finalize } shape: RollbackRequest now embeds
RollbackStageRequest as `stage` rather than duplicating the ManualRollbackKind
`kind` field. The v1 rollback handler reads the kind via the embedded stage,
requiring it (as the update handler requires its stage), consistent with the
existing finalize handling.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Paco Huelsz (frhuelsz)
force-pushed
the
user/bfjelds/rollback-grpc-promotion
branch
from
August 11, 2026 18:12
9f77da5 to
7327eed
Compare
Paco Huelsz (frhuelsz)
previously approved these changes
Aug 11, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (3)
proto/trident/v1preview/rollback_service.proto:9
- Top-level comment references
CompletedResponse.servicing_kind, but the v1 servicing proto defines the field asCompleted.servicing_kind(insideServicingResponse’scompletedoneof). This makes the comment inaccurate/misleading for readers looking up the field.
// once RollbackStage/RollbackFinalize/Rollback started reporting
// ServicingKind::NoneRequired for a no-op the same way every other
// servicing RPC does (see CompletedResponse.servicing_kind on
// trident.v1.ServicingResponse), trident-acl-agent no longer needs a
proto/trident/v1/rollback_service.proto:11
- Comment refers to
ServicingResponse.servicing_kind, butservicing_kindis a field on theCompletedmessage (reachable viaServicingResponse.completed). As written, this points readers to a non-existent field.
// preview once RollbackStage/RollbackFinalize/Rollback started reporting
// ServicingKind::NoneRequired for a no-op (same as every other servicing
// RPC), which removed trident-acl-agent's only reason to call it - see
// ServicingResponse.servicing_kind in servicing.proto.
crates/trident/src/engine/manual_rollback/mod.rs:93
- Rust doc comment points to
ServicingResponse.servicing_kind, butservicing_kindexists on theCompletedmessage (insideServicingResponse.completed). This is an incorrect reference and can confuse readers.
/// Mirrors `engine::update::update()`'s `(ExitKind, ServicingType)` return
/// shape: a no-op ("nothing to roll back") reports
/// `(ExitKind::Done, ServicingType::NoActiveServicing)` rather than a bare
/// `ExitKind::Done` that's indistinguishable from a real rollback at the
/// gRPC layer (see `ServicingResponse.servicing_kind` in servicing.proto).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 14457889-5a36-4098-8d4d-9375806aacbf
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (3)
proto/trident/v1preview/rollback_service.proto:10
- The comment references
CompletedResponse.servicing_kind, butservicing.protodefines the message asCompletedinsideServicingResponse(field path isServicingResponse.completed.servicing_kind). Updating the reference will prevent confusion for client implementers.
// once RollbackStage/RollbackFinalize/Rollback started reporting
// ServicingKind::NoneRequired for a no-op the same way every other
// servicing RPC does (see CompletedResponse.servicing_kind on
// trident.v1.ServicingResponse), trident-acl-agent no longer needs a
// separate precondition query to detect "nothing to roll back" - it reads
proto/trident/v1/rollback_service.proto:11
- The comment points to
ServicingResponse.servicing_kind, butservicing_kindis nested underServicingResponse.completedinservicing.proto(ServicingResponse.completed.servicing_kind). Updating this reference makes the contract documentation accurate.
// ServicingKind::NoneRequired for a no-op (same as every other servicing
// RPC), which removed trident-acl-agent's only reason to call it - see
// ServicingResponse.servicing_kind in servicing.proto.
crates/trident/src/engine/manual_rollback/mod.rs:93
- This doc comment references
ServicingResponse.servicing_kind, but the proto definesservicing_kindunderServicingResponse.completed(seeproto/trident/v1/servicing.proto). Pointing at the correct field path will help readers find the right place.
/// gRPC layer (see `ServicingResponse.servicing_kind` in servicing.proto).
Paco Huelsz (frhuelsz)
approved these changes
Aug 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Promotes manual-rollback's
Rollback/RollbackStage/RollbackFinalizefromtrident.v1preview.RollbackServiceto the stabletrident.v1.RollbackService.Align manual rollback's no-op reporting with
update()/install(): a no-op rollback (empty rollback chain, or host not in a rollback-eligible state) now reports(ExitKind::Done, ServicingType::NoActiveServicing)instead of a bareExitKind::Done.Context
This is the first step in enabling trident-acl-agent to run updates and rollbacks. Related PRs:
Validation