don't wait for first kubelet to be ready and drop dummy deploy #43835

mikedanese · 2017-03-30T01:29:21Z

Per #43815 (comment), I suggest that we drop both the node ready and the dummy deployment check altogether for 1.6 and move them to a validation phase for 1.7.

I really think we should drop these checks altogether. CreateClientAndWaitForAPI should create a client and wait for the API, not create dummy deployments and wait for nodes to register and be healthy. These are end to end validations and this is the wrong place to do this stuff. We need an explicit final validation phase for this.

Fix a deadlock in kubeadm master initialization.

Fixes #43815

k8s-reviewable · 2017-03-30T01:30:27Z

This change is

feiskyer · 2017-03-30T02:47:09Z

cmd/kubeadm/app/master/apiclient.go

 		return true, nil
 	})

-	if err := createAndWaitForADummyDeployment(client); err != nil {


Do we still need this check if the first node has already ready?

I suggest we move this and the node ready check to a seperate final validation step. Do you mean we should drop it permanently?

Hmm, just noticed #43824 did this just after firstNodeIsReady

I don't think this is the right place to check that. Commented below.

jbeda · 2017-03-30T03:14:23Z

This LGTM but, as noted in #43815, I prefer #43824. I'm cool with either, though.

jbeda · 2017-03-30T03:15:27Z

cmd/kubeadm/app/master/apiclient.go


-		fmt.Printf("[apiclient] First node is ready after %f seconds\n", time.Since(start).Seconds())
+		fmt.Printf("[apiclient] First node has registered after %f seconds\n", time.Since(start).Seconds())


Nit: you change this comment but don't fix up line 47. Most users won't parse registered vs. ready though.

mikedanese · 2017-03-30T15:39:15Z

@jbeda I really think we should drop these checks altogether. CreateClientAndWaitForAPI should create a client and wait for the API, not create dummy deployments and wait for nodes to register and be healthy. These are end to end validations and this is the wrong place to do this stuff. We need an explicit final validation phase for this.

and skip dummy deployment

kensimon · 2017-03-30T16:54:51Z

I agree with @mikedanese that it would be better to separate the validation steps so that it's easier to automate the kubeadm init && kubectl apply cni-plugin.yaml workflows, or at least have let kubeadm manage the network plugin application for you (see kubernetes/kubeadm#214)

In the mean time I'm good with this PR or mine, but I'll leave it to someone else to make the final judgement here. Thanks everyone for your help!

pipejakob · 2017-03-30T18:49:18Z

/lgtm

Code looks fine to me, but other reviewers are probably more familiar with the expectations around initialization. I'm happy so long as end-to-end testing works.

k8s-github-robot · 2017-03-30T18:49:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mikedanese, pipejakob

Needs approval from an approver in each of these OWNERS Files:

~~cmd/kubeadm/OWNERS~~ [mikedanese]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

luxas · 2017-03-30T20:16:21Z

If we do this (which we probably should), we should really make sure it doesn't break us again. The dummy depkoyment was added because of race conditions when the controller manager and/or scheduler said they were ready while they weren't IIRC (deadlock). So I think the problem with removing this is that we might start getting these race conditions (flakes) instead. I'm not 100 % sure this applies anymore but it should be noted.

Can't we just tolerate the taint for the dummy deployment?

mikedanese · 2017-03-30T20:34:55Z

The race was due to immediately creating the kube-discovery deployment and should no longer be a problem. I have been and will continue to test this extensively with @pipejakob before we cut 1.6.1

kensimon · 2017-03-30T20:50:30Z

@luxas apparently tolerations don't work for scheduling new pods on NotReady nodes, the scheduler always excludes those nodes from scheduling regardless of tolerations. (see the discussion on the other PR: #43824 (comment))

But if people are waiting on "which PR?" to proceed, I'm totally willing to say let's just do this one, whatever gets this out the door quickest since 1.6.0 is kind of DOA for new clusters due to this.

k8s-github-robot · 2017-03-30T23:57:23Z

Automatic merge from submit-queue

…3835-release-1.6 Automatic merge from submit-queue Automated cherry pick of #43835 release 1.6 Automated cherry pick of #43835 release 1.6 fixes: #43815

JunkTapes · 2017-03-31T12:02:39Z

@mikedanese how do i apply your patch to my kubernetes installment?

lucianomagrao · 2017-03-31T12:48:51Z

@Sadpyro change your apt repo to unstable and update kubeadm

luxas · 2017-03-31T13:21:18Z

LGTM to me as well given the comments above

lucianomagrao · 2017-03-31T14:32:20Z

@Sadpyro

cat <<EOF >/etc/apt/sources.list.d/kubernetes.list
deb http://apt.kubernetes.io/ kubernetes-xenial-unstable main
EOF

bgrant0607 · 2017-03-31T20:51:42Z

@mikedanese Do you intend to cherrypick this into the release branch?

mikedanese · 2017-03-31T21:01:06Z

@bgrant0607 it has already been cherry-picked #43837

k8s-cherrypick-bot · 2017-03-31T21:03:35Z

Removing label cherrypick-candidate because no release milestone was set. This is an invalid state and thus this PR is not being considered for cherry-pick to any release branch. Please add an appropriate release milestone and then re-add the label.

…pick-of-#43835-release-1.6 Automatic merge from submit-queue Automated cherry pick of kubernetes#43835 release 1.6 Automated cherry pick of kubernetes#43835 release 1.6 fixes: kubernetes#43815

k8s-cherrypick-bot · 2017-06-28T00:05:29Z

Commit found in the "release-1.6" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

mikedanese assigned luxas Mar 30, 2017

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 30, 2017

k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note-label-needed labels Mar 30, 2017

mikedanese mentioned this pull request Mar 30, 2017

Automated cherry pick of #43835 release 1.6 #43837

Merged

mikedanese assigned jbeda Mar 30, 2017

mikedanese mentioned this pull request Mar 30, 2017

kubeadm 1.6.0 (only 1.6.0!!) is broken due to unconfigured CNI making kubelet NotReady #43815

Closed

feiskyer reviewed Mar 30, 2017

View reviewed changes

jbeda reviewed Mar 30, 2017

View reviewed changes

don't wait for first kubelet to be ready

8955711

and skip dummy deployment

mikedanese force-pushed the kubeadm-fix branch from 5c8842c to 8955711 Compare March 30, 2017 16:25

mikedanese added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Mar 30, 2017

mikedanese assigned pipejakob Mar 30, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 30, 2017

arthur0 mentioned this pull request Mar 30, 2017

kubeadm init: Tolerate node network not being ready #43824

Closed

mikedanese added the priority/P0 label Mar 30, 2017

mikedanese modified the milestones: v1.6, v1.6.1 Mar 30, 2017

k8s-github-robot merged commit 57b7c75 into kubernetes:master Mar 30, 2017

coeki mentioned this pull request Mar 31, 2017

1.6.0 kubelet changed behaviour for cgroups on Centos kubernetes/release#306

Closed

mikedanese deleted the kubeadm-fix branch March 31, 2017 13:43

mikedanese added the cherrypick-candidate label Mar 31, 2017

k8s-cherrypick-bot removed the cherrypick-candidate label Mar 31, 2017

mikedanese modified the milestones: v1.6, v1.6.1 Mar 31, 2017

mikedanese added the cherrypick-candidate label Mar 31, 2017

andrewrynhard mentioned this pull request Apr 1, 2017

self-hosted option in kubeadm fails #43949

Closed

luxas mentioned this pull request Apr 1, 2017

Make the kubeadm on the release-1.6 a little bit more stable #43950

Closed

jellonek mentioned this pull request Apr 4, 2017

kubeadm: Wait for node before updating labels and taints #44031

Merged

Miyurz mentioned this pull request Apr 28, 2017

kubeadm blocking waiting for the control plane to become ready #45064

Closed

k8s-cherrypick-bot removed the cherrypick-candidate label Jun 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

don't wait for first kubelet to be ready and drop dummy deploy #43835

don't wait for first kubelet to be ready and drop dummy deploy #43835

mikedanese commented Mar 30, 2017 •

edited

Loading

k8s-reviewable commented Mar 30, 2017

feiskyer Mar 30, 2017

mikedanese Mar 30, 2017

feiskyer Mar 30, 2017

mikedanese Mar 30, 2017

jbeda commented Mar 30, 2017

jbeda Mar 30, 2017

mikedanese Mar 30, 2017

mikedanese commented Mar 30, 2017

kensimon commented Mar 30, 2017

pipejakob commented Mar 30, 2017

k8s-github-robot commented Mar 30, 2017

luxas commented Mar 30, 2017

mikedanese commented Mar 30, 2017 •

edited

Loading

kensimon commented Mar 30, 2017

k8s-github-robot commented Mar 30, 2017

JunkTapes commented Mar 31, 2017

lucianomagrao commented Mar 31, 2017

luxas commented Mar 31, 2017

lucianomagrao commented Mar 31, 2017 •

edited by mikedanese

Loading

bgrant0607 commented Mar 31, 2017

mikedanese commented Mar 31, 2017

k8s-cherrypick-bot commented Mar 31, 2017

k8s-cherrypick-bot commented Jun 28, 2017


		fmt.Printf("[apiclient] First node is ready after %f seconds\n", time.Since(start).Seconds())
		fmt.Printf("[apiclient] First node has registered after %f seconds\n", time.Since(start).Seconds())

don't wait for first kubelet to be ready and drop dummy deploy #43835

don't wait for first kubelet to be ready and drop dummy deploy #43835

Conversation

mikedanese commented Mar 30, 2017 • edited Loading

k8s-reviewable commented Mar 30, 2017

feiskyer Mar 30, 2017

Choose a reason for hiding this comment

mikedanese Mar 30, 2017

Choose a reason for hiding this comment

feiskyer Mar 30, 2017

Choose a reason for hiding this comment

mikedanese Mar 30, 2017

Choose a reason for hiding this comment

jbeda commented Mar 30, 2017

jbeda Mar 30, 2017

Choose a reason for hiding this comment

mikedanese Mar 30, 2017

Choose a reason for hiding this comment

mikedanese commented Mar 30, 2017

kensimon commented Mar 30, 2017

pipejakob commented Mar 30, 2017

k8s-github-robot commented Mar 30, 2017

luxas commented Mar 30, 2017

mikedanese commented Mar 30, 2017 • edited Loading

kensimon commented Mar 30, 2017

k8s-github-robot commented Mar 30, 2017

JunkTapes commented Mar 31, 2017

lucianomagrao commented Mar 31, 2017

luxas commented Mar 31, 2017

lucianomagrao commented Mar 31, 2017 • edited by mikedanese Loading

bgrant0607 commented Mar 31, 2017

mikedanese commented Mar 31, 2017

k8s-cherrypick-bot commented Mar 31, 2017

k8s-cherrypick-bot commented Jun 28, 2017

mikedanese commented Mar 30, 2017 •

edited

Loading

mikedanese commented Mar 30, 2017 •

edited

Loading

lucianomagrao commented Mar 31, 2017 •

edited by mikedanese

Loading