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

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

Merged
merged 2 commits into from Nov 12, 2018

Conversation

@fabriziopandini
Copy link
Contributor

fabriziopandini commented Oct 19, 2018

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

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Oct 19, 2018

@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

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
Copy link
Member

neolit123 left a comment

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

overall, i think this is fine.

Show resolved Hide resolved cmd/kubeadm/app/apis/kubeadm/v1beta1/types.go Outdated

// Proxy defines the options for the proxier add-on installed in the cluster.
// Configurability of the proxy can be achieved via component config as well.
Proxy Proxy `json:"dns"`

This comment has been minimized.

@neolit123

neolit123 Oct 19, 2018

Member

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.

This comment has been minimized.

@timothysc

timothysc Oct 29, 2018

Member

json:"proxy"

Show resolved Hide resolved cmd/kubeadm/app/apis/kubeadm/v1beta1/types.go Outdated
// add-on type is kube-dns).
// If empty, kubeadm will automatically select CoreDNS image using image repository and the supported CoreDNS version.
// In case this value is set, kubeadm does not change automatically the CoreDNS version during upgrades.
Image string `json:"image,omitempty"`

This comment has been minimized.

@neolit123

neolit123 Oct 19, 2018

Member

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
Copy link
Contributor

rosti left a comment

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

Show resolved Hide resolved cmd/kubeadm/app/apis/kubeadm/v1beta1/types.go Outdated

// ImageRepository, if set this fields allows to override the default image repository
// defined in ClusterConfiguration
ImageRepository string `json:"imageRepository,omitempty"`

This comment has been minimized.

@rosti

rosti Oct 20, 2018

Contributor

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).

This comment has been minimized.

@fabriziopandini

fabriziopandini Oct 29, 2018

Contributor

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

This comment has been minimized.

@timothysc

timothysc Oct 29, 2018

Member

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

Show resolved Hide resolved cmd/kubeadm/app/apis/kubeadm/v1beta1/types.go Outdated
type Proxy struct {
// ImageRepository, if set this fields allows to override the default image repository
// defined in ClusterConfiguration
ImageRepository string `json:"imageRepository,omitempty"`

This comment has been minimized.

@rosti

rosti Oct 20, 2018

Contributor

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

// ExtraArgs is a set of extra flags to pass to the dns add-on or override
// default ones in form of <flagname>=<value>
// TODO: Ideally we would like to switch all components to use ComponentConfig + ConfigMaps.
ExtraArgs map[string]string `json:"extraArgs,omitempty"`

This comment has been minimized.

@rosti

rosti Oct 20, 2018

Contributor

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

This comment has been minimized.

@fabriziopandini

fabriziopandini Oct 29, 2018

Contributor

This was discussed and punted out ...

@@ -106,12 +106,23 @@ type ClusterConfiguration struct {
// CertificatesDir specifies where to store or look for all required certificates.
CertificatesDir string `json:"certificatesDir"`

// ImageRepository what container registry to pull control plane images from
// ImageRepository what container registry to pull control plane images from.

This comment has been minimized.

@rosti

rosti Oct 20, 2018

Contributor

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.

type Proxy struct {
// ImageRepository, if set this fields allows to override the default image repository
// defined in ClusterConfiguration
ImageRepository string `json:"imageRepository,omitempty"`

This comment has been minimized.

@timothysc

timothysc Oct 23, 2018

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

almost all seem pretty ImageMeta

This comment has been minimized.

@fabriziopandini

fabriziopandini Oct 29, 2018

Contributor

@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

This comment has been minimized.

@timothysc

timothysc Oct 29, 2018

Member

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

@timothysc
Copy link
Member

timothysc left a comment

Similar comments to a different PR.


// Proxy defines the options for the proxier add-on installed in the cluster.
// Configurability of the proxy can be achieved via component config as well.
Proxy Proxy `json:"dns"`

This comment has been minimized.

@timothysc

timothysc Oct 29, 2018

Member

json:"proxy"

CoreDNS DNSAddOnType = "CoreDNS"

// KubeDNS add-on type
KubeDNS DNSAddOnType = "kube-dns"

This comment has been minimized.

@timothysc

timothysc Oct 29, 2018

Member

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.

This comment has been minimized.

@luxas

luxas Nov 8, 2018

Member

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


// ImageRepository, if set this fields allows to override the default image repository
// defined in ClusterConfiguration
ImageRepository string `json:"imageRepository,omitempty"`

This comment has been minimized.

@timothysc

timothysc Oct 29, 2018

Member

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

@fabriziopandini fabriziopandini force-pushed the fabriziopandini:kubeadm-addons-config branch from d0391cb to e412d24 Nov 7, 2018

@k8s-ci-robot k8s-ci-robot added size/XL and removed size/M labels Nov 7, 2018

@fabriziopandini fabriziopandini force-pushed the fabriziopandini:kubeadm-addons-config branch from e412d24 to 7222325 Nov 7, 2018

@fabriziopandini 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

@fabriziopandini

This comment has been minimized.

Copy link
Contributor

fabriziopandini commented Nov 7, 2018

@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
Copy link
Member

neolit123 left a comment

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

can have a look again tomorrow.


// ImageTag allows to specify a tag for the image.
// In case this value is set, kubeadm does not change automatically the version of the above components during upgrades.
ImageTag string `json:"imageTag,omitempty"`

This comment has been minimized.

@neolit123

neolit123 Nov 8, 2018

Member

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.

@@ -53,12 +53,15 @@ var storeCertsInSecretsDeprecationMessage = "featureGates:StoreCertsInSecrets ha
var highAvailabilityMessage = "featureGates:HighAvailability has been removed in v1.12\n" +
"\tThis feature has been replaced by the kubeadm join --control-plane workflow."

var coreDNSMessage = "featureGates:CoreDNS has been removed in v1.13\n" +
"\tUse kubeadm-config to select which DND add-on to install."

This comment has been minimized.

@neolit123

neolit123 Nov 8, 2018

Member

DND add-on -> DNS addon

@rosti

rosti approved these changes Nov 8, 2018

Copy link
Contributor

rosti left a comment

@fabriziopandini looks great! Thanks for this!

if out.FeatureGates == nil {
out.FeatureGates = map[string]bool{}
}
if in.DNS.Type == kubeadm.KubeDNS {

This comment has been minimized.

@rosti

rosti Nov 8, 2018

Contributor

I'd rather have this as just:

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

func imageToImageMeta(image string) (kubeadm.ImageMeta, error) {

This comment has been minimized.

@rosti

rosti Nov 8, 2018

Contributor

Probably change this to etcdImageToImageMeta?

@luxas

luxas approved these changes Nov 8, 2018

Copy link
Member

luxas left a comment

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

CoreDNS DNSAddOnType = "CoreDNS"

// KubeDNS add-on type
KubeDNS DNSAddOnType = "kube-dns"

This comment has been minimized.

@luxas

luxas Nov 8, 2018

Member

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

// InitFeatureGates are the default feature gates for the init command
var InitFeatureGates = FeatureList{
SelfHosting: {FeatureSpec: utilfeature.FeatureSpec{Default: false, PreRelease: utilfeature.Deprecated}, HiddenInHelpText: true, DeprecationMessage: selfHostingDeprecationMessage},
StoreCertsInSecrets: {FeatureSpec: utilfeature.FeatureSpec{Default: false, PreRelease: utilfeature.Deprecated}, HiddenInHelpText: true, DeprecationMessage: storeCertsInSecretsDeprecationMessage},
HighAvailability: {FeatureSpec: utilfeature.FeatureSpec{Default: false, PreRelease: utilfeature.Deprecated}, HiddenInHelpText: true, DeprecationMessage: highAvailabilityMessage},
CoreDNS: {FeatureSpec: utilfeature.FeatureSpec{Default: true, PreRelease: utilfeature.GA}},
CoreDNS: {FeatureSpec: utilfeature.FeatureSpec{Default: true, PreRelease: utilfeature.Deprecated}, HiddenInHelpText: true, DeprecationMessage: coreDNSMessage},

This comment has been minimized.

@luxas

luxas Nov 8, 2018

Member

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

if featureName == string(k) {
return true
return v.PreRelease != utilfeature.Deprecated

This comment has been minimized.

@luxas

luxas Nov 8, 2018

Member

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

}

// If we're dry-running, we don't need to wait for the new DNS addon to become ready
if !dryRun {
dnsDeployment, err := client.AppsV1().Deployments(metav1.NamespaceSystem).Get(installedDeploymentName, metav1.GetOptions{})
dnsDeployment, err := client.AppsV1().Deployments(metav1.NamespaceSystem).Get(string(installedDeploymentName), metav1.GetOptions{})

This comment has been minimized.

@luxas

luxas Nov 8, 2018

Member

doesn't need conversion I think

@@ -152,7 +151,7 @@ func removeOldDNSDeploymentIfAnotherDNSIsUsed(cfg *kubeadmapi.InitConfiguration,

// We don't want to wait for the DNS deployment above to become ready when dryrunning (as it never will)
// but here we should execute the DELETE command against the dryrun clientset, as it will only be logged
err := apiclient.DeleteDeploymentForeground(client, metav1.NamespaceSystem, deploymentToDelete)
err := apiclient.DeleteDeploymentForeground(client, metav1.NamespaceSystem, string(deploymentToDelete))

This comment has been minimized.

@luxas

luxas Nov 8, 2018

Member

doesn't need conversion

@k8s-ci-robot k8s-ci-robot added the lgtm label Nov 8, 2018

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Nov 8, 2018

[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

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

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Nov 9, 2018

/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

This comment has been minimized.

Copy link
Contributor

fabriziopandini commented Nov 10, 2018

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

@fabriziopandini 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 fabriziopandini force-pushed the fabriziopandini:kubeadm-addons-config branch from 254f1b6 to dc3c01c Nov 10, 2018

fabriziopandini added some commits Nov 10, 2018

@fabriziopandini fabriziopandini force-pushed the fabriziopandini:kubeadm-addons-config branch from dc3c01c to 446d806 Nov 10, 2018

@fabriziopandini

This comment has been minimized.

Copy link
Contributor

fabriziopandini commented Nov 10, 2018

/retest

@timothysc
Copy link
Member

timothysc left a comment

reapplying
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Nov 12, 2018

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

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Nov 12, 2018

/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 k8s-ci-robot merged commit 5fb8229 into kubernetes:master Nov 12, 2018

18 checks passed

cla/linuxfoundation fabriziopandini 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-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-e2e-kubeadm-gce 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

@fabriziopandini fabriziopandini deleted the fabriziopandini:kubeadm-addons-config branch Dec 15, 2018

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