wip: Alow configuration of waitForPodsReady feature - #2140
Open
MaysaMacedo wants to merge 1 commit into
Open
Conversation
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
WalkthroughThis PR adds by-workload gang scheduling timeout, recovery timeout, and requeuing configuration, removes the ChangesGang scheduling configuration
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ByWorkload
participant buildWaitForPodsReady
participant buildRequeuingStrategy
ByWorkload->>buildWaitForPodsReady: timeout and requeuing settings
buildWaitForPodsReady->>buildRequeuingStrategy: RequeuingStrategy
buildRequeuingStrategy-->>buildWaitForPodsReady: configapi.RequeuingStrategy
buildWaitForPodsReady-->>ByWorkload: configapi.WaitForPodsReady
Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 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 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: MaysaMacedo The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
Contributor
Author
|
/cc @kannon92 |
![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 `@pkg/apis/kueueoperator/v1/types.go`:
- Around line 401-425: Add lower-bound validation markers to the
RequeuingStrategy fields so negative values are rejected before they reach
buildRequeuingStrategy. Update the BackoffLimitCount, BackoffBaseSeconds, and
BackoffMaxSeconds definitions in RequeuingStrategy to include appropriate
+kubebuilder:validation:Minimum constraints, matching the existing validation
style used in this type. Keep the JSON field names and comments intact while
ensuring the generated schema enforces valid non-negative backoff settings.
- Around line 368-388: Add non-negative validation to both TimeoutSeconds and
RecoveryTimeoutSeconds in the workload readiness spec so invalid negative values
are rejected at CRD admission. Update the markers on the fields in types.go with
kubebuilder Minimum constraints consistent with other numeric fields in this
API, and then run make generate so the regenerated CRD manifests reflect the new
validation.
In `@pkg/configmap/configmap.go`:
- Around line 164-178: Add table-driven tests in configmap_test.go to cover the
new mapping branches in buildRequeuingStrategy and buildWaitForPodsReady. Verify
buildRequeuingStrategy returns nil for nil input, maps
BackoffLimitCount/BackoffBaseSeconds/BackoffMaxSeconds, and leaves Timestamp
unset when TimeReference is empty. Also cover buildWaitForPodsReady for timeout
override, recovery timeout, requeuing strategy passthrough, and the nil
TimeReference path, using the existing helper names so the cases are easy to
locate.
🪄 Autofix (Beta)
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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 8880f7a9-227c-4f54-ac26-7475b2670324
⛔ Files ignored due to path filters (4)
pkg/apis/kueueoperator/v1/zz_generated.deepcopy.gois excluded by!**/zz_generated*pkg/generated/applyconfiguration/kueueoperator/v1/byworkload.gois excluded by!**/generated/**pkg/generated/applyconfiguration/kueueoperator/v1/requeuingstrategy.gois excluded by!**/generated/**pkg/generated/applyconfiguration/utils.gois excluded by!**/generated/**
📒 Files selected for processing (3)
manifests/kueue.openshift.io_kueues.yamlpkg/apis/kueueoperator/v1/types.gopkg/configmap/configmap.go
Comment on lines
+164
to
+178
| func buildRequeuingStrategy(s *kueue.RequeuingStrategy) *configapi.RequeuingStrategy { | ||
| if s == nil { | ||
| return nil | ||
| } | ||
| out := &configapi.RequeuingStrategy{ | ||
| BackoffLimitCount: s.BackoffLimitCount, | ||
| BackoffBaseSeconds: s.BackoffBaseSeconds, | ||
| BackoffMaxSeconds: s.BackoffMaxSeconds, | ||
| } | ||
| if s.TimeReference != "" { | ||
| ts := configapi.RequeuingTimestamp(s.TimeReference) | ||
| out.Timestamp = &ts | ||
| } | ||
| return out | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C3 'buildRequeuingStrategy|RecoveryTimeoutSeconds|RequeuingStrategy' pkg/configmap/configmap_test.goRepository: openshift/kueue-operator
Length of output: 162
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## configmap.go outline\n'
ast-grep outline pkg/configmap/configmap.go --view expanded
printf '\n## configmap_test.go outline\n'
ast-grep outline pkg/configmap/configmap_test.go --view expanded
printf '\n## relevant test text\n'
rg -n -C4 'buildWaitForPodsReady|WaitForPodsReady|Requeuing|RecoveryTimeout|TimeoutOverride|TimeReference|buildRequeuingStrategy' pkg/configmap/configmap_test.go pkg/configmap/configmap.goRepository: openshift/kueue-operator
Length of output: 6302
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '## files'
git ls-files 'pkg/configmap/*'
echo '## outline of configmap.go'
ast-grep outline pkg/configmap/configmap.go --view expanded
echo '## outline of configmap_test.go'
ast-grep outline pkg/configmap/configmap_test.go --view expanded
echo '## search all tests for relevant mapping coverage'
rg -n -C3 'buildWaitForPodsReady|buildRequeuingStrategy|RecoveryTimeoutSeconds|RequeuingStrategy|TimeReference|TimeoutOverride|WaitForPodsReady' pkg test . -g '*_test.go'Repository: openshift/kueue-operator
Length of output: 2598
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,260p' pkg/configmap/configmap_test.goRepository: openshift/kueue-operator
Length of output: 6947
Add table-driven coverage for the new config mapping branches. pkg/configmap/configmap_test.go should cover buildRequeuingStrategy plus the buildWaitForPodsReady cases for timeout override, recovery timeout, requeuing strategy passthrough, and nil TimeReference.
🤖 Prompt for 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.
In `@pkg/configmap/configmap.go` around lines 164 - 178, Add table-driven tests in
configmap_test.go to cover the new mapping branches in buildRequeuingStrategy
and buildWaitForPodsReady. Verify buildRequeuingStrategy returns nil for nil
input, maps BackoffLimitCount/BackoffBaseSeconds/BackoffMaxSeconds, and leaves
Timestamp unset when TimeReference is empty. Also cover buildWaitForPodsReady
for timeout override, recovery timeout, requeuing strategy passthrough, and the
nil TimeReference path, using the existing helper names so the cases are easy to
locate.
MaysaMacedo
force-pushed
the
wait-for-pods-ready-customization
branch
from
July 30, 2026 19:25
f61c796 to
6934b55
Compare
MaysaMacedo
changed the title
MaysaMacedo
marked this pull request as ready for review
July 30, 2026 19:26
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 `@pkg/apis/kueueoperator/v1/types.go`:
- Around line 311-315: Retain the deprecated None enum value and
GangSchedulingPolicyNone constant alongside GangSchedulingPolicyByWorkload in
pkg/apis/kueueoperator/v1/types.go. Regenerate
manifests/kueue.openshift.io_kueues.yaml so the v1 schema includes None as a
temporary compatibility value.
- Around line 367-373: Make the documented 1800-second timeout effective
everywhere: in pkg/apis/kueueoperator/v1/types.go lines 367-373, add the
Kubebuilder default marker; regenerate manifests/kueue.openshift.io_kueues.yaml
lines 235-243 so the schema contains default: 1800; update
pkg/configmap/configmap.go lines 147-149 to fall back to 1800 seconds for
existing CRs with no value; and update pkg/configmap/configmap_test.go lines
144-146, 264-266, and 329-336 to expect that default, including a custom-timeout
case.
In `@pkg/configmap/configmap_test.go`:
- Around line 281-287: Update the ByWorkload fixture in the config mapping test
to include the required Admission field, setting it to
GangSchedulingWorkloadAdmissionParallel while preserving the existing
RequeuingStrategy values.
🪄 Autofix (Beta)
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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 95c2b6ec-347c-45cc-8d4c-6b81643142bc
⛔ Files ignored due to path filters (5)
pkg/apis/kueueoperator/v1/zz_generated.deepcopy.gois excluded by!**/zz_generated*pkg/generated/applyconfiguration/kueueoperator/v1/byworkload.gois excluded by!**/generated/**pkg/generated/applyconfiguration/kueueoperator/v1/gangscheduling.gois excluded by!**/generated/**pkg/generated/applyconfiguration/kueueoperator/v1/requeuingstrategy.gois excluded by!**/generated/**pkg/generated/applyconfiguration/utils.gois excluded by!**/generated/**
📒 Files selected for processing (4)
manifests/kueue.openshift.io_kueues.yamlpkg/apis/kueueoperator/v1/types.gopkg/configmap/configmap.gopkg/configmap/configmap_test.go
Comment on lines
+311
to
315
| // +kubebuilder:validation:Enum=ByWorkload;"" | ||
| type GangSchedulingPolicy string | ||
|
|
||
| const ( | ||
| GangSchedulingPolicyByWorkload GangSchedulingPolicy = "ByWorkload" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Keep None readable during the v1 API transition. Removing it from the only stored API version invalidates already persisted configuration and blocks future updates.
pkg/apis/kueueoperator/v1/types.go#L311-L315: retainNoneas a deprecated enum/constant until migration is complete.manifests/kueue.openshift.io_kueues.yaml#L258-L260: regenerate the schema with the temporary compatibility value included.
📍 Affects 2 files
pkg/apis/kueueoperator/v1/types.go#L311-L315(this comment)manifests/kueue.openshift.io_kueues.yaml#L258-L260
🤖 Prompt for 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.
In `@pkg/apis/kueueoperator/v1/types.go` around lines 311 - 315, Retain the
deprecated None enum value and GangSchedulingPolicyNone constant alongside
GangSchedulingPolicyByWorkload in pkg/apis/kueueoperator/v1/types.go. Regenerate
manifests/kueue.openshift.io_kueues.yaml so the v1 schema includes None as a
temporary compatibility value.
Comment on lines
+367
to
+373
| // timeoutSeconds defines the time for an admitted workload to have it's pods scheduled. | ||
| // When the timeoutSeconds is expired, the workload is evicted and requeued. | ||
| // The value must be between 1 and 31536000 (one year in seconds). Defaults to 1800 seconds. | ||
| // +kubebuilder:validation:Minimum=1 | ||
| // +kubebuilder:validation:Maximum=31536000 | ||
| // +optional | ||
| TimeoutSeconds int32 `json:"timeoutSeconds,omitempty"` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make the 1800-second default effective end-to-end. The API documentation promises 1800 seconds, but omitted fields currently produce timeout: 0s.
pkg/apis/kueueoperator/v1/types.go#L367-L373: add the Kubebuilder default marker for 1800 seconds.manifests/kueue.openshift.io_kueues.yaml#L235-L243: regenerate withdefault: 1800.pkg/configmap/configmap.go#L147-L149: fall back to 1800 seconds for existing CRs that lack the new field.pkg/configmap/configmap_test.go#L144-L146: expect the effective default.pkg/configmap/configmap_test.go#L264-L266: expect the effective default.pkg/configmap/configmap_test.go#L329-L336: expect the effective default and add a custom-timeout case.
📍 Affects 4 files
pkg/apis/kueueoperator/v1/types.go#L367-L373(this comment)manifests/kueue.openshift.io_kueues.yaml#L235-L243pkg/configmap/configmap.go#L147-L149pkg/configmap/configmap_test.go#L144-L146pkg/configmap/configmap_test.go#L264-L266pkg/configmap/configmap_test.go#L329-L336
🤖 Prompt for 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.
In `@pkg/apis/kueueoperator/v1/types.go` around lines 367 - 373, Make the
documented 1800-second timeout effective everywhere: in
pkg/apis/kueueoperator/v1/types.go lines 367-373, add the Kubebuilder default
marker; regenerate manifests/kueue.openshift.io_kueues.yaml lines 235-243 so the
schema contains default: 1800; update pkg/configmap/configmap.go lines 147-149
to fall back to 1800 seconds for existing CRs with no value; and update
pkg/configmap/configmap_test.go lines 144-146, 264-266, and 329-336 to expect
that default, including a custom-timeout case.
Comment on lines
281
to
+287
| ByWorkload: &kueue.ByWorkload{ | ||
| Admission: kueue.GangSchedulingWorkloadAdmissionSequential, | ||
| RequeuingStrategy: &kueue.RequeuingStrategy{ | ||
| TimeReference: kueue.EvictionTimestamp, | ||
| BackoffLimitCount: 5, | ||
| BackoffBaseSeconds: 120, | ||
| BackoffMaxSeconds: 7200, | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use a CRD-valid ByWorkload input.
admission is required by the API/CRD, but this test omits it. Set Admission: GangSchedulingWorkloadAdmissionParallel so the mapping test represents a deployable configuration.
Proposed fix
ByWorkload: &kueue.ByWorkload{
+ Admission: kueue.GangSchedulingWorkloadAdmissionParallel,
RequeuingStrategy: &kueue.RequeuingStrategy{📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ByWorkload: &kueue.ByWorkload{ | |
| Admission: kueue.GangSchedulingWorkloadAdmissionSequential, | |
| RequeuingStrategy: &kueue.RequeuingStrategy{ | |
| TimeReference: kueue.EvictionTimestamp, | |
| BackoffLimitCount: 5, | |
| BackoffBaseSeconds: 120, | |
| BackoffMaxSeconds: 7200, | |
| }, | |
| ByWorkload: &kueue.ByWorkload{ | |
| Admission: kueue.GangSchedulingWorkloadAdmissionParallel, | |
| RequeuingStrategy: &kueue.RequeuingStrategy{ | |
| TimeReference: kueue.EvictionTimestamp, | |
| BackoffLimitCount: 5, | |
| BackoffBaseSeconds: 120, | |
| BackoffMaxSeconds: 7200, | |
| }, |
🤖 Prompt for 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.
In `@pkg/configmap/configmap_test.go` around lines 281 - 287, Update the
ByWorkload fixture in the config mapping test to include the required Admission
field, setting it to GangSchedulingWorkloadAdmissionParallel while preserving
the existing RequeuingStrategy values.
|
@MaysaMacedo: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
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
2 participants
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.
To be defined:
Summary by CodeRabbit
New Features
timeoutandrecoveryTimeoutbehavior when pods fail to become ready.Behavior Changes
Tests