kubeadm: Restructure internal config usage and fix bugs #63799

luxas · 2018-05-14T13:49:45Z

What this PR does / why we need it:

Moves the generic LoadYAML function from the versioned, external API package to a helper library so it can be consumed more easily
Makes the upgrading code use the internal version of the API (which always should be used anyway)
Moves all config-loading code to configutil, together with the migration code needed. This way we have everything in one centralized place, instead of duplicating that logic N times.
Makes kubeadm init use configutil for the reasons mentioned above.

This PR is needed in order to support multiple external API groups (like v1alpha2)

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

Special notes for your reviewer:
This PR depends on:

Please review only the last (third) commit

Release note:

NONE

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

liztio · 2018-05-14T17:07:31Z

Needs a ./hack/update-bazel.sh looks like

timothysc

Started review but stopped b/c of alias naming. Avoid alias version naming b/c, that will just be extra work in the future with 0 gain.

timothysc · 2018-05-14T19:37:33Z

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

 	"k8s.io/apimachinery/pkg/runtime"
 	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
 	"k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm"
+	kubeadmfuzzer "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm/fuzzer"


alias seems unnecessary.

timothysc · 2018-05-14T19:39:17Z

cmd/kubeadm/app/cmd/config.go

@@ -29,7 +29,8 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	clientset "k8s.io/client-go/kubernetes"
 	kubeadmapi "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm"
-	kubeadmapiext "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm/v1alpha1"
+	kubeadmscheme "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm/scheme"
+	kubeadmapiv1alpha1 "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm/v1alpha1"


I'd just call it kubeadmapi for future proofing the code in some spots.

Yeah this is a lesson from the client wrapping, avoid version naming, b/c every update is useless churn.

neolit123 · 2018-05-15T13:30:26Z

upgrade_test.go needs LoadYAML() from the util package now too.

…ternal version of the API internally. Fix bugs

liztio · 2018-05-15T14:51:39Z

@luxas it looks like there's some overlap between this and #63788?

luxas · 2018-05-15T14:56:22Z

@liztio The commit here is included in #63788. #63788 depends on this PR

liztio · 2018-05-15T18:46:19Z

cmd/kubeadm/app/apis/kubeadm/v1alpha1/upgrade_test.go

 )

-const test196 = "testdata/kubeadm196.yaml"


Where'd this test end up?

Ugh. I moved it to testdata but somehow I didn't commit it. I'll send a non-critical PR with that test later

liztio · 2018-05-15T20:00:38Z

/lgtm

timothysc

/lgtm
/approve

k8s-ci-robot · 2018-05-15T20:27:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liztio, luxas, 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~~ [luxas,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-05-16T02:55:10Z

Automatic merge from submit-queue (batch tested with PRs 63314, 63884, 63799, 63521, 62242). If you want to cherry-pick this change to another branch, please follow the instructions here.

@liztio

Automatic merge from submit-queue. 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: Add (duplicated) v1alpha2 Config API **What this PR does / why we need it**: Work in progress PR to add a (initially duplicated) `v1alpha2` we can iterate on during the cycle. **Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*: Part of kubernetes/community#2131 **Special notes for your reviewer**: This PR depends on: - [x] #63782 - [x] #63783 - [x] #63787 - [x] #63799 The first commit is from #63799. The second commit duplicates v1alpha1, but updates timestamps, and doesn't require the `upgrade.go`. The third commit does the mechanical bump of using v1alpha1 -> v1alpha2 The fourth commit updates bazel **Release note**: ```release-note [action required] kubeadm now uses an upgraded API version for the configuration file, `kubeadm.k8s.io/v1alpha2`. kubeadm in v1.11 will still be able to read `v1alpha1` configuration, and will automatically convert the configuration to `v1alpha2` internally and when storing the configuration in the ConfigMap in the cluster. ``` @kubernetes/sig-cluster-lifecycle-pr-reviews @liztio

@dims

Automatic merge from submit-queue. 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: Remove the `.CloudProvider` and `.PrivilegedPods` configuration option **What this PR does / why we need it**: Removes the `.CloudProvider` option, it has been experimental for a long time. People should now use external cloud providers, which is beta in v1.11. Most importantly, you can get the exact same behavior in the API by utilizing the `.*ExtraArgs` and `.*ExtraVolumes` fields. Removes `.PrivilegedPods` as that serves a super small edge case with the legacy cloud provider, and only for openstack. **Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*: Part of kubernetes/community#2131 **Special notes for your reviewer**: Depends on PRs: - [x] #63799 - [x] #63788 **Release note**: ```release-note [action required] In the new v1alpha2 kubeadm Configuration API, the `.CloudProvider` and `.PrivilegedPods` fields don't exist anymore. Instead, you should use the out-of-tree cloud provider implementations which are beta in v1.11. If you have to use the legacy in-tree cloud providers, you can rearrange your config like the example below. If you need to use the `.PrivilegedPods` functionality, you can still edit the manifests in `/etc/kubernetes/manifests/`, and set `.SecurityContext.Privileged=true` for the apiserver and controller manager. --- kind: MasterConfiguration apiVersion: kubeadm.k8s.io/v1alpha2 apiServerExtraArgs: cloud-provider: "{cloud}" cloud-config: "{path}" apiServerExtraVolumes: - name: cloud hostPath: "{path}" mountPath: "{path}" controllerManagerExtraArgs: cloud-provider: "{cloud}" cloud-config: "{path}" controllerManagerExtraVolumes: - name: cloud hostPath: "{path}" mountPath: "{path}" --- ``` @kubernetes/sig-cluster-lifecycle-pr-reviews @dims @liztio

@liztio

Automatic merge from submit-queue (batch tested with PRs 63871, 63927, 63966, 63957, 63844). 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: Remove the never-used .Etcd.SelfHosted field **What this PR does / why we need it**: These API types were added to support the self-hosting etcd feature, which in the end never was merged. Hence these API types are unused and should be removed. Perfect timing to do that is now in our new `v1alpha2` scheme. **Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*: Part of kubernetes/community#2131 **Special notes for your reviewer**: Depends on PRs: - [x] #63799 - [x] #63788 **Release note**: ```release-note kubeadm has removed `.Etcd.SelfHosting` from its configuration API. It was never used in practice. ``` @kubernetes/sig-cluster-lifecycle-pr-reviews @liztio

@liztio

Automatic merge from submit-queue. 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>. Add roundtrip, defaulting, upgrading and validation unit tests for the kubeadm API types **What this PR does / why we need it**: Follows up from #63799, as well as net-new unit testing for our serialization/deserialization package. This tests our API machinery pretty much end to end. This is more important now given we now support two external types: #63788 **Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*: Part of kubernetes/community#2131 **Special notes for your reviewer**: **Release note**: ```release-note NONE ``` @kubernetes/sig-cluster-lifecycle-pr-reviews @liztio

k8s-ci-robot requested review from fabriziopandini and stealthybox May 14, 2018 13:49

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubeadm labels May 14, 2018

luxas assigned timothysc May 14, 2018

luxas mentioned this pull request May 14, 2018

kubeadm: Add (duplicated) v1alpha2 Config API #63788

Merged

4 tasks

luxas mentioned this pull request May 14, 2018

Review how kubeadm parses yaml and converts resources to internal API objects kubernetes/kubeadm#322

Closed

timothysc suggested changes May 14, 2018

View reviewed changes

luxas force-pushed the kubeadm_restructure_internal_config_usage branch from 70a7a83 to 0833705 Compare May 15, 2018 13:15

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 15, 2018

luxas force-pushed the kubeadm_restructure_internal_config_usage branch from 0833705 to c93de69 Compare May 15, 2018 13:27

luxas mentioned this pull request May 15, 2018

kubeadm: Remove the .CloudProvider and .PrivilegedPods configuration option #63866

Merged

2 tasks

kubeadm: Handle config loading only in one place, and only use the in…

f95e63c

…ternal version of the API internally. Fix bugs

luxas force-pushed the kubeadm_restructure_internal_config_usage branch from c93de69 to b8c19e0 Compare May 15, 2018 14:44

This was referenced May 15, 2018

kubeadm: Remove .ClusterName but add .Metadata to the config API #63869

Closed

kubeadm: Remove the never-used .Etcd.SelfHosted field #63871

Merged

autogenerated bazel

cae656b

luxas force-pushed the kubeadm_restructure_internal_config_usage branch from b8c19e0 to cae656b Compare May 15, 2018 15:37

liztio reviewed May 15, 2018

View reviewed changes

k8s-ci-robot assigned liztio May 15, 2018

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

timothysc approved these changes May 15, 2018

View reviewed changes

k8s-github-robot merged commit 2e61da1 into kubernetes:master May 16, 2018

luxas mentioned this pull request May 16, 2018

Add roundtrip, defaulting, upgrading and validation unit tests for the kubeadm API types #63917

Merged

luxas mentioned this pull request Jun 12, 2018

v1alpha2 versioning for kubeadm configuration structs kubernetes/kubeadm#750

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubeadm: Restructure internal config usage and fix bugs #63799

kubeadm: Restructure internal config usage and fix bugs #63799

luxas commented May 14, 2018 •

edited

Loading

liztio commented May 14, 2018

timothysc left a comment

timothysc May 14, 2018

timothysc May 14, 2018

timothysc May 14, 2018

neolit123 commented May 15, 2018

liztio commented May 15, 2018

luxas commented May 15, 2018

liztio May 15, 2018

luxas May 15, 2018

liztio commented May 15, 2018

timothysc left a comment

k8s-ci-robot commented May 15, 2018

k8s-github-robot commented May 16, 2018

kubeadm: Restructure internal config usage and fix bugs #63799

kubeadm: Restructure internal config usage and fix bugs #63799

Conversation

luxas commented May 14, 2018 • edited Loading

liztio commented May 14, 2018

timothysc left a comment

Choose a reason for hiding this comment

timothysc May 14, 2018

Choose a reason for hiding this comment

timothysc May 14, 2018

Choose a reason for hiding this comment

timothysc May 14, 2018

Choose a reason for hiding this comment

neolit123 commented May 15, 2018

liztio commented May 15, 2018

luxas commented May 15, 2018

liztio May 15, 2018

Choose a reason for hiding this comment

luxas May 15, 2018

Choose a reason for hiding this comment

liztio commented May 15, 2018

timothysc left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented May 15, 2018

k8s-github-robot commented May 16, 2018

luxas commented May 14, 2018 •

edited

Loading