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: add configuration option to not taint master #55479

Merged
merged 1 commit into from Feb 12, 2018

Conversation

@ijc
Contributor

ijc commented Nov 10, 2017

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

This comment has been minimized.

Contributor

ijc commented Nov 10, 2017

/area kubeadm
/assign @mikedanese

@dims

This comment has been minimized.

Member

dims commented Nov 13, 2017

/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

This comment has been minimized.

Contributor

ijc commented Nov 21, 2017

@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

This comment has been minimized.

Contributor

ijc commented Jan 5, 2018

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

This comment has been minimized.

Contributor

k8s-ci-robot commented Jan 5, 2018

@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

This comment has been minimized.

Member

errordeveloper commented Jan 9, 2018

Perhaps I should rebase nonetheless?

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

@ijc

This comment has been minimized.

Contributor

ijc commented Jan 9, 2018

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

This comment has been minimized.

Contributor

ijc commented Jan 9, 2018

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

This comment has been minimized.

Contributor

ijc commented Jan 9, 2018

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

This comment has been minimized.

Member

errordeveloper commented Jan 9, 2018

/test pull-kubernetes-verify

@errordeveloper

This comment has been minimized.

Member

errordeveloper commented Jan 9, 2018

@ijc

This comment has been minimized.

Contributor

ijc commented Jan 10, 2018

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

@ijc

This comment has been minimized.

Contributor

ijc commented Jan 10, 2018

@errordeveloper seems the tests are passing now.

@dixudx

Add a flag for NoTaintMaster.
Others lgtm.

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

This comment has been minimized.

@dixudx

dixudx Jan 14, 2018

Member

Does this means a master taint is added by default?

Shall we add an option NoTaintMaster for this as well?

This comment has been minimized.

@ijc

ijc Jan 15, 2018

Contributor

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

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

This comment has been minimized.

@dixudx

dixudx Jan 14, 2018

Member

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.

This comment has been minimized.

@ijc

ijc Jan 15, 2018

Contributor

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.

This comment has been minimized.

@dixudx

dixudx Jan 15, 2018

Member

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?

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

@ijc

This comment has been minimized.

Contributor

ijc commented Jan 15, 2018

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

@ijc

This comment has been minimized.

Contributor

ijc commented Jan 15, 2018

Of course I spotted immediately after commenting that I had forgotten to update the call in NewCmdMarkMaster to actually do something...

@ijc

This comment has been minimized.

Contributor

ijc commented Jan 15, 2018

I've manually run though using config file with noTaintMaster: true and noTaintMaster: false and with no line at all as well as non-config file case both with and without --no-master-taint. DTRT in all 5 cases.

I've not run through the manual kubeadm alpha phase based stuff yet, still wondering how best to do that.

@ijc

This comment has been minimized.

Contributor

ijc commented Jan 15, 2018

I've not run through the manual kubeadm alpha phase based stuff yet, still wondering how best to do that.

I did:

$ kubeadm init --ignore-preflight-errors=all --kubernetes-version v1.9.1 --no-taint-master
$ kubectl describe node # => should be untainted
$ kubeadm alpha phase mark-master
$ kubectl describe node # => should now be tainted
$ kubeadm alpha phase mark-master --no-taint-master
$ kubectl describe node # => should now be untainted
$ kubeadm alpha phase mark-master 
$ kubectl describe node # => should now be tainted
$ cat /etc/kubeadm/kubeadm.yaml
apiVersion: kubeadm.k8s.io/v1alpha1
kind: MasterConfiguration
kubernetesVersion: v1.9.1   
noTaintMaster: true
$ kubeadm alpha phase mark-master --config /etc/kubeadm/kubeadm.yaml
$ kubectl describe node # => should now be untainted

Tainting was as expected/required at each step.

@timothysc

This comment has been minimized.

Member

timothysc commented Jan 17, 2018

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

@ijc

This comment has been minimized.

Contributor

ijc commented Jan 19, 2018

@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

This comment has been minimized.

Member

errordeveloper commented Jan 22, 2018

I agree that this is very useful.

@ijc

This comment has been minimized.

Contributor

ijc commented Jan 24, 2018

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

@timothysc

This comment has been minimized.

Member

timothysc commented Feb 1, 2018

@ijc yes please.

@ijc

This comment has been minimized.

Contributor

ijc commented Feb 1, 2018

@timothysc rebase done and force pushed, thanks.

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

This comment has been minimized.

@timothysc

timothysc Feb 7, 2018

Member

@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

This comment has been minimized.

@ijc

ijc Feb 7, 2018

Contributor

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.

This comment has been minimized.

@errordeveloper

errordeveloper Feb 12, 2018

Member

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

kubeadm: add configuration option to not taint master
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

This comment has been minimized.

Contributor

ijc commented Feb 12, 2018

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

This comment has been minimized.

Member

errordeveloper commented Feb 12, 2018

/lgtm

Thanks, Ian :)

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

@timothysc

/approve

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Feb 12, 2018

[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:

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

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Feb 12, 2018

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-merge-robot k8s-merge-robot merged commit fd55cb2 into kubernetes:master Feb 12, 2018

13 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation ijc 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-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-unit Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

@ijc ijc deleted the ijc:kubeadm-optional-master-taint branch Feb 13, 2018

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