-
Notifications
You must be signed in to change notification settings - Fork 482
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Mitigate potential range panic #724
base: master
Are you sure you want to change the base?
Mitigate potential range panic #724
Conversation
When a podgroup has a MinMember set to 0 and no pods exist, the reconciler can encounter a range exception. I have observed this on v0.26.7. The underlying code is still the same so it is likely the same weakness exists in the currently.
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. Instructions 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/test-infra repository. |
|
Welcome @mariusvniekerk! |
Hi @mariusvniekerk. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions 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/test-infra repository. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mariusvniekerk The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
✅ Deploy Preview for kubernetes-sigs-scheduler-plugins canceled.
|
/ok-to-test |
I think we should always ensure the PodGroup's .minMember is greater than 0: diff --git a/apis/scheduling/v1alpha1/types.go b/apis/scheduling/v1alpha1/types.go
index 86901dd5..8aaf6aa5 100644
--- a/apis/scheduling/v1alpha1/types.go
+++ b/apis/scheduling/v1alpha1/types.go
@@ -137,6 +137,7 @@ type PodGroupSpec struct {
// MinMember defines the minimal number of members/tasks to run the pod group;
// if there's not enough resources to start all tasks, the scheduler
// will not start anyone.
+ // +kubebuilder:validation:Minimum=1
MinMember int32 `json:"minMember,omitempty"`
// MinResources defines the minimal resource of members/tasks to run the pod group; After that, run |
I assumed that it could be validly be 0 if relying on .minResources instead. |
IMHO, I think the case (0 .minMember along with non-empty .minResources) is not legit. Are you have some real-world usecase using this semantics? |
What type of PR is this?
/kind bug
What this PR does / why we need it:
When a podgroup has a MinMember set to 0 and no pods exist, the reconciler can encounter a range exception.
I have observed this on v0.26.7. The underlying code is still the same so it is likely the same weakness exists in the currently.
The crash appears as follows
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?
NONE