diff --git a/crates/trident-acl-agent/src/annotations/orchestrator.rs b/crates/trident-acl-agent/src/annotations/orchestrator.rs index a776bf367..746ec484e 100644 --- a/crates/trident-acl-agent/src/annotations/orchestrator.rs +++ b/crates/trident-acl-agent/src/annotations/orchestrator.rs @@ -234,6 +234,7 @@ impl Orchestrator { trident_error: None, from_version: None, to_version: None, + trident_version: Some(crate::AGENT_VERSION.to_string()), started_utc: now, last_updated_utc: now, finished_utc: Some(now), diff --git a/crates/trident-acl-agent/src/annotations/protocol.rs b/crates/trident-acl-agent/src/annotations/protocol.rs index 60e5809a0..66840eb67 100644 --- a/crates/trident-acl-agent/src/annotations/protocol.rs +++ b/crates/trident-acl-agent/src/annotations/protocol.rs @@ -19,8 +19,9 @@ use serde::{Deserialize, Serialize}; use url::Url; use uuid::Uuid; -use crate::core::{ - config::DEFAULT_ANNOTATION_PREFIX, error::AgentError, trident::TridentClientError, +use crate::{ + core::{config::DEFAULT_ANNOTATION_PREFIX, error::AgentError, trident::TridentClientError}, + AGENT_VERSION, }; /// Sentinel `kind` used when Trident reported a remote failure without any @@ -150,6 +151,13 @@ pub struct UpdateStatus { pub from_version: Option, #[serde(default, skip_serializing_if = "Option::is_none")] pub to_version: Option, + /// Version of trident-acl-agent that wrote this status (`AGENT_VERSION`). + /// Packaged and versioned together with tridentd via the same RPM build + /// (see `packaging/rpm/trident.spec`'s single `TRIDENT_VERSION`-stamped + /// `%build` step for both `-p trident` and `-p trident-acl-agent`), so + /// this value doubles as the tridentd version for that install. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub trident_version: Option, pub started_utc: DateTime, pub last_updated_utc: DateTime, #[serde(default, skip_serializing_if = "Option::is_none")] @@ -271,16 +279,27 @@ impl UpdateStatus { trident_error: None, from_version, to_version, + trident_version: Some(AGENT_VERSION.to_string()), started_utc, last_updated_utc: finished_or_started, finished_utc, } } + /// Refreshes a status immediately before it is written to the node + /// (the common path for every annotation write - see `publish_status`). + /// Restamps `trident_version` with the currently-running agent's + /// `AGENT_VERSION` rather than trusting whatever value the status + /// already carries: a completed status can be loaded from + /// `state.json` (written by a possibly older agent) and republished + /// later by a newer agent (e.g. `recover_from_trident_state` replaying + /// a cached commit/operation status), so only re-stamping here + /// guarantees the annotation reflects the agent that actually wrote it. pub fn refreshed_for_write(&self) -> Self { let mut refreshed = self.clone(); refreshed.last_updated_utc = Utc::now(); refreshed.message = truncate_message(refreshed.message); + refreshed.trident_version = Some(AGENT_VERSION.to_string()); refreshed } @@ -307,16 +326,22 @@ impl UpdateStatus { self } - /// Compares two statuses ignoring `last_updated_utc`. + /// Compares two statuses ignoring `last_updated_utc` and + /// `trident_version`. /// /// `publish_status` stamps a fresh `last_updated_utc` on every write via /// `refreshed_for_write`, so a straight `PartialEq` between an /// already-on-the-node status and a cached/completed one to decide /// whether a re-publish is needed would never be equal after the first /// publish - triggering another watch event, another "different" - /// comparison, and another publish, forever. Callers that only care - /// whether the *content* already matches (and so a re-publish would be a - /// no-op) must use this instead of `==`/`!=`. + /// comparison, and another publish, forever. `trident_version` is + /// excluded for the same reason: `refreshed_for_write` also + /// unconditionally restamps it with the currently-running agent's + /// `AGENT_VERSION`, so after an upgrade a cached status carrying the old + /// version would otherwise never compare equal to the just-republished + /// one, reintroducing the same infinite re-publish loop. Callers that + /// only care whether the *content* already matches (and so a re-publish + /// would be a no-op) must use this instead of `==`/`!=`. pub fn same_content(&self, other: &Self) -> bool { self.schema_version == other.schema_version && self.node_update_id == other.node_update_id @@ -441,6 +466,39 @@ mod tests { ); } + #[test] + fn refreshed_for_write_restamps_trident_version() { + // Regression test: a completed status can be loaded from + // state.json (written by a possibly older agent) and republished + // later by a newer agent (recover_from_trident_state replaying a + // cached commit/operation status via publish_status). Verify + // refreshed_for_write - the common path for every annotation write + // - always overwrites trident_version with the currently-running + // AGENT_VERSION, rather than preserving whatever an older/stale + // status carries. + let request = sample_request(RequestedOperation::Finalize); + let mut stale = UpdateStatus::new( + &request, + Operation::Finalize, + request.operation_id.clone(), + StatusCode::Success, + "finalize completed", + Some("1.0.0".to_string()), + Some("2.0.0".to_string()), + fixed_time(0), + Some(fixed_time(5)), + ); + stale.trident_version = Some("0.0.1-old-agent".to_string()); + + let republished = stale.refreshed_for_write(); + + assert_eq!( + republished.trident_version.as_deref(), + Some(AGENT_VERSION), + "refreshed_for_write must restamp trident_version with the current agent's AGENT_VERSION" + ); + } + #[test] fn same_content_detects_real_differences() { let request = sample_request(RequestedOperation::Finalize); @@ -470,6 +528,34 @@ mod tests { assert!(!success.same_content(&failed)); } + #[test] + fn same_content_ignores_trident_version() { + // Regression test: refreshed_for_write() always restamps + // trident_version with the currently-running agent's AGENT_VERSION + // (see refreshed_for_write_restamps_trident_version). If + // same_content compared trident_version, a cached status from + // before an agent upgrade would never again compare equal to the + // freshly-republished one, reintroducing the infinite re-publish + // loop same_content exists to prevent. + let request = sample_request(RequestedOperation::Finalize); + let mut cached = UpdateStatus::new( + &request, + Operation::Finalize, + request.operation_id.clone(), + StatusCode::Success, + "finalize completed", + Some("1.0.0".to_string()), + Some("2.0.0".to_string()), + fixed_time(0), + Some(fixed_time(5)), + ); + cached.trident_version = Some("0.0.1-old-agent".to_string()); + let republished = cached.refreshed_for_write(); + + assert_ne!(cached.trident_version, republished.trident_version); + assert!(cached.same_content(&republished)); + } + #[test] fn truncates_messages_longer_than_2048_bytes() { let request = sample_request(RequestedOperation::Stage); @@ -525,6 +611,7 @@ mod tests { assert_eq!(json["message"], "stage completed"); assert_eq!(json["fromVersion"], "1.0.0"); assert_eq!(json["toVersion"], "2.0.0"); + assert_eq!(json["tridentVersion"], AGENT_VERSION); assert!(json.get("startedUtc").is_some()); assert!(json.get("lastUpdatedUtc").is_some()); assert!(json.get("finishedUtc").is_some()); @@ -532,6 +619,24 @@ mod tests { assert!(json.get("nodeUpdateId").is_some()); } + #[test] + fn new_always_populates_trident_version_with_agent_version() { + let request = sample_request(RequestedOperation::Stage); + let status = UpdateStatus::new( + &request, + Operation::Stage, + request.operation_id.clone(), + StatusCode::Success, + "stage completed", + None, + None, + fixed_time(0), + Some(fixed_time(5)), + ); + + assert_eq!(status.trident_version.as_deref(), Some(AGENT_VERSION)); + } + #[test] fn stage_failure_annotation_has_operation_failed_code() { let request = sample_request(RequestedOperation::Stage); @@ -839,6 +944,7 @@ mod tests { }, "fromVersion": { "type": "string" }, "toVersion": { "type": "string" }, + "tridentVersion": { "type": "string", "description": "Version of trident-acl-agent that wrote this status. Packaged and released together with tridentd, so this is also the tridentd version." }, "startedUtc": { "type": "string", "format": "date-time" }, "lastUpdatedUtc": { "type": "string", "format": "date-time", "description": "When the agent last wrote this status. The agent refreshes it on every write, including a periodic InProgress heartbeat, so AKS-RP and the watchdog can tell a working agent from a stuck one." }, "finishedUtc": { "type": "string", "format": "date-time" } diff --git a/crates/trident-acl-agent/src/annotations/state.rs b/crates/trident-acl-agent/src/annotations/state.rs index eabf20db6..53e0125fe 100644 --- a/crates/trident-acl-agent/src/annotations/state.rs +++ b/crates/trident-acl-agent/src/annotations/state.rs @@ -185,8 +185,8 @@ impl StateStore { .entry(status.operation_id.clone()) .or_default(); match status.operation { - Operation::Commit => entry.commit = Some(status.clone()), - _ => entry.operation = Some(status.clone()), + Operation::Commit => entry.commit = Some(strip_trident_version(&status)), + _ => entry.operation = Some(strip_trident_version(&status)), } }) } @@ -215,14 +215,27 @@ impl StateStore { .entry(status.operation_id.clone()) .or_default(); match status.operation { - Operation::Commit => entry.commit = Some(status.clone()), - _ => entry.operation = Some(status.clone()), + Operation::Commit => entry.commit = Some(strip_trident_version(&status)), + _ => entry.operation = Some(strip_trident_version(&status)), } state.pending_commit = None; }) } } +// `UpdateStatus::trident_version` is stamped on every annotation publish +// (see `refreshed_for_write`), so the persisted copy is never read back as +// a trusted value - only ever overwritten before republish. Keeping it out +// of `state.json` entirely avoids a cross-version rollback hazard: an +// older trident-acl-agent binary (from before this field existed) still +// derives `UpdateStatus` with `deny_unknown_fields`, so it would fail to +// parse a state file containing an unrecognized `tridentVersion` key. +fn strip_trident_version(status: &UpdateStatus) -> UpdateStatus { + let mut persisted = status.clone(); + persisted.trident_version = None; + persisted +} + #[cfg(test)] mod tests { use super::*; @@ -359,6 +372,33 @@ mod tests { assert!(entry.commit.is_none()); } + #[test] + fn remember_completed_strips_trident_version_before_persisting() { + // Regression test: UpdateStatus is embedded directly in + // CompletedEntry with deny_unknown_fields, so persisting + // trident_version to state.json would break a pre-this-field + // trident-acl-agent binary reading the file back after an A/B + // rollback. The persisted value is never trusted anyway - + // refreshed_for_write always restamps it before republish - so it + // must never round-trip through disk. + let (_dir, store) = store(); + let mut status = sample_status(Operation::Finalize); + status.trident_version = Some("1.2.3".to_string()); + store + .remember_completed(status) + .expect("remember_completed should succeed"); + + let raw = fs::read_to_string(store.path()).expect("state.json should exist"); + assert!( + !raw.contains("tridentVersion"), + "state.json must not persist tridentVersion: {raw}" + ); + + let state = store.load().expect("load should succeed"); + let entry = state.completed.get("op-1").expect("entry should exist"); + assert_eq!(entry.operation.as_ref().unwrap().trident_version, None); + } + #[test] fn set_and_clear_pending_commit_round_trip() { let (_dir, store) = store(); diff --git a/docs/Explanation/Trident-ACL-Agent.md b/docs/Explanation/Trident-ACL-Agent.md index 3de81e919..d107b54ae 100644 --- a/docs/Explanation/Trident-ACL-Agent.md +++ b/docs/Explanation/Trident-ACL-Agent.md @@ -164,6 +164,7 @@ A status annotation (`/update-status` or "message": "staged update to 202606.29.0", "fromVersion": "202606.15.0", "toVersion": "202606.29.0", + "tridentVersion": "0.22.0", "startedUtc": "2026-06-29T12:00:00Z", "lastUpdatedUtc": "2026-06-29T12:03:41Z", "finishedUtc": "2026-06-29T12:03:41Z" @@ -190,6 +191,11 @@ A status annotation (`/update-status` or Trident reported failure without a structured error payload) and an optional `location` (`path`/`line` in Trident's source), letting an orchestrator key off structured fields instead of parsing `message`. +- `tridentVersion` is the agent's own build version (`AGENT_VERSION`), + stamped on every status write. `tridentd` and `trident-acl-agent` are + built and released together from a single RPM spec + (`packaging/rpm/trident.spec`), so this value doubles as the `tridentd` + version for that install. - `startedUtc`/`lastUpdatedUtc`/`finishedUtc` bound the operation: `lastUpdatedUtc` refreshes on a heartbeat cadence while `code` is `InProgress` (see [below](#pre-post-reboot-state-and-the-watchdog));