fix: reject stale copies of completed workflows using resourceVersion comparison#16357
Conversation
… comparison. Fixes argoproj#16305 Replace the recently-completed workflow cache and the last-seen-version annotation machinery with a single UID-keyed record of the last resourceVersion written by the controller, compared numerically using k8s.io/apimachinery/pkg/util/resourceversion. The recently-completed cache recorded namespace/name keys but was checked with name only, so it never matched and stale copies of completed or deleted workflows could be reprocessed. Keying by UID also avoids wrongly rejecting a retried workflow or a recreated workflow that reuses a name. The controller no longer writes the workflows.argoproj.io/last-seen-version annotation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Alan Clucas <alan@clucas.org>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
💤 Files with no reviewable changes (1)
👮 Files not reviewed due to content moderation or server errors (3)
📝 Walkthrough
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Joibel
added
cherry-pick/3.7
Joibel
added
cherry-pick/3.7
|
❌ Cherry-pick failed for 3.7. Please check the workflow logs for details. |
|
❌ Cherry-pick failed for 4.0. Please check the workflow logs for details. |
|
Not cherry-picking to 3.7 because the k8s libs don't support comparing resource versions |
|
❌ Cherry-pick failed for 3.7. Please check the workflow logs for details. |
Actually, going to try the same tactic for 3.7 as 4.0. |
3 participants
Fixes #16305
Motivation
The recently-completed workflow cache added in #12198 records informer keys
(
namespace/name) butprocessNextItemchecked it withnameonly. AKubernetes name can never contain
/, so the check never matched and the"Cache: Rejecting recently deleted" guard was dead code: a stale, still-running
copy of a recently completed or deleted workflow re-entering the informer
(e.g. via a relist after watch interruption) could be reprocessed,
potentially recreating pods.
Fixing just the key would introduce a new problem: a name-keyed membership
check would wrongly reject a retried workflow, or a recreated workflow
reusing a name, for up to 10 minutes after completion.
Since #15090 the controller also tracks staleness for in-flight workflows via
the
workflows.argoproj.io/last-seen-versionannotation plus an in-memoryUID→resourceVersion map, using equality comparison because resourceVersions
could not officially be ordered.
k8s.io/apimachinery/pkg/util/resourceversionnow provides sanctioned ordered comparison of resourceVersions of the same
object, which lets both mechanisms be replaced by one simpler, UID-keyed one.
Modifications
recentCompletionsandlastSeenVersionswith a singlelastWrittenVersionsmap, keyed by workflow UID, recording the lastresourceVersion this controller wrote (or saw complete/be deleted) and
when the workflow completed.
isOutdatednow compares resourceVersions withresourceversion.CompareResourceVersionand reports whether the recordedversion was the workflow's completed/deleted state: stale copies of
in-flight workflows are requeued (as before), stale copies of completed
workflows are dropped. Incomparable resourceVersions fail open (the
workflow is processed) with a warning.
check: the recorded completion resourceVersion acts as a floor, so only
copies strictly older than it are rejected. A retry mutates the same
object (same UID) and therefore always carries a newer resourceVersion;
the controller's first write of the retried workflow then clears the
completed state, restoring requeue semantics for in-flight staleness.
A recreated workflow reusing a name has a new UID and is untracked.
persistUpdates/persistWorkflowSizeLimitErrrecord the Update responseresourceVersion; the controller no longer writes the
workflows.argoproj.io/last-seen-versionannotation, and the constant isremoved. Existing annotations on workflows are ignored and harmless.
cache's retention); expiry scans are throttled to once per minute.
A stale completion event cannot regress the recorded version.
Verification
TestIsOutdated,TestExpireCompletedVersions) cover theRecently completed workflow cache stores namespace/name but checks name #16305 regression scenario, the full retry cycle (newer copy passes while
the completed record stands; the first controller write after retry clears
the completed state so stale copies requeue rather than drop), name reuse,
fail-open on incomparable resourceVersions, no-regress on stale completion
events, and record expiry. Neither previous mechanism had unit coverage.
go test ./workflow/controller/... ./workflow/common/...passes.golangci-lint runon the changed packages reports only pre-existingissues on untouched lines.
Documentation
Not needed: this is internal controller bookkeeping. The removed
last-seen-versionannotation was never documented.AI
The idea to swtich to resourceVersion was mine.
This PR was developed with Claude Code (Claude Fable 5): analysis of the issue, the code changes, tests, and this PR text, reviewed and directed by me.