UsePolicyConfigMap for kube-scheduler #3581

whs · 2017-10-10T04:05:44Z

Continued from #3546

In this version, a single option usePolicyConfigMap is added that will install scheduler.addons.k8s.io, which contains a default configmap.

k8s-ci-robot · 2017-10-10T04:05:52Z

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

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. I understand the commands that are listed here.

chrislovecnm · 2017-10-11T04:21:13Z

Looked at this an I am wondering if we can make the API cooler.... meaning being able to tweak the scheduler.... make it so a user can set the configmap.

The other options which may be easier is to expose addons API to create cluster. Which I am planning on doing, then they can override the configmap.

@justinsb thoughts?

chrislovecnm · 2017-10-11T04:21:44Z

/ok-to-test

Let's get this out of the way

whs · 2017-10-11T04:27:21Z

An idea: another way you can put scheduler configuration in is by giving it the file. We're running our kube-up based production cluster using this option, by uploading the scheduler policy to the master node and bind mount into the container.

This might be achievable using fileAssets. Another plus is that after changing fileAssets the user probably want to rolling update their cluster so the scheduler will be restarted. I'm not sure how hard is this implementation.

chrislovecnm · 2017-10-11T06:17:19Z

pkg/apis/kops/v1alpha1/componentconfig.go

- // PolicyConfigMap is the name of configmap to use for scheduler policy
- PolicyConfigMap string `json:"policyConfigMap,omitempty" flag:"policy-configmap"`
- // PolicyConfigMapNamespace is the namespace containing the configmap
- PolicyConfigMapNamespace string `json:"policyConfigMapNamespace,omitempty" flag:"policy-configmap-namespace"`


Is there any reason that this is not kube-system? From a best practice standpoint would we ever put is somewhere else?

The default value of --policy-configmap-namespace is already kube-system, so I removed the configuration option

chrislovecnm · 2017-10-11T06:17:34Z

An idea: another way you can put scheduler configuration in is by giving it the file. We're running our kube-up based production cluster using this option, by uploading the scheduler policy to the master node and bind mount into the container.

This might be achievable using fileAssets. Another plus is that after changing fileAssets the user probably want to rolling update their cluster so the scheduler will be restarted. I'm not sure how hard is this implementation.

Externalizing the addons API will do all of this and more :) https://github.com/kubernetes/kops/blob/master/channels/pkg/api/channel.go is channels. Channels runs inside of protokube and applys addons with kubectl apply.

The workflow would be

User to define addons with the API above stored in multiple files.
create a cluster - not certain if we need an API value or not in the cluster spec
update cluster would read the addon's files during the bootstrap builder, and the files on disk would have precedence over the built-in files if they match. Files that do not match are added.
the cluster launches and addons are created. One of the addons could be the configmap for scheduler

We would take this PR, and build upon it.

k8s-github-robot · 2017-10-21T08:03:54Z

@whs PR needs rebase

chrislovecnm · 2017-10-26T23:31:03Z

@whs can we get a re-base, and then I think we are good to go.
@andrewsykim any comments?

andrewsykim

1 comment

andrewsykim · 2017-10-26T23:39:17Z

pkg/model/components/kubescheduler_test.go

 return &api.Cluster{
 Spec: api.ClusterSpec{
 CloudProvider: "aws",
 KubernetesVersion: "v1.4.0",
 KubeScheduler: &api.KubeSchedulerConfig{
- PolicyConfigMap: "scheduler-config",


Any reason for removing PolicyConfigMap? It seems the name of the ConfigMap changes from scheduler-config -> scheduler-policy between the 2 versions.

Sorry if I'm missing some context, reading through the other PR now :)

Yeah, so aftering reading through the PRs, my only question is around why we chose to change the ConfigMap name here, it seems it can break backwards compatability or leave dangling resources. Does scheduler create the ConfigMap scheduler-policy by default if it doesn't exist?

That's an oversight on my part. I'll rebase and change the name back. Thanks for the catch

Actually I just realized. The name "scheduler-config" is not actually in use anywhere except as test data. The actual config name in the cluster is configurable by this PolicyConfigMap field (which has no default value).

ahh, missed that it's just test data, thanks!

whs · 2017-10-27T02:03:59Z

I've rebased. I think we should be safe with changing the name, as in the old PR the user can choose any name and namespace anyway (and we don't have a default value).

The old documentation also suggested the name "scheduler-policy".

andrewsykim · 2017-10-27T02:29:09Z

pkg/model/components/kubescheduler.go

@@ -39,8 +39,8 @@ func (b *KubeSchedulerOptionsBuilder) BuildOptions(o interface{}) error {

 config := clusterSpec.KubeScheduler

- if config.PolicyConfigMap != "" && b.IsKubernetesLT("v1.7.0") {
- return fmt.Errorf("policyConfigMap is only supported in Kubernetes 1.7.0 or later")
+ if config.UsePolicyConfigMap != nil && b.IsKubernetesLT("v1.7.0") {


Should this be Kubernetes version greater than?

IsKubernetesGTE *

I'm not sure I under your question right. This policyConfigMap feature was added in Kubernetes 1.7.0, and so I added a check that if the Kubernetes version is under that it will throw an error.

You are correct, it's getting late for me :P

andrewsykim · 2017-10-27T02:33:59Z

lgtm

whs · 2017-10-27T02:36:35Z

I think you missed a slash?

andrewsykim · 2017-10-27T02:43:40Z

/lgtm

k8s-github-robot · 2017-10-27T02:43:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewsykim

The full list of commands accepted by this bot can be found here.

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

~~OWNERS~~ [andrewsykim]

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 · 2017-10-27T02:44:29Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2017-10-27T03:11:08Z

Automatic merge from submit-queue.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 10, 2017

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 10, 2017

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 11, 2017

chrislovecnm reviewed Oct 11, 2017

View reviewed changes

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 21, 2017

andrewsykim reviewed Oct 26, 2017

View reviewed changes

whs added 2 commits October 27, 2017 08:56

Generate scheduler policy by dynamic cluster addons

a708919

Updated docs for UsePolicyConfigMap

170bf52

whs force-pushed the master branch from d8c467a to 170bf52 Compare October 27, 2017 02:01

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 27, 2017

andrewsykim reviewed Oct 27, 2017

View reviewed changes

k8s-ci-robot assigned andrewsykim Oct 27, 2017

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

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 27, 2017

k8s-github-robot merged commit 66f7400 into kubernetes:master Oct 27, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UsePolicyConfigMap for kube-scheduler #3581

UsePolicyConfigMap for kube-scheduler #3581

whs commented Oct 10, 2017

k8s-ci-robot commented Oct 10, 2017

chrislovecnm commented Oct 11, 2017

chrislovecnm commented Oct 11, 2017

whs commented Oct 11, 2017 •

edited

Loading

chrislovecnm Oct 11, 2017

whs Oct 11, 2017

chrislovecnm commented Oct 11, 2017 •

edited

Loading

k8s-github-robot commented Oct 21, 2017

chrislovecnm commented Oct 26, 2017

andrewsykim left a comment

andrewsykim Oct 26, 2017

andrewsykim Oct 26, 2017

andrewsykim Oct 26, 2017

whs Oct 27, 2017

whs Oct 27, 2017

andrewsykim Oct 27, 2017

whs commented Oct 27, 2017

andrewsykim Oct 27, 2017

andrewsykim Oct 27, 2017

whs Oct 27, 2017

andrewsykim Oct 27, 2017

andrewsykim commented Oct 27, 2017

whs commented Oct 27, 2017

andrewsykim commented Oct 27, 2017

k8s-github-robot commented Oct 27, 2017

k8s-github-robot commented Oct 27, 2017

k8s-github-robot commented Oct 27, 2017

UsePolicyConfigMap for kube-scheduler #3581

UsePolicyConfigMap for kube-scheduler #3581

Conversation

whs commented Oct 10, 2017

k8s-ci-robot commented Oct 10, 2017

chrislovecnm commented Oct 11, 2017

chrislovecnm commented Oct 11, 2017

whs commented Oct 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrislovecnm commented Oct 11, 2017 • edited Loading

k8s-github-robot commented Oct 21, 2017

chrislovecnm commented Oct 26, 2017

andrewsykim 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

whs commented Oct 27, 2017

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

andrewsykim commented Oct 27, 2017

whs commented Oct 27, 2017

andrewsykim commented Oct 27, 2017

k8s-github-robot commented Oct 27, 2017

k8s-github-robot commented Oct 27, 2017

k8s-github-robot commented Oct 27, 2017

whs commented Oct 11, 2017 •

edited

Loading

chrislovecnm commented Oct 11, 2017 •

edited

Loading