Refactor the kube-controller-manager ComponentConfig structs #66993

luxas · 2018-08-05T18:17:48Z

What this PR does / why we need it:

This PR cleans up the kube-controller-manager structs in the componentconfig package and fixes various structural issues in the current code, in order to make it possible to later move the code out to external API groups (as a starting point GenericControllerManagerConfiguration to k8s.io/controller-manager).

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
ref: kubernetes/community#2354

This PR depends on:

Special notes for your reviewer:

Please only review the following commits:

Refactor the k-c-m ComponentConfig structs to they can be moved out
Fixup cmd/kube-controller-manager code after struct changes.

Release note:

NONE

/assign @sttts @stewart-yu @liggitt @thockin

luxas · 2018-08-05T19:26:07Z

/hold
This PR is WIP currently and depends on an other PR but I really want to get feedback on the direction. After this PR merges we can move out the controller manager structs properly to staging repos

stewart-yu · 2018-08-06T00:00:51Z

pkg/apis/componentconfig/v1alpha1/types.go

+	CloudProvider CloudProviderConfiguration `json:"cloudProvider"`
+	// externalCloudVolumePlugin specifies the plugin to use when cloudProvider is "external".
+	// It is currently used by the in repo cloud providers to handle node and volume control in the KCM.
+	ExternalCloudVolumePlugin string `json:"externalCloudVolumePlugin"`


ExternalCloudVolumePlugin not belong to cloud-controller-manager now, should not place in KubeCloudSharedConfiguration. WDYT

stewart-yu · 2018-08-06T00:02:41Z

pkg/apis/componentconfig/v1alpha1/types.go

+	// '-foo' means "disable 'foo'"
+	// first item for a particular name wins
+	Controllers []string `json:"controllers"`
+	// DebuggingConfiguration holds configuration for Debugging related features.


Controllers also belong to kube-cc, does we want add it to cloud-cc?

stewart-yu · 2018-08-06T03:04:16Z

pkg/apis/componentconfig/v1alpha1/defaults.go

 	zero := metav1.Duration{}
-	// Port
 	if obj.Address == "" {
 		obj.Address = "0.0.0.0"
 	}
 	if obj.RouteReconciliationPeriod == zero {
 		obj.RouteReconciliationPeriod = metav1.Duration{Duration: 10 * time.Second}


Does RouteReconciliationPeriod in KubeCloudSharedConfiguration?

@liggitt

Automatic merge from submit-queue (batch tested with PRs 67160, 67090, 67159, 66866, 62111). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Remove references to `pkg/apis/componentconfig/install` and fix inconsistencies **What this PR does / why we need it**: This PR fixes a bunch of problems with the (deprecated, monolithic, to be removed) componentconfig API group: - As discussed with @liggitt, the current structure of k-c-m componentconfig types is really bad, and we don't want anyone to think it can be serialized (not that we have any such code, but anyway). So we deciided to remove all JSON tags from the types to be consistent, register the violations, then move types out, get the structure right, and **first then** add JSON tags. - There should not be an `install` package for componentconfigs that installs stuff in `legacyscheme`, removed the package and references to it. - Added myself and @sttts to approvers for `pkg/apis/componentconfig`, which we intend to remove in favor for dedicated API groups in different staging repos. - Removed the componentconfig types from the API testing in `pkg/api/testing`, that roundtrip and defaulting testing will exist in the dedicated API groups instead. Added a TODO to add roundtrip, defaulting and type tag testing later. - Made the `register.go` files more consistent to the common template. **Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*: ref kubernetes/community#2354 **Special notes for your reviewer**: This PR must merge before kubernetes#66722 and kubernetes#66993 **Release note**: ```release-note NONE ``` @kubernetes/sig-api-machinery-pr-reviews @kubernetes/sig-cluster-lifecycle-pr-reviews /assign @sttts @thockin @jbeda @liggitt

sttts · 2018-08-29T20:18:06Z

TestSchedulerOptions is broken, nil pointer panic.

jbeda · 2018-08-29T20:34:01Z

/approve

Based on @sttts giving it an lgtm.

liggitt · 2018-08-29T21:08:39Z

pkg/scheduler/apis/config/v1alpha1/zz_generated.conversion.go

@@ -110,12 +108,14 @@ func autoConvert_v1alpha1_KubeSchedulerConfiguration_To_config_KubeSchedulerConf
 	if err := Convert_v1alpha1_KubeSchedulerLeaderElectionConfiguration_To_config_KubeSchedulerLeaderElectionConfiguration(&in.LeaderElection, &out.LeaderElection, s); err != nil {
 		return err
 	}
-	if err := configv1alpha1.Convert_v1alpha1_ClientConnectionConfiguration_To_config_ClientConnectionConfiguration(&in.ClientConnection, &out.ClientConnection, s); err != nil {
+	// TODO: Inefficient conversion - can we improve it?
+	if err := s.Convert(&in.ClientConnection, &out.ClientConnection, 0); err != nil {


this conversion isn't right

ugh, I have no idea what happened there wrt the autogenerated code. Can't reproduce either

I have seen this independently from this PR. Looks like we have another code generation bug, maybe in the vicinity of Makefile.generated_files. /cc @thockin

liggitt · 2018-08-29T21:12:20Z

/hold

there are some outstanding issues to resolve before merge:

test failure
unexpected generated conversion in scheduler (maybe related to test failure)
apparent double flag registration (for Debugging.AddFlags() and CloudProvider.AddFlags())

k8s-ci-robot · 2018-08-30T16:31:16Z

New changes are detected. LGTM label has been removed.

k8s-ci-robot · 2018-08-30T16:31:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jbeda, luxas, sttts

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:

~~api/OWNERS~~ [jbeda]
~~cmd/cloud-controller-manager/OWNERS~~ [jbeda,luxas]
~~cmd/controller-manager/OWNERS~~ [jbeda,sttts]
~~cmd/kube-controller-manager/OWNERS~~ [jbeda]
~~pkg/apis/componentconfig/OWNERS~~ [jbeda,luxas,sttts]

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

luxas · 2018-08-30T16:33:19Z

Ugh, I have no idea what happened with the generated code on the last push. Good you and the tests noticed the wrongly-autogenerated file in the last push.
Now that is fixed though, files generated correctly (as everytime but last time 😟)

apparent double flag registration (for Debugging.AddFlags() and CloudProvider.AddFlags())

adressed

luxas · 2018-08-30T17:11:13Z

/hold cancel
Bad autogenerated code removed, so now this passes all tests.
Please re-review 🙏

luxas · 2018-08-30T17:44:55Z

/retest

liggitt · 2018-08-31T18:39:59Z

lgtm, but has conflicts according to github

sttts · 2018-08-31T18:48:18Z

cmd/controller-manager/app/options/generic.go

 	fs.DurationVar(&o.ControllerStartInterval.Duration, "controller-start-interval", o.ControllerStartInterval.Duration, "Interval between starting controller managers.")
+	// TODO: complete the work of the cloud-controller-manager (and possibly other consumers of this code) respecting the --controllers flag
+	fs.StringSliceVar(&o.Controllers, "controllers", o.Controllers, fmt.Sprintf(""+
+		"A list of controllers to enable.  '*' enables all on-by-default controllers, 'foo' enables the controller "+


nit: space too much

sttts · 2018-08-31T18:48:32Z

cmd/controller-manager/app/options/generic.go

+	if err := o.Debugging.ApplyTo(&cfg.Debugging); err != nil {
+		return err
+	}
+	cfg.Port = o.Port


newline above port

sttts · 2018-08-31T18:48:48Z

cmd/controller-manager/app/options/generic.go

 	if o == nil {
 		return nil
 	}

 	errs := []error{}
+	errs = append(errs, o.Debugging.Validate()...)
+	allControllersSet := sets.NewString(allControllers...)


newline above

…can be moved out

@stewart-yu

@stewart-yu

luxas · 2018-09-02T12:04:39Z

Rebased, e2e's are green, and both @sttts and @liggitt have given their LGTMs (thanks).
Applying the label

k8s-github-robot · 2018-09-02T12:18:05Z

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

k8s-github-robot · 2018-09-02T13:25:10Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here: https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md.

k8s-ci-robot assigned liggitt Aug 5, 2018

k8s-ci-robot added the release-note-none Denotes a PR that doesn't merit a release note. label Aug 5, 2018

k8s-ci-robot assigned stewart-yu, sttts and thockin Aug 5, 2018

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. labels Aug 5, 2018

k8s-ci-robot requested review from cheftako and davidopp August 5, 2018 18:17

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 5, 2018

k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/new-api labels Aug 5, 2018

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 5, 2018

stewart-yu reviewed Aug 6, 2018

View reviewed changes

stewart-yu mentioned this pull request Aug 6, 2018

Moving GenericControllerManagerConfiguration from pkg/apis/componentconfig to k8s.io/controller-manager #67015

Closed

luxas changed the title ~~Clean up the componentconfig types and fix API violations~~ WIP: Clean up the componentconfig types and fix API violations Aug 6, 2018

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 6, 2018

luxas mentioned this pull request Aug 7, 2018

Remove references to pkg/apis/componentconfig/install and fix inconsistencies #67090

Merged

luxas force-pushed the cleanup_componentconfig_types branch from 6c17d45 to 584e09b Compare August 8, 2018 21:07

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 8, 2018

luxas changed the title ~~WIP: Clean up the componentconfig types and fix API violations~~ Clean up the componentconfig types and fix API violations Aug 9, 2018

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 Aug 9, 2018

luxas force-pushed the cleanup_componentconfig_types branch from de82b24 to e54c52b Compare August 9, 2018 20:06

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

luxas changed the title ~~Clean up the componentconfig types and fix API violations~~ Refactor the kube-controller-manager ComponentConfig structs Aug 9, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 29, 2018

liggitt reviewed Aug 29, 2018

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 29, 2018

luxas force-pushed the cleanup_componentconfig_types branch from ed69833 to b4a51f1 Compare August 30, 2018 16:31

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 30, 2018

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

luxas added this to the v1.12 milestone Aug 30, 2018

sttts reviewed Aug 31, 2018

View reviewed changes

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

luxas added 3 commits September 2, 2018 14:10

Refactor the kube-controller-manager ComponentConfig structs to they …

3187f22

…can be moved out

Fixup cmd/*controller-manager code after struct changes. Co-authored by

8aaa527

@stewart-yu

autogenerated

b17d7bf

luxas force-pushed the cleanup_componentconfig_types branch from b4a51f1 to b17d7bf Compare September 2, 2018 11:11

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

luxas added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 2, 2018

k8s-github-robot merged commit b8e97d3 into kubernetes:master Sep 2, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor the kube-controller-manager ComponentConfig structs #66993

Refactor the kube-controller-manager ComponentConfig structs #66993

luxas commented Aug 5, 2018 •

edited by liggitt

luxas commented Aug 5, 2018

stewart-yu Aug 6, 2018

stewart-yu Aug 6, 2018

stewart-yu Aug 6, 2018

luxas Aug 8, 2018

sttts commented Aug 29, 2018

jbeda commented Aug 29, 2018

liggitt Aug 29, 2018

luxas Aug 30, 2018

sttts Aug 30, 2018

liggitt commented Aug 29, 2018

k8s-ci-robot commented Aug 30, 2018

k8s-ci-robot commented Aug 30, 2018

luxas commented Aug 30, 2018

luxas commented Aug 30, 2018

luxas commented Aug 30, 2018

liggitt commented Aug 31, 2018 •

edited

sttts Aug 31, 2018

luxas Sep 2, 2018

sttts Aug 31, 2018

luxas Sep 2, 2018

sttts Aug 31, 2018

luxas Sep 2, 2018

luxas commented Sep 2, 2018

k8s-github-robot commented Sep 2, 2018

k8s-github-robot commented Sep 2, 2018

Refactor the kube-controller-manager ComponentConfig structs #66993

Refactor the kube-controller-manager ComponentConfig structs #66993

Conversation

luxas commented Aug 5, 2018 • edited by liggitt

luxas commented Aug 5, 2018

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

sttts commented Aug 29, 2018

jbeda commented Aug 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented Aug 29, 2018

k8s-ci-robot commented Aug 30, 2018

k8s-ci-robot commented Aug 30, 2018

luxas commented Aug 30, 2018

luxas commented Aug 30, 2018

luxas commented Aug 30, 2018

liggitt commented Aug 31, 2018 • 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

luxas commented Sep 2, 2018

k8s-github-robot commented Sep 2, 2018

k8s-github-robot commented Sep 2, 2018

luxas commented Aug 5, 2018 •

edited by liggitt

liggitt commented Aug 31, 2018 •

edited