acl: add tridentVersion to the update-status annotation - #762
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. |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds tridentVersion metadata to ACL update-status annotations.
Changes:
- Extends
UpdateStatusand its JSON schema. - Populates the field from
AGENT_VERSION. - Adds serialization and constructor tests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
annotations/protocol.rs |
Adds the version field, serialization, schema, and tests. |
annotations/orchestrator.rs |
Populates the field for invalid-request statuses. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Azure Pipelines: Successfully started running 1 pipeline(s). There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (1)
crates/trident-acl-agent/src/annotations/protocol.rs:248
- Issue:
tridentVersionis stamped when a status is constructed, not on every write as the field contract states. Evidence:publish_statuscan republish anUpdateStatusloaded from the completed-state cache, andrefreshed_for_write()currently updates only the timestamp/message, so after an agent upgrade that write can still advertise the older writer's version. Suggestion: overwritetrident_versionwith the currentAGENT_VERSIONinrefreshed_for_write()(and cover republishing a cached status with a stale or absent value).
trident_version: Some(AGENT_VERSION.to_string()),
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (1)
crates/trident-acl-agent/src/annotations/protocol.rs:291
- Issue: Treating
trident_versionas stable content can create an endless PATCH/watch loop after a cached status is republished by a newer agent. Evidence:recover_from_trident_stateandreconcile_nodecompare the unchanged persisted status with the node annotation viasame_content;publish_statusrestamps only its cloned wire value, so the cache retains the old version and differs again on every subsequent reconcile. Suggestion: treat this write-time metadata likelast_updated_utcand exclude it from deduplication (and add a stale-cache/second-reconcile regression case).
&& self.trident_version == other.trident_version
bfjelds (bfjelds)
force-pushed
the
user/bfjelds/acl-trident-version
branch
from
September 1, 2026 20:53
5b2a2b1 to
23f5892
Compare
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The version is consistently published, safely excluded from persisted state and deduplication, and covered by regression tests.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Balanced
bfjelds (bfjelds)
force-pushed
the
user/bfjelds/acl-trident-version
branch
from
September 1, 2026 22:19
23f5892 to
b98f23f
Compare
bfjelds (bfjelds)
changed the base branch from
main
to
user/bfjelds/acl-error-object
September 1, 2026 22:20
| trident_error: None, | ||
| from_version: None, | ||
| to_version: None, | ||
| trident_version: Some(crate::AGENT_VERSION.to_string()), |
Contributor
There was a problem hiding this comment.
nit, import AGENT_VERSION directly
| trident_error: None, | ||
| from_version: None, | ||
| to_version: None, | ||
| trident_version: Some(crate::AGENT_VERSION.to_string()), |
Contributor
There was a problem hiding this comment.
this will hold for now, but in the future we may want to not assume taa and tridentd are perfectly coupled
Paco Huelsz (frhuelsz)
approved these changes
Sep 1, 2026
Adds an optional tridentVersion field to the ACL A/B update-status annotation, populated from trident-acl-agent's own AGENT_VERSION on every status write. trident-acl-agent and tridentd are built together from a single spec file (packaging/rpm/trident.spec), with one TRIDENT_VERSION stamp applied to both -p trident and -p trident-acl-agent in the same %build step, so this value doubles as the tridentd version for any RPM-installed node.
refreshed_for_write() is the common path for every annotation write (publish_status), but only new() stamped AGENT_VERSION. A completed status loaded from state.json (written by a possibly older agent) and later republished by a newer agent - e.g. recover_from_trident_state replaying a cached commit/operation status - kept whatever trident_version the persisted status already carried, so the annotation no longer identified the agent that actually wrote it. Stamp AGENT_VERSION in refreshed_for_write() instead, and add a regression test.
…ersistence - same_content() compared trident_version, but refreshed_for_write() always restamps it with the current agents AGENT_VERSION on every publish. Comparing it reintroduces the infinite re-publish loop same_content exists to prevent once the agent is upgraded. - UpdateStatus is embedded directly in CompletedEntry (state.json) with deny_unknown_fields. Persisting trident_version there would break an older trident-acl-agent binary reading the file back after an A/B rollback. The persisted value is never trusted anyway, so strip it before writing to disk. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
bfjelds (bfjelds)
force-pushed
the
user/bfjelds/acl-trident-version
branch
from
September 1, 2026 23:40
b98f23f to
5a53d18
Compare
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.
Adds an optional tridentVersion field to the ACL A/B update-status annotation, populated from trident-acl-agent's own AGENT_VERSION on every status write.
trident-acl-agent and tridentd are built together from a single spec file (packaging/rpm/trident.spec), with one TRIDENT_VERSION stamp applied to both -p trident and -p trident-acl-agent in the same %build step, so this value doubles as the tridentd version for any RPM-installed node.
Validated: https://dev.azure.com/mariner-org/ECF/_build/results?buildId=1195298&view=logs&j=09bab65c-0c5c-54aa-a756-16c7a376f940&t=09bab65c-0c5c-54aa-a756-16c7a376f940