[migration] Mapping Layer - 2. Convert predicates/priorities configurations to a framework plugin one. #83099

ahg-g · 2019-09-25T05:33:45Z

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

What this PR does / why we need it:

To facilitate predicates/priorities migration to the framework. This PR converts configured predicates/priorities into their equivalent framework plugins for the one that actually have a mapping.

This PR is based on the changes in #83080

Which issue(s) this PR fixes:
Part of #82708

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

k8s-ci-robot · 2019-09-25T05:34:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahg-g

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~~ [ahg-g]

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

ahg-g · 2019-09-25T05:34:48Z

/assign @Huang-Wei @alculquicondor

ahg-g · 2019-09-25T05:39:14Z

I still need to add tests for this, but just to give an idea how this whole mapping layer will look like.

pkg/scheduler/factory/factory.go

liu-cong · 2019-09-25T14:40:43Z

pkg/scheduler/factory/factory.go

@@ -395,7 +395,17 @@ func (c *Configurator) CreateFromKeys(predicateKeys, priorityKeys sets.String, e
 		return nil, err
 	}

-	framework, err := framework.NewFramework(c.registry, c.plugins, c.pluginConfig)
+	// Combine all framework configurations.
+	// TODO(ahg-g): handle duplicate and disabled plugins.


What's the consequence of not handling these for now?

duplicate will be handled by the framework instantiation logic, disabled plugins does not apply yet since we don't have default plugins yet.

pkg/scheduler/framework/plugins/default_registry.go

pkg/scheduler/factory/factory.go

pkg/scheduler/factory/factory_test.go

pkg/scheduler/factory/factory.go

alculquicondor · 2019-09-26T22:05:51Z

pkg/scheduler/factory/factory.go

@@ -516,7 +598,7 @@ func (c *Configurator) getPluginArgs() (*PluginFactoryArgs, error) {
 		StorageClassInfo:               &predicates.CachedStorageClassInfo{StorageClassLister: c.storageClassLister},
 		VolumeBinder:                   c.volumeBinder,
 		HardPodAffinitySymmetricWeight: c.hardPodAffinitySymmetricWeight,
-	}, nil
+	}, &plugins.ConfigProducerArgs{}, nil


Maybe let's add this once we figure that it is needed?

I think it is fine, we will add HardPodAffinitySymmetricWeight pretty soon.

ok. However, they can be separate methods. Also, there seems to be no need to return an error :)

They can, but I don't think they should because both will be configuring very similar parameters.

removed the err, didn't see it before!

alculquicondor · 2019-09-26T22:21:15Z

pkg/scheduler/framework/plugins/default_registry.go

-// NewConfigProducerRegistry creates a new producer registry.
-func NewConfigProducerRegistry() *ConfigProducerRegistry {
+// NewDefaultConfigProducerRegistry creates a new producer registry.
+func NewDefaultConfigProducerRegistry() *ConfigProducerRegistry {


It doesn't look like a Default so far

We will add the registration of the various plugins in this function.

Right, that's what I figured. However, our tests are expecting a clean Registry.

Modified factory_test.go to start from an empty registry

alculquicondor · 2019-09-26T22:28:59Z

pkg/scheduler/framework/v1alpha1/framework.go

@@ -49,6 +49,8 @@ type framework struct {
 	postBindPlugins       []PostBindPlugin
 	unreservePlugins      []UnreservePlugin
 	permitPlugins         []PermitPlugin
+	plugins               *config.Plugins


instead of adding these here (and never GC the config), they should go in factory.Config. They are only useful for factory_test.go anyways.

Aren't we planning to remove factory.Config in the future?

In that scenario, we shouldn't be testing the Config, but rather that the proper plugins were instantiated.

that practically means we need to write a function for the framework to re-create the config from the plugin arrays, I don't want to do that :)

I feel factory.Config already has too many arguments, adding more just makes it more cluttered and confusing.

I feel factory.Config already has too many arguments, adding more just makes it more cluttered and confusing.

I agree. My only point is that configuration should only exist at startup and be collected after that.

I think naming is less of a problem than having living objects that we do not care about at runtime.
For now, this is my suggestion:
In Config we have a struct named Framework that stores the parameters to NewFramework. That is implicitly a configuration because it lives inside the Config struct. The instantiation of the framework itself is moved to scheduler.NewFromConfig.

I don't think having the Config here is a big problem, they are implementations details of the framework anyways. However, I feel a bit hesitant on adding the GetConfiguration to the Framework interface, which could open the door for changing the config after framework initialization. Having said that, I don't think it's too big an issue.

One way to get rid of this is to test the unexported factory method of getPriorityConfigs and getPredicateConfigs, but I prefer to test exported methods.

Ideally we can avoid the new GetConfiguration interface method but I don't hold strongly.

we can't do the instantiation there because the framework is needed when making generic scheduler. I will move those to Config though.

Sg. FYI @ahmad-diaa for when you work on this refactoring.

ahg-g · 2019-09-27T19:03:20Z

/test pull-kubernetes-integration

liu-cong

/lgtm

liu-cong · 2019-09-27T19:08:06Z

pkg/scheduler/factory/factory_test.go

+		PriorityToConfigProducer:  make(map[string]frameworkplugins.ConfigProducer),
+	}
+	configProducerRegistry.RegisterPredicate(predicateOneName,
+		func(_ frameworkplugins.ConfigProducerArgs) (config.Plugins, []config.PluginConfig) {


nit: it seems that the you can define a generic producer func and reuse it for predicate one, two, etc.

liu-cong · 2019-09-27T19:27:22Z

pkg/scheduler/framework/v1alpha1/framework.go

@@ -49,6 +49,8 @@ type framework struct {
 	postBindPlugins       []PostBindPlugin
 	unreservePlugins      []UnreservePlugin
 	permitPlugins         []PermitPlugin
+	plugins               *config.Plugins


I don't think having the Config here is a big problem, they are implementations details of the framework anyways. However, I feel a bit hesitant on adding the GetConfiguration to the Framework interface, which could open the door for changing the config after framework initialization. Having said that, I don't think it's too big an issue.

One way to get rid of this is to test the unexported factory method of getPriorityConfigs and getPredicateConfigs, but I prefer to test exported methods.

Ideally we can avoid the new GetConfiguration interface method but I don't hold strongly.

liu-cong · 2019-09-27T19:32:23Z

pkg/scheduler/factory/factory.go

-// It is used by the scheduler and other components, such as k8s.io/autoscaler/cluster-autoscaler.
-func (c *Configurator) GetPredicates(predicateKeys sets.String) (map[string]predicates.FitPredicate, error) {
-	pluginArgs, err := c.getPluginArgs()
+func (c *Configurator) getPredicateConfigs(predicateKeys sets.String) (map[string]predicates.FitPredicate, *config.Plugins, []config.PluginConfig, error) {


The logic to produce the eventual algorithm vs. framework plugin output has some subtleties. It depends on a few things - algorithm keys in policy, algorithm to plugin config producer registry, ordered list of predicates, etc. I suggest that we add some high level comments here to explain this. Same for the getPriorityConfig method.

alculquicondor · 2019-09-27T19:36:10Z

/hold

for squash and remaining comments. @liu-cong, careful with LGTMing an approver. The PR could get merged accidentally before we actually intend to.

Update: Oops, it looks like Abdullah had already hold this :)

ahg-g · 2019-09-27T19:39:40Z

/hold

for squash and remaining comments. @liu-cong, careful with LGTMing an approver. The PR could get merged accidentally before we actually intend to.

True, I already put a hold when the PR was created.

liu-cong · 2019-09-27T19:44:38Z

/hold
for squash and remaining comments. @liu-cong, careful with LGTMing an approver. The PR could get merged accidentally before we actually intend to.

True, I already put a hold when the PR was created.

An approver doesn't need an approve?

ahg-g · 2019-09-27T19:48:06Z

/hold
for squash and remaining comments. @liu-cong, careful with LGTMing an approver. The PR could get merged accidentally before we actually intend to.

True, I already put a hold when the PR was created.

An approver doesn't need an approve?

no, but need an lgtm from a reviewer I believe.

alculquicondor

/lgtm

Feel free to squash now.

alculquicondor · 2019-09-27T20:44:22Z

pkg/scheduler/factory/factory_test.go

@@ -771,9 +771,9 @@ func TestCreateWithFrameworkPlugins(t *testing.T) {
 		t.Errorf("unexpected priorities diff (-want, +got): %s", diff)
 	}

-	gotPlugins, _ := c.Framework.GetConfiguration()
+	gotPlugins := c.Plugins


nit: variable not needed

ahg-g · 2019-09-27T21:08:56Z

/lgtm

Feel free to squash now.

done.

alculquicondor · 2019-09-30T14:18:00Z

pkg/scheduler/internal/queue/scheduling_queue_test.go

@@ -167,6 +168,10 @@ func (*fakeFramework) NodeInfoSnapshot() *schedulernodeinfo.Snapshot {
 	return nil
 }

+func (*fakeFramework) GetConfiguration() (*config.Plugins, []config.PluginConfig) {


…nfigs.

alculquicondor · 2019-09-30T14:41:17Z

/lgtm

ahg-g · 2019-09-30T14:52:59Z

/hold cancel

k8s-ci-robot requested review from hex108 and smarterclayton September 25, 2019 05:33

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

ahg-g changed the title ~~Migration Mapping Layer - 2. Convert predicates/priorities configurations to a framework plugin one.~~ Mapping Layer - 2. Convert predicates/priorities configurations to a framework plugin one. Sep 25, 2019

k8s-ci-robot assigned alculquicondor and Huang-Wei Sep 25, 2019

alculquicondor reviewed Sep 25, 2019

View reviewed changes

pkg/scheduler/factory/factory.go Outdated Show resolved Hide resolved

pkg/scheduler/factory/factory.go Outdated Show resolved Hide resolved

pkg/scheduler/factory/factory.go Outdated Show resolved Hide resolved

liu-cong reviewed Sep 25, 2019

View reviewed changes

ahg-g changed the title ~~Mapping Layer - 2. Convert predicates/priorities configurations to a framework plugin one.~~ [migration] Mapping Layer - 2. Convert predicates/priorities configurations to a framework plugin one. Sep 26, 2019

ahg-g force-pushed the ahg-config-layer2 branch from 27ccba3 to c8c669c Compare September 26, 2019 19:25

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 26, 2019

ahg-g force-pushed the ahg-config-layer2 branch 3 times, most recently from d266c3d to 98b9abb Compare September 26, 2019 19:48

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 26, 2019

ahg-g force-pushed the ahg-config-layer2 branch from 3bd65b3 to 2e75dea Compare September 26, 2019 20:16

alculquicondor reviewed Sep 26, 2019

View reviewed changes

liu-cong reviewed Sep 27, 2019

View reviewed changes

k8s-ci-robot assigned liu-cong Sep 27, 2019

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

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 27, 2019

alculquicondor reviewed Sep 27, 2019

View reviewed changes

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

ahg-g force-pushed the ahg-config-layer2 branch from c6ae0a9 to 87ad919 Compare September 27, 2019 21:08

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 27, 2019

alculquicondor reviewed Sep 30, 2019

View reviewed changes

Convert predicates/priorities configurations to a framework plugin co…

6120fe9

…nfigs.

ahg-g force-pushed the ahg-config-layer2 branch from 87ad919 to 6120fe9 Compare September 30, 2019 14:37

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

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 30, 2019

k8s-ci-robot merged commit a87c1b2 into kubernetes:master Sep 30, 2019

k8s-ci-robot added this to the v1.17 milestone Sep 30, 2019

ahg-g deleted the ahg-config-layer2 branch January 10, 2020 15:38

[migration] Mapping Layer - 2. Convert predicates/priorities configurations to a framework plugin one. #83099

[migration] Mapping Layer - 2. Convert predicates/priorities configurations to a framework plugin one. #83099

Conversation

ahg-g commented Sep 25, 2019

k8s-ci-robot commented Sep 25, 2019

ahg-g commented Sep 25, 2019

ahg-g commented Sep 25, 2019

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

ahg-g Sep 27, 2019 • edited

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

ahg-g commented Sep 27, 2019

liu-cong 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

alculquicondor commented Sep 27, 2019 • edited

ahg-g commented Sep 27, 2019

liu-cong commented Sep 27, 2019

ahg-g commented Sep 27, 2019

alculquicondor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahg-g commented Sep 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alculquicondor commented Sep 30, 2019

ahg-g commented Sep 30, 2019

ahg-g Sep 27, 2019 •

edited

alculquicondor commented Sep 27, 2019 •

edited