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

[*-controller-manager]get rid of copied fields in the options and using componentconfig fields #70472

Merged
merged 3 commits into from Jan 16, 2019

Conversation

@stewart-yu
Copy link
Contributor

stewart-yu commented Oct 31, 2018

What type of PR is this?
/kind cleanup
/kind feature

What this PR does / why we need it:

  • There are so many coped fields in kube-controller manager( betwwen config and option), such as
    AttachDetachControllerOptions and AttachDetachControllerConfiguration, and so on.
  • We should not maintain the copied fields. Get rid of copied fields in the options and directly point the flags to the componentconfig fields in this PR.

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

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE
@sttts

This comment has been minimized.

Copy link
Contributor

sttts commented Oct 31, 2018

I would prefer to wrap the componentconfig types with an options struct using embedding. Then we can keep the current structure. Is that feasible?

@stewart-yu

This comment has been minimized.

Copy link
Contributor Author

stewart-yu commented Oct 31, 2018

I would prefer to wrap the componentconfig types with an options struct using embedding. Then we can keep the current structure. Is that feasible?

Yes, for now. but i have question:
Later, we should add --config for kube-controller manager, that is to say add two domain

ComponentConfig kubectrlmrgconfig.KubeControllerManagerConfiguration
ConfigFile string

in KubeControllerManagerOptions struct at least, should we keep those option https://github.com/kubernetes/kubernetes/blob/master/cmd/kube-controller-manager/app/options/options.go#L58-L80 in KubeControllerManagerOptions struct?

@stewart-yu stewart-yu force-pushed the stewart-yu:stewart-kube-controller-manager-optionToconfig branch 2 times, most recently from 957eb5b to d7f1933 Nov 1, 2018

@stewart-yu stewart-yu force-pushed the stewart-yu:stewart-kube-controller-manager-optionToconfig branch 2 times, most recently from c6c6c68 to 68e09a1 Nov 1, 2018

@sttts

This comment has been minimized.

Copy link
Contributor

sttts commented Nov 1, 2018

in KubeControllerManagerOptions struct at least, should we keep those option https://github.com/kubernetes/kubernetes/blob/master/cmd/kube-controller-manager/app/options/options.go#L58-L80 in KubeControllerManagerOptions struct?

This leads to the precedence question of flags vs. component config, doesn't it? IMO flags should override config values. In other words: we have to load the config and then apply flags to the structs of the config object. I think the kubelet has some (hacky) solution for that.

@stewart-yu stewart-yu force-pushed the stewart-yu:stewart-kube-controller-manager-optionToconfig branch from 68e09a1 to ddbdb7b Nov 1, 2018

@mbohlool

This comment has been minimized.

Copy link
Member

mbohlool commented Nov 1, 2018

/assign @cheftako

@stewart-yu

This comment has been minimized.

Copy link
Contributor Author

stewart-yu commented Jan 13, 2019

/assign @sttts
rebased, PTAL

@stewart-yu

This comment has been minimized.

Copy link
Contributor Author

stewart-yu commented Jan 14, 2019

/test pull-kubernetes-kubemark-e2e-gce-big

Debugging *DebuggingOptions
Controllers []string
*kubectrlmgrconfig.GenericControllerManagerConfiguration
DebuggingFlag *DebuggingOptions

This comment has been minimized.

@sttts

sttts Jan 14, 2019

Contributor

why not just Debugging ?

This comment has been minimized.

@stewart-yu

stewart-yu Jan 15, 2019

Author Contributor

The origin thoughts is that do not override Debugging in GenericControllerManagerConfiguration. But seems override is OK.
fixed now

ConfigureCloudRoutes bool
NodeSyncPeriod metav1.Duration
*kubectrlmgrconfig.KubeCloudSharedConfiguration
CloudProviderFlag *CloudProviderOptions

This comment has been minimized.

@sttts

sttts Jan 14, 2019

Contributor

Why the Flag postfix?

This comment has been minimized.

@stewart-yu

stewart-yu Jan 15, 2019

Author Contributor

fixed

@stewart-yu stewart-yu force-pushed the stewart-yu:stewart-kube-controller-manager-optionToconfig branch 2 times, most recently from 7b40312 to ca6aa48 Jan 15, 2019

@stewart-yu

This comment has been minimized.

Copy link
Contributor Author

stewart-yu commented Jan 15, 2019

/test pull-kubernetes-e2e-kops-aws

1 similar comment
@stewart-yu

This comment has been minimized.

Copy link
Contributor Author

stewart-yu commented Jan 15, 2019

/test pull-kubernetes-e2e-kops-aws

Debugging *DebuggingOptions
Controllers []string
*kubectrlmgrconfig.GenericControllerManagerConfiguration
Debugging *DebuggingOptions
}

// NewGenericControllerManagerConfigurationOptions returns generic configuration default values for both
// the kube-controller-manager and the cloud-contoller-manager. Any common changes should
// be made here. Any individual changes should be made in that controller.
func NewGenericControllerManagerConfigurationOptions(cfg kubectrlmgrconfig.GenericControllerManagerConfiguration) *GenericControllerManagerConfigurationOptions {

This comment has been minimized.

@sttts

sttts Jan 15, 2019

Contributor

wondering, shouldn't we have cfg *kubectrlmgrconfig.GenericControllerManagerConfiguration here and reference the pointer value below? In the next step we want to do something like in #72037, i.e. we let the flags point to the config values directly in order to override values.

This comment has been minimized.

@stewart-yu

stewart-yu Jan 16, 2019

Author Contributor

fixed

@stewart-yu stewart-yu force-pushed the stewart-yu:stewart-kube-controller-manager-optionToconfig branch from ca6aa48 to ac79dd8 Jan 16, 2019

@stewart-yu stewart-yu force-pushed the stewart-yu:stewart-kube-controller-manager-optionToconfig branch 2 times, most recently from 8e0d75d to 7cfe058 Jan 16, 2019

@stewart-yu stewart-yu force-pushed the stewart-yu:stewart-kube-controller-manager-optionToconfig branch from 7cfe058 to 0805362 Jan 16, 2019

@stewart-yu stewart-yu force-pushed the stewart-yu:stewart-kube-controller-manager-optionToconfig branch from 0805362 to a84d331 Jan 16, 2019

@sttts

This comment has been minimized.

Copy link
Contributor

sttts commented Jan 16, 2019

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label Jan 16, 2019

@sttts

This comment has been minimized.

Copy link
Contributor

sttts commented Jan 16, 2019

/retest

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jan 16, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: stewart-yu, 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:

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

@k8s-ci-robot k8s-ci-robot merged commit 2652e17 into kubernetes:master Jan 16, 2019

18 checks passed

cla/linuxfoundation stewart-yu authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-e2e-kubeadm-gce Skipped
pull-kubernetes-godeps Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details

@stewart-yu stewart-yu deleted the stewart-yu:stewart-kube-controller-manager-optionToconfig branch Jan 16, 2019

@luxas
Copy link
Member

luxas left a comment

Thanks @stewart-yu, very needed cleanup!!

@sttts sttts added this to Done in component-base Jan 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment