Add Bind extension point of the scheduling framework #78513

chenchun · 2019-05-30T10:29:23Z

What type of PR is this?
/kind feature
/priority important-soon
/sig scheduling

What this PR does / why we need it:
Add Bind extension point of the scheduling framework

Which issue(s) this PR fixes:
Fixes #78006

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Add Bind extension point of the scheduling framework

k8s-ci-robot · 2019-05-30T10:29:30Z

Hi @chenchun. 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.

draveness · 2019-05-30T10:43:30Z

pkg/scheduler/framework/v1alpha1/interface.go

@@ -42,6 +42,8 @@ const (
 	Unschedulable
 	// Wait is used when a permit plugin finds a pod scheduling should wait.
 	Wait
+	// Whether a bind plugin chooses to skip binding


Please start comments with Skip ...

test/integration/scheduler/framework_test.go

pkg/scheduler/scheduler.go

pkg/scheduler/framework/v1alpha1/framework.go

wgliang · 2019-06-11T03:40:06Z

/ok-to-test

draveness · 2019-06-18T17:06:25Z

/assign @bsalamat

bsalamat

Thanks, @chenchun! Generally LGTM. Only a couple of minor comments.

bsalamat · 2019-06-19T21:57:13Z

pkg/scheduler/framework/v1alpha1/interface.go

+	// RunBindPlugins runs the set of configured bind plugins. A bind plugin may choose
+	// whether or not to handle the given Pod. If a bind plugin chooses to skip the
+	// binding, it should return code=4("skip") status. Otherwise, it should return "Error"
+	// or "Success". If none of the plugins handled binding, RunBindPlugins returns


RunBindPlugins should one day return an error if all bind plugins 'Skip' the pod, but this is fine for now that we want to run the default bind function if none of the bind plugins process the pod. My hope is that the current bind function will one day be a (default) plugin.

Maybe we can do it in a following PR.

I opened an issue #79215 to track this and I could raise a PR after this get merged.

RunBindPlugins should one day return an error if all bind plugins 'Skip' the pod, but this is fine for now that we want to run the default bind function if none of the bind plugins process the pod. My hope is that the current bind function will one day be a (default) plugin.

This is a great idea

pkg/scheduler/scheduler.go

bsalamat · 2019-06-19T23:34:35Z

test/integration/scheduler/framework_test.go

+		},
+	}
+	// Set reserve and bind config for testing
+	pluginConfig := []schedulerconfig.PluginConfig{


Not related to this PR, but we should make providing a pluginConfig for plugins optional so that something like this is not necessary if one does not need to pass any args to plugins.

Actually plugin config can be nil. I opened #80567 to clean up the verbose code.

test/integration/scheduler/framework_test.go

bsalamat

/lgtm
/approve

Thanks, @chenchun!

k8s-ci-robot · 2019-06-21T22:40:02Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~pkg/scheduler/OWNERS~~ [bsalamat]
~~test/integration/scheduler/OWNERS~~ [bsalamat]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

chenchun · 2019-06-22T01:49:29Z

/retest

tedyu · 2019-06-22T14:30:57Z

test/integration/scheduler/framework_test.go

@@ -599,6 +637,165 @@ func TestUnreservePlugin(t *testing.T) {
 	}
 }

+// TestBindPlugin tests invocation of bind plugins.


It seems this test is flaky:

https://prow.k8s.io/view/gcs/kubernetes-jenkins/pr-logs/pull/79294/pull-kubernetes-integration/1142430172373323779/

Met the same problem in https://github.com/kubernetes/kubernetes/pull/78513/files

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 30, 2019

k8s-ci-robot requested review from k82cn and wgliang May 30, 2019 10:30

chenchun mentioned this pull request May 30, 2019

Add Bind extension point of the scheduling framework #78006

Closed

k8s-ci-robot added area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels May 30, 2019

chenchun force-pushed the binder branch from bfa30ee to 3d31541 Compare May 30, 2019 10:37

draveness reviewed May 30, 2019

View reviewed changes

test/integration/scheduler/framework_test.go Outdated Show resolved Hide resolved

LexCC suggested changes May 30, 2019

View reviewed changes

pkg/scheduler/scheduler.go Show resolved Hide resolved

wgliang reviewed May 31, 2019

View reviewed changes

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

chenchun force-pushed the binder branch from 3d31541 to c81fd43 Compare June 4, 2019 14:28

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 11, 2019

k8s-ci-robot assigned bsalamat Jun 18, 2019

bsalamat reviewed Jun 19, 2019

View reviewed changes

chenchun force-pushed the binder branch from c81fd43 to 621bf90 Compare June 20, 2019 04:18

Add Bind extension point of the scheduling framework

b339c0a

chenchun force-pushed the binder branch from 621bf90 to b339c0a Compare June 20, 2019 07:57

bsalamat reviewed Jun 21, 2019

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 21, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 21, 2019

k8s-ci-robot merged commit 6efc5a8 into kubernetes:master Jun 22, 2019

chenchun deleted the binder branch June 22, 2019 04:19

tedyu reviewed Jun 22, 2019

View reviewed changes

tedyu mentioned this pull request Jun 22, 2019

Use RWMutex to improve locking for serviceCache #79294

Merged

draveness mentioned this pull request Jun 22, 2019

Revert "Add Bind extension point of the scheduling framework" #79296

Merged

chenchun mentioned this pull request Jun 23, 2019

Add Bind extension point to the scheduling framework #79313

Merged

rikatz mentioned this pull request Oct 9, 2020

kubectl create ingress #94327

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Bind extension point of the scheduling framework #78513

Add Bind extension point of the scheduling framework #78513

chenchun commented May 30, 2019

k8s-ci-robot commented May 30, 2019

draveness May 30, 2019 •

edited

Loading

wgliang commented Jun 11, 2019

draveness commented Jun 18, 2019

bsalamat left a comment

bsalamat Jun 19, 2019

chenchun Jun 20, 2019

draveness Jun 20, 2019 •

edited

Loading

ProgramerGu Sep 16, 2019

bsalamat Jun 19, 2019

chenchun Jun 20, 2019

chenchun Jul 25, 2019

bsalamat left a comment

k8s-ci-robot commented Jun 21, 2019

chenchun commented Jun 22, 2019

tedyu Jun 22, 2019

draveness Jun 22, 2019

Add Bind extension point of the scheduling framework #78513

Add Bind extension point of the scheduling framework #78513

Conversation

chenchun commented May 30, 2019

k8s-ci-robot commented May 30, 2019

draveness May 30, 2019 • edited Loading

Choose a reason for hiding this comment

wgliang commented Jun 11, 2019

draveness commented Jun 18, 2019

bsalamat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

draveness Jun 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsalamat left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 21, 2019

chenchun commented Jun 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

draveness May 30, 2019 •

edited

Loading

draveness Jun 20, 2019 •

edited

Loading