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
Add Scheduler validation check for redeclared policy configs #83963
Conversation
/retest |
Hi @damemi Also, always remember to add a test. |
@alculquicondor Agree that I did take the wrong direction with this PR, but I spoke with @ahg-g and it seems like we may want to do this check when actually registering the custom policies, as in here: kubernetes/pkg/scheduler/algorithm_factory.go Line 368 in 6b2b5f2
What do you think? Can probably accomplish the same thing in either spot |
that's right, probably both should do the work. However, the validation.go file has more user-facing semantics. |
Aldo's suggestion SGTM. |
b43e6ad
to
d33b7fa
Compare
Ok, updated this PR to do validation based on the arguments given in the policies in |
/assign @liu-cong |
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.
Can you add a unit test for this
// by examining the specified predicate arguments | ||
func validatePredicateRedeclared(predicates map[string]schedulerapi.PredicatePolicy, predicate schedulerapi.PredicatePolicy) error { | ||
var predicateType string | ||
if predicate.Argument.LabelsPresence != nil { |
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 think you need to check whether predicate.Argument is nil or not.
Currently nil
argument is valid if the name of the predicate matches a predefined one (I don't know if this is desired or not), see
kubernetes/pkg/scheduler/algorithm_factory.go
Line 281 in e1f86e3
} else if policy.Argument.LabelsPresence != nil { |
d33b7fa
to
5296650
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.
Unit tests, please :)
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.
Thanks Mike, can you please add a compatibility test as well.
4a3de97
to
f587f58
Compare
f587f58
to
0f15705
Compare
/retest |
1 similar comment
/retest |
expected: errors.New("Predicate 'customPredicate2' redeclares custom predicate 'ServiceAffinity', from:'customPredicate1'"), | ||
}, | ||
{ | ||
name: "invalid redeclared custom priority", |
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.
nit: a different name from the previous one
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.
invalid redeclared custom predicate
and invalid redeclared custom priority
? They are testing the same thing, but for predicate/priority
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahg-g, damemi 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 |
/retest |
1 similar comment
/retest |
/retest Review the full test history for this PR. Silence the bot with an |
What type of PR is this?
/kind bug
What this PR does / why we need it:
The ability to re-declare custom policy configs was not entirely intentional, and this adds validation to disallow that in the new framework
Which issue(s) this PR fixes:
Fixes #83472