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

feat: implement "post-filter" extension point for scheduling framework #78097

Merged

Conversation

@draveness
Copy link
Member

commented May 20, 2019

What type of PR is this?

/kind feature
/priority important-soon
/sig scheduling

What this PR does / why we need it:

Implement "post-filter" extension point for scheduling framework

Which issue(s) this PR fixes:

Fixes #78004

KEP: kubernetes/enhancements#624

Does this PR introduce a user-facing change?:

Implement "post-filter" extension point for scheduling framework
@draveness

This comment has been minimized.

Copy link
Member Author

commented May 20, 2019

/retest

@draveness draveness force-pushed the draveness:feature/post-filter-extension-point branch from b306f35 to 2e3ccae May 21, 2019

@draveness draveness force-pushed the draveness:feature/post-filter-extension-point branch 2 times, most recently from f4fb477 to 1ca2105 May 21, 2019

@draveness

This comment has been minimized.

Copy link
Member Author

commented May 21, 2019

/assign @bsalamat

@draveness

This comment has been minimized.

Copy link
Member Author

commented May 21, 2019

/retest

@draveness

This comment has been minimized.

Copy link
Member Author

commented May 21, 2019

/test pull-kubernetes-e2e-gce-100-performance

@draveness draveness marked this pull request as ready for review May 21, 2019

@draveness draveness marked this pull request as ready for review May 21, 2019

@draveness

This comment has been minimized.

Copy link
Member Author

commented May 24, 2019

Kindly ping @bsalamat for review. :)

@draveness draveness force-pushed the draveness:feature/post-filter-extension-point branch from 1ca2105 to cbe12c6 May 31, 2019

@draveness draveness force-pushed the draveness:feature/post-filter-extension-point branch from c4df3b8 to b9c3581 Jul 31, 2019

@draveness draveness force-pushed the draveness:feature/post-filter-extension-point branch from b9c3581 to c445929 Jul 31, 2019

@@ -214,6 +214,21 @@ func (g *genericScheduler) Schedule(pod *v1.Pod, nodeLister algorithm.NodeLister
return result, err
}

nodeToStatuses := make(framework.NodeToStatusesMap, len(failedPredicateMap))
for nodeName, reasons := range failedPredicateMap {

This comment has been minimized.

Copy link
@ahg-g

ahg-g Jul 31, 2019

Member

I think we want findNodesThatFit to return NodeToStatusesMap type (call it filteredNodesStatuses) in addition to failedPredicateMap. This means couple of things:

  1. add a NodeToStatusesMap member (filteredNodesStatuses) to FitError.

  2. iterate over the filteredNodesStatuses in Error after the failedPredicateMap iteration.

As part of fixing #80775, we will add logic to nodesWherePreemptionMightHelp to examine filteredNodesStatuses for filters that can help with preemption.

The idea is to really separate predicate reason from filter statues. Moreover, I don't think we want to pass predicate failures to post-filter, I think we only want to pass filters failures only, and so separating the maps will help with that as well.

This comment has been minimized.

Copy link
@draveness

draveness Jul 31, 2019

Author Member

Moreover, I don't think we want to pass predicate failures to post-filter, I think we only want to pass filters failures only

I agree on this, WDYT @bsalamat

This comment has been minimized.

Copy link
@bsalamat

bsalamat Jul 31, 2019

Member

+1. Thanks!

Plugin
// PostFilter is called by the scheduling framework after a list of nodes
// passed the filtering phase.
PostFilter(pc *PluginContext, pod *v1.Pod, nodes []*v1.Node, nodeToStatuses NodeToStatusesMap) *Status

This comment has been minimized.

Copy link
@ahg-g

ahg-g Jul 31, 2019

Member

might be expensive to pass the map by value, should we just pass a pointer? also I would name the variable filteredNodesStatuses.

This comment has been minimized.

Copy link
@draveness

draveness Jul 31, 2019

Author Member

might be expensive to pass the map by value, should we just pass a pointer?

No, it's not. A map is a pointer itself, passing a pointer to the map or a map make no difference here.

This comment has been minimized.

Copy link
@ahg-g

ahg-g Jul 31, 2019

Member

SG

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

@draveness draveness force-pushed the draveness:feature/post-filter-extension-point branch 3 times, most recently from cc8980d to 232d003 Aug 1, 2019

@draveness

This comment has been minimized.

Copy link
Member Author

commented Aug 1, 2019

@bsalamat @ahg-g All the comments are addressed, please take another look.

@draveness draveness force-pushed the draveness:feature/post-filter-extension-point branch from 232d003 to 383f56e Aug 1, 2019

@ahg-g
Copy link
Member

left a comment

a small nit, otherwise looks good to me, and thanks for adding a unit test to generic_scheduler!

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

This comment has been minimized.

Copy link
@ahg-g

ahg-g Aug 1, 2019

Member

nit: I would use %q for the plugin and pod names so that they get logged quoted.

This comment has been minimized.

Copy link
@draveness

draveness Aug 2, 2019

Author Member

nit: I would use %q for the plugin and pod names so that they get logged quoted.

I'm trying to be consistent with other logs here, and I'll open a follow-up PR for this. #80885

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

@draveness draveness force-pushed the draveness:feature/post-filter-extension-point branch from 383f56e to 11859d9 Aug 2, 2019

@draveness draveness force-pushed the draveness:feature/post-filter-extension-point branch from 11859d9 to feb6485 Aug 2, 2019

@draveness

This comment has been minimized.

Copy link
Member Author

commented Aug 2, 2019

@ahg-g All comments have been addressed, please take another look

@ahg-g
Copy link
Member

left a comment

/lgtm

Congrats, I think this is the last extension point.

@k8s-ci-robot k8s-ci-robot added the lgtm label Aug 2, 2019

@draveness

This comment has been minimized.

Copy link
Member Author

commented Aug 2, 2019

Kindly ping @bsalamat for approval

@bsalamat
Copy link
Member

left a comment

/lgtm
/approve

Thanks, @draveness!

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2019

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@fejta-bot

This comment has been minimized.

Copy link

commented Aug 2, 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.

2 similar comments
@fejta-bot

This comment has been minimized.

Copy link

commented Aug 3, 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.

@fejta-bot

This comment has been minimized.

Copy link

commented Aug 3, 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.

@draveness

This comment has been minimized.

Copy link
Member Author

commented Aug 3, 2019

/retest

@draveness

This comment has been minimized.

Copy link
Member Author

commented Aug 3, 2019

@k8s-ci-robot k8s-ci-robot merged commit cc270c1 into kubernetes:master Aug 3, 2019

23 checks passed

cla/linuxfoundation draveness 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

@k8s-ci-robot k8s-ci-robot added this to the v1.16 milestone Aug 3, 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.