kubeadm: add configuration option to not taint master #55479

ijc · 2017-11-10T13:33:28Z

What this PR does / why we need it:

Although tainting the master is normally a good and proper thing to do in some situations (docker for mac in our case, but I suppose minikube and such as well) having a single host configuration is desirable.

In linuxkit we have a workaround to remove the taint after initialisation. With the change here we could simply populate /etc/kubeadm/kubeadm.yaml with noTaintMaster: true instead and have it never be tainted in the first place.

I have only added this to the config file and not to the CLI since AIUI the latter is somewhat deprecated.

The code also arranges to remove an existing taint if it is unwanted. I'm unsure if this behaviour is correct or desirable, I think a reasonable argument could be made for leaving an existing taint in place too.

Signed-off-by: Ian Campbell ijc@docker.com

Release note:

Since the requirement for this option is rather niche and not best practice in the majority of cases I'm not sure if it warrants mentioning in the release notes? If it were then perhaps

`kubeadm init` can now omit the tainting of the master node if configured to do so in `kubeadm.yaml`.

ijc · 2017-11-10T13:59:15Z

/area kubeadm
/assign @mikedanese

dims · 2017-11-13T14:46:56Z

/ok-to-test

luxas

cc @kubernetes/sig-cluster-lifecycle-pr-reviews
I'm sorry @ijc, it might be too late to get this into v1.9 at this time I think.
If you can make the SIG meeting tomorrow or the kubeadm meeting on Weds though; we might be able to briefly discuss it and prioritize accordingly.

I'm sorry that no-one responded to this earlier, but we didn't get notified. ^ is the Github team tag to use for getting the notification across to the right people

ijc · 2017-11-21T12:13:58Z

@luxas thanks, I think this isn't terribly urgent for 1.9, so long as it is on the train for 1.10, we can live with the workaround we have for now.

I'll aim to make it to the kubeadm meeting on Wednesday (tomorrow).

ijc · 2018-01-05T14:37:48Z

Sorry, I never made it to the Wednesday meeting last year, things were a bit hectic.

Is now the right time in the v1.10 release cycle to revisit this? (I think/hope so based on kubeadm).

Despite being 1500+ commits behind this still appears to still merge cleanly. Perhaps I should rebase nonetheless? (I'll check it still builds when merged with master in any case)

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

k8s-ci-robot · 2018-01-05T14:37:55Z

@ijc: Reiterating the mentions to trigger a notification:
@kubernetes/sig-cluster-lifecycle-pr-reviews

In response to this:

Sorry, I never made it to the Wednesday meeting last year, things were a bit hectic.

Is now the right time in the v1.10 release cycle to revisit this? (I think/hope so based on kubeadm).

Despite being 1500+ commits behind this still appears to still merge cleanly. Perhaps I should rebase nonetheless? (I'll check it still builds when merged with master in any case)

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

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.

errordeveloper · 2018-01-09T12:12:19Z

Perhaps I should rebase nonetheless?

@ijc have you had a chance to do that yet?

ijc · 2018-01-09T12:26:15Z

Perhaps I should rebase nonetheless?
@ijc have you had a chance to do that yet?

Nope. Shall I infer from your question that you think I should?

ijc · 2018-01-09T16:13:24Z

Nope. Shall I infer from your question that you think I should?

@errordeveloper I decided that I could/should and have rebased and done a force push.

ijc · 2018-01-09T16:34:31Z

pull-kubernetes-verify failed with godep: error downloading dep (bitbucket.org/ww/goautoneg): exit status 255 which seems like an instance of #27517.

/test pull-kubernetes-verify

errordeveloper · 2018-01-09T16:55:43Z

/test pull-kubernetes-verify

errordeveloper · 2018-01-09T16:57:48Z

see also: prometheus/common#17

ijc · 2018-01-10T09:42:53Z

Lets see if bitbucket became happier overnight:
/test pull-kubernetes-verify
/test pull-kubernetes-e2e-gce-device-plugin-gpu

ijc · 2018-01-10T11:05:22Z

@errordeveloper seems the tests are passing now.

dixudx

Add a flag for NoTaintMaster.
Others lgtm.

dixudx · 2018-01-14T09:49:27Z

cmd/kubeadm/app/cmd/phases/markmaster.go

@@ -75,7 +75,7 @@ func NewCmdMarkMaster() *cobra.Command {
 			client, err := kubeconfigutil.ClientSetFromFile(kubeConfigFile)
 			kubeadmutil.CheckErr(err)

-			err = markmasterphase.MarkMaster(client, internalcfg.NodeName)
+			err = markmasterphase.MarkMaster(client, internalcfg.NodeName, true)


Does this means a master taint is added by default?

Shall we add an option NoTaintMaster for this as well?

I suppose we might as well, I'll do this too.

dixudx · 2018-01-14T09:51:13Z

cmd/kubeadm/app/cmd/init.go

@@ -383,7 +383,7 @@ func (i *Init) Run(out io.Writer) error {
 	}

 	// PHASE 4: Mark the master with the right label/taint
-	if err := markmasterphase.MarkMaster(client, i.cfg.NodeName); err != nil {
+	if err := markmasterphase.MarkMaster(client, i.cfg.NodeName, !i.cfg.NoTaintMaster); err != nil {


Should add a BoolVar flag no-taint-master to AddInitConfigFlags.
If not, cfg.NoTaintMaster will always be false and cannot be override by parsing arguments.

cfg.NoTaintMaster can be set in the config file. For testing I have /etc/kubeadm/kubeadm.yaml containing:

apiVersion: kubeadm.k8s.io/v1alpha1 kind: MasterConfiguration kubernetesVersion: v1.9.1 noTaintMaster: true

and am running kubeadm init --config /etc/kubeadm/kubeadm.yaml.

My understanding was that the config file mechanism was the future and the CLI variants were deprecated, perhaps I understood wrongly? I'll add the lines to AddInitConfigFlags, it looks like it will be pretty straightforward.

My understanding was that the config file mechanism was the future and the CLI variants were deprecated

I agree. But for now, we still use arguments. @luxas WDYT?

ijc · 2018-01-15T12:09:14Z

@dixudx I added --no-taint-master to both kubeadm init and kubeadm alpha phase mark-master as suggested. I smoke tested and since it seemed pretty straightforward I pushed while I run through the other combinations.

timothysc · 2018-01-17T23:52:08Z

Why is the post provision step not sufficient?
It's more of an exception than the norm.

ijc · 2018-01-19T10:18:57Z

@timothysc I figured minikube and docker for mac between them were not that all that exceptional and a post provision step is one more thing to do wrong/differently or to bit rot. d4m would certainly use this functionality if it were available (I don't know about minikube, I'm only speculating that it would be useful for that use case too).

I'll hold off on the rebase until we've decided whether or not this is a worthwhile change.

errordeveloper · 2018-01-22T12:49:27Z

I agree that this is very useful.

ijc · 2018-01-24T11:18:05Z

Ping @luxas / @timothysc — is it worth my rebasing this PR?

timothysc · 2018-02-01T00:12:37Z

@ijc yes please.

ijc · 2018-02-01T16:12:22Z

@timothysc rebase done and force pushed, thanks.

timothysc · 2018-02-07T03:33:46Z

cmd/kubeadm/app/cmd/init.go

@@ -197,6 +197,8 @@ func AddInitConfigFlags(flagSet *flag.FlagSet, cfg *kubeadmapiext.MasterConfigur
 	flagSet.Var(utilflag.NewMapStringString(&cfg.APIServerExtraArgs), "apiserver-extra-args", "A set of extra flags to pass to the API Server or override default ones in form of <flagname>=<value>")
 	flagSet.Var(utilflag.NewMapStringString(&cfg.ControllerManagerExtraArgs), "controller-manager-extra-args", "A set of extra flags to pass to the Controller Manager or override default ones in form of <flagname>=<value>")
 	flagSet.Var(utilflag.NewMapStringString(&cfg.SchedulerExtraArgs), "scheduler-extra-args", "A set of extra flags to pass to the Scheduler or override default ones in form of <flagname>=<value>")
+
+	flagSet.BoolVar(&cfg.NoTaintMaster, "no-taint-master", cfg.NoTaintMaster, "Do not taint the master node.")


@errordeveloper - ping here, b/c you were making the case for it, but there is a push to try and not add any new flags. /cc @luxas

I added the flags after it was requested in #55479 (comment), but I deliberately kept it as a separate commit which could easily be dropped.

I agree with the push back on new flags, so perhaps we could add just the config file field for now.

Although tainting the master is normally a good and proper thing to do in some situations (docker for mac in our case, but I suppose minikube and such as well) having a single host configuration is desirable. In linuxkit we have a [workaround](https://github.com/linuxkit/linuxkit/blob/443e47c408cad0f1b29a457700d15b2c85ec407f/projects/kubernetes/kubernetes/kubeadm-init.sh#L19...L22) to remove the taint after initialisation. With the change here we could simply populate /etc/kubeadm/kubeadm.yaml` with `noTaintMaster: true` instead and have it never be tainted in the first place. I have only added this to the config file and not to the CLI since AIUI the latter is somewhat deprecated. The code also arranges to _remove_ an existing taint if it is unwanted. I'm unsure if this behaviour is correct or desirable, I think a reasonable argument could be made for leaving an existing taint in place too. Signed-off-by: Ian Campbell <ijc@docker.com>

ijc · 2018-02-12T17:15:40Z

I've backed out the commit which added the new flags. I kept one of the hunks in cmd/kubeadm/app/cmd/phases/markmaster.go (by folding it into the first patch) since I think it is clearer that way than hardcoding true.

errordeveloper · 2018-02-12T17:22:02Z

/lgtm

Thanks, Ian :)

timothysc

/approve

k8s-ci-robot · 2018-02-12T22:30:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: errordeveloper, ijc, 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 OWNERS Files:

~~cmd/kubeadm/OWNERS~~ [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-02-12T23:44:39Z

Automatic merge from submit-queue (batch tested with PRs 59767, 56454, 59237, 59730, 55479). If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-github-robot assigned lukemarsden and mikedanese Nov 10, 2017

k8s-ci-robot added the area/kubeadm label Nov 10, 2017

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 13, 2017

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

ijc force-pushed the kubeadm-optional-master-taint branch from c97d7ff to 6fe3505 Compare November 20, 2017 11:18

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

luxas reviewed Nov 20, 2017

View reviewed changes

k8s-ci-robot added the sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. label Nov 20, 2017

ijc force-pushed the kubeadm-optional-master-taint branch from 6fe3505 to 38fa58a Compare January 9, 2018 16:13

dixudx suggested changes Jan 14, 2018

View reviewed changes

kubernetes deleted a comment from k8s-github-robot Jan 15, 2018

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

timothysc assigned timothysc and errordeveloper and unassigned lukemarsden and mikedanese Jan 19, 2018

ijc force-pushed the kubeadm-optional-master-taint branch from 2cda9bc to 52af642 Compare February 1, 2018 16:10

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

timothysc reviewed Feb 7, 2018

View reviewed changes

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

ijc force-pushed the kubeadm-optional-master-taint branch from 52af642 to 24bb0d5 Compare February 7, 2018 16:37

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

ijc force-pushed the kubeadm-optional-master-taint branch from 24bb0d5 to a4e00ff Compare February 12, 2018 17:14

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

timothysc approved these changes Feb 12, 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 Feb 12, 2018

k8s-github-robot merged commit fd55cb2 into kubernetes:master Feb 12, 2018

ijc deleted the kubeadm-optional-master-taint branch February 13, 2018 10:59

ijc mentioned this pull request Apr 9, 2018

Use noTaintMaster in kubeadm.conf linuxkit/kubernetes#72

Open

ijc mentioned this pull request Jun 14, 2018

Fix kubeadm taints to not override existing node taints #65068

Merged

kubeadm: add configuration option to not taint master #55479

kubeadm: add configuration option to not taint master #55479

Conversation

ijc commented Nov 10, 2017 • edited Loading

ijc commented Nov 10, 2017

dims commented Nov 13, 2017

luxas left a comment

Choose a reason for hiding this comment

ijc commented Nov 21, 2017

ijc commented Jan 5, 2018

k8s-ci-robot commented Jan 5, 2018

errordeveloper commented Jan 9, 2018

ijc commented Jan 9, 2018

ijc commented Jan 9, 2018

ijc commented Jan 9, 2018

errordeveloper commented Jan 9, 2018

errordeveloper commented Jan 9, 2018

ijc commented Jan 10, 2018

ijc commented Jan 10, 2018

dixudx left a comment

Choose a reason for hiding this comment

dixudx Jan 14, 2018

Choose a reason for hiding this comment

ijc Jan 15, 2018

Choose a reason for hiding this comment

dixudx Jan 14, 2018

Choose a reason for hiding this comment

ijc Jan 15, 2018

Choose a reason for hiding this comment

dixudx Jan 15, 2018

Choose a reason for hiding this comment

ijc commented Jan 15, 2018

timothysc commented Jan 17, 2018

ijc commented Jan 19, 2018

errordeveloper commented Jan 22, 2018

ijc commented Jan 24, 2018

timothysc commented Feb 1, 2018

ijc commented Feb 1, 2018

timothysc Feb 7, 2018

Choose a reason for hiding this comment

ijc Feb 7, 2018

Choose a reason for hiding this comment

errordeveloper Feb 12, 2018 • edited Loading

Choose a reason for hiding this comment

ijc commented Feb 12, 2018

errordeveloper commented Feb 12, 2018

timothysc left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 12, 2018

k8s-github-robot commented Feb 12, 2018

ijc commented Nov 10, 2017 •

edited

Loading

errordeveloper Feb 12, 2018 •

edited

Loading