Get the etcd data path from kubeadm config or etcd pod manifest on kubeadm reset #70003

yagonobre · 2018-10-19T06:08:21Z

What type of PR is this?
/kind bug

What this PR does / why we need it:
This clean up the correct etcd data path on kubeadm reset

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

Does this PR introduce a user-facing change?:

Kubeadm reset now clean up custom etcd data path

/sig cluster-lifecycle

neolit123 · 2018-10-19T08:53:57Z

/kind bug
/assign @fabriziopandini

@yagonobre
thank you, could you please add a release note explaining the change in one sentence.

neolit123

@yagonobre

thank you for this bug fix PR!
i have added some minor comments but in general - LGTM.

neolit123 · 2018-10-19T08:57:09Z

cmd/kubeadm/app/cmd/reset.go

+		if err == nil {
+			dirsToClean = append(dirsToClean, etcdDataDir)
+		} else {
+			glog.Warningf("[reset] WARNING: invalid etcd manifest, etcd data directorie will not be cleaned")


please change to:

glog.Warningf("[reset] etcd data path will not be cleaned: %v", err)

i'm assuming this is what we are going to get on external etcd?

neolit123 · 2018-10-19T09:00:37Z

cmd/kubeadm/app/cmd/reset.go

 	"k8s.io/kubernetes/pkg/util/initsystem"
 	utilsexec "k8s.io/utils/exec"
 )

+const etcdVolumeName = "etcd-data"


please move this constant to the function that uses it.

neolit123 · 2018-10-19T09:16:04Z

cmd/kubeadm/app/cmd/reset.go

@@ -175,6 +183,25 @@ func (r *Reset) Run(out io.Writer) error {
 	return nil
 }

+func getEtcdDataDir(manifestPath string) (string, error) {


SGTM, using the podspec volume.

neolit123 · 2018-10-19T09:16:54Z

cmd/kubeadm/app/cmd/reset_test.go

+
+		dataDir, err := getEtcdDataDir(manifestPath)
+		if (err != nil) != test.expectErr {
+			t.Errorf(


t.Fatalf()?

neolit123 · 2018-10-19T09:17:31Z

cmd/kubeadm/app/cmd/reset_test.go

+		dataDir, err := getEtcdDataDir(manifestPath)
+		if (err != nil) != test.expectErr {
+			t.Errorf(
+				"getEtcdDataDir failed\n%s\nexpected error: %t\n\tgot: %t\nerror: %v",


please use https://github.com/lithammer/dedent
we already vendor it in kubeadm.

neolit123 · 2018-10-19T09:17:53Z

cmd/kubeadm/app/cmd/reset_test.go

+		}
+
+		if dataDir != test.dataDir {
+			t.Errorf("getEtcdDataDir failed\n%s\n\texpected: %s\ngot: %s", name, test.dataDir, dataDir)


we should use t.Fatalf() + dedent here too.

neolit123 · 2018-10-19T09:18:19Z

cmd/kubeadm/app/cmd/reset_test.go

+		if test.writeManifest {
+			err := ioutil.WriteFile(manifestPath, []byte(test.podYaml), 0644)
+			if err != nil {
+				t.Fatalf("failed to write pod manifest\n%s\n\tfatal error: %v", name, err)


please use https://github.com/lithammer/dedent
we already vendor it in kubeadm.

yagonobre · 2018-10-19T14:45:53Z

@neolit123 updated

fabriziopandini

@yagonobre thanks for this PR!

My first impression reading this PR is that the etcd folder detection process should initially try to read the kubeadm config and only as a fallback parse the etcd manifest. This could have several advantages, especially if we consider all the variants of etcd we supports (external etcd Vs local etcd Vs stacked etcd)
What do you think about?

@timothysc @detiber opinions?

yagonobre · 2018-10-19T16:02:47Z

@fabriziopandini This will just apply when used local/stacked etcd and both use the same spec. But where I can get the kubeadm config? in the configmap?

fabriziopandini · 2018-10-24T15:20:59Z

@yagonobre
Yes, kubeadm config is a config map under kube-system namespace and we are using it as a "source of truth" about kubeadm clusters. Utilities for working with this config map are in util\config (even if some refactoring is still ongoing).

However, as suggested in my previous I would appreciate some other opinion about this and then decide if to iterate on this PR or if to leave this PR as it is (+ eventually open an issue for keeping track of the possible improvement with kubeadm config map)

timothysc · 2018-10-24T21:55:31Z

My first impression reading this PR is that the etcd folder detection process should initially try to read the kubeadm config and only as a fallback parse the etcd manifest. This could have several advantages, especially if we consider all the variants of etcd we supports (external etcd Vs local etcd Vs stacked etcd)
What do you think about?

agreed

/assign @timothysc

yagonobre · 2018-10-25T21:31:35Z

@fabriziopandini @timothysc updated!

fabriziopandini · 2018-10-26T09:09:14Z

/approve
/ok-to-test

yagonobre · 2018-10-26T11:46:30Z

/retest

yagonobre · 2018-10-26T14:34:34Z

/retest

k8s-ci-robot · 2018-10-26T14:46:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini, yagonobre

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:

~~cmd/kubeadm/OWNERS~~ [fabriziopandini]

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

k8s-ci-robot · 2018-10-26T14:46:37Z

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

Test name	Commit	Details	Rerun command
pull-kubernetes-local-e2e-containerized	8948828b8827bcdeea6b33cbde9f1fb9a349d240	link	`/test pull-kubernetes-local-e2e-containerized`

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.

yagonobre · 2018-10-26T14:48:33Z

/retest

fabriziopandini · 2018-10-26T15:18:49Z

@yagonobre it seems you have to run /hack/update-bazel.sh to fix your test

yagonobre · 2018-10-26T15:30:29Z

@fabriziopandini done

yagonobre · 2018-10-26T16:57:08Z

/remove-area kubelet
/remove-area kubectl
/remove-kind documentation
/remove-sig cli
/remove-sig node
/remove-sig storage
/remove-sig testing

neolit123 · 2018-10-28T17:38:49Z

@yagonobre please squash the commits to 1 or 2 (one for code, one for autogenerated bazel).
thanks.

…k on kubeadm reset

yagonobre · 2018-10-28T20:07:54Z

@neolit123 done

neolit123 · 2018-10-28T20:12:07Z

LGTM
waiting on CI and further comments, if any.
@kubernetes/sig-cluster-lifecycle-pr-reviews

timothysc

Thx for the patch!

Comments for next time, and they are non-blocking

/lgtm

timothysc · 2018-10-29T19:33:15Z

cmd/kubeadm/app/cmd/reset.go

@@ -175,6 +185,33 @@ func (r *Reset) Run(out io.Writer) error {
 	return nil
 }

+func getEtcdDataDir(manifestPath string, client clientset.Interface) (string, error) {
+	const etcdVolumeName = "etcd-data"


const for this really shouldn't be localized.

timothysc · 2018-10-29T19:33:42Z

cmd/kubeadm/app/cmd/reset.go

+
+	etcdPod, err := utilstaticpod.ReadStaticPodFromDisk(manifestPath)
+	if err != nil {
+		return "", err


it's weird to return empty strings vs. nil objects.

k8s-ci-robot requested review from chuckha and luxas October 19, 2018 06:08

k8s-ci-robot added the area/kubeadm label Oct 19, 2018

k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Oct 19, 2018

k8s-ci-robot assigned fabriziopandini Oct 19, 2018

k8s-ci-robot removed the needs-kind Indicates a PR lacks a `kind/foo` label and requires one. label Oct 19, 2018

neolit123 reviewed Oct 19, 2018

View reviewed changes

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Oct 19, 2018

fabriziopandini reviewed Oct 19, 2018

View reviewed changes

k8s-ci-robot assigned timothysc Oct 24, 2018

yagonobre changed the title ~~Get the etcd data path from etcd pod manifest on kubeadm reset~~ Get the etcd data path from kubeadm config or etcd pod manifest on kubeadm reset Oct 25, 2018

yagonobre force-pushed the kubeadm-clean-etcd-dir branch from ecc491c to 8d980bf Compare October 26, 2018 00:37

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 26, 2018

k8s-ci-robot added sig/storage Categorizes an issue or PR as relevant to SIG Storage. sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Oct 26, 2018

yagonobre force-pushed the kubeadm-clean-etcd-dir branch from 8948828 to 839e8da Compare October 26, 2018 14:46

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 26, 2018

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 26, 2018

yagonobre added 2 commits October 28, 2018 17:04

Get etcd data dir from kubeadm config or use etcd manifest as fallbac…

0c9e922

…k on kubeadm reset

Update bazel

b35e22a

yagonobre force-pushed the kubeadm-clean-etcd-dir branch from 839e8da to b35e22a Compare October 28, 2018 20:07

timothysc reviewed Oct 29, 2018

View reviewed changes

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

k8s-ci-robot merged commit ab61dcd into kubernetes:master Oct 29, 2018

yagonobre deleted the kubeadm-clean-etcd-dir branch November 4, 2018 02:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get the etcd data path from kubeadm config or etcd pod manifest on kubeadm reset #70003

Get the etcd data path from kubeadm config or etcd pod manifest on kubeadm reset #70003

yagonobre commented Oct 19, 2018 •

edited

neolit123 commented Oct 19, 2018

neolit123 left a comment

neolit123 Oct 19, 2018

neolit123 Oct 19, 2018

neolit123 Oct 19, 2018

neolit123 Oct 19, 2018

neolit123 Oct 19, 2018

neolit123 Oct 19, 2018

neolit123 Oct 19, 2018

yagonobre commented Oct 19, 2018

fabriziopandini left a comment

yagonobre commented Oct 19, 2018 •

edited

fabriziopandini commented Oct 24, 2018

timothysc commented Oct 24, 2018

yagonobre commented Oct 25, 2018

fabriziopandini commented Oct 26, 2018

yagonobre commented Oct 26, 2018

yagonobre commented Oct 26, 2018

k8s-ci-robot commented Oct 26, 2018

k8s-ci-robot commented Oct 26, 2018 •

edited

yagonobre commented Oct 26, 2018

fabriziopandini commented Oct 26, 2018

yagonobre commented Oct 26, 2018

yagonobre commented Oct 26, 2018

neolit123 commented Oct 28, 2018 •

edited

yagonobre commented Oct 28, 2018

neolit123 commented Oct 28, 2018

timothysc left a comment

timothysc Oct 29, 2018

timothysc Oct 29, 2018

Get the etcd data path from kubeadm config or etcd pod manifest on kubeadm reset #70003

Get the etcd data path from kubeadm config or etcd pod manifest on kubeadm reset #70003

Conversation

yagonobre commented Oct 19, 2018 • edited

neolit123 commented Oct 19, 2018

neolit123 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

yagonobre commented Oct 19, 2018

fabriziopandini left a comment

Choose a reason for hiding this comment

yagonobre commented Oct 19, 2018 • edited

fabriziopandini commented Oct 24, 2018

timothysc commented Oct 24, 2018

yagonobre commented Oct 25, 2018

fabriziopandini commented Oct 26, 2018

yagonobre commented Oct 26, 2018

yagonobre commented Oct 26, 2018

k8s-ci-robot commented Oct 26, 2018

k8s-ci-robot commented Oct 26, 2018 • edited

yagonobre commented Oct 26, 2018

fabriziopandini commented Oct 26, 2018

yagonobre commented Oct 26, 2018

yagonobre commented Oct 26, 2018

neolit123 commented Oct 28, 2018 • edited

yagonobre commented Oct 28, 2018

neolit123 commented Oct 28, 2018

timothysc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yagonobre commented Oct 19, 2018 •

edited

yagonobre commented Oct 19, 2018 •

edited

k8s-ci-robot commented Oct 26, 2018 •

edited

neolit123 commented Oct 28, 2018 •

edited