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

Scheduler can recieve its policy configuration from a ConfigMap #43892

Merged
merged 4 commits into from
Apr 9, 2017

Conversation

bsalamat
Copy link
Member

What this PR does / why we need it: This PR adds the ability to scheduler to receive its policy configuration from a ConfigMap. Before this, scheduler could receive its policy config only from a file. The logic to watch the ConfigMap object will be added in a subsequent PR.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:

Release note:

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 31, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@k8s-github-robot k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Mar 31, 2017
@bsalamat
Copy link
Member Author

@kubernetes/sig-scheduling-pr-reviews

@bsalamat
Copy link
Member Author

@davidopp

@bsalamat
Copy link
Member Author

Looks like I need to manually add some conversion functions. I will add them soon.

@bsalamat bsalamat force-pushed the sched_conf1 branch 2 times, most recently from 054a7a1 to 9c16267 Compare April 1, 2017 01:43
@@ -63,7 +63,9 @@ func (s *SchedulerServer) AddFlags(fs *pflag.FlagSet) {
fs.Int32Var(&s.Port, "port", s.Port, "The port that the scheduler's http service runs on")
fs.StringVar(&s.Address, "address", s.Address, "The IP address to serve on (set to 0.0.0.0 for all interfaces)")
fs.StringVar(&s.AlgorithmProvider, "algorithm-provider", s.AlgorithmProvider, "The scheduling algorithm provider to use, one of: "+factory.ListAlgorithmProviders())
fs.StringVar(&s.PolicyConfigFile, "policy-config-file", s.PolicyConfigFile, "File with scheduler policy configuration")
fs.StringVar(&s.PolicyConfigFile, "policy-config-file", s.PolicyConfigFile, "File with scheduler policy configuration. This file is used if policy ConfigMap object does not exists or scheduler is using legacy policy config.")
fs.StringVar(&s.PolicyConfigMapName, "policy-configmap", s.PolicyConfigMapName, "Name of the ConfigMap object that contains scheduler's policy configuration. It must exist in the system namespace before scheduler initialization.")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/It must exist in the system namespace before scheduler initialization./It must exist in the system namespace before scheduler initialization if scheduler is not using legacy policy config.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

fs.StringVar(&s.PolicyConfigFile, "policy-config-file", s.PolicyConfigFile, "File with scheduler policy configuration")
fs.StringVar(&s.PolicyConfigFile, "policy-config-file", s.PolicyConfigFile, "File with scheduler policy configuration. This file is used if policy ConfigMap object does not exists or scheduler is using legacy policy config.")
fs.StringVar(&s.PolicyConfigMapName, "policy-configmap", s.PolicyConfigMapName, "Name of the ConfigMap object that contains scheduler's policy configuration. It must exist in the system namespace before scheduler initialization.")
fs.BoolVar(&s.UseLegacyPolicyConfig, "use-legacy-policy-config", false, "When set to true, scheduler will ignore PolicyConfigMapName and uses PolicyConfigFile")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the PolicyConfigMapName and PolicyConfigFile are some internal used words, how about s/PolicyConfigMapName and uses PolicyConfigFile/policy config map and uses policy config file.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Done.

// uses PolicyConfigFile. If UseLegacyPolicyConfig is false and
// PolicyConfigMapName is not empty, the ConfigMap object with this name must
// exist before scheduler initialization.
PolicyConfigMapName string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If our component config reduces to a config map, should all other params shuffle to config map and be removed from the component config?

Also we need a means to generate a config map with all the defaults.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also do we need to know it's a configmap? Isn't it just loading a file and the config map is an implementation detail.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@timothysc This PR is not going to make any changes to scheduler's component config. This is only about scheduler's policy config. Policy config used to be a file whose path is provided in the component config. With this PR we support the file and also a ConfigMap to configure scheduler's policy.
This PR implements part of "Approach 2" as described in:
https://docs.google.com/document/d/19AKH6V6ejOeIvyGtIPNvRMR4Yi_X8U3Q1zz2fgTNhvM

@timothysc timothysc self-assigned this Apr 3, 2017
@timothysc timothysc added the sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. label Apr 3, 2017
@timothysc timothysc added this to the v1.7 milestone Apr 3, 2017
@timothysc
Copy link
Member

This is super important we get this right, and @k8s-mirror-cluster-lifecycle-misc will need to be involved.

Copy link
Member

@timothysc timothysc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some chicken-egg questions that need to be resolved 1st.

return nil, fmt.Errorf("ConfigMap %v has %v entries in its 'Data'. It must have only one.", len(policyConfigMap.Data), sc.policyConfigMap)
}
policyConfigMapFound = true
// This loop should iterate only once, as we have already checked the length of Data.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just get data directly?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The key can be any arbitrary string and we don't know it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, yes. never mind :).

stop := make(chan struct{})
informerFactory.Start(stop)

defer close(stop)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

informerFactory.Start(stop)

sched.Run()
defer close(stop)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

informerFactory.Start(stop)

sched.Run()
defer close(stop)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prefer just after "stop".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any style guide or recommendation on this? Other tests in the file use this pattern and I think consistency is more important, unless there is a strong reason to use a different pattern.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, the style is from golang, prefer to call "defer" close to where the object was build, e.g. for "sync.Mutex", we'll call

l.Lock()
defer l.Unlock()

// other codes.

For the consistency, I'd like to correct other lines in this file :).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, which line do you mean "Other tests in the file use this pattern"? Here's an example about defer.

 56 func TestUnschedulableNodes(t *testing.T) {
 57     _, s := framework.RunAMaster(nil)
 58     defer s.Close()
 59
 60     ns := framework.CreateTestingNamespace("unschedulable-nodes", s, t)
 61     defer framework.DeleteTestingNamespace(ns, s, t)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, sorry, I didn't understand what you meant by your first comment. Done.

sched.Run()
defer close(stop)

DoTestUnschedulableNodes(t, clientSet, ns, informerFactory.Core().V1().Nodes().Lister())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This verification can not make sure the config from ConfigMap was took place; default config can also pass this test, right?

Similar to the following test, whether return err can not make sure the config was updated as expected; I'd like suggest to find a way to check the data/config with the scheduler.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scheduler does not expose its internal configuration for testing. I could change things in scheduler to make it testable, but but I am not changing any logic around applying the config to the scheduler in this PR. That's why I thought those tests may not belong to this PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's probably fine to do in a different PR, but I think as part of this overall feature it's important to test that the values read from the ConfigMap were actually applied by the scheduler. (I realize we had no useful testing for the file case previously.) I think you were mentioning the other day that the factory test could be adapted to do this? Or something simple like what @k82cn suggested in the next comment (just tweak one policy and see that it is respected in how a pod is scheduled).

}
policyConfigMapFound = true
// This loop should iterate only once, as we have already checked the length of Data.
for _, val := range policyConfigMap.Data {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prefer to get data directly and remove the comments :).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see my previous answer.

informerFactory.Start(stop)

sched.Run()
defer close(stop)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prefer just after "stop:=..."

sched.Run()
defer close(stop)

DoTestUnschedulableNodes(t, clientSet, ns, informerFactory.Core().V1().Nodes().Lister())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The verification can not check whether the config from ConfigMap was set, as the default config can also pass it.

Similar comments to the following cases, the err from app.CreateScheduler can not make sure the config was set in sched; prefer to check sched's data directly or only enable part of predicates/priorities to check the result.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is the same point as above. I will try to add a test.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, similar point; used to close and re-open browser, the previous comments was "lost" but it's here when I submit new comments :(. Sorry for the confuse.

The sched's did not expose data to check; but I think we can only enable part of predicates/priorities to check, e.g. disable "PortFit" predicates and dispatch two pods with same port to the same host , the second one should NOT be Unscheduable. But I think you will get a better test than that :).

stop := make(chan struct{})
informerFactory.Start(stop)

defer close(stop)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

// PolicyConfigMapName is the name of the ConfigMap object that specifies
// the scheduler's policy config. If UseLegacyPolicyConfig is true, scheduler
// uses PolicyConfigFile. If UseLegacyPolicyConfig is false and
// PolicyConfigMapName is not empty, the ConfigMap object with this name must
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@bsalamat bsalamat force-pushed the sched_conf1 branch 2 times, most recently from 7d7c7c6 to 66ba4c1 Compare April 5, 2017 00:02
Copy link
Member

@davidopp davidopp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only skimmed it, looks good overall, just a few comments.

configData, err := ioutil.ReadFile(sc.policyFile)
if err != nil {
return nil, fmt.Errorf("unable to read policy config: %v", err)
// If there we are in legacy mode or ConfigMap name is empty, try to use
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/there we/we/

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

}

// Create implements the interface for the Configurator, hence it is exported
// even through the struct is not.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/through/though/

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -63,7 +63,9 @@ func (s *SchedulerServer) AddFlags(fs *pflag.FlagSet) {
fs.Int32Var(&s.Port, "port", s.Port, "The port that the scheduler's http service runs on")
fs.StringVar(&s.Address, "address", s.Address, "The IP address to serve on (set to 0.0.0.0 for all interfaces)")
fs.StringVar(&s.AlgorithmProvider, "algorithm-provider", s.AlgorithmProvider, "The scheduling algorithm provider to use, one of: "+factory.ListAlgorithmProviders())
fs.StringVar(&s.PolicyConfigFile, "policy-config-file", s.PolicyConfigFile, "File with scheduler policy configuration")
fs.StringVar(&s.PolicyConfigFile, "policy-config-file", s.PolicyConfigFile, "File with scheduler policy configuration. This file is used if policy ConfigMap is not provided or scheduler is using legacy policy config.")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say "or --use-legacy-policy-config==true" instead of "or scheduler is using legacy policy config"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -63,7 +63,9 @@ func (s *SchedulerServer) AddFlags(fs *pflag.FlagSet) {
fs.Int32Var(&s.Port, "port", s.Port, "The port that the scheduler's http service runs on")
fs.StringVar(&s.Address, "address", s.Address, "The IP address to serve on (set to 0.0.0.0 for all interfaces)")
fs.StringVar(&s.AlgorithmProvider, "algorithm-provider", s.AlgorithmProvider, "The scheduling algorithm provider to use, one of: "+factory.ListAlgorithmProviders())
fs.StringVar(&s.PolicyConfigFile, "policy-config-file", s.PolicyConfigFile, "File with scheduler policy configuration")
fs.StringVar(&s.PolicyConfigFile, "policy-config-file", s.PolicyConfigFile, "File with scheduler policy configuration. This file is used if policy ConfigMap is not provided or scheduler is using legacy policy config.")
fs.StringVar(&s.PolicyConfigMapName, "policy-configmap", s.PolicyConfigMapName, "Name of the ConfigMap object that contains scheduler's policy configuration. It must exist in the system namespace before scheduler initialization if scheduler is not using legacy policy config.")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say "if --use-legacy-policy-config==false" instead of "if scheduler is not using legacy policy config"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

sched.Run()
defer close(stop)

DoTestUnschedulableNodes(t, clientSet, ns, informerFactory.Core().V1().Nodes().Lister())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's probably fine to do in a different PR, but I think as part of this overall feature it's important to test that the values read from the ConfigMap were actually applied by the scheduler. (I realize we had no useful testing for the file case previously.) I think you were mentioning the other day that the factory test could be adapted to do this? Or something simple like what @k82cn suggested in the next comment (just tweak one policy and see that it is respected in how a pod is scheduled).

// the scheduler's policy config. If UseLegacyPolicyConfig is true, scheduler
// uses PolicyConfigFile. If UseLegacyPolicyConfig is false and
// PolicyConfigMapName is not empty, the ConfigMap object with this name must
// exist in the default system namespace ("kube-system") before scheduler
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given multi-schedulers and #42961, I think we should probably allow for the flexibility of specifying another namespace.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@wanghaoran1988
Copy link
Contributor

@bsalamat I think maybe you should update the rbac bootstrap policy for the scheduler as you involve new resource type to access.

var policy schedulerapi.Policy

namespace := sc.policyConfigMapNamespace
if namespace == "" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it better to add to the defaults.go ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Done.

}
// If not in legacy mode, try to find policy ConfigMap.
if !sc.useLegacyPolicyConfig && len(sc.policyConfigMap) != 0 {
policyConfigMap, err := sc.GetClient().CoreV1().ConfigMaps(namespace).Get(sc.policyConfigMap, metav1.GetOptions{})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have default policyConfigMapName ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, we don't. If it is empty, we ignore it.

@bsalamat
Copy link
Member Author

bsalamat commented Apr 6, 2017

@wanghaoran1988 is the rbac permissions needed to let scheduler access its ConfigMap?

@wanghaoran1988
Copy link
Contributor

@bsalamat I am not completely sure about that, I met that problem when I update the scheduler lock location in this #42961 , you can have a try to enable rbac in your cluster.

@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 8, 2017
@timothysc timothysc added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 8, 2017
@timothysc
Copy link
Member

lgtm to me now, if someone else wants to once over.

fs.StringVar(&s.PolicyConfigFile, "policy-config-file", s.PolicyConfigFile, "File with scheduler policy configuration")
fs.StringVar(&s.PolicyConfigFile, "policy-config-file", s.PolicyConfigFile, "File with scheduler policy configuration. This file is used if policy ConfigMap is not provided or --use-legacy-policy-config==true")
fs.StringVar(&s.PolicyConfigMapName, "policy-configmap", s.PolicyConfigMapName, "Name of the ConfigMap object that contains scheduler's policy configuration. It must exist in the system namespace before scheduler initialization if --use-legacy-policy-config==false")
fs.StringVar(&s.PolicyConfigMapNamespace, "policy-configmap-namespace", s.PolicyConfigMapNamespace, "The namespace where policy ConfigMap is located. The system namespace will be used if this is not provided or is empty.")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default value should be "kube-system"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, we do not need that if we have that set in the defaults.go @k82cn

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the default.go will execute? AFAIK, the default will be set by apiserver; not sure when the componentconfig is update.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

En, it's set in NewSchedulerServer manually. Also prefer to the set the default value to kube-system so the help message [default=kube-system] will be included.

Anyway, not a block comments.

@@ -41,4 +41,10 @@ type SchedulerExtender interface {
// onto machines.
type ScheduleAlgorithm interface {
Schedule(*v1.Pod, NodeLister) (selectedMachine string, err error)
// Predicates() returns a pointer to a map of predicate functions. This is
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

d/a pointer to/
or
s/pointer/reference/

@k82cn
Copy link
Member

k82cn commented Apr 9, 2017

LGTM, minor comments.

@wanghaoran1988
Copy link
Contributor

@liggitt @jayunit100 I am not sure whether openshift will enable user to use configMap change the policy config, but I know that openshift are try to reuse the controller rolebindings, if so, we need update the bootstrap policy for scheduler controller, and I prefer we have an default policy-configmap if we enabled them by default.

@liggitt
Copy link
Member

liggitt commented Apr 9, 2017

This is a very similar discussion to the one kube-dns had, which wanted to make use of dynamic config, and started by making the pod watch a config map. That was modified to allow reading config from a file instead, which allowed mounting the config map holding the config into the pod. This meant no API entanglement or permissions needed for purposes of reading the config, and no modifications to the dns pod whether it was reading from a static file or one mounted from a config map.

@k82cn
Copy link
Member

k82cn commented Apr 9, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 9, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: bsalamat, k82cn
We suggest the following additional approver: @smarterclayton

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 6e3bd08 into kubernetes:master Apr 9, 2017
@bsalamat bsalamat deleted the sched_conf1 branch April 10, 2017 18:04
@smarterclayton
Copy link
Contributor

smarterclayton commented Apr 12, 2017 via email


// TestSchedulerCreationInLegacyMode ensures that creation of the scheduler
// works fine when legacy mode is enabled.
func TestSchedulerCreationInLegacyMode(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seeing test failures on this

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bsalamat
Copy link
Member Author

/ref #41600

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.