Alow configuration of waitForPodsReady feature#2140
Draft
MaysaMacedo wants to merge 1 commit into
Draft
Conversation
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
WalkthroughThis PR adds gang scheduling requeuing configuration support: new ChangesGang scheduling requeuing controls
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant Config as GangSchedulingPolicy
participant Builder as buildWaitForPodsReady
participant Helper as buildRequeuingStrategy
Config->>Builder: ByWorkload config (Timeout, RecoveryTimeout, RequeuingStrategy)
Builder->>Builder: set Timeout, RecoveryTimeout if provided
Builder->>Helper: RequeuingStrategy (if non-nil)
Helper-->>Builder: configapi.RequeuingStrategy
Builder-->>Config: configapi.WaitForPodsReady
Related issues: None specified in the provided information. Related PRs: None specified in the provided information. Suggested labels: api-change, enhancement Suggested reviewers: None specified in the provided information. 🐰 A hop, a skip, a backoff base, 🚥 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
+368
to
+388
|
|
||
| // timeoutSeconds defines the time for an admitted workload to reach the PodsReady=true condition. | ||
| // When the timeout is exceeded, the workload is evicted and requeued in the same cluster queue. | ||
| // Defaults to 300 seconds. | ||
| // +optional | ||
| TimeoutSeconds *int32 `json:"timeoutSeconds,omitempty"` | ||
|
|
||
| // recoveryTimeoutSeconds defines an opt-in timeout measured since the last transition to the | ||
| // PodsReady=false condition after a Workload is admitted and running. Such a transition | ||
| // may happen when a Pod failed and the replacement Pod is awaited to be scheduled. | ||
| // After exceeding the timeout the corresponding job gets suspended again and requeued | ||
| // after the backoff delay. | ||
| // Defaults to the value of timeout. Set to 0 to disable recovery timeout. | ||
| // +optional | ||
| RecoveryTimeoutSeconds *int32 `json:"recoveryTimeoutSeconds,omitempty"` | ||
|
|
||
| // requeuingStrategy defines the strategy for requeuing a Workload that was evicted | ||
| // due to Pod readiness timeout. | ||
| // +optional | ||
| RequeuingStrategy *RequeuingStrategy `json:"requeuingStrategy,omitempty"` | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Missing lower-bound validation on TimeoutSeconds/RecoveryTimeoutSeconds.
Neither new field has a +kubebuilder:validation:Minimum marker, unlike other numeric fields in this same file (e.g. usageHalfLifeTimeSeconds, usageSamplingIntervalSeconds both specify minimum/maximum). Negative values will pass CRD admission and then flow unclamped into time.Duration(*bw.TimeoutSeconds) * time.Second in buildWaitForPodsReady (pkg/configmap/configmap.go), producing a negative timeout. Upstream kueue itself added explicit non-negative validation for its own waitForPodsReady.timeout-style fields, confirming this is a recognized failure class.
As per path instructions for **/*.go, "Integer overflow: bounds-check user-supplied sizes."
🛠️ Suggested fix
// timeoutSeconds defines the time for an admitted workload to reach the PodsReady=true condition.
// When the timeout is exceeded, the workload is evicted and requeued in the same cluster queue.
// Defaults to 300 seconds.
// +optional
+ // +kubebuilder:validation:Minimum=1
TimeoutSeconds *int32 `json:"timeoutSeconds,omitempty"`
// recoveryTimeoutSeconds defines an opt-in timeout measured since the last transition to the
// PodsReady=false condition after a Workload is admitted and running. Such a transition
// may happen when a Pod failed and the replacement Pod is awaited to be scheduled.
// After exceeding the timeout the corresponding job gets suspended again and requeued
// after the backoff delay.
// Defaults to the value of timeout. Set to 0 to disable recovery timeout.
// +optional
+ // +kubebuilder:validation:Minimum=0
RecoveryTimeoutSeconds *int32 `json:"recoveryTimeoutSeconds,omitempty"`Requires make generate to update manifests/kueue.openshift.io_kueues.yaml accordingly.
📝 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
| // timeoutSeconds defines the time for an admitted workload to reach the PodsReady=true condition. | |
| // When the timeout is exceeded, the workload is evicted and requeued in the same cluster queue. | |
| // Defaults to 300 seconds. | |
| // +optional | |
| TimeoutSeconds *int32 `json:"timeoutSeconds,omitempty"` | |
| // recoveryTimeoutSeconds defines an opt-in timeout measured since the last transition to the | |
| // PodsReady=false condition after a Workload is admitted and running. Such a transition | |
| // may happen when a Pod failed and the replacement Pod is awaited to be scheduled. | |
| // After exceeding the timeout the corresponding job gets suspended again and requeued | |
| // after the backoff delay. | |
| // Defaults to the value of timeout. Set to 0 to disable recovery timeout. | |
| // +optional | |
| RecoveryTimeoutSeconds *int32 `json:"recoveryTimeoutSeconds,omitempty"` | |
| // requeuingStrategy defines the strategy for requeuing a Workload that was evicted | |
| // due to Pod readiness timeout. | |
| // +optional | |
| RequeuingStrategy *RequeuingStrategy `json:"requeuingStrategy,omitempty"` | |
| } | |
| // timeoutSeconds defines the time for an admitted workload to reach the PodsReady=true condition. | |
| // When the timeout is exceeded, the workload is evicted and requeued in the same cluster queue. | |
| // Defaults to 300 seconds. | |
| // +optional | |
| // +kubebuilder:validation:Minimum=1 | |
| TimeoutSeconds *int32 `json:"timeoutSeconds,omitempty"` | |
| // recoveryTimeoutSeconds defines an opt-in timeout measured since the last transition to the | |
| // PodsReady=false condition after a Workload is admitted and running. Such a transition | |
| // may happen when a Pod failed and the replacement Pod is awaited to be scheduled. | |
| // After exceeding the timeout the corresponding job gets suspended again and requeued | |
| // after the backoff delay. | |
| // Defaults to the value of timeout. Set to 0 to disable recovery timeout. | |
| // +optional | |
| // +kubebuilder:validation:Minimum=0 | |
| RecoveryTimeoutSeconds *int32 `json:"recoveryTimeoutSeconds,omitempty"` | |
| // requeuingStrategy defines the strategy for requeuing a Workload that was evicted | |
| // due to Pod readiness timeout. | |
| // +optional | |
| RequeuingStrategy *RequeuingStrategy `json:"requeuingStrategy,omitempty"` | |
| } |
🤖 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 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.
Source: Path instructions
Comment on lines
+401
to
+425
| // RequeuingStrategy defines the strategy for requeuing a Workload evicted due to Pod readiness. | ||
| type RequeuingStrategy struct { | ||
| // timeReference defines which point in time is used as the reference for re-queuing a Workload | ||
| // that was evicted due to Pod readiness. Allowed values are Eviction and Creation. | ||
| // Eviction (default) uses the time of the Evicted condition with PodsReadyTimeout reason. | ||
| // Creation uses the Workload .metadata.creationTimestamp. | ||
| // +optional | ||
| TimeReference RequeuingTimeReference `json:"timeReference,omitempty"` | ||
|
|
||
| // backoffLimitCount defines the maximum number of re-queuing retries. | ||
| // Once the number is reached, the workload is deactivated (spec.active set to false). | ||
| // When not set, the workload is requeued indefinitely. | ||
| // +optional | ||
| BackoffLimitCount *int32 `json:"backoffLimitCount,omitempty"` | ||
|
|
||
| // backoffBaseSeconds defines the base for the exponential backoff for re-queuing an evicted workload. | ||
| // The consecutive requeue delays are computed as backoffBaseSeconds * 2^(retryCount-1) + jitter. | ||
| // Defaults to 60. | ||
| // +optional | ||
| BackoffBaseSeconds *int32 `json:"backoffBaseSeconds,omitempty"` | ||
|
|
||
| // backoffMaxSeconds defines the maximum backoff time to re-queue an evicted workload. | ||
| // Defaults to 3600. | ||
| // +optional | ||
| BackoffMaxSeconds *int32 `json:"backoffMaxSeconds,omitempty"` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Missing lower-bound validation on backoff fields.
BackoffLimitCount, BackoffBaseSeconds, and BackoffMaxSeconds also lack +kubebuilder:validation:Minimum markers, so negative values are accepted and passed through unchanged in buildRequeuingStrategy (pkg/configmap/configmap.go), producing a nonsensical backoff configuration for Kueue's requeuing logic.
🛠️ Suggested fix
// backoffLimitCount defines the maximum number of re-queuing retries.
// Once the number is reached, the workload is deactivated (spec.active set to false).
// When not set, the workload is requeued indefinitely.
// +optional
+ // +kubebuilder:validation:Minimum=0
BackoffLimitCount *int32 `json:"backoffLimitCount,omitempty"`
// backoffBaseSeconds defines the base for the exponential backoff for re-queuing an evicted workload.
// The consecutive requeue delays are computed as backoffBaseSeconds * 2^(retryCount-1) + jitter.
// Defaults to 60.
// +optional
+ // +kubebuilder:validation:Minimum=1
BackoffBaseSeconds *int32 `json:"backoffBaseSeconds,omitempty"`
// backoffMaxSeconds defines the maximum backoff time to re-queue an evicted workload.
// Defaults to 3600.
// +optional
+ // +kubebuilder:validation:Minimum=1
BackoffMaxSeconds *int32 `json:"backoffMaxSeconds,omitempty"`📝 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
| // RequeuingStrategy defines the strategy for requeuing a Workload evicted due to Pod readiness. | |
| type RequeuingStrategy struct { | |
| // timeReference defines which point in time is used as the reference for re-queuing a Workload | |
| // that was evicted due to Pod readiness. Allowed values are Eviction and Creation. | |
| // Eviction (default) uses the time of the Evicted condition with PodsReadyTimeout reason. | |
| // Creation uses the Workload .metadata.creationTimestamp. | |
| // +optional | |
| TimeReference RequeuingTimeReference `json:"timeReference,omitempty"` | |
| // backoffLimitCount defines the maximum number of re-queuing retries. | |
| // Once the number is reached, the workload is deactivated (spec.active set to false). | |
| // When not set, the workload is requeued indefinitely. | |
| // +optional | |
| BackoffLimitCount *int32 `json:"backoffLimitCount,omitempty"` | |
| // backoffBaseSeconds defines the base for the exponential backoff for re-queuing an evicted workload. | |
| // The consecutive requeue delays are computed as backoffBaseSeconds * 2^(retryCount-1) + jitter. | |
| // Defaults to 60. | |
| // +optional | |
| BackoffBaseSeconds *int32 `json:"backoffBaseSeconds,omitempty"` | |
| // backoffMaxSeconds defines the maximum backoff time to re-queue an evicted workload. | |
| // Defaults to 3600. | |
| // +optional | |
| BackoffMaxSeconds *int32 `json:"backoffMaxSeconds,omitempty"` | |
| // RequeuingStrategy defines the strategy for requeuing a Workload evicted due to Pod readiness. | |
| type RequeuingStrategy struct { | |
| // timeReference defines which point in time is used as the reference for re-queuing a Workload | |
| // that was evicted due to Pod readiness. Allowed values are Eviction and Creation. | |
| // Eviction (default) uses the time of the Evicted condition with PodsReadyTimeout reason. | |
| // Creation uses the Workload .metadata.creationTimestamp. | |
| // +optional | |
| TimeReference RequeuingTimeReference `json:"timeReference,omitempty"` | |
| // backoffLimitCount defines the maximum number of re-queuing retries. | |
| // Once the number is reached, the workload is deactivated (spec.active set to false). | |
| // When not set, the workload is requeued indefinitely. | |
| // +optional | |
| // +kubebuilder:validation:Minimum=0 | |
| BackoffLimitCount *int32 `json:"backoffLimitCount,omitempty"` | |
| // backoffBaseSeconds defines the base for the exponential backoff for re-queuing an evicted workload. | |
| // The consecutive requeue delays are computed as backoffBaseSeconds * 2^(retryCount-1) + jitter. | |
| // Defaults to 60. | |
| // +optional | |
| // +kubebuilder:validation:Minimum=1 | |
| BackoffBaseSeconds *int32 `json:"backoffBaseSeconds,omitempty"` | |
| // backoffMaxSeconds defines the maximum backoff time to re-queue an evicted workload. | |
| // Defaults to 3600. | |
| // +optional | |
| // +kubebuilder:validation:Minimum=1 | |
| BackoffMaxSeconds *int32 `json:"backoffMaxSeconds,omitempty"` |
🤖 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 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.
Source: Path instructions
There was a problem hiding this comment.
@kannon92 Glad it helped! Let me know if you'd like me to open a follow-up issue for the min-value validation, or if you're planning to address it in this PR.
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.
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