-
Notifications
You must be signed in to change notification settings - Fork 919
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
fix bug that spreadconstraints are not validated correctly #3232
fix bug that spreadconstraints are not validated correctly #3232
Conversation
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## master #3232 +/- ##
==========================================
+ Coverage 49.07% 49.15% +0.07%
==========================================
Files 203 206 +3
Lines 18354 18377 +23
==========================================
+ Hits 9008 9033 +25
Misses 8856 8856
+ Partials 490 488 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we need to specify in the API declaration as well:
karmada/pkg/apis/policy/v1alpha1/propagation_types.go
Lines 233 to 240 in 0652cfb
// MaxGroups restricts the maximum number of cluster groups to be selected. | |
// +optional | |
MaxGroups int `json:"maxGroups,omitempty"` | |
// MinGroups restricts the minimum number of cluster groups to be selected. | |
// Defaults to 1. | |
// +optional | |
MinGroups int `json:"minGroups,omitempty"` |
pkg/util/helper/policy.go
Outdated
@@ -25,6 +25,11 @@ func SetDefaultSpreadConstraints(spreadConstraints []policyv1alpha1.SpreadConstr | |||
klog.Infof("Setting default MinGroups to 1") | |||
spreadConstraints[i].MinGroups = 1 | |||
} | |||
|
|||
if spreadConstraints[i].MaxGroups == 0 && spreadConstraints[i].MinGroups > 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For spreadConstraints[i].MinGroups > 0
, this part of the judgment seems redundant. It has been ensured before that the value is greater than 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If minGroups is less than zero? This part of the judgment is necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this a situation that has been intercepted by validate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
intercepted
No, mutating webhook works first!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend using +kubebuilder:validation:Minimum=1
and +kubebuilder:default=1
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, mutating webhook works first!
Ok
I recommend using
+kubebuilder:validation:Minimum=1
.
Does this conflict with // +optional
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend using
+kubebuilder:validation:Minimum=1
and+kubebuilder:default=1
.
I don't see any difference in performance, because the webhook will always be called, but this part of the verification is done by the apiserver.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that there is a validating webhook, it is better to hand over the verification work to webhook instead of webhook and apiserver co-work this validating work.
if constraint.MaxGroups < 0 { | ||
allErrs = append(allErrs, field.Invalid(fldPath.Index(index), constraint, "maxGroups lower than 0 is not allowed")) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to check that MaxGroups
cannot be less than MinGroups
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e need to check that
MaxGroups
cannot be less thanMinGroups
?
Why not? MaxGroups
should not be always less than MinGroups
, otherwise why is it named MaxGroups
and MinGroups
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I missed it. There's already a judgment in the logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MaxGroups
could equal 0? If so, it means what? i.e., HPA minReplicas could only be >=1. I also think MinGroups
should >=1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, after mutating, MaxGroups
could be more than 0 or less than 0, never be 0.
You're right. |
How about this comment?
|
Sorry, I accidentally clicked the close button. |
IMOP, it will be enough. |
b5278f5
to
6570284
Compare
/assign |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The validation part sounds good to me. And I believe this PR is more like focus on the validation instead of the default value:)
@@ -231,6 +231,7 @@ type SpreadConstraint struct { | |||
SpreadByLabel string `json:"spreadByLabel,omitempty"` | |||
|
|||
// MaxGroups restricts the maximum number of cluster groups to be selected. | |||
// Defaults to MinGroups. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the MaxGroups
deserves a default value.
In general, the default value should be a common one that meets most of the use cases.
Here, the valid range of MinGroups
is [1, max(int)], in order to let the scheduler works out(or, try to) a result we set the defaults to 1
, it seems reasonable, but for MaxGroups
which is essentially a limitation to the scheduler that can't spread the group that much, we don't have enough evidence to show that users always set MaxGroups
with the same value as MinGroups
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RainbowMango Hi, thanks for your replies!
I agree with most of your thoughts, but MaxGroups
would be zero which is an incomprehensible value for karmada-scheduler
without a default value. In other words, how should we(or karmada-scheduler
does) understand MaxGroups
without a default value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how should we(or karmada-scheduler does) understand MaxGroups without a default value?
It perhaps can't happen because there is a validation to ensure MaxGroups
can't be lower than MinGroups
(1).
right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MaxGroups is not required, so it’s default to 0.
@RainbowMango I don't know if you have any other suggestions, if not, I think this PR can be closed. |
I'm still thinking if the |
Okay, I'll only retain the validation part for this PR due to the difference of opinion for |
OK, thanks. |
Signed-off-by: whitewindmills <jayfantasyhjh@gmail.com>
6570284
to
ae325b8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
Thanks.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: RainbowMango The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind feature
/kind cleanup
What this PR does / why we need it:
fix bug that spreadconstraints are not validated correctly
Which issue(s) this PR fixes:
Fixes #3231
Special notes for your reviewer:
Does this PR introduce a user-facing change?: