feat: add preferred type to retry nodeAntiAffinity. Fixes #13969 - #16610
feat: add preferred type to retry nodeAntiAffinity. Fixes #13969#16610somaz94 wants to merge 4 commits into
Conversation
Signed-off-by: somaz <genius5711@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRetry node anti-affinity now supports ChangesRetry node anti-affinity
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant RetryOnDifferentHost
participant RetryAffinityHelper
participant KubernetesScheduler
RetryOnDifferentHost->>RetryAffinityHelper: process failed hostnames
alt Preferred mode
RetryAffinityHelper->>KubernetesScheduler: add weighted preferred NotIn terms
else Required or empty mode
RetryAffinityHelper->>KubernetesScheduler: add required NotIn terms
end
KubernetesScheduler-->>RetryOnDifferentHost: return updated affinity
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/executor_swagger.md`:
- Around line 3392-3411: Update the RetryNodeAntiAffinityType documentation in
the Swagger section to state that an omitted or empty type defaults to Required,
and describe the behavioral difference between Required and Preferred modes. Add
this information to the generated field or type description while preserving the
existing allowed-values list.
In `@docs/walk-through/retrying-failed-or-errored-steps.md`:
- Around line 34-37: Qualify Preferred node anti-affinity behavior across all
three documented sites: in docs/walk-through/retrying-failed-or-errored-steps.md
lines 34-37, state that retries may use previously tried hosts when other
scheduling constraints permit; in docs/fields.md lines 4582-4587, replace “must
remain schedulable” with equivalent conditional wording; and in
.features/pending/retry-node-anti-affinity-preferred.md lines 6-9, replace
“every eligible host has failed” with “every eligible host has been tried” and
clarify that scheduling is only possible when other constraints permit it.
In `@workflow/util/retry/retry.go`:
- Around line 88-99: Update the existing-term merge in the preferred
node-affinity flow to set targetTerms[i].Weight to
PreferredHostAntiAffinityWeight before returning targetAffinity. Keep the
current Key/Operator matching and hostname deduplication behavior unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ea3c9182-7a7e-40c8-9f24-877cde46e9e2
⛔ Files ignored due to path filters (5)
api/openapi-spec/swagger.jsonis excluded by!**/api/openapi-spec/*.jsonpkg/apis/workflow/v1alpha1/generated.pb.gois excluded by!**/*.pb.go,!**/*.pb.gopkg/apis/workflow/v1alpha1/openapi_generated.gois excluded by!**/*_generated.gosdks/java/client/docs/IoArgoprojWorkflowV1alpha1RetryAffinity.mdis excluded by!**/sdks/java/client/**sdks/java/client/docs/IoArgoprojWorkflowV1alpha1RetryNodeAntiAffinity.mdis excluded by!**/sdks/java/client/**
📒 Files selected for processing (22)
.features/pending/retry-node-anti-affinity-preferred.mdapi/jsonschema/schema.jsondocs/executor_swagger.mddocs/fields.mddocs/walk-through/retrying-failed-or-errored-steps.mdmanifests/base/crds/full/argoproj.io_clusterworkflowtemplates.yamlmanifests/base/crds/full/argoproj.io_cronworkflows.yamlmanifests/base/crds/full/argoproj.io_workflows.yamlmanifests/base/crds/full/argoproj.io_workflowtasksets.yamlmanifests/base/crds/full/argoproj.io_workflowtemplates.yamlmanifests/base/crds/minimal/argoproj.io_workflowtasksets.yamlmanifests/quick-start-minimal.yamlmanifests/quick-start-mysql.yamlmanifests/quick-start-postgres.yamlmanifests/quick-start-telemetry.yamlpkg/apis/workflow/v1alpha1/generated.protopkg/apis/workflow/v1alpha1/workflow_types.gopkg/plugins/executor/swagger.ymlworkflow/controller/retry_tweak.goworkflow/controller/retry_tweak_test.goworkflow/util/retry/retry.goworkflow/util/retry/retry_test.go
…ng term Signed-off-by: somaz <genius5711@gmail.com>
tico24
left a comment
tico24
left a comment
There was a problem hiding this comment.
Docs only from me: docs/walk-through/retrying-failed-or-errored-steps.md and the feature note. The rest is for @argoproj/argo-workflows-approvers.
I checked the page against retry.go and retry_tweak.go and it matches what the code does, including nodeAntiAffinity: {} still behaving as it did before. Few small things inline.
| * `backoff` is an exponential back-off | ||
| * `nodeAntiAffinity` prevents running steps on the same host. Current implementation allows only empty `nodeAntiAffinity` (i.e. `nodeAntiAffinity: {}`) and by default it uses label `kubernetes.io/hostname` as the selector. | ||
| * `nodeAntiAffinity` prevents running steps on the same host. By default it uses label `kubernetes.io/hostname` as the selector. | ||
| Its `type` controls how strictly earlier hosts are avoided: |
There was a problem hiding this comment.
The retryPolicy bullet above tags OnTransientError with the version it landed in. Worth doing the same for type? On an older version the field just gets pruned and there's nothing here to explain why.
There was a problem hiding this comment.
Good point — done in 2df54d81c, tagged the same way as the retryPolicy bullet:
Its
typecontrols how strictly earlier hosts are avoided (available after v4.2.0).
I picked v4.2.0 by inference rather than knowledge: main is at v4.1.0-rc2 with no release-4.1 branch cut yet, so this looks like it misses the 4.1 train. If you're targeting something else, say the word — it's a one-word change and I'd rather have it right than have the page lie the way you describe.
| * `nodeAntiAffinity` prevents running steps on the same host. By default it uses label `kubernetes.io/hostname` as the selector. | ||
| Its `type` controls how strictly earlier hosts are avoided: | ||
| * `Required` (the default, and what an empty `nodeAntiAffinity: {}` gives you) excludes those hosts outright, so a retry stays unschedulable once every eligible host has been tried. | ||
| * `Preferred` only de-prioritises them, so a retry can be scheduled onto a previously tried host once every eligible host has been tried. This is a preference rather than a guarantee — other scheduling constraints can still leave the retry pending. Use this when the retry limit is higher than the number of eligible nodes. |
There was a problem hiding this comment.
Three sentences on this line, and two on line 34. Mind splitting?
There was a problem hiding this comment.
Split in 2df54d81c — one sentence per line now, on both the line you flagged and line 34:
* `nodeAntiAffinity` prevents running steps on the same host.
By default it uses label `kubernetes.io/hostname` as the selector.
Its `type` controls how strictly earlier hosts are avoided (available after v4.2.0).
* `Required` is the default, and is what an empty `nodeAntiAffinity: {}` gives you.
It excludes previously tried hosts outright, so a retry stays unschedulable once every eligible host has been tried.
* `Preferred` only de-prioritises them.
A retry can then still be scheduled onto a previously tried host once every eligible host has been tried.
This is a preference rather than a guarantee, so other scheduling constraints can still leave the retry pending.
Use it when the retry limit is higher than the number of eligible nodes.I also swapped the em-dash in the "preference rather than a guarantee" sentence for a plain clause while I was in there.
| * `Preferred` only de-prioritises them, so a retry can be scheduled onto a previously tried host once every eligible host has been tried. This is a preference rather than a guarantee — other scheduling constraints can still leave the retry pending. Use this when the retry limit is higher than the number of eligible nodes. | ||
|
|
||
| ```yaml | ||
| retryStrategy: |
There was a problem hiding this comment.
Indented four spaces, where the block at the top of the page isn't, so the whitespace shows up in the output. Did you mean this to sit inside the bullet above?
There was a problem hiding this comment.
No, that was unintentional — I lifted the snippet out of the full manifest above and kept its original indentation, so it inherited four spaces it didn't need. Dedented to column 0 in 2df54d81c to match the block at the top of the page:
retryStrategy:
limit: 10
affinity:
nodeAntiAffinity:
type: Preferredmake docs is clean on all three changes — typos, cspell (0 issues across 135 files) and properdocs build --strict all pass.
Signed-off-by: somaz <genius5711@gmail.com>
2 participants
Fixes #13969
Motivation
retryStrategy.affinity.nodeAntiAffinityalways excludes the hosts earlier attempts ran on with arequiredDuringSchedulingIgnoredDuringExecutionterm.Once every eligible host has been tried, there is no host left that satisfies the term and the retry stays unschedulable, which is the case the issue describes.
This adds the soft variant so retries stay schedulable. It is the first of the two features requested in #13969; the cap on how many hosts are excluded is left for a separate PR, as suggested in the issue.
Modifications
RetryNodeAntiAffinitywas an empty struct documented as "a placeholder for future expansion". It now carries atypefield:Requiredkeeps the current behaviour and is the default, so an existingnodeAntiAffinity: {}is unaffected.Preferredemits apreferredDuringSchedulingIgnoredDuringExecutionterm instead, at weight 100.AddHostnamesToAffinityis untouched. The preferred path is a sibling function,AddHostnamesToPreferredAffinity, which merges into an existing term for the same host selector (de-duplicating hosts) and otherwise appends, leaving any preference the user configured themselves in place.I kept the weight fixed at 100 rather than exposing it, to keep this change small. Happy to make it configurable if you would prefer that.
Verification
New unit tests:
TestAddHostnamesToPreferredAffinitycovers the empty affinity, emptyNodeAffinity, merge-into-existing-term (with de-duplication), append-alongside-an-unrelated-preference, and leave-the-required-term-alone cases.TestRetryOnDifferentHostAntiAffinityTypecovers the controller dispatch: unset type, explicitRequired,Preferred, and nonodeAntiAffinityat all.Both
Requiredcases assert that the preferred list stays empty and thePreferredcase asserts that the required term stays nil, since a leftover required term would keep the retry unschedulable and defeat the point of the feature.I checked the tests actually exercise the new branch by temporarily forcing the dispatch to the required path: only the
Preferredsubtest fails, the others still pass.Note for anyone reproducing this locally on macOS: the generated files only refresh with
gmake ... -B, and the defaultmakeis 3.81, which cannot parse theifeqblocks in the Makefile.Documentation
docs/walk-through/retrying-failed-or-errored-steps.mdis where users meet this feature, and it currently states that only an emptynodeAntiAffinityis allowed. That sentence is replaced with a description of both types and a short example.docs/fields.mdand the swagger/JSON schema outputs are regenerated, so the new field also shows up in the Fields reference.A feature note is included at
.features/pending/retry-node-anti-affinity-preferred.mdwith a worked example.AI
Claude Code was used as a coding assistant for the implementation, tests and this description.
Everything was reviewed and verified locally before opening (
gmake codegen -B,gmake lint,gmake docsand the unit tests all pass, and the generated files are real codegen output). I take responsibility for the contents of this PR.Summary by CodeRabbit