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

Remove alwaysCheckAllPredicates from NewGenericScheduler #86369

Merged

Conversation

@Huang-Wei
Copy link
Member

Huang-Wei commented Dec 17, 2019

What type of PR is this?

/kind cleanup
/sig scheduling

What this PR does / why we need it:

AlwaysCheckAllPredicates used to serve as a debug option in scheduler Policy API, but it's seldom used. Now scheduler has (almost) migrated to the new framework, it doesn't make much sense to still support this option. This PR invalidate this option, and mark it as DEPRECATED in API.

alwaysCheckAllPredicates doesn't fit in NewGenericScheduler any more, instead, we will plumb this option in framework to ensure it works in RunFilterPlugins.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

`AlwaysCheckAllPredicates` is deprecated in scheduler Policy API.

/cc @ahg-g @liggitt

@k8s-ci-robot k8s-ci-robot requested review from ahg-g and liggitt Dec 17, 2019
@Huang-Wei Huang-Wei force-pushed the Huang-Wei:deprecate-AlwaysCheckAllPredicates branch 2 times, most recently from 8d6b4ec to c91237a Dec 17, 2019
@Huang-Wei

This comment has been minimized.

Copy link
Member Author

Huang-Wei commented Dec 18, 2019

/retest

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Dec 18, 2019

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

@Huang-Wei Huang-Wei force-pushed the Huang-Wei:deprecate-AlwaysCheckAllPredicates branch from c91237a to 3f17102 Dec 18, 2019
@ahg-g

This comment has been minimized.

Copy link
Member

ahg-g commented Dec 19, 2019

/lgtm

@Huang-Wei

This comment has been minimized.

Copy link
Member Author

Huang-Wei commented Dec 20, 2019

Kindly ping @liggitt .

@@ -47,6 +47,7 @@ type Policy struct {
// the configured predicates even after one or more of them fails.
// When the flag is set to false, scheduler skips checking the rest
// of the predicates after it finds one predicate that failed.
// DEPRECATED

This comment has been minimized.

Copy link
@liggitt

liggitt Dec 20, 2019

Member

note the config version this will be removed in?

@@ -43,6 +43,7 @@ type Policy struct {
// the configured predicates even after one or more of them fails.
// When the flag is set to false, scheduler skips checking the rest
// of the predicates after it finds one predicate that failed.
// DEPRECATED

This comment has been minimized.

Copy link
@liggitt

liggitt Dec 20, 2019

Member

note the config version this will be removed in?

This comment has been minimized.

Copy link
@liggitt

liggitt Dec 20, 2019

Member

Per https://kubernetes.io/docs/reference/using-api/deprecation-policy/#deprecating-parts-of-the-api, we can remove this in v2 of this type. Until then, this should remain supported.

// When AlwaysCheckAllPredicates is set to true, scheduler checks all the configured
// predicates even after one or more of them fails.
if policy.AlwaysCheckAllPredicates {
c.alwaysCheckAllPredicates = policy.AlwaysCheckAllPredicates

This comment has been minimized.

Copy link
@liggitt

liggitt Dec 20, 2019

Member

is this removing the code that honors the field? that's not how deprecation periods work... we announce a deprecation, then once the deprecation period or API version is removed, we remove the supporting code.

This comment has been minimized.

Copy link
@liggitt

liggitt Dec 20, 2019

Member

it looks like compatibility was already broken in dc3d1bd#diff-c237cdd9e4cb201118ca380732d7f361L695-L700 when the behavior driven by this field was removed

This comment has been minimized.

Copy link
@ahg-g

ahg-g Dec 20, 2019

Member

To restore the behavior, we can plump the flag to the framework and have it examined when RunFilterPlugins since predicates are now Filter plugins.

This comment has been minimized.

Copy link
@ahg-g

ahg-g Dec 20, 2019

Member

PR that restores the behaviour: #86496

Copy link
Member

ahg-g left a comment

Can you please revert the change to legacy_types.go and staging/src/k8s.io/kube-scheduler/config/v1/types.go

I will send a follow up PR to plump the flag to the framework so that we can examine it when running the Filters.

@Huang-Wei

This comment has been minimized.

Copy link
Member Author

Huang-Wei commented Dec 20, 2019

Can you please revert the change to legacy_types.go and staging/src/k8s.io/kube-scheduler/config/v1/types.go

I will send a follow up PR to plump the flag to the framework so that we can examine it when running the Filters.

Sure. Per offline discussion, we will deprecate this option when we deprecate Policy(in favor of CC v1). In this PR, we will just remove the parameter in the internal logic of call NewScheduler. Instead, as @ahg-g mentioned, we will plump the flag to the framework so that we will still honor it.

Thanks @liggitt @ahg-g .

@Huang-Wei Huang-Wei force-pushed the Huang-Wei:deprecate-AlwaysCheckAllPredicates branch from 3f17102 to 1f78a93 Dec 20, 2019
@k8s-ci-robot k8s-ci-robot removed the lgtm label Dec 20, 2019
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Dec 20, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Huang-Wei

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

@ahg-g

This comment has been minimized.

Copy link
Member

ahg-g commented Dec 20, 2019

Can you change the title please.

@ahg-g

This comment has been minimized.

Copy link
Member

ahg-g commented Dec 20, 2019

/retest

@Huang-Wei Huang-Wei changed the title Deprecate AlwaysCheckAllPredicates in scheduler Policy API Remove `AlwaysCheckAllPredicates` from NewGenericScheduler Dec 20, 2019
@Huang-Wei Huang-Wei changed the title Remove `AlwaysCheckAllPredicates` from NewGenericScheduler Remove alwaysCheckAllPredicates from NewGenericScheduler Dec 20, 2019
@Huang-Wei

This comment has been minimized.

Copy link
Member Author

Huang-Wei commented Dec 20, 2019

@ahg-g updated the title and description.

Copy link
Member

ahg-g left a comment

/lgtm

// When AlwaysCheckAllPredicates is set to true, scheduler checks all the configured
// predicates even after one or more of them fails.
if policy.AlwaysCheckAllPredicates {
c.alwaysCheckAllPredicates = policy.AlwaysCheckAllPredicates

This comment has been minimized.

Copy link
@ahg-g

ahg-g Dec 20, 2019

Member

PR that restores the behaviour: #86496

@k8s-ci-robot k8s-ci-robot added the lgtm label Dec 20, 2019
@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Dec 21, 2019

/retest

@k8s-ci-robot k8s-ci-robot merged commit 84198bf into kubernetes:master Dec 21, 2019
12 of 15 checks passed
12 of 15 checks passed
pull-kubernetes-e2e-gce
pull-kubernetes-kubemark-e2e-gce-big Job triggered.
Details
tide Not mergeable. Job pull-kubernetes-e2e-gce has not succeeded.
Details
cla/linuxfoundation Huang-Wei 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-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-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
@k8s-ci-robot k8s-ci-robot added this to the v1.18 milestone Dec 21, 2019
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Dec 21, 2019

@Huang-Wei: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-e2e-gce 1f78a93 link /test pull-kubernetes-e2e-gce

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@Huang-Wei Huang-Wei deleted the Huang-Wei:deprecate-AlwaysCheckAllPredicates branch Dec 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.