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

Fixes 78001 The implementation of Filter extension for the new framework #78477

Merged
merged 1 commit into from Jul 22, 2019

Conversation

@YoubingLi
Copy link
Contributor

commented May 29, 2019

What type of PR is this?

/kind feature

What this PR does / why we need it:

The implementation of Filter extension point

Which issue(s) this PR fixes:

Fixes #78001

Special notes for your reviewer:

/assign @bsalamat
/assign @Huang-Wei

Does this PR introduce a user-facing change?:

no

   Add Filter extension point to the scheduling framework.
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented May 29, 2019

Hi @YoubingLi. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

Show resolved Hide resolved pkg/scheduler/framework/v1alpha1/framework.go Outdated
Show resolved Hide resolved pkg/scheduler/core/generic_scheduler.go Outdated
Show resolved Hide resolved pkg/scheduler/core/generic_scheduler.go Outdated
Show resolved Hide resolved pkg/scheduler/framework/v1alpha1/framework.go Outdated
Show resolved Hide resolved pkg/scheduler/framework/v1alpha1/framework.go Outdated
Show resolved Hide resolved pkg/scheduler/framework/v1alpha1/interface.go Outdated
Show resolved Hide resolved pkg/scheduler/framework/v1alpha1/interface.go Outdated

@YoubingLi YoubingLi force-pushed the YoubingLi:filter branch 3 times, most recently from 139ff34 to ee79c27 May 30, 2019

@YoubingLi YoubingLi force-pushed the YoubingLi:filter branch from ee79c27 to 33754ed Jun 14, 2019

@YoubingLi YoubingLi force-pushed the YoubingLi:filter branch from 33754ed to 909dc84 Jun 14, 2019

@YoubingLi

This comment has been minimized.

Copy link
Contributor Author

commented Jun 14, 2019

@bsalamat

Bobby, I didn't touch the logic of  findNodesThatFit in my PR.

But I think we should add logic to check if the evaluation node is in the input slice nodes. 
Maybe the node has been excluded by Filter plugin.

        nodeName := g.cache.NodeTree().Next()
@bsalamat

This comment has been minimized.

Copy link
Member

commented Jun 14, 2019

@bsalamat

Bobby, I didn't touch the logic of  findNodesThatFit in my PR.

But I think we should add logic to check if the evaluation node is in the input slice nodes. 
Maybe the node has been excluded by Filter plugin.

        nodeName := g.cache.NodeTree().Next()

You are right. I think our only viable solution is to let findNodesThatFit call Filter plugins for each node, very similar how it invokes each predicate for each node.

@YoubingLi YoubingLi force-pushed the YoubingLi:filter branch from bc660c8 to 94a679f Jul 3, 2019

@bsalamat
Copy link
Member

left a comment

Looks good. Just a few minor nits in the comments.

Show resolved Hide resolved pkg/scheduler/framework/v1alpha1/interface.go Outdated
Show resolved Hide resolved pkg/scheduler/framework/v1alpha1/interface.go Outdated
Show resolved Hide resolved pkg/scheduler/framework/v1alpha1/interface.go Outdated

@YoubingLi YoubingLi force-pushed the YoubingLi:filter branch from 94a679f to 5765f22 Jul 4, 2019

@YoubingLi YoubingLi referenced this pull request Jul 4, 2019

Closed

REQUEST: New membership for YoubingLi #987

6 of 6 tasks complete
Show resolved Hide resolved pkg/scheduler/framework/v1alpha1/interface.go Outdated
Show resolved Hide resolved pkg/scheduler/framework/v1alpha1/interface.go Outdated
Show resolved Hide resolved pkg/scheduler/core/generic_scheduler.go Outdated
Show resolved Hide resolved pkg/scheduler/framework/v1alpha1/framework.go Outdated

for _, p := range f.filterPlugins {
status := p.Filter(pc, pod, nodeInfo)
if !status.IsSuccess() {

This comment has been minimized.

Copy link
@bsalamat

bsalamat Jul 9, 2019

Member

+1 to @ahg-g's suggestion. It makes sense to convert all codes other than "Unschedulable" to an Error

@YoubingLi YoubingLi force-pushed the YoubingLi:filter branch from 5765f22 to 4621207 Jul 9, 2019

@YoubingLi

This comment has been minimized.

Copy link
Contributor Author

commented Jul 9, 2019

@bsalamat @ahg-g

All comments have been addressed.

@draveness

This comment has been minimized.

Copy link
Member

commented Jul 9, 2019

/retest

1 similar comment
@YoubingLi

This comment has been minimized.

Copy link
Contributor Author

commented Jul 9, 2019

/retest

@YoubingLi

This comment has been minimized.

Copy link
Contributor Author

commented Jul 16, 2019

@bsalamat @ahg-g

Please help review the latest diff.
@draveness
Copy link
Member

left a comment

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Jul 16, 2019


for _, p := range f.filterPlugins {
status := p.Filter(pc, pod, nodeInfo)
if !status.IsSuccess() {

This comment has been minimized.

Copy link
@ahg-g

ahg-g Jul 16, 2019

Member

@YoubingLi this hasn't been addressed yet. I left a comment with the suggestion.

if !status.IsSuccess() {
if status.Code() != Unschedulable {
klog.Errorf("RunFilterPlugins: error while running %s filter plugin for pod %s: %s",
p.Name(), pod.Name, status.Message())

This comment has been minimized.

Copy link
@ahg-g

ahg-g Jul 16, 2019

Member

The suggestion in the previous comment is to do something like this:

errMsg := fmt.Sprintf("RunFilterPlugins: error while running %s filter plugin for pod %s: %s",
					p.Name(), pod.Name, status.Message())
klog.Errorf(errMsg)
return NewStatus(Error, errMsg)

This comment has been minimized.

Copy link
@YoubingLi

YoubingLi Jul 17, 2019

Author Contributor

It has been addressed in the latest diff

@YoubingLi YoubingLi force-pushed the YoubingLi:filter branch from 4621207 to 6ad238a Jul 17, 2019

@k8s-ci-robot k8s-ci-robot removed the lgtm label Jul 17, 2019

@ahg-g

This comment has been minimized.

Copy link
Member

commented Jul 17, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Jul 17, 2019

@YoubingLi YoubingLi force-pushed the YoubingLi:filter branch from 6ad238a to 7f9dd94 Jul 17, 2019

@k8s-ci-robot k8s-ci-robot removed the lgtm label Jul 17, 2019

@draveness

This comment has been minimized.

Copy link
Member

commented Jul 17, 2019

/retest

@draveness
Copy link
Member

left a comment

/lgtm

@YoubingLi

This comment has been minimized.

Copy link
Contributor Author

commented Jul 19, 2019

@bsalamat

Please take a look the new diff.

@bsalamat
Copy link
Member

left a comment

/lgtm
/approve

Thanks, @YoubingLi!

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Jul 22, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bsalamat, YoubingLi

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

@k8s-ci-robot k8s-ci-robot merged commit f31d786 into kubernetes:master Jul 22, 2019

23 checks passed

cla/linuxfoundation YoubingLi authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
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-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-iscsi Skipped.
pull-kubernetes-e2e-gce-iscsi-serial Skipped.
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
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
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
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.