Scheduler: change config.Plugins from pointers to objects #98663

gavinfish · 2021-02-01T15:00:44Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Replacement of #95287

Which issue(s) this PR fixes:

Fix #95190

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

none

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

k8s-ci-robot · 2021-02-01T15:00:52Z

@gavinfish: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

gavinfish · 2021-02-07T15:23:05Z

/retest

gavinfish · 2021-02-08T12:38:54Z

/retest

gavinfish · 2021-02-08T15:08:16Z

/test pull-kubernetes-verify

gavinfish · 2021-02-09T01:12:12Z

/test pull-kubernetes-bazel-test
/test pull-kubernetes-e2e-kind

gavinfish · 2021-02-09T13:40:59Z

/assign @alculquicondor

alculquicondor

looking pretty good

alculquicondor · 2021-02-09T15:26:51Z

pkg/scheduler/apis/config/types_test.go

 					Enabled: []Plugin{
 						{Name: "DefaultPlugin1"},
 						{Name: "DefaultPlugin2"},
 						{Name: "CustomPlugin"},
 					},
 				},
-				PostFilter: &PluginSet{},


alculquicondor · 2021-02-09T15:32:03Z

pkg/scheduler/framework/runtime/framework.go

@@ -108,17 +108,17 @@ type extensionPoint struct {

 func (f *frameworkImpl) getExtensionPoints(plugins *config.Plugins) []extensionPoint {


can we change extensionPoint to not hold pointers? evaluate if it simplifies things, otherwise leave it.

should be fine, just wait for all test results.

alculquicondor · 2021-02-09T15:32:31Z

pkg/scheduler/framework/runtime/framework_test.go

@@ -1280,8 +1280,8 @@ func TestFilterPluginsWithNominatedPods(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			registry := Registry{}
 			cfgPls := &config.Plugins{
-				PreFilter: &config.PluginSet{},
-				Filter:    &config.PluginSet{},
+				PreFilter: config.PluginSet{},


you can remove these lines, that is the point :)

cleaned all the missed ones.

alculquicondor · 2021-02-09T15:36:41Z

pkg/scheduler/apis/config/v1beta1/conversion.go

+	return nil
+}
+
+func convertToInternalPluginSet(in *v1beta1.PluginSet, out *config.PluginSet, s conversion.Scope) error {


Can you use Convert_v1beta1_PluginSet_To_config_PluginSet instead?

alculquicondor · 2021-02-09T15:37:30Z

pkg/scheduler/apis/config/v1beta1/conversion.go

+	return nil
+}
+
+func convertToExternalPluginSet(in config.PluginSet, out *v1beta1.PluginSet, s conversion.Scope) error {


Can you use Convert_config_PluginSet_To_v1beta1_PluginSet instead?

alculquicondor · 2021-02-09T16:29:00Z

pkg/scheduler/apis/config/v1beta1/conversion.go

 		return err
 	}
 	return nil
 }

-func convertToInternalPluginSet(in *v1beta1.PluginSet, out *config.PluginSet, s conversion.Scope) error {


I see that I wasn't clear. I meant that you should prefer the autogenerated functions

gavinfish · 2021-02-10T06:40:19Z

/test pull-kubernetes-bazel-test
/test pull-kubernetes-integration

gavinfish · 2021-02-10T07:34:53Z

/test pull-kubernetes-bazel-test

alculquicondor · 2021-02-10T14:59:44Z

pkg/scheduler/apis/config/v1beta1/conversion.go

-	}
-	if err := Convert_v1beta1_PluginSet_To_config_PluginSet(in.Filter, &out.Filter, s); err != nil {
-		return err
+	if in.QueueSort != nil {


Trick to avoid so much code repetition:

func Convert_v1beta1_PluginConfig_To_config_PluginConfig(in *v1beta1.PluginConfig, out *config.PluginConfig, s conversion.Scope) error { if in == nil { return nil } return autoConvert_v1beta1_PluginConfig_To_config_PluginConfig(in, out, s) }

alculquicondor · 2021-02-10T15:35:07Z

/approve

good for squash

gavinfish · 2021-02-10T15:47:17Z

squashed.

alculquicondor

/approve
/lgtm

Thanks!

k8s-ci-robot · 2021-02-10T15:57:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, gavinfish

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:

~~cmd/kube-scheduler/OWNERS~~ [alculquicondor]
~~pkg/scheduler/OWNERS~~ [alculquicondor]
~~test/integration/scheduler/OWNERS~~ [alculquicondor]

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

fejta-bot · 2021-02-10T18:45:56Z

/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.

Huang-Wei · 2021-02-10T20:38:18Z

LGTM.

@alculquicondor Given that having a concrete value in internal API is a good practice, do we plan to change the internal Pluginsto values as well? (so that we don't need to init the Plugins inside a scheduler CC)

kubernetes/pkg/scheduler/apis/config/types.go

Line 123 in a1e310b

Plugins *Plugins

alculquicondor · 2021-02-10T20:43:54Z

It's a good suggestion, but I don't mind that one as much (I don't think changing it leads to significantly less lines of code).

@gavinfish feel free to submit a PR for it with the catch that if it doesn't simplify much we might discard the change.

Huang-Wei · 2021-02-10T21:20:23Z

It's a good suggestion, but I don't mind that one as much (I don't think changing it leads to significantly less lines of code).

True, most of the tests instantiate Plugins directly, instead of scheduler CC.

Huang-Wei · 2021-02-10T23:07:52Z

/retest

k8s-ci-robot added needs-priority Indicates a PR lacks a `priority/foo` label and requires one. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Feb 1, 2021

k8s-ci-robot requested review from Huang-Wei and vishh February 1, 2021 15:02

gavinfish force-pushed the sched-remove-plugin-pt branch 3 times, most recently from e07610b to a0628ac Compare February 2, 2021 04:56

k8s-ci-robot added area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Feb 2, 2021

gavinfish force-pushed the sched-remove-plugin-pt branch from a0628ac to 82cad2e Compare February 7, 2021 14:32

gavinfish changed the title ~~[WIP] Scheduler: change config.Plugins from pointers to objects~~ Scheduler: change config.Plugins from pointers to objects Feb 7, 2021

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 7, 2021

gavinfish force-pushed the sched-remove-plugin-pt branch from 82cad2e to 0a409e6 Compare February 8, 2021 14:21

gavinfish force-pushed the sched-remove-plugin-pt branch from 0a409e6 to 264832a Compare February 8, 2021 16:21

k8s-ci-robot assigned alculquicondor Feb 9, 2021

alculquicondor reviewed Feb 9, 2021

View reviewed changes

gavinfish force-pushed the sched-remove-plugin-pt branch from 72c2a8e to 5e979f5 Compare February 10, 2021 05:49

alculquicondor reviewed Feb 10, 2021

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 10, 2021

Change config.Plugins from pointers to objects

74315f9

gavinfish force-pushed the sched-remove-plugin-pt branch from fcd281d to 74315f9 Compare February 10, 2021 15:46

alculquicondor reviewed Feb 10, 2021

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 10, 2021

k8s-ci-robot merged commit 838bb6a into kubernetes:master Feb 10, 2021

k8s-ci-robot added this to the v1.21 milestone Feb 10, 2021

gavinfish deleted the sched-remove-plugin-pt branch February 11, 2021 12:52

gavinfish mentioned this pull request Feb 11, 2021

Scheduler: evaluate wether worth to change internal Plugins to values #98998

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scheduler: change config.Plugins from pointers to objects #98663

Scheduler: change config.Plugins from pointers to objects #98663

gavinfish commented Feb 1, 2021 •

edited

k8s-ci-robot commented Feb 1, 2021

gavinfish commented Feb 7, 2021

gavinfish commented Feb 8, 2021

gavinfish commented Feb 8, 2021

gavinfish commented Feb 9, 2021

gavinfish commented Feb 9, 2021

alculquicondor left a comment

alculquicondor Feb 9, 2021

alculquicondor Feb 9, 2021

gavinfish Feb 9, 2021

alculquicondor Feb 9, 2021

gavinfish Feb 9, 2021

alculquicondor Feb 9, 2021

gavinfish Feb 9, 2021

alculquicondor Feb 9, 2021

gavinfish Feb 9, 2021

alculquicondor Feb 9, 2021

gavinfish Feb 10, 2021

gavinfish commented Feb 10, 2021

gavinfish commented Feb 10, 2021

alculquicondor Feb 10, 2021

gavinfish Feb 10, 2021

alculquicondor commented Feb 10, 2021

gavinfish commented Feb 10, 2021

alculquicondor left a comment

k8s-ci-robot commented Feb 10, 2021

fejta-bot commented Feb 10, 2021

Huang-Wei commented Feb 10, 2021

alculquicondor commented Feb 10, 2021

Huang-Wei commented Feb 10, 2021

Huang-Wei commented Feb 10, 2021

		@@ -108,17 +108,17 @@ type extensionPoint struct {

		func (f frameworkImpl) getExtensionPoints(plugins config.Plugins) []extensionPoint {

Scheduler: change config.Plugins from pointers to objects #98663

Scheduler: change config.Plugins from pointers to objects #98663

Conversation

gavinfish commented Feb 1, 2021 • edited

k8s-ci-robot commented Feb 1, 2021

gavinfish commented Feb 7, 2021

gavinfish commented Feb 8, 2021

gavinfish commented Feb 8, 2021

gavinfish commented Feb 9, 2021

gavinfish commented Feb 9, 2021

alculquicondor left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gavinfish commented Feb 10, 2021

gavinfish commented Feb 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alculquicondor commented Feb 10, 2021

gavinfish commented Feb 10, 2021

alculquicondor left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 10, 2021

fejta-bot commented Feb 10, 2021

Huang-Wei commented Feb 10, 2021

alculquicondor commented Feb 10, 2021

Huang-Wei commented Feb 10, 2021

Huang-Wei commented Feb 10, 2021

gavinfish commented Feb 1, 2021 •

edited