Improve kubeadm reset #37831

luxas · 2016-12-01T16:52:18Z

Depends on: #36474
Broken out from: #37568
Carries: #35709, @camilocot

This makes the kubeadm reset command more robust and user-friendly.
I'll rebase after #36474 merges...

cc-ing reviewers: @mikedanese @errordeveloper @dgoodwin @jbeda

k8s-oncall · 2016-12-01T16:52:27Z

This change is

kad · 2016-12-01T19:40:41Z

cmd/kubeadm/app/cmd/reset.go

+	dockerCheck := preflight.ServiceCheck{Service: "docker"}
+	if warnings, errors := dockerCheck.Check(); len(warnings) == 0 && len(errors) == 0 {
+		fmt.Println("[reset] Stopping all running docker containers...")
+		if err := exec.Command("sh", "-c", "docker ps | grep 'k8s_' | awk '{print $1}' | xargs -r docker rm --force --volumes").Run(); err != nil {


what would happen if non-docker container runtime used ?

we don't support other runtimes than docker for the moment, so that's not a problem for now.
this code just moved from below

kad · 2016-12-01T19:42:44Z

cmd/kubeadm/app/cmd/reset.go

+	}
+
+	fmt.Println("[reset] Unmounting directories in /var/lib/kubelet...")
+	umountDirsCmd := "cat /proc/mounts | awk '{print $2}' | grep '/var/lib/kubelet' | xargs -r umount"


maybe instead of embedding "shell" script here, write small function to call umount individually and report error on particular path ?

dgoodwin

Everything else looks good to me Lucas. New output is improved and I see we're hitting /etc/cni/net.d now which is probably good.

dgoodwin · 2016-12-02T18:55:52Z

cmd/kubeadm/app/cmd/reset.go

@@ -45,7 +46,7 @@ func NewCmdReset(out io.Writer) *cobra.Command {

 	cmd.PersistentFlags().BoolVar(
 		&skipPreFlight, "skip-preflight-checks", false,
-		"skip preflight checks normally run before modifying the system",
+		"preflight checks normally run before modifying the system, pass this argument to skip those checks",


Personally prefer the original version, it reads just as clearly to me and is more concise which I like for CLI help options.

Either way though we should capitalize first letter, this is inconsistent with all the other init --help options right now.

k8s-ci-robot · 2016-12-04T17:03:29Z

Jenkins GCI GKE smoke e2e failed for commit bf6b95447c1355cec93044bd849b0ca108b7bf5e. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-12-04T17:08:21Z

Jenkins GCE Node e2e failed for commit bf6b95447c1355cec93044bd849b0ca108b7bf5e. Full PR test history.

The magic incantation to run this job again is @k8s-bot node e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-12-04T17:08:29Z

Jenkins unit/integration failed for commit bf6b95447c1355cec93044bd849b0ca108b7bf5e. Full PR test history.

The magic incantation to run this job again is @k8s-bot unit test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-12-04T17:09:36Z

Jenkins Kubemark GCE e2e failed for commit bf6b95447c1355cec93044bd849b0ca108b7bf5e. Full PR test history.

The magic incantation to run this job again is @k8s-bot kubemark e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-12-04T17:09:51Z

Jenkins CRI GCE Node e2e failed for commit bf6b95447c1355cec93044bd849b0ca108b7bf5e. Full PR test history.

The magic incantation to run this job again is @k8s-bot cri node e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-12-04T17:11:11Z

Jenkins GCI GCE e2e failed for commit bf6b95447c1355cec93044bd849b0ca108b7bf5e. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

luxas · 2016-12-04T18:13:31Z

@dgoodwin When it looks good to you, you should simply be able to comment "/lgtm" and the bot will apply the label.
Also included a third commit, which makes reset drain the node and remove it from the cluster.

k8s-ci-robot · 2016-12-04T19:36:15Z

Jenkins GKE smoke e2e failed for commit d350e98965768598ee58688ba7f53ded56733031. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-12-04T20:23:39Z

Jenkins GCE etcd3 e2e failed for commit d350e98965768598ee58688ba7f53ded56733031. Full PR test history.

The magic incantation to run this job again is @k8s-bot gce etcd3 e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-12-04T21:09:32Z

Jenkins kops AWS e2e failed for commit 65edcc0. Full PR test history.

The magic incantation to run this job again is @k8s-bot kops aws e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-12-04T21:12:03Z

Jenkins GCE e2e failed for commit 65edcc0. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

luxas · 2016-12-05T14:43:13Z

@k8s-bot cvm gce e2e test this

dgoodwin

Just some text change requsts.

dgoodwin · 2016-12-05T18:44:24Z

cmd/kubeadm/app/cmd/reset.go

@@ -48,118 +49,167 @@ func NewCmdReset(out io.Writer) *cobra.Command {
 		"skip preflight checks normally run before modifying the system",
 	)

+	cmd.PersistentFlags().BoolVar(
+		&removeNode, "remove-node", true,
+		"remove this node from the pool of nodes in this cluster",


We need to standardize here but it looks like the winner so far is to capitalize "Remove".

dgoodwin · 2016-12-05T18:47:12Z

cmd/kubeadm/app/cmd/reset.go

+
+func drainNode(removeNode bool) error {
+
+	fmt.Println("[reset] Draining this node...")


I would avoid using "this" here, doesn't read quite right. How about we move this below the hostname check, and if it passes output "Draining node: hostname".

dgoodwin · 2016-12-05T18:50:17Z

cmd/kubeadm/app/cmd/reset.go

+	}
+
+	if removeNode {
+		fmt.Println("[reset] Removing this node from the cluster...")


Per above I would suggest "Removing node: hostname" or just "Removing node..."

dgoodwin · 2016-12-05T18:59:54Z

cmd/kubeadm/app/cmd/reset.go

-		if err != nil {
-			fmt.Printf("failed to remove file: [%v]\n", err)
-		}
+		fmt.Printf("[reset] Failed to drain node: [%v]\n", err)


We should probably anticipate this reset will be run on a lot of nodes that never made it to the cluster, or have since been deleted by other means, so for the text here because the method might fail to drain, or to remove, we shouldn't output "drain" explicitly here.

How about "Failed to cleanup node: [%v]" and let the inner error message indicate if it was drain or remove that went wrong.

dgoodwin · 2016-12-05T19:02:16Z

cmd/kubeadm/app/cmd/reset.go

-	defer d.Close()
-	names, err := d.Readdirnames(-1)
+	// Drain and maybe remove the node from the cluster
+	err := drainNode(r.removeNode)


drainAndRemoveNode might be a little more clear.

dgoodwin · 2016-12-05T19:04:41Z

cmd/kubeadm/app/cmd/reset.go

+	fmt.Println("[reset] Draining this node...")
+	hostname, err := os.Hostname()
+	if err != nil {
+		return fmt.Errorf("failed to detect the hostname of this node. Won't drain this node and remove it from the cluster.")


Suggest just "failed to detect node hostname".

dgoodwin · 2016-12-05T19:05:17Z

cmd/kubeadm/app/cmd/reset.go

+	kubeConfigPath := path.Join(kubeadmapi.GlobalEnvParams.KubernetesDir, "kubelet.conf")
+	_, err = exec.Command("kubectl", "--kubeconfig", kubeConfigPath, "drain", hostname, "--delete-local-data", "--force", "--ignore-daemonsets").Output()
+	if err != nil {
+		return fmt.Errorf("failed to drain this node [%v]", err)


Suggest dropping "this".

dgoodwin · 2016-12-05T19:06:04Z

cmd/kubeadm/app/cmd/reset.go

+
+		_, err = exec.Command("kubectl", "--kubeconfig", kubeConfigPath, "delete", "node", hostname).Output()
+		if err != nil {
+			return fmt.Errorf("failed to remove this node [%v]", err)


Suggest dropping "this" here as well.

luxas · 2016-12-05T20:02:28Z

@dgoodwin Done, I think I fixed all your output nits.
PTAL

…p the wrong config on the next kubeadm init run

… user-friendly output

k8s-ci-robot · 2016-12-06T13:19:37Z

Jenkins verification failed for commit 6fa116b3df801784de5f412fb07ac0fce1f35896. Full PR test history.

The magic incantation to run this job again is @k8s-bot verify test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

…de should be removed from the cluster as well

kad · 2016-12-06T14:02:14Z

cmd/kubeadm/app/cmd/reset.go

+
+	fmt.Printf("[reset] Draining node: %q\n", hostname)
+
+	output, err = exec.Command("kubectl", "--kubeconfig", kubeConfigPath, "drain", hostname, "--delete-local-data", "--force", "--ignore-daemonsets").Output()


we need to add HttpProxyCheck() in RunResetCheck(). but we need somehow get API URL which written in kubelet.conf.
Also, FileAvailableCheck{Path: "/etc/kubernetes/kubelet.conf"} needed there if we want to use kubectl here.
And InPathCheck{executable: "kubectl", mandatory: true}

Would these be covered sufficiently by the err handling?

The reason I don't want to add those checks is that the drain/remove functions aren't critical in any way, if it doesn't work, it just won't do the drain and remove things.

checks can be done optional based on state of "remove-node". if user specifies flag, he/she assumes that it would be done gracefully.

Still, it will try to drain regardless of remove-node...
At least, this is totally fine for the next stable release, it improves and fixes lots of things

The TODO I added was to switch over to the go native client, but we're gonna do that in time for v1.6 when we have more time and are confident that the versioned go client (client-go) works properly.

comment applicable to both invocations of kubectl, before client-go used.
It doesn't make sense to fail with debug message if we already in the beginning can warn user about potential issue. That's the whole reason for prechecks.

This situation is very different with reset however, we're expecting that the system could easily have failed to cluster up, so the drain operation may well fail and we intend to keep on going. Provided the error messages resulting from attempting to drain are logical, it's probably ok to let the error for whatever happens bubble up, as we're going to proceed either way.

That said, @luxas what happens on pure nodes here? Is kubectl configured there? Does draining only work on masters?

It works on nodes as well, otherwise I wouldn't have added it here...
The /etc/kubernetes/kubelet.conf file exists everywhere when using kubeadm, and kubectl is installed everywhere, so when going the opinionated route with debs and rpms, it just "should work (tm)" 😄

dgoodwin · 2016-12-06T15:29:09Z

/LGTM

luxas · 2016-12-06T15:36:06Z

Thanks @dgoodwin!

k8s-github-robot · 2016-12-07T01:41:33Z

Automatic merge from submit-queue (batch tested with PRs 38194, 37594, 38123, 37831, 37084)

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 1, 2016

luxas added the release-note-none Denotes a PR that doesn't merit a release note. label Dec 1, 2016

luxas assigned dgoodwin and mikedanese Dec 1, 2016

k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 1, 2016

kad reviewed Dec 1, 2016

View reviewed changes

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

dgoodwin suggested changes Dec 4, 2016

View reviewed changes

luxas force-pushed the improve_reset branch from 0ad0da3 to bf6b954 Compare December 4, 2016 16:52

luxas force-pushed the improve_reset branch from bf6b954 to 4c237c1 Compare December 4, 2016 18:12

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

luxas force-pushed the improve_reset branch from 4c237c1 to d350e98 Compare December 4, 2016 19:15

luxas mentioned this pull request Dec 4, 2016

kubeadm UX improvements for the v1.5 stable release #37568

Merged

luxas force-pushed the improve_reset branch from d350e98 to 65edcc0 Compare December 4, 2016 20:35

dgoodwin suggested changes Dec 5, 2016

View reviewed changes

luxas force-pushed the improve_reset branch from 65edcc0 to a3da04c Compare December 5, 2016 20:01

luxas added 2 commits December 6, 2016 14:45

Remove the cni directory when resetting; otherwise kubelet can pick u…

51d733f

…p the wrong config on the next kubeadm init run

Improve the kubeadm reset command. Reorder the functions and log more…

67d4dda

… user-friendly output

luxas force-pushed the improve_reset branch from a3da04c to 6fa116b Compare December 6, 2016 12:46

Drain node on kubeadm reset and make it possible to specify if the no…

7a463ef

…de should be removed from the cluster as well

luxas force-pushed the improve_reset branch from 6fa116b to 7a463ef Compare December 6, 2016 13:44

kad reviewed Dec 6, 2016

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 6, 2016

luxas mentioned this pull request Dec 6, 2016

Cordon, drain and remove a node from the cluster when running kubeadm reset kubernetes/kubeadm#42

Closed

k8s-github-robot merged commit 4eb4777 into kubernetes:master Dec 7, 2016


		func drainNode(removeNode bool) error {

		fmt.Println("[reset] Draining this node...")


		fmt.Printf("[reset] Draining node: %q\n", hostname)

		output, err = exec.Command("kubectl", "--kubeconfig", kubeConfigPath, "drain", hostname, "--delete-local-data", "--force", "--ignore-daemonsets").Output()

Improve kubeadm reset #37831

Improve kubeadm reset #37831

Conversation

luxas commented Dec 1, 2016 • edited

k8s-oncall commented Dec 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgoodwin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Dec 4, 2016

k8s-ci-robot commented Dec 4, 2016

k8s-ci-robot commented Dec 4, 2016

k8s-ci-robot commented Dec 4, 2016

k8s-ci-robot commented Dec 4, 2016

k8s-ci-robot commented Dec 4, 2016

luxas commented Dec 4, 2016

k8s-ci-robot commented Dec 4, 2016

k8s-ci-robot commented Dec 4, 2016

k8s-ci-robot commented Dec 4, 2016

k8s-ci-robot commented Dec 4, 2016

luxas commented Dec 5, 2016

dgoodwin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luxas commented Dec 5, 2016

k8s-ci-robot commented Dec 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgoodwin commented Dec 6, 2016

luxas commented Dec 6, 2016

k8s-github-robot commented Dec 7, 2016

luxas commented Dec 1, 2016 •

edited