Skip to content

trident-acl-agent: rewrite as annotation-based ACL update agent - #730

Merged
bfjelds (bfjelds) merged 54 commits into
mainfrom
user/bfjelds/acl-agent-rollback-grpc-rust
Aug 27, 2026
Merged

trident-acl-agent: rewrite as annotation-based ACL update agent#730
bfjelds (bfjelds) merged 54 commits into
mainfrom
user/bfjelds/acl-agent-rollback-grpc-rust

Conversation

@bfjelds

@bfjelds bfjelds (bfjelds) commented Aug 5, 2026

Copy link
Copy Markdown
Member

Summary

Context

This is the second step in enabling trident-acl-agent to run updates and rollbacks. Related PRs:

Validation

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
1 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

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.

Pull request overview

This PR rewrites trident-acl-agent (Harpoon) into an annotation-driven ACL update agent that watches a Kubernetes Node annotation to drive Trident update/rollback/commit flows over gRPC, persists post-reboot state, and publishes status back to the Node. It also wires the agent into packaging (systemd + RPM) and adds supporting config/state/k8s client code plus unit tests (including an in-process mock tridentd).

Changes:

  • Add an annotation-based reconcile loop that stages/finalizes updates, stages/finalizes rollbacks, and resumes post-reboot commit using persisted state.
  • Introduce wire types + validation for request/status annotations, plus a gRPC client wrapper and an in-process mock tridentd for unit testing.
  • Package the agent with a systemd unit and RPM spec integration; expand workspace dependencies to include kube-rs/k8s-openapi/toml.

Reviewed changes

Copilot reviewed 13 out of 14 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
packaging/systemd/trident-acl-agent.service Adds systemd unit to run the ACL agent on hosts.
packaging/rpm/trident.spec Installs/enables the new systemd unit and ships the agent binary in the RPM.
crates/trident-acl-agent/src/trident.rs Implements the gRPC client wrapper for update/commit/rollback calls to tridentd.
crates/trident-acl-agent/src/state.rs Implements persisted state store for completed operations and pending post-reboot commit.
crates/trident-acl-agent/src/orchestrator.rs Adds the main reconcile loop (watch request annotation, drive Trident RPCs, publish status).
crates/trident-acl-agent/src/mock_tridentd.rs Adds an in-process fake tridentd server for unit tests of the gRPC client/orchestrator.
crates/trident-acl-agent/src/main.rs Adds CLI/config loading and selects between omaha-only vs label/annotation orchestration mode.
crates/trident-acl-agent/src/lib.rs Refactors existing Harpoon/Omaha logic into library form and exposes new agent components.
crates/trident-acl-agent/src/k8s.rs Adds a thin kube-rs wrapper for get/watch/patch of the Node object.
crates/trident-acl-agent/src/config.rs Adds TOML config parsing with defaults and CLI override handling.
crates/trident-acl-agent/src/annotations.rs Adds request/status schema types, semantic validation, and design-doc conformance tests.
crates/trident-acl-agent/Cargo.toml Adds dependencies needed for the new agent and its unit tests.
Cargo.toml Adds workspace dependency entries for kube-rs/k8s-openapi and toml.
Cargo.lock Updates lockfile for newly introduced dependencies.

Comment thread crates/trident-acl-agent/src/orchestrator.rs Outdated
Comment thread crates/trident-acl-agent/src/trident.rs
Comment thread crates/trident-acl-agent/src/config.rs Outdated
Comment thread crates/trident-acl-agent/src/annotations.rs Outdated
Comment thread crates/trident-acl-agent/src/state.rs Outdated
Comment thread crates/trident-acl-agent/Cargo.toml Outdated

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.

Pull request overview

Copilot reviewed 13 out of 14 changed files in this pull request and generated no new comments.

Suppressed comments (4)

crates/trident-acl-agent/src/orchestrator.rs:50

  • The reboot path shells out via std::process::Command, bypassing the repo’s standard dependency execution wrapper (osutils::dependencies::Dependency) which provides consistent resolution/error reporting (and is already used for systemctl elsewhere in the workspace). Using the wrapper here avoids inconsistent failures and makes missing binaries/reporting uniform.
impl RebootHandle for SystemRebooter {
    fn reboot(&self) -> Result<(), anyhow::Error> {
        for candidate in [
            ("reboot", Vec::<&str>::new()),
            ("systemctl", vec!["reboot"]),

crates/trident-acl-agent/src/orchestrator.rs:12

  • After switching the reboot implementation away from std::process::Command, the process::Command import becomes unused and will trigger warnings (and potentially CI failures if warnings are denied).
use std::{collections::BTreeMap, process::Command};

crates/trident-acl-agent/Cargo.toml:54

  • hyper-util is added with an inline version in this crate’s manifest. This repo generally centralizes third-party versions in the workspace root and uses { workspace = true } in per-crate manifests (e.g. crates/osutils/Cargo.toml). Consider adding hyper-util to [workspace.dependencies] in the root Cargo.toml and switching this entry to { workspace = true } to keep dependency versions consistent across the workspace.
hyper-util = { version = "0.1", features = ["tokio"] }

crates/trident-acl-agent/src/annotations.rs:113

  • UpdateRequest::validate() claims to enforce the request annotation's formal JSON Schema, but it does not validate that operation_id is a UUID. Since operationId is specified as format: uuid / UUID regex in the embedded schema and is used as a key in caches/status mapping, invalid values should be rejected early to avoid mismatched/deduped operations.
    pub fn validate(self) -> Result<Self, String> {
        if self.schema_version != SCHEMA_VERSION {
            return Err(format!("unsupported schemaVersion {}", self.schema_version));
        }
        match self.operation {

bfjelds (bfjelds) added a commit that referenced this pull request Aug 5, 2026
- SystemRebooter now routes through osutils::dependencies::Dependency
  instead of raw process::Command, matching the rest of the codebase
  (crates/trident/src/reboot.rs uses the same pattern) and getting
  uniform actionable errors on a missing/failing systemctl
- host_configuration_from_image now builds YAML via serde_yaml instead
  of format!, avoiding malformed/misinterpreted YAML if a URL or hash
  contains YAML-special characters
- from_toml error message now names trident-acl-agent.conf instead of
  the generic "config.toml"
- CURRENT_VERSION_STUB changed to a sentinel that cannot collide with a
  real AKS/Trident release version string, preventing a spurious
  AlreadyAtTarget short-circuit
- StateStore::save now writes to a temp file and renames atomically
  instead of truncate-then-write, so a crash/power-loss around a real
  reboot cannot corrupt state.json
- hyper-util moved to [workspace.dependencies] and referenced via
  workspace = true, matching repo convention

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 8c06585d-a82d-475f-a802-83fdfa012d86
@bfjelds
bfjelds (bfjelds) requested a lite review from Copilot August 5, 2026 23:40

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.

Pull request overview

Copilot reviewed 13 out of 14 changed files in this pull request and generated no new comments.

Suppressed comments (3)

crates/trident-acl-agent/src/k8s.rs:110

  • watch_node() sets the Kubernetes watch request timeoutSeconds to watch_poll_interval (default 2s). That causes the watch connection to be intentionally torn down and re-established every couple seconds even when healthy, increasing API-server load and generating unnecessary reconnect churn (contradicting the comment that this is a backoff ceiling). Use a longer, fixed watch timeout (or omit it) and keep watch_poll_interval for backoff/retry behavior instead.
        let watcher_config = watcher::Config::default()
            .fields(&format!("metadata.name={name}"))
            .timeout(self.poll_interval.as_secs().max(1) as u32);

crates/trident-acl-agent/src/main.rs:64

  • is_network_target() allocates a new String for every log record (format!("{prefix}::") inside any(...)). This runs on the hot path of log filtering and can add noticeable overhead under verbose logging. Use strip_prefix/starts_with("::") to check module prefixes without allocation.
fn is_network_target(target: &str) -> bool {
    NETWORK_LOG_TARGETS
        .iter()
        .any(|prefix| target == *prefix || target.starts_with(&format!("{prefix}::")))
}

crates/trident-acl-agent/src/trident.rs:366

  • consume_servicing_stream() builds an owned String for every streamed log record (format!(...)), even when the selected log level is disabled. Since servicing streams can be chatty, this creates avoidable allocation overhead. Log with format args directly so the formatting only occurs when the level is enabled.
            Some(ResponseBody::Log(log_record)) => {
                let msg = format!("[Trident:{operation}] {}", log_record.message);
                match log_record.level() {
                    LogLevel::Unspecified | LogLevel::Trace => log::trace!("{msg}"),
                    LogLevel::Debug => log::debug!("{msg}"),

Comment thread crates/trident-acl-agent/src/core/config.rs
Comment thread crates/trident-acl-agent/src/config.rs Outdated
@bfjelds
bfjelds (bfjelds) requested a lite review from Copilot August 6, 2026 19:13
bfjelds (bfjelds) added a commit that referenced this pull request Aug 6, 2026
…uess

--validate-connection kubernetes formatted config.kubernetes.api_server
directly in its log/error messages. After making api_server an optional
override (rebased from PR #730), that field no longer reflects the real
server when unset - the actual server comes from kubeconfig in that
case. Expose the resolved cluster_url from NodeClient (captured from
the kube::Config it builds) and use that for logging instead, so the
message is accurate whether the server came from an override or from
kubeconfig.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 5520c873-dc93-4e62-b794-3cbb96b11bb6

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.

bfjelds (bfjelds) added a commit that referenced this pull request Aug 6, 2026
…livery

Add pre- and post-config trident-acl-agent --validate-connection checks
to the run-ab-update storm test case, exercising the kubeconfig-server
fix from PR #730:

- Before prepareVmForAclAgent writes a config: tridentd succeeds (socket
  activation needs no config), kubernetes and nebraska fail (unreachable
  compiled-in defaults).
- After prepareVmForAclAgent writes a real config: all three succeed.
@bfjelds
bfjelds (bfjelds) requested a lite review from Copilot August 6, 2026 20:11

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.

Comment thread crates/trident-acl-agent/src/main.rs Outdated
Paco Huelsz (frhuelsz) pushed a commit that referenced this pull request Aug 11, 2026
- SystemRebooter now routes through osutils::dependencies::Dependency
  instead of raw process::Command, matching the rest of the codebase
  (crates/trident/src/reboot.rs uses the same pattern) and getting
  uniform actionable errors on a missing/failing systemctl
- host_configuration_from_image now builds YAML via serde_yaml instead
  of format!, avoiding malformed/misinterpreted YAML if a URL or hash
  contains YAML-special characters
- from_toml error message now names trident-acl-agent.conf instead of
  the generic "config.toml"
- CURRENT_VERSION_STUB changed to a sentinel that cannot collide with a
  real AKS/Trident release version string, preventing a spurious
  AlreadyAtTarget short-circuit
- StateStore::save now writes to a temp file and renames atomically
  instead of truncate-then-write, so a crash/power-loss around a real
  reboot cannot corrupt state.json
- hyper-util moved to [workspace.dependencies] and referenced via
  workspace = true, matching repo convention

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 8c06585d-a82d-475f-a802-83fdfa012d86
@frhuelsz
Paco Huelsz (frhuelsz) force-pushed the user/bfjelds/acl-agent-rollback-grpc-rust branch from 9af6d0c to dee5266 Compare August 11, 2026 18:12
Comment thread crates/trident-acl-agent/src/annotations.rs Outdated
Doc comments no longer name or cite section numbers of the (unlinked)
accepted design doc; reworded prose to stand on its own. Also updated
lib.rs module doc to describe the current annotation-protocol behavior
directly rather than as a change from history. No behavior change.
This filename referred to the same private accepted-design doc whose
links were already removed; it does not correspond to a file in this
repo. Reworded to "the design doc" without a path. No behavior change.
annotations/mod.rs, core/mod.rs, and omahaonly/mod.rs each restated a
description of their own module already summarized in lib.rs ' s
top-level module list. Removed the duplicate module-level doc
comments and folded annotations '  submodule breakdown into its lib.rs
bullet instead of keeping it in two places.

Also re-verified all other outstanding frhuelsz PR 730 review threads:
the main.rs logging-filter/cli.rs/connection_check.rs restructuring
comments are already satisfied by the current code (FilteredLogger
already lives in osutils::logging and is reused here; Args/
ConnectionTarget already live in cli.rs; validate_connection already
lives in connection_check.rs), and the suggestion to fold
build_machine_id() into its caller no longer applies now that it has
multiple callers (orchestrator.rs, omahaonly). No behavior change.
- re-export core::nebraska at crate root so crate::nebraska intra-doc
  links and README examples resolve (was silently broken by the earlier
  core/ module reorg)
- FilteredLogger::log() now also honors the wrapped inner loggers own
  enabled(), instead of only checking its own level/target filter
- StateStore::save() creates its temp file with create_new (not
  create+truncate) so a stale temp file left by a prior crash cannot
  keep broader permissions than STATE_FILE_MODE; a stale file is removed
  and recreated rather than reused
PR #751 (merged into main) moved MultiLogger/LogFilter from trident into
a new osutils::logging/ directory module, while this branch had
independently added its own osutils::logging.rs (single file) containing
FilteredLogger - same module path, different shapes, causing an E0761
module-ambiguity build error after rebasing onto post-#751 main.

Resolved by treating #751's shared LogFilter as the canonical
implementation (it already supports everything FilteredLogger did, via
its existing with_global_filter builder, matching the exact pattern
trident/src/main.rs already uses for its own noisy-target suppression)
and porting trident-acl-agent's two call sites onto it, rather than
keeping a parallel duplicate wrapper type:

- Deleted crates/osutils/src/logging.rs (FilteredLogger).
- crates/trident-acl-agent/src/main.rs: added a small build_logger()
  helper that chains LogFilter::with_global_filter() over
  NETWORK_LOG_TARGETS, replacing FilteredLogger::new(). max_level is now
  computed inline (verbosity.max(network_verbosity)) since LogFilter
  has no equivalent to FilteredLogger::max_level().

Verified: cargo build/test/clippy/fmt clean for osutils, trident-acl-agent,
and trident (386+148+158 tests pass).

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.

Pull request overview

Copilot reviewed 41 out of 49 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

Makefile:437

  • Same issue as update-notice: validate-notice runs cargo about for multiple targets (per packaging/notice/about.toml), but cargo fetch --locked without explicit --target only fetches sources for the host default target. This can still lead to missing license sections for target-specific crates.

Fetch with explicit targets to match about.toml before running cargo-about.

validate-notice:
	@echo ""
	@echo "Validating third-party NOTICE..."
	@mkdir -p target
	cargo fetch --locked
	cargo about $(CARGO_ABOUT_ARGS) -o $(NOTICE_JSON)

Comment thread Makefile
Comment thread crates/trident-acl-agent/src/annotations/orchestrator.rs Outdated
Comment thread crates/trident-acl-agent/src/annotations/orchestrator.rs Outdated
Comment thread crates/trident-acl-agent/src/annotations/orchestrator.rs
Comment thread crates/trident-acl-agent/src/annotations/orchestrator.rs
Comment thread crates/trident-acl-agent/src/annotations/orchestrator.rs
Comment thread crates/trident-acl-agent/src/annotations/orchestrator.rs Outdated
Comment thread crates/trident-acl-agent/src/annotations/orchestrator.rs Outdated
Comment thread crates/trident-acl-agent/src/annotations/orchestrator.rs Outdated
Addresses PR 730 review comments on annotations/orchestrator.rs:

- state.rs: add StateStore::update() atomic load->mutate->save combinator
  and remember_completed_and_clear_pending(), so a completed record and a
  pending-commit clear happen in one write instead of two separate saves
  with a crash window between them. Used by every post-reboot completion
  path (resume_pending_commit success, reboot-failure branches, and the
  connect-error degraded path).

- recover_from_trident_state: reorder so a pending post-reboot commit is
  resumed before the first Kubernetes call, not after - commit() is a
  purely local tridentd call and is exactly the time-sensitive step a k8s
  outage must not block. Return type changed to Result<LoopControl, Error>
  so a request reconstructed via the new function below can also trigger
  a real reboot during startup recovery, not just at request time.

- New reconstruct_without_pending_record(): when state.json has no
  pendingCommit at all for an in-flight finalize/rollback (missing
  entirely, or lost across the reboot), compare current_active_version()
  against the request's target before ever calling commit() - which is
  state-changing and can silently report Success for a no-op or destroy
  an armed-but-unbooted update if called as an unconditional probe.
  active == target hands off to the existing reconstruction path;
  active != target runs the request fresh instead of guessing further.
  Distinguishing  never rebooted from rebooted rebooted" from "rebooted, firmware fell back
  would need Trident to expose boot history over gRPC, which it does not
  today - documented as a known remaining gap.

- reconcile_node: a retried request with the same operationId as an
  outstanding pendingCommit now resumes that commit instead of falling
  through to handle_finalize/handle_rollback, which would re-drive
  UpdateFinalize/RollbackFinalize against a boot that may already be
  armed or in flight.

- resume_pending_commit: re-issue the reboot when the boot marker still
  matches (agent restarted before the original reboot ever took effect),
  instead of only logging and waiting forever.

- handle_finalize: require reboot_status == RebootRequired before arming
  pendingCommit and rebooting, mirroring handle_rollback's existing
  servicing_kind check - previously any Ok(_) from update_finalize was
  treated as boot armed, so a no-op finalize could reboot a node and
  report a false-positive Success.

- handle_stage: on Nebraska CheckOutcome::UpdateInProgress (a stage
  interrupted mid-download by a crash/reboot), send a compensating Failed
  event to clear the wedge before reporting failure, instead of silently
  leaving the instance permanently stuck.

- commit_result_to_status / reconstruct_commit_result_to_status: treat
  servicing_kind == NoneRequired as nothing committed rather than
  Success.

- map_trident_failure renamed to map_trident_commit_failure and restricted
  to the post-reboot commit-status builders; finalize_failure_status,
  rollback_stage_failure_status, and rollback_finalize_failure_status now
  always report OperationFailed, since TargetBootFailed is contractually
  reserved for the commit status.

- orchestrator.rs:116 nit: added the missing blank line between reboot()
  and run().

Verified: cargo build/test/clippy/fmt clean across the workspace (901
tests pass, including 2 new tests covering the NoneRequired servicing_kind
fix and 2 existing tests updated to assert the corrected
OperationFailed-not-TargetBootFailed pre-reboot behavior).
- config.rs: interpolate the ENV_PREFIX_* consts into the envy context
  messages instead of hardcoding the prefix strings, so they cannot drift
  if a prefix const ever changes.
- version.rs: add a doc comment to the public current_active_version().
- osrelease.rs: extract a shared parse_line() helper used by read_key,
  OsRelease::parse, and ExtensionRelease::parse - previously each carried
  its own copy of the same KEY=VALUE trim/unquote logic. Added direct
  unit test coverage for read_key (previously only exercised indirectly
  via trident-acl-agent's wrapper tests), including a test asserting it
  agrees with OsRelease::parse on the same input.

Left crates/trident-acl-agent/src/cli.rs's --validate-connection as a
flag rather than a subcommand: it is an orthogonal early-exit diagnostic
mode layered on top of the agent's otherwise-single run mode, not a
distinct action that would benefit from clap subcommand structure.

Verified: cargo build/test/clippy/fmt clean.

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.

Pull request overview

Copilot reviewed 40 out of 49 changed files in this pull request and generated 1 comment.

Comment on lines +40 to 45
fn build_logger<L: log::Log>(inner: L, args: &Args) -> LogFilter<L> {
NETWORK_LOG_TARGETS.iter().fold(
LogFilter::new(inner).with_max_level(args.verbosity),
|logger, target| logger.with_global_filter(*target, args.network_verbosity),
)
}
recover_from_trident_state's one-shot self.k8s.get_node(...) call (used
only when there is no pendingCommit to resume locally) had no retry at
all, unlike the watch loop's stream (which already retries/backs off
via kube::runtime::watchers default_backoff()) and the terminal-status
publish path (best_effort_publish_terminal). A transient k8s hiccup at
that exact moment propagated straight through ? into a process exit.

Added get_node_with_retry(): a bounded retry (3 attempts, 2s backoff,
matching best_effort_publish_terminal's shape) around that single call,
returning NodeGone immediately without retrying since that's terminal.

Verified: cargo build/test/clippy/fmt clean across the workspace.
…itly

Expand reconstruct_without_pending_record's doc comment with an
explicit desired-vs-implemented-vs-needed breakdown of the design doc's
4-branch degraded-recovery reconstruction (2.3), so the remaining gap
(no gRPC-exposed boot history to distinguish a firmware fallback from
never having rebooted) is discoverable directly on the function instead
of only in review-comment history.

Doc-only change, no behavior change.

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.

Pull request overview

Copilot reviewed 40 out of 49 changed files in this pull request and generated 1 comment.

Comment on lines +999 to +1003
/// KNOWN GAP: the design doc's degraded-recovery path (2.3) has 4
/// branches; this implements only 2 of them, folding the other 2
/// together as "run fresh":
/// 1. active == target -> run `commit()` [done]
/// 2. active != target, boot attempted+failed -> report `TargetBootFailed` [missing]
… gap

Adds a second, independent signal for confirming a reboot happened when
state.json carries no pendingCommit record for an in-flight finalize/
rollback, on top of the existing active-version-vs-target comparison:

- osutils::machine_id::boot_time(): reads /proc/stat's btime line (the
  current boot's absolute wall-clock start time - the same value
  systemctl show -p KernelTimestamp exposes, read directly with no
  subprocess/systemd dependency). No persistent storage assumption
  required, unlike a journald-based approach.

- reboot_confirmed_since_arming(): compares that boot time against the
  finalize/rollback's own terminal status' finished_utc, which is
  already published to the Node's update-status annotation *before* the
  reboot is triggered (external to local disk, so it survives even when
  state.json itself does not). If the current boot started after that
  timestamp, a reboot has demonstrably happened since, regardless of
  which version the node landed on - so it's safe to call commit() and
  let Trident's own response (already handled by
  reconstruct_commit_result_to_status's indicates_target_boot_failed
  check) distinguish success from a firmware fallback.

- reconstruct_without_pending_record now treats "already at target
  version" OR "reboot confirmed via boot time" as proof a boot happened,
  instead of only the version comparison. This closes the specific gap
  called out in the prior commit's doc comment for the common case
  where operation_status was successfully published before the reboot -
  a real firmware fallback in that case is now correctly reported as
  TargetBootFailed instead of being silently retried as a fresh
  finalize/rollback.

Remaining corner (still requires Trident's own boot history over gRPC,
not addressed here): operation_status itself absent or missing
finished_utc (e.g. the pre-reboot status publish also failed) - falls
back to the existing safe-but-imprecise "run fresh" behavior. Updated
reconstruct_without_pending_record's doc comment accordingly.

Verified: cargo build/test/clippy/fmt clean across the workspace (908
tests pass, including 3 new osutils::machine_id::boot_time tests and 4
new orchestrator::reboot_confirmed_since_arming tests).
…r seen

reconstruct_without_pending_record previously computed the version-match
check unconditionally, even when operation_status was entirely None (the
agent never started processing this operationId at all - it's set to
InProgress immediately on dispatch, before anything else). A version
match alone in that state isn't proof this specific request caused
anything: the node could already be on targetVersion for an unrelated
reason (a prior, already-completed operation; a manually re-imaged
node), in which case handle_finalize/handle_rollback's own
AlreadyAtTarget check is what should produce the status - not a
speculative commit() call via the degraded-recovery path.

Fixed by short-circuiting to running the request fresh (extracted into a
small run_request_fresh() helper, reused by the existing dispatch site)
as soon as operation_status is None, before ever computing
current_active_version()/the version comparison. The version-match and
boot-time checks now only run when there's at least some record
(InProgress or terminal) that the agent previously engaged with this
operationId.

Updated reconstruct_without_pending_record's doc comment to describe
this precondition explicitly.

Verified: cargo build/test/clippy/fmt clean across the workspace (908
tests pass, unchanged pass count - this is a control-flow-ordering fix,
not new observable behavior any current test exercises).
…erationId

reconstruct_without_pending_record trusted whatever UpdateStatus happened
to be sitting in the Node's update-status annotation, without checking
that it actually belongs to the request currently being reconstructed.
Snapshot::from_node parses operation_status purely from the annotation
key's current contents - it never cross-references it against the
request's own operationId.

Concretely: if AKS-RP writes a brand-new finalize (operationId Y), and
the update-status annotation still holds a terminal status from an
earlier, unrelated, already-completed operation (operationId X, with its
own old finished_utc), reboot_confirmed_since_arming would compare the
node's boot time against X's finished_utc - which has nothing to do with
Y - and could easily conclude "a reboot has happened since Y was armed"
even though Y was never processed at all. That would route to commit()
for an operation that was never even started, instead of correctly
running it fresh.

Fixed by filtering operation_status down to Some(status) only when
status.operation_id == request.operation_id, folding a stale/mismatched
status into the same "run fresh" path as an entirely absent one.

Updated reconstruct_without_pending_record's doc comment to describe
this precondition.

Verified: cargo build/test/clippy/fmt clean across the workspace (908
tests pass, unchanged pass count - this guards against a scenario no
current test happens to construct).

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.

Pull request overview

Copilot reviewed 40 out of 49 changed files in this pull request and generated no new comments.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

docs/Explanation/Trident-ACL-Agent.md:326

  • The docs say TRIDENT_ACL_AGENT_NEBRASKA_ENDPOINT/_APP_ID/_TRACK are only used by --validate-connection nebraska, but the code also uses these settings for the legacy omaha-only mode (run_omaha_only reads config.nebraska.*). This makes the configuration guidance inaccurate for anyone opting into omaha-only and could lead to a misconfigured deployment.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants