Support additional kube-scheduler config parameters via config file #8407

rralcala · 2020-01-24T03:16:48Z

Mentioned in #6942

This change allows using the --config flag and a generated configfile to set
options that were not previously supported and the use via flags is deprecated.
(https://kubernetes.io/docs/reference/command-line-tools-reference/kube-scheduler/)

I thought that it might be better to have them in a config file to ensure
support in newer kubernetes versions.

It also makes it easy to add more.

Mentioned in kubernetes#6942 This change allows using the --config flag and a generated configfile to set options that were not previously supported and the use via flags is deprecated. (https://kubernetes.io/docs/reference/command-line-tools-reference/kube-scheduler/) I thought that it might be better to have them in a config file to ensure support in newer kubernetes versions. It also makes it easy to add more.

k8s-ci-robot · 2020-01-24T03:16:57Z

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

pkg/apis/kops/componentconfig.go

pkg/configbuilder/buildconfigfile.go

pkg/apis/kops/componentconfig.go

pkg/configbuilder/buildconfigfile.go

johngmyers · 2020-01-24T04:23:04Z

/ok-to-test

johngmyers

It seems the code is still calling flagbuilder.BuildFlagsList() to reflecting the KubeSchedulerConfig for passing values as flags, as well as having configbuilder.BuildConfigYaml() to reflect the same KubeSchedulerConfig to pass the same values in a yaml config file. This is redundant and confusing, likely to lead to problems where a change affects one but not the other. Any given value should be passed as either a flag or in the yaml config file, not both.

I see, different struct tags.

pkg/apis/kops/componentconfig.go

johngmyers · 2020-01-25T04:39:11Z

nodeup/pkg/model/kube_scheduler.go

@@ -79,6 +82,23 @@ func (b *KubeSchedulerBuilder) Build(c *fi.ModelBuilderContext) error {
 		})
 	}

+	if b.Cluster.Spec.KubeScheduler.KubeConfig == nil {
+		b.Cluster.Spec.KubeScheduler.KubeConfig = &defaultKubeConfig


Shouldn't filling in defaults happen in kops.FillDefaults()?

So, I looked up for this can couldn't find the default, I guess that's why the default was living in this file before the change, so I just kept it.

pkg/configbuilder/buildconfigfile.go

johngmyers · 2020-01-25T05:33:54Z

pkg/apis/kops/v1alpha1/componentconfig.go

+	// Burst sets the maximum qps to send to apiserver after the burst quota is exhausted
+	Burst *int32 `json:"burst,omitempty" configfile:"ClientConnection.Burst"`
+	// KubeConfig overrides the default kubeconfig path.
+	KubeConfig *string `json:"kubeConfig,omitempty" configfile:"ClientConnection.Kubeconfig"`


The configfile tags here and in v1alpha2 seem unnecessary.

pkg/apis/kops/componentconfig.go

pkg/configbuilder/buildconfigfile.go

Co-Authored-By: John Gardiner Myers <jgmyers@proofpoint.com>

johngmyers · 2020-01-25T19:34:59Z

/retest

johngmyers · 2020-01-25T19:40:30Z

pkg/apis/kops/v1alpha2/componentconfig.go

+	// Qps sets the maximum qps to send to apiserver after the burst quota is exhausted
+	Qps *resource.Quantity `json:"qps,omitempty" configfile:"ClientConnection.QPS"`
+	// Burst sets the maximum qps to send to apiserver after the burst quota is exhausted
+	Burst *int32 `json:"burst,omitempty" configfile:"ClientConnection.Burst"`


Would it make sense to change one of the e2e tests to fill in these two values? That would make it more likely we'd detect k/k changing the name or type of these settings.

johngmyers · 2020-01-25T19:46:20Z

/retest

rralcala · 2020-01-25T20:38:39Z

Updated based on feedback, I'm ok with the e2e test, just let me know where it should be part of this PR or a separate one, the other things are addressed, about the generalization, we were facing qps problems with another daemon so I was planning to send that PR as a followup to this one to avoid changing to many things and also propose the overall idea first. Thanks for all the input so far!!

johngmyers · 2020-01-25T22:03:35Z

/retest

rralcala · 2020-01-25T22:53:52Z

/retest

johngmyers · 2020-01-25T23:50:57Z

/retest

johngmyers · 2020-01-25T23:55:30Z

BuildConfigYaml seems a bit specific as it knows that a resource.Quantity gets converted into a float32.

johngmyers · 2020-01-25T23:58:42Z

I think the e2e test would be best as part of this PR if possible.

rralcala · 2020-01-26T00:31:15Z

BuildConfigYaml seems a bit specific as it knows that a resource.Quantity gets converted into a float32.

flagbuilder.BuildFlagsList does the same and it's used in several places, and I couldn't use floatX in KubeSchedulerConfig so that's why I went with resource.Quantity since I found it being used in other Config structs.

nodeup/pkg/model/kube_scheduler.go

nodeup/pkg/model/kube_scheduler_test.go

pkg/apis/kops/v1alpha2/componentconfig.go

rdrgmnzs · 2020-01-27T19:34:32Z

Hi @rralcala can you add Qps and Burst back to pkg/apis/kops/v1alpha1/componentconfig.go and pkg/apis/kops/v1alpha2/componentconfig.go, that way we can re-gen the api machinery correctly for those API versions.

rdrgmnzs · 2020-01-27T21:09:13Z

Thanks for all the work on the PR and addressing all the comments @rralcala ! We can split the e2e tests in a separate PR, will ping you on Slack directly to get that going.

/lgtm
/approve

k8s-ci-robot · 2020-01-27T21:09:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rdrgmnzs, rralcala

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:

~~OWNERS~~ [rdrgmnzs]

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

rifelpet · 2020-01-29T05:30:25Z

@rralcala it looks like this may have caused a regression in our E2E suite when running k8s 1.11 clusters:

https://testgrid.k8s.io/sig-cluster-lifecycle-kops#kops-aws-k8s-1.11

@hakman and I found that the error message in the kube-scheduler log is:

no kind "KubeSchedulerConfiguration" is registered for version "kubescheduler.config.k8s.io/v1alpha1"

I see in the changelog that KubeSchedulerConfiguration used to be under componentconfig/v1alpha1 but was moved to kubescheduler.config.k8s.io/v1alpha1 in k8s 1.12. Rather than supporting both api groups how about we increase the version check to only use the config file in k8s 1.12?

rifelpet · 2020-01-29T05:34:12Z

nodeup/pkg/model/kube_scheduler.go

 // Build is responsible for building the manifest for the kube-scheduler
 func (b *KubeSchedulerBuilder) Build(c *fi.ModelBuilderContext) error {
 	if !b.IsMaster {
 		return nil
 	}
-
+	useConfigFile := b.IsKubernetesGTE("1.11")


for a followup PR:

Suggested change

useConfigFile := b.IsKubernetesGTE("1.11")

useConfigFile := b.IsKubernetesGTE("1.12")

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 24, 2020

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 24, 2020

k8s-ci-robot requested review from joshbranham and robinpercy January 24, 2020 03:18

Removes unused func

13047db

johngmyers reviewed Jan 24, 2020

View reviewed changes

pkg/apis/kops/componentconfig.go Outdated Show resolved Hide resolved

johngmyers reviewed Jan 24, 2020

View reviewed changes

pkg/configbuilder/buildconfigfile.go Outdated Show resolved Hide resolved

rralcala added 2 commits January 23, 2020 19:58

Fixes incorrect tag name and copyright year

7a01739

Fix for copyright and ran update-bazel.sh

447b46d

johngmyers requested changes Jan 24, 2020

View reviewed changes

pkg/configbuilder/buildconfigfile.go Outdated Show resolved Hide resolved

pkg/apis/kops/componentconfig.go Outdated Show resolved Hide resolved

pkg/configbuilder/buildconfigfile.go Outdated Show resolved Hide resolved

pkg/configbuilder/buildconfigfile.go Outdated Show resolved Hide resolved

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 Jan 24, 2020

rralcala added 3 commits January 23, 2020 20:31

Updates based on feedback

b994559

Fix broken test after last change

ac9c368

Fixing ci tests

117353c

johngmyers requested changes Jan 25, 2020

View reviewed changes

johngmyers reviewed Jan 25, 2020

View reviewed changes

rralcala and others added 3 commits January 25, 2020 10:07

Update pkg/configbuilder/buildconfigfile.go

069e74a

Co-Authored-By: John Gardiner Myers <jgmyers@proofpoint.com>

Update pkg/configbuilder/buildconfigfile.go

99c57b6

Co-Authored-By: John Gardiner Myers <jgmyers@proofpoint.com>

Update pkg/configbuilder/buildconfigfile.go

f5c8e46

Co-Authored-By: John Gardiner Myers <jgmyers@proofpoint.com>

johngmyers reviewed Jan 25, 2020

View reviewed changes

Updates based on feedback

1298d54

johngmyers requested changes Jan 26, 2020

View reviewed changes

Adds test and fixes incorrect flag

03af6b8

k8s-ci-robot assigned rdrgmnzs Jan 27, 2020

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 27, 2020

k8s-ci-robot merged commit ace4c60 into kubernetes:master Jan 27, 2020

k8s-ci-robot added this to the v1.18 milestone Jan 27, 2020

rifelpet reviewed Jan 29, 2020

View reviewed changes

rralcala mentioned this pull request Jan 29, 2020

Fixes regression in e2e tests #8430

Merged

This was referenced Jul 1, 2020

Allow kops to generate kube-scheduler config flags. #6942

Closed

Support custom kube-scheduler configuration #7186

Closed

ddelange mentioned this pull request Mar 9, 2022

Scheduling policies are removed (moved to scheduler config) in k8s v1.23 #13352

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support additional kube-scheduler config parameters via config file #8407

Support additional kube-scheduler config parameters via config file #8407

rralcala commented Jan 24, 2020

k8s-ci-robot commented Jan 24, 2020

johngmyers commented Jan 24, 2020

johngmyers left a comment •

edited

Loading

johngmyers Jan 25, 2020

rralcala Jan 25, 2020

rralcala Jan 25, 2020

johngmyers Jan 25, 2020

rralcala Jan 25, 2020

johngmyers commented Jan 25, 2020

johngmyers Jan 25, 2020

johngmyers commented Jan 25, 2020

rralcala commented Jan 25, 2020

johngmyers commented Jan 25, 2020

rralcala commented Jan 25, 2020

johngmyers commented Jan 25, 2020

johngmyers commented Jan 25, 2020

johngmyers commented Jan 25, 2020 •

edited

Loading

rralcala commented Jan 26, 2020 •

edited

Loading

rdrgmnzs commented Jan 27, 2020

rdrgmnzs commented Jan 27, 2020

k8s-ci-robot commented Jan 27, 2020

rifelpet commented Jan 29, 2020

rifelpet Jan 29, 2020

	useConfigFile := b.IsKubernetesGTE("1.11")
	useConfigFile := b.IsKubernetesGTE("1.12")

Support additional kube-scheduler config parameters via config file #8407

Support additional kube-scheduler config parameters via config file #8407

Conversation

rralcala commented Jan 24, 2020

k8s-ci-robot commented Jan 24, 2020

johngmyers commented Jan 24, 2020

johngmyers left a comment • edited Loading

Choose a reason for hiding this comment

johngmyers Jan 25, 2020

Choose a reason for hiding this comment

rralcala Jan 25, 2020

Choose a reason for hiding this comment

rralcala Jan 25, 2020

Choose a reason for hiding this comment

johngmyers Jan 25, 2020

Choose a reason for hiding this comment

rralcala Jan 25, 2020

Choose a reason for hiding this comment

johngmyers commented Jan 25, 2020

johngmyers Jan 25, 2020

Choose a reason for hiding this comment

johngmyers commented Jan 25, 2020

rralcala commented Jan 25, 2020

johngmyers commented Jan 25, 2020

rralcala commented Jan 25, 2020

johngmyers commented Jan 25, 2020

johngmyers commented Jan 25, 2020

johngmyers commented Jan 25, 2020 • edited Loading

rralcala commented Jan 26, 2020 • edited Loading

rdrgmnzs commented Jan 27, 2020

rdrgmnzs commented Jan 27, 2020

k8s-ci-robot commented Jan 27, 2020

rifelpet commented Jan 29, 2020

rifelpet Jan 29, 2020

Choose a reason for hiding this comment

johngmyers left a comment •

edited

Loading

johngmyers commented Jan 25, 2020 •

edited

Loading

rralcala commented Jan 26, 2020 •

edited

Loading