Skip to content
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 - default CoreDNS FeatureGate to true #63509

Merged
merged 2 commits into from May 15, 2018

Conversation

@detiber
Copy link
Member

detiber commented May 7, 2018

What this PR does / why we need it:

This PR updates kubeadm to deploy CoreDNS rather than KubeDNS by default for new installs.

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

Release note:

kubeadm will now deploy CoreDNS by default instead of KubeDNS

Currently, this does not effect upgrades. Also, documentation updates will need to be coordinated with this change.

@detiber

This comment has been minimized.

Copy link
Member Author

detiber commented May 7, 2018

/assign @timothysc
/assign @luxas

@detiber

This comment has been minimized.

Copy link
Member Author

detiber commented May 7, 2018

@detiber

This comment has been minimized.

Copy link
Member Author

detiber commented May 7, 2018

/test pull-kubernetes-e2e-kops-aws

@detiber

This comment has been minimized.

Copy link
Member Author

detiber commented May 8, 2018

/retest

_, exists := featureGate[fgKey]
if !exists {
if fgValue.Default {
featureGate[fgKey] = true

This comment has been minimized.

@neolit123

neolit123 May 8, 2018

Member

@detiber

Jason, wouldn't the above append all feature gates that the user didn't specify on the command line and set them to true? say if DynamicKubeletConfig was not set by the user this would override the default from false to true.

please, correct me if i'm wrong.

This comment has been minimized.

@detiber

detiber May 8, 2018

Author Member

@neolit123 It will only set the ones that have a default value of true. Without this, updating the default value of the feature has no effect. That said, I'm not a fan of this hack, and will take another look today to see if there is a better way to bubble up the feature default value :)

This comment has been minimized.

@timothysc

timothysc May 8, 2018

Member

This is super weird... Why wouldn't we populate the struct from the flag itself?

Is this an artifact of feature-gate mimicking.

This comment has been minimized.

@detiber

detiber May 9, 2018

Author Member

I've changed course on this and found a better fix that also does the proper thing for upgrade. I'm still working on fixing the plan and upgrade tests to handle it properly, though

@detiber detiber force-pushed the detiber:CoreDNSDefault branch from e2c7f30 to f26c5fe May 9, 2018

@k8s-ci-robot k8s-ci-robot added size/L and removed size/S labels May 9, 2018

if enabled, ok := featureList[string(featureName)]; ok {
return enabled
}
return InitFeatureGates[string(featureName)].Default
}

This comment has been minimized.

@neolit123

neolit123 May 9, 2018

Member

+1 seems like the better solution compared to the extra loop to verify Default: true FGs.

if printKubeDNS {
fmt.Fprintf(tabw, "Kube DNS\t%s\t%s\n", kubeDNSBeforeVersion, kubeDNSAfterVersion)
}

This comment has been minimized.

@neolit123

neolit123 May 9, 2018

Member

given the DNSType extension i would refactor the above to be a switch / case instead of handling it with if / then. it would then need less line changes once more DNS types are introduced.

This comment has been minimized.

@detiber

detiber May 9, 2018

Author Member

Latest updates the conditionals that set printKubeDNS/printCoreDNS and the before/after versions, but since we need to potentially print multiples I think it still made sense to leave the if statements in place for the actual printing of the output.

This comment has been minimized.

@neolit123

neolit123 May 9, 2018

Member

a map can be used too, but it kind of depends if it's OK to print names in the tabs \t:
this also solves the case where the before and after types differ (?). to my understanding this was possible.

// define names for known DNS types here
var dnsNames = make(map[string]string)
dnsNames[constants.CoreDNS] = "CoreDNS"
dnsNames[constants.KubeDNS] = "KubeDNS"

dnsBefore, isKnownDNSBefore := dnsNames[upgrade.Before.DNSType]
dnsAfter, isKnownDNSAfter := dnsNames[upgrade.After.DNSType]

if isKnownDNSBefore && isKnownDNSAfter {
	fmt.Printf("DNS upgrade\t%s %s\t%s %s\n", dnsBefore, upgrade.Before.DNSVersion, dnsAfter, upgrade.After.DNSVersion)
} else {
	// do we need to print an error?
	fmt.Printf("DNS upgrade encountered unknown type. Before: %s, After: %s\n", upgrade.Before.DNSType, upgrade.After.DNSType)  
}

This comment has been minimized.

@detiber

detiber May 10, 2018

Author Member

Oooh, I like that!

Playing around with this a bit, and I think it might make the output a bit confusing compared to the surrounding context. That said, I think the output from the current state of this PR is a bit confusing as well.

I think it would be worth putting some thought into how to properly handle this from a UX perspective, since there are a few different cases that we need to take into account:

  • component changes, such as Kube DNS to CoreDNS
  • new components added (possibly new default addons that weren't available for the previous version)
  • components removed (if we potentially would ever want to deprecate an addon, or if multiple addons are merged into a single addon)

Ideally, in the case that we are replacing a component we should break it out from the component upgrade output and draw attention to it in a way that makes it clear to the user what is happening.

@neolit123 do you have any objections with continuing that discussion on a separate issue that can be addressed as a followup?

This comment has been minimized.

@fabriziopandini

fabriziopandini May 10, 2018

Contributor

@detiber I'm not sure I got the full problem here, but I think that we should consider also to use mutators to "fix" older configuration in order to adapt to the latest change in code.
In this case, when upgrading, if the fetureflag CoreDNS is not set, we should add fetureflag coreDNS=false in order to avoid an unexpected switch to CoreDNS.
WDYT?

This comment has been minimized.

@detiber

detiber May 10, 2018

Author Member

@fabriziopandini If this was just a case of changing a default for a choice of options that were going to be supported long term, then I would agree with trying to use some type of a mutator to persist the existing config. However, since Kube DNS is being phased out, I think a user should be required to opt-in in order to keep using it.

This comment has been minimized.

@neolit123

neolit123 May 10, 2018

Member

Ideally, in the case that we are replacing a component we should break it out from the component upgrade output and draw attention to it in a way that makes it clear to the user what is happening.

@neolit123 do you have any objections with continuing that discussion on a separate issue that can be addressed as a followup?

@detiber sounds good, that would probably be the correct course. 👍

This comment has been minimized.

@detiber

This comment has been minimized.

@timothysc

timothysc May 14, 2018

Member

Put a // TODO: comment with the notes above this and link to a separate issue please.

This comment has been minimized.

@detiber

detiber May 14, 2018

Author Member

@timothysc done

@neolit123

This comment has been minimized.

Copy link
Member

neolit123 commented May 9, 2018

cmd/kubeadm/app/phases/upgrade needs a bazel update.
the e2e-gce failed test seems unrelated (i think).

@detiber detiber force-pushed the detiber:CoreDNSDefault branch from f26c5fe to fde0ea8 May 9, 2018

@k8s-ci-robot k8s-ci-robot added size/XL and removed size/L labels May 9, 2018

@detiber detiber changed the title [WIP] kubeadm - default CoreDNS FeatureGate to true kubeadm - default CoreDNS FeatureGate to true May 10, 2018

@timothysc
Copy link
Member

timothysc left a comment

/approve

Please add the comment listed above then lgtm.

if printKubeDNS {
fmt.Fprintf(tabw, "Kube DNS\t%s\t%s\n", kubeDNSBeforeVersion, kubeDNSAfterVersion)
}

This comment has been minimized.

@timothysc

timothysc May 14, 2018

Member

Put a // TODO: comment with the notes above this and link to a separate issue please.

// DeployedDNSAddon returns the type of DNS addon currently deployed
func DeployedDNSAddon(client clientset.Interface) (string, string, error) {
deploymentsClient := client.AppsV1().Deployments(api.NamespaceSystem)
deployments, err := deploymentsClient.List(metav1.ListOptions{LabelSelector: "k8s-app=kube-dns"})

This comment has been minimized.

@timothysc

timothysc May 14, 2018

Member

So long as we always used this label even for previous CoreDNS deploys, this is ok.

This comment has been minimized.

@detiber

detiber May 14, 2018

Author Member

Yes, this label has been used consistently within kubeadm and this is also consistent with the use in /cluster for use with the dns pod autoscaler used there.

@detiber detiber force-pushed the detiber:CoreDNSDefault branch 2 times, most recently from 5eb74ae to 6e4c0e4 May 14, 2018

@detiber

This comment has been minimized.

Copy link
Member Author

detiber commented May 14, 2018

/retest

@timothysc
Copy link
Member

timothysc left a comment

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label May 14, 2018

@detiber detiber force-pushed the detiber:CoreDNSDefault branch from 6e4c0e4 to 41e85e3 May 15, 2018

@k8s-ci-robot k8s-ci-robot removed the lgtm label May 15, 2018

@detiber

This comment has been minimized.

Copy link
Member Author

detiber commented May 15, 2018

rebased to resolve merge conflict

@neolit123

This comment has been minimized.

Copy link
Member

neolit123 commented May 15, 2018

/test pull-kubernetes-kubemark-e2e-gce

@detiber detiber force-pushed the detiber:CoreDNSDefault branch from 41e85e3 to 6b35d75 May 15, 2018

@detiber

This comment has been minimized.

Copy link
Member Author

detiber commented May 15, 2018

rebased to resolve merge conflict

rajansandeep and others added some commits May 3, 2018

@detiber detiber force-pushed the detiber:CoreDNSDefault branch from 6b35d75 to 08ba47b May 15, 2018

@luxas

This comment has been minimized.

Copy link
Member

luxas commented May 15, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label May 15, 2018

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented May 15, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: detiber, 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:

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

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented May 15, 2018

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

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented May 15, 2018

Automatic merge from submit-queue (batch tested with PRs 63658, 63509, 63800, 63586, 63840). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 67200c9 into kubernetes:master May 15, 2018

8 of 18 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-e2e-gce
Details
pull-kubernetes-e2e-gce Job triggered.
Details
pull-kubernetes-e2e-gce-100-performance Job triggered.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job triggered.
Details
pull-kubernetes-e2e-kops-aws Job triggered.
Details
pull-kubernetes-integration Job triggered.
Details
pull-kubernetes-kubemark-e2e-gce Job triggered.
Details
pull-kubernetes-kubemark-e2e-gce-big Job triggered.
Details
pull-kubernetes-node-e2e Job triggered.
Details
pull-kubernetes-verify Job triggered.
Details
cla/linuxfoundation detiber authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-typecheck Job succeeded.
Details
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented May 15, 2018

@detiber: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-integration 08ba47b link /test pull-kubernetes-integration

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@neolit123 neolit123 referenced this pull request May 22, 2018

Closed

document "CoreDNS by default in v1.11" #843

7 of 11 tasks complete
@neolit123

This comment has been minimized.

Copy link
Member

neolit123 commented May 23, 2018

@detiber

hi, the docs mention:

Note that if you are running CoreDNS in your cluster already, prior to upgrade, your existing Corefile will be
**overwritten** by the one created during upgrade. **You should save your existing ConfigMap
if you have customized it.** You may re-apply your customizations after the new ConfigMap is
up and running.

This process will be modified for the GA release of this feature, such that an existing
Corefile will not be overwritten.

https://kubernetes.io/docs/tasks/administer-cluster/coredns/#installing-coredns-with-kubeadm

is this process modified? should the docs be updated for the above?

@luxas

This comment has been minimized.

Copy link
Member

luxas commented May 23, 2018

is this process modified? should the docs be updated for the above?

I think what's there is still true.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.