kubeadm: Reduce the usage of InitConfiguration #72111

rosti · 2018-12-17T16:15:24Z

What type of PR is this?
/kind cleanup

What this PR does / why we need it:

Back in the v1alpha1 we had a single flat MasterConfiguration and whenever some function needed something from it, it took a pointer to MasterConfiguration argument. In v1beta1 this configuration is no longer flat, but composite with InitConfiguration bearing the closest resemblance to the old MasterConfiguration. Most of kubeadm still works with InitConfiguration even though most functions need only a portion or even a single field from the config.
This change strives to reduce the unnecessary usage of InitConfiguration. The approach taken here is to switch APIs, that take InitConfiguration to some other type of argument (usually ClusterConfiguration).

Which issue(s) this PR fixes:

Refs kubernetes/kubeadm#1084

Special notes for your reviewer:

This PR is marked as a WIP as I want some feedback on the scope of changes and weather it is too broad.
The commits aren't squashed yet, as it would be easier to drop something upon feedback if needed. Also, it might be a lot easier to review this on a per commit basis (rather than as a whole).

/cc @kubernetes/sig-cluster-lifecycle-pr-reviews
/area kubeadm
/assign @fabriziopandini
/assign @timothysc

Does this PR introduce a user-facing change?:

NONE

rosti · 2018-12-17T16:16:03Z

/priority backlog

neolit123

thanks @rosti
did a pass over the diff and the change LGTM.

/lgtm
/assign @fabriziopandini
for approve.

yagonobre

LGTM
thanks @rosti

yagonobre · 2018-12-17T17:40:15Z

/test pull-kubernetes-e2e-kops-aws

fabriziopandini

@rosti 💯for this cleanup!
I have left some minor comments but this IMO can merge as soon as you remove WIP.

cmd/kubeadm/app/phases/kubelet/config.go

cmd/kubeadm/app/phases/controlplane/manifests.go

cmd/kubeadm/app/util/endpoint.go

cmd/kubeadm/app/phases/etcd/local.go

fabriziopandini · 2018-12-28T10:16:49Z

cmd/kubeadm/app/preflight/checks.go

@@ -973,11 +973,11 @@ func RunJoinNodeChecks(execer utilsexec.Interface, cfg *kubeadmapi.JoinConfigura
 }

 // RunOptionalJoinNodeChecks executes all individual, applicable to node configuration dependant checks
-func RunOptionalJoinNodeChecks(execer utilsexec.Interface, initCfg *kubeadmapi.InitConfiguration, ignorePreflightErrors sets.String) error {
+func RunOptionalJoinNodeChecks(execer utilsexec.Interface, cfg *kubeadmapi.ClusterConfiguration, ignorePreflightErrors sets.String) error {


what about changing ClusterConfiguration to KubeProxy configuration only?

We have a couple of options for this one:

Keep the func named RunOptionalJoinNodeChecks and use ClusterConfiguration.

Rename the func to RunOptionalKubeProxyChecks and pass KubeProxy configuration.

I don't think it's expected and acceptable for the func to be named RunOptionalJoinNodeChecks and to accept KubeProxy configuration directly. This is unexpected for the reader of the code and is bound to raise questions.

we can keep this as is, as we might decide to add more checks in the function.
hopefully not, though.

rosti · 2019-01-24T15:12:55Z

One of the commits (a9fd78dc49a) can probably be moved into a separate PR. Though I am not sure about this and it probably belongs here.

timothysc · 2019-01-25T02:31:19Z

@rosti - please squash your commits.

rosti · 2019-01-25T12:21:31Z

@timothysc will do it, but just before I pull together the final version of things. Waiting for @fabriziopandini to review it.

fabriziopandini · 2019-01-25T16:14:01Z

@rosti green light for me! let's get this in as soon as possible
/approve

k8s-ci-robot · 2019-01-25T16:14:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini, rosti

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:

~~cmd/kubeadm/OWNERS~~ [fabriziopandini]

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

For historical reasons InitConfiguration is used almost everywhere in kubeadm as a carrier of various configuration components such as ClusterConfiguration, local API server endpoint, node registration settings, etc. Since v1alpha2, InitConfiguration is meant to be used solely as a way to supply the kubeadm init configuration from a config file. Its usage outside of this context is caused by technical dept, it's clunky and requires hacks to fetch a working InitConfiguration from the cluster (as it's not stored in the config map in its entirety). This change is a small step towards removing all unnecessary usages of InitConfiguration. It reduces its usage by replacing it in some places with some of the following: - ClusterConfiguration only. - APIEndpoint (as local API server endpoint). - NodeRegistrationOptions only. - Some combinations of the above types, or if single fields from them are used, only those field. Signed-off-by: Rostislav M. Georgiev <rostislavg@vmware.com>

rosti · 2019-01-28T12:30:31Z

Commits squashed, WIP tag removed, this is finally ready.

neolit123 · 2019-01-28T13:18:13Z

/lgtm
/hold
/retest

rosti · 2019-01-28T13:57:19Z

/test pull-kubernetes-node-e2e

neolit123 · 2019-01-28T14:09:02Z

/retest

rosti · 2019-01-29T10:06:16Z

/test pull-kubernetes-node-e2e

neolit123 · 2019-01-29T11:32:05Z

/hold cancel

fejta-bot · 2019-01-29T16:41:42Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Dec 17, 2018

k8s-ci-robot assigned fabriziopandini and timothysc Dec 17, 2018

k8s-ci-robot added priority/backlog Higher priority than priority/awaiting-more-evidence. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Dec 17, 2018

k8s-ci-robot assigned neolit123 Dec 17, 2018

neolit123 approved these changes Dec 17, 2018

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 17, 2018

yagonobre approved these changes Dec 17, 2018

View reviewed changes

fabriziopandini reviewed Dec 28, 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 Dec 29, 2018

rosti force-pushed the reduce-initcfg branch from a0d1591 to 872f59a Compare January 24, 2019 13:08

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

rosti force-pushed the reduce-initcfg branch from 99e35fe to 80e2a3c Compare January 28, 2019 12:29

rosti changed the title ~~[WIP] kubeadm: Reduce the usage of InitConfiguration~~ kubeadm: Reduce the usage of InitConfiguration Jan 28, 2019

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 Jan 28, 2019

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Jan 28, 2019

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

k8s-ci-robot merged commit b8b689a into kubernetes:master Jan 29, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubeadm: Reduce the usage of InitConfiguration #72111

kubeadm: Reduce the usage of InitConfiguration #72111

rosti commented Dec 17, 2018 •

edited

rosti commented Dec 17, 2018

neolit123 left a comment

yagonobre left a comment

yagonobre commented Dec 17, 2018

fabriziopandini left a comment

fabriziopandini Dec 28, 2018

rosti Jan 24, 2019

neolit123 Jan 24, 2019

rosti commented Jan 24, 2019

timothysc commented Jan 25, 2019

rosti commented Jan 25, 2019

fabriziopandini commented Jan 25, 2019

k8s-ci-robot commented Jan 25, 2019

rosti commented Jan 28, 2019

neolit123 commented Jan 28, 2019

rosti commented Jan 28, 2019

neolit123 commented Jan 28, 2019

rosti commented Jan 29, 2019

neolit123 commented Jan 29, 2019

fejta-bot commented Jan 29, 2019

kubeadm: Reduce the usage of InitConfiguration #72111

kubeadm: Reduce the usage of InitConfiguration #72111

Conversation

rosti commented Dec 17, 2018 • edited

rosti commented Dec 17, 2018

neolit123 left a comment

Choose a reason for hiding this comment

yagonobre left a comment

Choose a reason for hiding this comment

yagonobre commented Dec 17, 2018

fabriziopandini left a comment

Choose a reason for hiding this comment

fabriziopandini Dec 28, 2018

Choose a reason for hiding this comment

rosti Jan 24, 2019

Choose a reason for hiding this comment

neolit123 Jan 24, 2019

Choose a reason for hiding this comment

rosti commented Jan 24, 2019

timothysc commented Jan 25, 2019

rosti commented Jan 25, 2019

fabriziopandini commented Jan 25, 2019

k8s-ci-robot commented Jan 25, 2019

rosti commented Jan 28, 2019

neolit123 commented Jan 28, 2019

rosti commented Jan 28, 2019

neolit123 commented Jan 28, 2019

rosti commented Jan 29, 2019

neolit123 commented Jan 29, 2019

fejta-bot commented Jan 29, 2019

rosti commented Dec 17, 2018 •

edited