kubeadm - addon configuration in the kubeadm config API. #70024

fabriziopandini · 2018-10-19T14:59:57Z

What this PR does / why we need it:
This PR introduces addon configuration in the kubeadm config API; as a consequence kubeadm feature flag CoreDNS is deprecated.

Which issue(s) this PR fixes :

Special notes for your reviewer:
This is a WIP, aimed at collecting feedbacks on the API design; since addon configuration includes image configuration, I took a look a 360° look at this topic and reviewed comments accordingly

/sig cluster-lifecycle
/kind documentation
/assign @timothysc
/assign @dtieber
/cc @rosti
@kubernetes/sig-cluster-lifecycle-pr-reviews
@neolit123
@chuckha

Release note:

Addon configuration is introduced in the kubeadm config API, while feature flag CoreDNS is now deprecated.

k8s-ci-robot · 2018-10-19T15:00:00Z

@fabriziopandini: GitHub didn't allow me to assign the following users: dtieber.

Note that only kubernetes members and repo collaborators can be assigned.
For more information please see the contributor guide

Details

In response to this:

What this PR does / why we need it:
This PR introduces add-on configuration in the kubeadm config API; as a consequence kubeadm feature flag CoreDNS is deprecated.

Which issue(s) this PR fixes :

Special notes for your reviewer:
This is a WIP, aimed at collecting feedbacks on the API design; since add-on configuration includes image configuration, I took a look a 360° look at this topic and reviewed comments accordingly

/sig cluster-lifecycle
/kind documentation
/assign @timothysc
/assign @dtieber
/cc @rosti
@kubernetes/sig-cluster-lifecycle-pr-reviews
@neolit123
@chuckha

Release note:
Add-on configuration is introduced in the kubeadm config API, while feature flag CoreDNS is now deprecated.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

neolit123

@fabriziopandini thanks.
added some comments + questions on which we can have a discussion.

overall, i think this is fine.

cmd/kubeadm/app/apis/kubeadm/v1beta1/types.go

neolit123 · 2018-10-19T19:32:55Z

cmd/kubeadm/app/apis/kubeadm/v1beta1/types.go

my original idea was to have these under a parent Addons sub-struct or a list, but this is fine too.
...unless if we decide at some point to add a bunch more core addons, which will then leave all of them floating in the root config.

json:"proxy"

cmd/kubeadm/app/apis/kubeadm/v1beta1/types.go

neolit123 · 2018-10-19T19:44:32Z

cmd/kubeadm/app/apis/kubeadm/v1beta1/types.go

it might be a good idea for us to outline that a tag is included here as part of Image.
e.g. Image = "image:tag".

we might get some reports in the lines of "hey, my custom coredns version upgrade is not working correctly", but that would be a side effect of the flexibility here, so it's a trade off.

rosti

Thanks @fabriziopandini ! Definitely a step in the right direction.

cmd/kubeadm/app/apis/kubeadm/v1beta1/types.go

rosti · 2018-10-20T13:00:41Z

cmd/kubeadm/app/apis/kubeadm/v1beta1/types.go

Not sure of what will be the use case of setting this to a different value (than ClusterConfiguration.ImageRepository, but not setting Image here directly).

by setting ImageRepository you can do kubeadm upgrade, while image binds you to an image:tag

@fabriziopandini I think we should create an ImageMeta as mentioned above

cmd/kubeadm/app/apis/kubeadm/v1beta1/types.go

rosti · 2018-10-20T13:31:59Z

cmd/kubeadm/app/apis/kubeadm/v1beta1/types.go

Not sure if this is reasonable for kube-proxy. But it's ok, as long as the same K8s version is used.

rosti · 2018-10-20T13:34:09Z

cmd/kubeadm/app/apis/kubeadm/v1beta1/types.go

We may have to have a version of ExtraArgs on a per-node basis...

This was discussed and punted out ...

rosti · 2018-10-20T13:36:16Z

cmd/kubeadm/app/apis/kubeadm/v1beta1/types.go

This should probably now state ... to pull images from. (remove the control plane part of it). Since this could be used for kube-proxy and pause images, which run on worker nodes too.

timothysc · 2018-10-23T02:16:29Z

cmd/kubeadm/app/apis/kubeadm/v1beta1/types.go

ImageRepository string `json:"imageRepository,omitempty"` Image string `json:"image,omitempty"` ExtraArgs map[string]string `json:"extraArgs,omitempty"`

almost all seem pretty ImageMeta

@timothysc if I got your point here it can work but we need to have CoreImageMeta (where only ExtraArgs override makes sense) and ExternalImageMeta (where it is possible to override imageRepository or image:tag).
WDYT?

Another detail to be addressed is the DNS substruct that that can be both a "coreComponent" (kube-dns) and an "externalComponent" (core-dns)... might be it is better to have different structs

I'm not following, I don't see a distinction between the two from the fields they would provide.

timothysc

Similar comments to a different PR.

timothysc · 2018-10-29T16:23:01Z

cmd/kubeadm/app/apis/kubeadm/v1beta1/types.go

json:"proxy"

timothysc · 2018-10-29T16:24:44Z

cmd/kubeadm/app/apis/kubeadm/v1beta1/types.go

blarg.. I need to talk with @kubernetes/sig-network-misc @thockin what is the deprecation plan?

I was hoping we could finally rid ourselves of this tech-debt.

kube-dns is GA and is gonna be so for a long time, we need to support it although it's not fun

timothysc · 2018-10-29T16:26:42Z

cmd/kubeadm/app/apis/kubeadm/v1beta1/types.go

@fabriziopandini I think we should create an ImageMeta as mentioned above

fabriziopandini · 2018-11-07T22:37:23Z

@timothysc @rosti @luxas @neolit123
Now this PR is aligned with what discussed today in the kubeadm office hour.
I still have to do some manual test on the upgrade part, but it will be great if you can give a first pass

neolit123

thanks for the update @fabriziopandini
went through the whole change and added only two comments.

can have a look again tomorrow.

neolit123 · 2018-11-07T23:50:58Z

cmd/kubeadm/app/apis/kubeadm/v1beta1/types.go

the k8s-infra team has plans to make architectures as part of the image tag for future releases.
e.g. pause:3.1-arm64
possibly platforms too e.g. pause:3.1-adm64-windows

just FYI, i think i don't see a problem with having the ImageTag like that.

neolit123 · 2018-11-07T23:59:46Z

cmd/kubeadm/app/features/features.go

DND add-on -> DNS addon

rosti

@fabriziopandini looks great! Thanks for this!

rosti · 2018-11-08T12:52:42Z

cmd/kubeadm/app/apis/kubeadm/v1alpha3/conversion.go

I'd rather have this as just:

out.FeatureGates["CoreDNS"] = (in.DNS.Type == kubeadm.CoreDNS)

rosti · 2018-11-08T12:55:20Z

cmd/kubeadm/app/apis/kubeadm/v1alpha3/conversion.go

Probably change this to etcdImageToImageMeta?

luxas

Thanks, good work!
Left some comment, the main one that we can't break the CLI, so deprecated feature gates can't error in validation. However, as agreed, I'll LGTM this now, and you'll fix up the deprecated feature gates' expected behavior later as a standalone change (this due also that it touches other deprecated feature gates, not just this one)
Needs a rebase though

/approve
/lgtm

luxas · 2018-11-08T21:26:04Z

cmd/kubeadm/app/apis/kubeadm/v1beta1/types.go

kube-dns is GA and is gonna be so for a long time, we need to support it although it's not fun

luxas · 2018-11-08T21:32:02Z

cmd/kubeadm/app/features/features.go

I'd still mark this GA, but deprecate and hide it

luxas · 2018-11-08T21:42:13Z

cmd/kubeadm/app/features/features.go

Let's change this to output the deprecation warning or something instead if deprecated. If this check returns false, validation is gonna error, which is something we can't let happen in this release

luxas · 2018-11-08T21:48:09Z

cmd/kubeadm/app/phases/upgrade/postupgrade.go

doesn't need conversion I think

luxas · 2018-11-08T21:48:16Z

cmd/kubeadm/app/phases/upgrade/postupgrade.go

doesn't need conversion

k8s-ci-robot · 2018-11-08T21:52:33Z

[APPROVALNOTIFIER] This PR is APPROVED

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

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

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

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

fejta-bot · 2018-11-09T00:08:33Z

/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 comment for consistent failures.

fabriziopandini · 2018-11-10T08:54:58Z

opened kubernetes/kubeadm#1226 for tracking implementation of requested feature gate warning as per @luxas comment

fabriziopandini · 2018-11-10T20:35:46Z

/retest

timothysc

reapplying
/lgtm

fejta-bot · 2018-11-12T07:14:04Z

/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 comment for consistent failures.

k8s-ci-robot assigned timothysc Oct 19, 2018

k8s-ci-robot requested a review from rosti October 19, 2018 15:00

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubeadm labels Oct 19, 2018

neolit123 reviewed Oct 19, 2018

View reviewed changes

rosti reviewed Oct 20, 2018

View reviewed changes

timothysc suggested changes Oct 23, 2018

View reviewed changes

timothysc reviewed Oct 29, 2018

View reviewed changes

k8s-ci-robot added sig/network Categorizes an issue or PR as relevant to SIG Network. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 29, 2018

fabriziopandini force-pushed the kubeadm-addons-config branch from d0391cb to e412d24 Compare November 7, 2018 21:46

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 7, 2018

fabriziopandini force-pushed the kubeadm-addons-config branch from e412d24 to 7222325 Compare November 7, 2018 22:22

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

fabriziopandini changed the title ~~[WIP] kubeadm - add-on configuration in the kubeadm config API.~~ kubeadm - add-on configuration in the kubeadm config API. Nov 7, 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 Nov 7, 2018

neolit123 reviewed Nov 8, 2018

View reviewed changes

rosti approved these changes Nov 8, 2018

View reviewed changes

k8s-ci-robot assigned luxas Nov 8, 2018

luxas approved these changes Nov 8, 2018

View reviewed changes

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

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

fabriziopandini force-pushed the kubeadm-addons-config branch from 7222325 to 254f1b6 Compare November 9, 2018 22:10

k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 9, 2018

fabriziopandini mentioned this pull request Nov 10, 2018

Give warning for deprecated featureGate flags kubernetes/kubeadm#1226

Closed

fabriziopandini changed the title ~~kubeadm - add-on configuration in the kubeadm config API.~~ kubeadm - addon configuration in the kubeadm config API. Nov 10, 2018

fabriziopandini force-pushed the kubeadm-addons-config branch from 254f1b6 to dc3c01c Compare November 10, 2018 14:54

fabriziopandini added 2 commits November 10, 2018 17:27

add DNS struct to kubeadm config

6759334

autogenerated

446d806

fabriziopandini force-pushed the kubeadm-addons-config branch from dc3c01c to 446d806 Compare November 10, 2018 16:28

timothysc approved these changes Nov 12, 2018

View reviewed changes

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

timothysc added this to the v1.13 milestone Nov 12, 2018

k8s-ci-robot merged commit 5fb8229 into kubernetes:master Nov 12, 2018

fabriziopandini deleted the kubeadm-addons-config branch December 15, 2018 08:29

kubeadm - addon configuration in the kubeadm config API. #70024

kubeadm - addon configuration in the kubeadm config API. #70024

Uh oh!

Conversation

fabriziopandini commented Oct 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Oct 19, 2018

Uh oh!

neolit123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rosti left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timothysc Oct 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timothysc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timothysc Oct 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabriziopandini commented Nov 7, 2018

Uh oh!

neolit123 left a comment

Choose a reason for hiding this comment

Uh oh!

neolit123 Nov 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rosti left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luxas left a comment

Choose a reason for hiding this comment

fabriziopandini commented Oct 19, 2018 •

edited

Loading

timothysc Oct 29, 2018 •

edited

Loading

timothysc Oct 29, 2018 •

edited

Loading

neolit123 Nov 7, 2018 •

edited

Loading