Skip to content
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

Merged
merged 2 commits into from Oct 30, 2019

Conversation

@damemi
Copy link
Contributor

damemi commented Oct 15, 2019

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

Scheduler policy configs can no longer be declared multiple times
@BenTheElder

This comment has been minimized.

Copy link
Member

BenTheElder commented Oct 15, 2019

/retest
kubernetes-sigs/kind#951 for pull-kubernetes-e2e-kind issue (worked around -- docker hub is down)

@alculquicondor

This comment has been minimized.

Copy link
Member

alculquicondor commented Oct 22, 2019

Hi @damemi
From the issue, it seems that we need a validator for the old path, rather than the framework path.
https://github.com/kubernetes/kubernetes/blob/master/pkg/scheduler/api/validation/validation.go#L34

Also, always remember to add a test.

@damemi

This comment has been minimized.

Copy link
Contributor Author

damemi commented Oct 22, 2019

Hi @damemi
From the issue, it seems that we need a validator for the old path, rather than the framework path.
https://github.com/kubernetes/kubernetes/blob/master/pkg/scheduler/api/validation/validation.go#L34

@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:

if policy.Argument.ServiceAntiAffinity != nil {

What do you think? Can probably accomplish the same thing in either spot

@alculquicondor

This comment has been minimized.

Copy link
Member

alculquicondor commented Oct 22, 2019

that's right, probably both should do the work. However, the validation.go file has more user-facing semantics.
WDYT @ahg-g?

@ahg-g

This comment has been minimized.

Copy link
Member

ahg-g commented Oct 22, 2019

that's right, probably both should do the work. However, the validation.go file has more user-facing semantics.
WDYT @ahg-g?

Aldo's suggestion SGTM.

@damemi damemi force-pushed the damemi:duplicate-policies branch from b43e6ad to d33b7fa Oct 23, 2019
@k8s-ci-robot k8s-ci-robot added size/M and removed size/S labels Oct 23, 2019
@damemi damemi changed the title Add Scheduler validation check for redeclared plugin configs Add Scheduler validation check for redeclared policy configs Oct 23, 2019
@damemi

This comment has been minimized.

Copy link
Contributor Author

damemi commented Oct 23, 2019

Ok, updated this PR to do validation based on the arguments given in the policies in validation.go. Because we have a sort of polymorphism with the argument types, I couldn't think of a much cleaner way to check them, but please review and if you have any ideas for how to refactor this it would be helpful

@ahg-g

This comment has been minimized.

Copy link
Member

ahg-g commented Oct 23, 2019

/assign @liu-cong

Copy link
Contributor

liu-cong left a comment

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 {

This comment has been minimized.

Copy link
@liu-cong

liu-cong Oct 23, 2019

Contributor

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

} else if policy.Argument.LabelsPresence != nil {

@damemi damemi force-pushed the damemi:duplicate-policies branch from d33b7fa to 5296650 Oct 23, 2019
Copy link
Member

alculquicondor left a comment

Unit tests, please :)

pkg/scheduler/api/validation/validation.go Outdated Show resolved Hide resolved
Copy link
Member

ahg-g left a comment

Thanks Mike, can you please add a compatibility test as well.

@damemi damemi force-pushed the damemi:duplicate-policies branch 2 times, most recently from 4a3de97 to f587f58 Oct 28, 2019
@damemi damemi force-pushed the damemi:duplicate-policies branch from f587f58 to 0f15705 Oct 28, 2019
@ahg-g

This comment has been minimized.

Copy link
Member

ahg-g commented Oct 28, 2019

/retest

1 similar comment
@ahg-g

This comment has been minimized.

Copy link
Member

ahg-g commented Oct 29, 2019

/retest

expected: errors.New("Predicate 'customPredicate2' redeclares custom predicate 'ServiceAffinity', from:'customPredicate1'"),
},
{
name: "invalid redeclared custom priority",

This comment has been minimized.

Copy link
@alculquicondor

alculquicondor Oct 29, 2019

Member

nit: a different name from the previous one

This comment has been minimized.

Copy link
@damemi

damemi Oct 29, 2019

Author Contributor

invalid redeclared custom predicate and invalid redeclared custom priority? They are testing the same thing, but for predicate/priority

@ahg-g

This comment has been minimized.

Copy link
Member

ahg-g commented Oct 29, 2019

/lgtm
/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Oct 29, 2019

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@damemi

This comment has been minimized.

Copy link
Contributor Author

damemi commented Oct 29, 2019

/retest

1 similar comment
@ahg-g

This comment has been minimized.

Copy link
Member

ahg-g commented Oct 29, 2019

/retest

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Oct 30, 2019

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@k8s-ci-robot k8s-ci-robot merged commit 8d8a068 into kubernetes:master Oct 30, 2019
15 checks passed
15 checks passed
cla/linuxfoundation damemi authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-kind Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details
@k8s-ci-robot k8s-ci-robot added this to the v1.17 milestone Oct 30, 2019
@damemi damemi deleted the damemi:duplicate-policies branch Oct 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

7 participants
You can’t perform that action at this time.