[reissue] kubeadm: Split out ClusterConfiguration from InitConfiguration #67441

rosti · 2018-08-15T17:01:53Z

As @luxas is not able to take care of #66219, I am reissuing the same change here. There are a few minor things added by me:

The original PR is rebased on latest master.
Some broken tests were fixed.
Some TODOs were added.
Run update-bazel and update-gofmt

Below is the text of the original PR by Lucas.

What this PR does / why we need it:

Splits MasterConfiguration to InitConfiguration and ClusterConfiguration as outlined in the kubeadm Config KEP. InitConfiguration holds init-only information, and ClusterConfiguration holds cluster-wide information. In the internal representation InitConfiguration wraps ClusterConfiguration as a field, but in serialized format they're different YAML documents.

Which issue(s) this PR fixes (optional, in fixes #(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
ref: kubernetes/kubeadm#911
Depends on:

Special notes for your reviewer:

Release note:

kubeadm: InitConfiguration now consists of two structs: InitConfiguration and ClusterConfiguration

@kubernetes/sig-cluster-lifecycle-pr-reviews

rosti · 2018-08-15T17:04:39Z

/assign @timothysc @fabriziopandini

rosti · 2018-08-15T17:07:24Z

/assign @luxas

neolit123 · 2018-08-15T23:56:21Z

thanks for doing this PR!
edit: we should probably close the original.

it needs a:

./hack/update-gofmt.sh
./hack/update-bazel.sh

but other than that i think we should merge ASAP and apply changes on top if needed.

fabriziopandini

@rosti +100 for jumping in and helping on @luxas work!

/approve

Being this so big, I kindly ask @timothysc or @liztio (or *) to give a second pass + final lgtm

chuckha · 2018-08-17T14:04:08Z

this code looks good to me. I'd rather get this merged ASAP as @neolit123 points out and fix anything that breaks before the release

timothysc

What's with all the TODOs? are we going to follow on or address here?

@fabriziopandini - I've been out for too long so I've lost state.

timothysc · 2018-08-20T21:38:58Z

cmd/kubeadm/app/apis/kubeadm/fuzzer/fuzzer_test.go

@@ -16,7 +16,12 @@ limitations under the License.

 package fuzzer

-import (
+// TODO: Fuzzing rouudtrip tests are currently disabled in the v1.12 cycle due to the
+// v1alpha2 -> v1alpha3 migration. As the ComponentConfigs were embedded in the structs


Can you open an issue here?

timothysc · 2018-08-20T21:51:32Z

cmd/kubeadm/app/cmd/config.go

-		API:               kubeadmapiv1alpha3.API{AdvertiseAddress: "1.2.3.4"},
-		BootstrapTokens:   []kubeadmapiv1alpha3.BootstrapToken{sillyToken},
-		KubernetesVersion: fmt.Sprintf("v1.%d.0", constants.MinimumControlPlaneVersion.Minor()+1),
+		// TODO: Probably move to getDefaultedClusterConfig?


I think, this one was a comment in the old PR. We decided to mark all unaddressed comments in the original PR as TODOs in the new one.

fabriziopandini · 2018-08-21T05:22:49Z

@timothysc this PR is already too big + it blocking other changes, so better to follow up on TODOs with separated PRs.
@rosti already created two of them, but IMO it is better to focus on completing users facing changes now while tackling TODOs only afterwards

timothysc · 2018-08-21T22:03:35Z

@rosti - needs a rebase

/lgtm

Trivial rebasement, fixed some broken tests, and inserted some TODOs: Rostislav M. Georgiev <rostislavg@vmware.com>

rosti · 2018-08-22T08:45:03Z

Rebased the PR.

fabriziopandini · 2018-08-22T20:16:24Z

/lgtm

k8s-github-robot · 2018-08-22T22:31:56Z

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

timothysc · 2018-08-22T23:22:52Z

/lgtm

k8s-ci-robot · 2018-08-22T23:22:57Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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,timothysc]

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

k8s-github-robot · 2018-08-22T23:46:58Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@luxas

…images Automatic merge from submit-queue (batch tested with PRs 67776, 67503, 67679, 67786, 67830). 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>. kubeadm: use ClusterConfiguration in images.go **What this PR does / why we need it**: This PR is the first in a series, targeting the replacement of InitConfiguration with ClusterConfiguration, when the former is not needed. Please, review only the last commit. Replace the unnecessary use of InitConfiguration in images.go with ClusterConfiguration. This changes the interfaces of the following functions: - GetKubeControlPlaneImage - GetEtcdImage - GetAllImages **Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*: refs kubernetes/kubeadm#963 **Special notes for your reviewer**: /cc @kubernetes/sig-cluster-lifecycle-pr-reviews /area kubeadm /kind enhancement /assign @luxas /assign @timothysc /assign @fabriziopandini Depends on: - [X] kubernetes#67441 **Release note**: ```release-note NONE ```

k8s-ci-robot requested review from fabriziopandini and xiangpengzhao August 15, 2018 17:02

k8s-ci-robot added area/kubeadm release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Aug 15, 2018

k8s-ci-robot assigned fabriziopandini and timothysc Aug 15, 2018

k8s-ci-robot assigned luxas Aug 15, 2018

fabriziopandini mentioned this pull request Aug 16, 2018

kubeadm: Split out ClusterConfiguration from InitConfiguration #66219

Closed

9 tasks

rosti force-pushed the kubeadm_clusterconfig branch 2 times, most recently from 97dcee5 to d3e7de0 Compare August 16, 2018 10:26

rosti mentioned this pull request Aug 16, 2018

kubeadm: use ClusterConfiguration in images.go #67503

Merged

1 task

fabriziopandini reviewed Aug 17, 2018

View reviewed changes

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

neolit123 mentioned this pull request Aug 17, 2018

Tracking issue for "Config to v1beta1" kubernetes/kubeadm#963

Closed

28 tasks

rosti force-pushed the kubeadm_clusterconfig branch from d3e7de0 to fc664de Compare August 17, 2018 13:59

rosti mentioned this pull request Aug 17, 2018

kubeadm: Migrate cert and PKI helpers to ClusterConfiguration #67542

Closed

2 tasks

timothysc reviewed Aug 20, 2018

View reviewed changes

rosti mentioned this pull request Aug 21, 2018

Re-enable fuzzer round trip tests kubernetes/kubeadm#1070

Closed

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

luxas added 2 commits August 22, 2018 11:43

kubeadm: Split out ClusterConfiguration from InitConfiguration

5224551

Trivial rebasement, fixed some broken tests, and inserted some TODOs: Rostislav M. Georgiev <rostislavg@vmware.com>

update some stuff

0fde05a

rosti force-pushed the kubeadm_clusterconfig branch from fc664de to 0fde05a Compare August 22, 2018 08:44

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

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

k8s-github-robot merged commit 51605c3 into kubernetes:master Aug 22, 2018

rosti deleted the kubeadm_clusterconfig branch November 22, 2018 15:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[reissue] kubeadm: Split out ClusterConfiguration from InitConfiguration #67441

[reissue] kubeadm: Split out ClusterConfiguration from InitConfiguration #67441

rosti commented Aug 15, 2018 •

edited

rosti commented Aug 15, 2018

rosti commented Aug 15, 2018

neolit123 commented Aug 15, 2018 •

edited

fabriziopandini left a comment

chuckha commented Aug 17, 2018

timothysc left a comment

timothysc Aug 20, 2018

timothysc Aug 20, 2018

rosti Aug 22, 2018

fabriziopandini commented Aug 21, 2018 •

edited

timothysc commented Aug 21, 2018

rosti commented Aug 22, 2018

fabriziopandini commented Aug 22, 2018

k8s-github-robot commented Aug 22, 2018

timothysc commented Aug 22, 2018

k8s-ci-robot commented Aug 22, 2018

k8s-github-robot commented Aug 22, 2018

[reissue] kubeadm: Split out ClusterConfiguration from InitConfiguration #67441

[reissue] kubeadm: Split out ClusterConfiguration from InitConfiguration #67441

Conversation

rosti commented Aug 15, 2018 • edited

rosti commented Aug 15, 2018

rosti commented Aug 15, 2018

neolit123 commented Aug 15, 2018 • edited

fabriziopandini left a comment

Choose a reason for hiding this comment

chuckha commented Aug 17, 2018

timothysc left a comment

Choose a reason for hiding this comment

timothysc Aug 20, 2018

Choose a reason for hiding this comment

timothysc Aug 20, 2018

Choose a reason for hiding this comment

rosti Aug 22, 2018

Choose a reason for hiding this comment

fabriziopandini commented Aug 21, 2018 • edited

timothysc commented Aug 21, 2018

rosti commented Aug 22, 2018

fabriziopandini commented Aug 22, 2018

k8s-github-robot commented Aug 22, 2018

timothysc commented Aug 22, 2018

k8s-ci-robot commented Aug 22, 2018

k8s-github-robot commented Aug 22, 2018

rosti commented Aug 15, 2018 •

edited

neolit123 commented Aug 15, 2018 •

edited

fabriziopandini commented Aug 21, 2018 •

edited