trident-acl-agent: rewrite as annotation-based ACL update agent - #730
Conversation
|
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. |
6598fdd to
c3dfd8d
Compare
There was a problem hiding this comment.
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
tridentdfor 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. |
There was a problem hiding this comment.
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 forsystemctlelsewhere 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, theprocess::Commandimport 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-utilis 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 addinghyper-utilto[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 thatoperation_idis a UUID. SinceoperationIdis specified asformat: 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 {
- 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
There was a problem hiding this comment.
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 requesttimeoutSecondstowatch_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 keepwatch_poll_intervalfor 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 newStringfor every log record (format!("{prefix}::")insideany(...)). This runs on the hot path of log filtering and can add noticeable overhead under verbose logging. Usestrip_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 ownedStringfor 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}"),
…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
…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.
- 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
9af6d0c to
dee5266
Compare
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.
…fig, rename GoalSource to Mode
- 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).
There was a problem hiding this comment.
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-noticerunscargo aboutfor multiple targets (perpackaging/notice/about.toml), butcargo fetch --lockedwithout explicit--targetonly 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)
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.
| 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.
| /// 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).
There was a problem hiding this comment.
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/_TRACKare only used by--validate-connection nebraska, but the code also uses these settings for the legacyomaha-onlymode (run_omaha_onlyreadsconfig.nebraska.*). This makes the configuration guidance inaccurate for anyone opting intoomaha-onlyand could lead to a misconfigured deployment.
Summary
trident-acl-agentimplements annotation-based design (https://msazure.visualstudio.com/One/_git/Compute-ACL-Update-Service?version=GCeb7e534b2415ad52b37ef22fd49685e81e56c8aa&path=/docs/update-trigger-design.md): watching Node's<prefix>/update-requestannotation, driving update stage/finalize, rollback stage/finalize, and commit gRPC operations against tridentd, and writing<prefix>/update-statusand<prefix>/update-commit-status.--validate-connectionfor kubernetes|tridentd|nebraska.Context
This is the second step in enabling trident-acl-agent to run updates and rollbacks. Related PRs:
Validation