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

Validate custom priority policy config. #84646

Merged
merged 1 commit into from Nov 4, 2019

Conversation

@liu-cong
Copy link
Contributor

liu-cong commented Nov 1, 2019

…rity and allow others.

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespace from that line:

/kind api-change
/kind cleanup
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

/kind bug
What this PR does / why we need it:

  • Do not validate redeclaration of custom predicates.
  • Validate no duplicate declaration of RequestedToCapacityRatio
  • Validate the weights across multiple declarations of LabelPreference/ServiceAntiAffinity are the same.
    See #84645

Which issue(s) this PR fixes:

Part of #84645

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Only validate duplication of the RequestedToCapacityRatio custom priority and allow other custom predicates/priorities

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:


@liu-cong

This comment has been minimized.

Copy link
Contributor Author

liu-cong commented Nov 1, 2019

/assign damemi
/assign ahg-g

@damemi

This comment has been minimized.

Copy link
Contributor

damemi commented Nov 1, 2019

one nit but otherwise lgtm if @ahg-g approves

@liu-cong

This comment has been minimized.

Copy link
Contributor Author

liu-cong commented Nov 1, 2019

@ahg-g
I am still reading the ServiceAffinity custom predicate. It only has a list of labels as args. It seems that we can enforce validation on this one too. But need to check if the following behave differently:

  1. One predicate with labels=["foo","bar"] vs.
  2. Two predicate with labels=["foo"] and labels=["bar"]

type ServiceAffinity struct {
nodeInfoLister schedulerlisters.NodeInfoLister
podLister schedulerlisters.PodLister
serviceLister corelisters.ServiceLister
labels []string
}

@damemi

This comment has been minimized.

Copy link
Contributor

damemi commented Nov 1, 2019

I am still reading the ServiceAffinity custom predicate. It only has a list of labels as args. It seems that we can enforce validation on this one too. But need to check if the following behave differently:

1. One predicate with `labels=["foo","bar"]` vs.

2. Two predicate with `labels=["foo"]` and `labels=["bar"]`

In the original issue for this, it's described that in this example the labels can be combined for the same effect (the only difference would be different weights)

@liu-cong liu-cong force-pushed the liu-cong:validation branch from 300be2b to 3068653 Nov 1, 2019
@liu-cong liu-cong changed the title Only validate duplication of the RequestedToCapacityRatio custom priority and allow other custom predicates/priorities Apply custom predicate/priority redeclaration validation to only applicable ones. Nov 1, 2019
@liu-cong liu-cong force-pushed the liu-cong:validation branch from 3068653 to 3659b4e Nov 4, 2019
@liu-cong liu-cong changed the title Apply custom predicate/priority redeclaration validation to only applicable ones. Validate custom priority policy config. Nov 4, 2019
- Do not validate redeclartion of custom predicates.
- Validate no duplicate declaration of RequestedToCapacityRatio
- Validate the weights across multiple declarations of LabelPreference/ServiceAntiAffinity are the same.
@liu-cong liu-cong force-pushed the liu-cong:validation branch from 3659b4e to b19f478 Nov 4, 2019
@ahg-g

This comment has been minimized.

Copy link
Member

ahg-g commented Nov 4, 2019

/approve
/lgtm

Thanks Cong!

@k8s-ci-robot k8s-ci-robot added the lgtm label Nov 4, 2019
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Nov 4, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahg-g, liu-cong

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

@liu-cong

This comment has been minimized.

Copy link
Contributor Author

liu-cong commented Nov 4, 2019

/retest

1 similar comment
@liu-cong

This comment has been minimized.

Copy link
Contributor Author

liu-cong commented Nov 4, 2019

/retest

@k8s-ci-robot k8s-ci-robot merged commit 077f825 into kubernetes:master Nov 4, 2019
15 checks passed
15 checks passed
cla/linuxfoundation liu-cong 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 Nov 4, 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.

None yet

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