unexpected deleting of contents of mount points due to symbolic link … #77507

cuericlee · 2019-05-06T12:54:45Z

…of KubeletRunDirectory

symbolic link of KubeletRunDirectory cause unexpected deleting of contents of mount points

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change

/kind bug

/kind cleanup
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #
In case KubeletRunDirectory holds a symbolic link, evaluate it

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

kubeadm: fix a bug related to volume unmount if the kubelet run directory is a symbolic link

k8s-ci-robot · 2019-05-06T12:54:52Z

Hi @cuericlee. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

cuericlee · 2019-05-06T13:22:38Z

/test

neolit123

thanks for the PR @cuericlee
added some comments.

@kubernetes/sig-cluster-lifecycle-pr-reviews
/priority awaiting-more-evidence

neolit123 · 2019-05-06T22:33:19Z

cmd/kubeadm/app/cmd/reset.go

@@ -179,20 +179,27 @@ func (r *Reset) Run(out io.Writer, client clientset.Interface, cfg *kubeadmapi.I

 	// Try to unmount mounted directories under kubeadmconstants.KubeletRunDirectory in order to be able to remove the kubeadmconstants.KubeletRunDirectory directory later
 	fmt.Printf("[reset] Unmounting mounted directories in %q\n", kubeadmconstants.KubeletRunDirectory)
-	umountDirsCmd := fmt.Sprintf("awk '$2 ~ path {print $2}' path=%s/ /proc/mounts | xargs -r umount", kubeadmconstants.KubeletRunDirectory)
+
+	// In case KubeletRunDirectory is symbolic link, kubeadm failed to find the exact mount points, so it likely clean up content from mount points under KubeletRunDirectory


please change this comment to:

// In case KubeletRunDirectory holds a symbolic link, evaluate it

neolit123 · 2019-05-06T22:37:39Z

cmd/kubeadm/app/cmd/reset.go

+	} else {
+        // Once success to unmount mounted directories then removeAll contents of /var/lib/kubelet avoid to remove moounted filesystem by accident
+        dirsToClean = append(dirsToClean, absoluteKubeletRunDirectory)
+  }


this } seems out of place?

also please change your text editor to use TABs instead of spaces.

neolit123 · 2019-05-06T22:38:50Z

cmd/kubeadm/app/cmd/reset.go

-	}
+	} else {
+        // Once success to unmount mounted directories then removeAll contents of /var/lib/kubelet avoid to remove moounted filesystem by accident
+        dirsToClean = append(dirsToClean, absoluteKubeletRunDirectory)


i guess this makes some sense, but then again why is KubeletRunDirectory a symbolic link on your system?

This is issue reported by user, who intend to replace the /var/lib/kubelet with additional disk space. It cause this issue while kubeadm reset remove the data from the mount points. It looks like risky operation, but we have to prevent such condition by any case.

/ok-to-test

i'm not sure this is a setup we should support, but i personally don't mind the symbolic link evaluation....let's leave this for others to comment.

neolit123 · 2019-05-06T22:41:16Z

cmd/kubeadm/app/cmd/reset.go


 	klog.V(1).Infof("[reset] Executing command %q", umountDirsCmd)
 	umountOutputBytes, err := exec.Command("sh", "-c", umountDirsCmd).Output()
 	if err != nil {
 		klog.Errorf("[reset] Failed to unmount mounted directories in %s: %s\n", kubeadmconstants.KubeletRunDirectory, string(umountOutputBytes))
-	}
+	} else {
+        // Once success to unmount mounted directories then removeAll contents of /var/lib/kubelet avoid to remove moounted filesystem by accident


// Only clean absoluteKubeletRunDirectory if umountDirsCmd passed without error

cuericlee · 2019-05-07T02:44:34Z

/test

neolit123 · 2019-05-07T15:22:16Z

pull-kubernetes-verify — Job failed.

this job is failing because you are using spaces, instead of tabs in your text editor.
see here: https://prow.k8s.io/view/gcs/kubernetes-jenkins/pr-logs/pull/77507/pull-kubernetes-verify/1125770764637704193

cuericlee · 2019-05-08T02:32:28Z

fix gofmt
/retest

neolit123 · 2019-05-09T14:38:16Z

cmd/kubeadm/app/cmd/reset.go

+	var absoluteKubeletRunDirectory string
+	absoluteKubeletRunDirectory, err = filepath.EvalSymlinks(kubeadmconstants.KubeletRunDirectory)
+	if err != nil {
+		klog.Errorf("[reset] Failed to evaluate KubeletRunDirectory directories: %v\n", err)


directories -> directory

neolit123 · 2019-05-09T14:41:55Z

@cuericlee

please add a release note under Does this PR introduce a user-facing change?:

kubeadm: fix a bug related to volume unmount if the kubelet run directory is a symbolic link

waiting on more opinions by @kubernetes/sig-cluster-lifecycle-pr-reviews
i'm personally not a big fan of using /var/run/kubelet as a symbolic link. explanation from @cuericlee is here:
#77507 (comment)

rosti

Thanks for tackling this @cuericlee !

This can be avoided by replacing the symlink with a binding mount. However, since symlinks are popular and easy way to do this and not handling things properly is causing data loss, then it's best to merge the fix when it's ready.

rosti · 2019-05-09T16:08:49Z

cmd/kubeadm/app/cmd/reset.go

+	if err != nil {
+		klog.Errorf("[reset] Failed to evaluate KubeletRunDirectory directories: %v\n", err)
+	}
+	umountDirsCmd := fmt.Sprintf("awk '$2 ~ path {print $2}' path=%s/ /proc/mounts | xargs -r umount", absoluteKubeletRunDirectory)


If absoluteKubeletRunDirectory is empty string (which will be if EvalSymlinks returns error), the awk command will return all mount paths in the system. With that in mind, and the fact that we are running as root, we'll get everything dismounted by accident.
Hence, the whole unmount dirs stuff in here needs to be performed in an else clause appended to the if that checks for error in EvalSymlinks.

cuericlee · 2019-05-12T03:44:53Z

/retest

neolit123 · 2019-05-12T12:08:39Z

@cuericlee
thanks for the update, please squash your commits to one and add a release note as i said here:
#77507 (comment)

thanks
/approve
/hold

neolit123 · 2019-05-12T12:08:49Z

/retest

k8s-ci-robot · 2019-05-12T12:09:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cuericlee, neolit123

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~~ [neolit123]

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

cuericlee · 2019-05-13T07:48:08Z

squash is done
/retest

neolit123 · 2019-05-13T12:43:57Z

cmd/kubeadm/app/cmd/reset.go

 	if err != nil {
-		klog.Errorf("[reset] Failed to unmount mounted directories in %s: %s\n", kubeadmconstants.KubeletRunDirectory, string(umountOutputBytes))
+		klog.Errorf("[reset] Failed to evaluate KubeletRunDirectory directory and skip umount and clean: %v\n", err)


please change this to:

klog.Errorf("[reset] Failed to evaluate the %q directory. Skipping its unmount and cleanup: %v", kubeadmconstants.KubeletRunDirectory, err)

kubeadmconstants.KubeletRunDirectory is print out on line 181. But it is fine, I could refine log error message for easy pd accordingly.
fmt.Printf("[reset] Unmounting mounted directories in %q\n", kubeadmconstants.KubeletRunDirectory)

…tory is a symbolic link unexpected deleting of contents of mount points due to symbolic link of KubeletRunDirectory

cuericlee · 2019-05-14T12:37:04Z

/retest

neolit123 · 2019-05-14T12:41:27Z

cmd/kubeadm/app/cmd/reset.go

+		// Only unmount mount points which start with "/var/lib/kubelet" or absolute path of symbolic link, and avoid using empty absoluteKubeletRunDirectory
+		umountDirsCmd := fmt.Sprintf("awk '$2 ~ path {print $2}' path=%s/ /proc/mounts | xargs -r umount", absoluteKubeletRunDirectory)
+		klog.V(1).Infof("[reset] Executing command %q", umountDirsCmd)
+		umountOutputBytes, err := exec.Command("sh", "-c", umountDirsCmd).Output()


this might need a CombinedOutput instead, but since it was part of the old code let's leave it as is...

neolit123 · 2019-05-14T12:41:53Z

/lgtm

cuericlee · 2019-05-19T11:28:52Z

@neo

/lgtm

@neolit123 anything is needed from me to make merge happen? It looks still hold.

neolit123 · 2019-05-19T12:12:11Z

/hold cancel

fejta-bot · 2019-05-19T15:29:55Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

k8s-ci-robot requested review from kad and luxas May 6, 2019 12:56

neolit123 reviewed May 6, 2019

View reviewed changes

k8s-ci-robot added priority/awaiting-more-evidence Lowest priority. Possibly useful, but not yet enough support to actually get it done. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels May 6, 2019

neolit123 reviewed May 6, 2019

View reviewed changes

neolit123 reviewed May 9, 2019

View reviewed changes

rosti suggested changes May 9, 2019

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 12, 2019

cuericlee force-pushed the patch-1 branch from 9464a6f to 73bdae7 Compare May 13, 2019 07:45

neolit123 reviewed May 13, 2019

View reviewed changes

kubeadm: fix a bug related to volume unmount if the kubelet run direc…

56ce743

…tory is a symbolic link unexpected deleting of contents of mount points due to symbolic link of KubeletRunDirectory

cuericlee force-pushed the patch-1 branch from 73bdae7 to 56ce743 Compare May 14, 2019 12:34

neolit123 reviewed May 14, 2019

View reviewed changes

k8s-ci-robot assigned neolit123 May 14, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 14, 2019

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 19, 2019

k8s-ci-robot merged commit c854f72 into kubernetes:master May 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

unexpected deleting of contents of mount points due to symbolic link … #77507

unexpected deleting of contents of mount points due to symbolic link … #77507

cuericlee commented May 6, 2019 •

edited

k8s-ci-robot commented May 6, 2019

cuericlee commented May 6, 2019

neolit123 left a comment

neolit123 May 6, 2019

cuericlee May 7, 2019

neolit123 May 6, 2019

cuericlee May 7, 2019

neolit123 May 6, 2019

cuericlee May 7, 2019

neolit123 May 7, 2019 •

edited

neolit123 May 6, 2019

cuericlee May 7, 2019

cuericlee commented May 7, 2019

neolit123 commented May 7, 2019

cuericlee commented May 8, 2019 •

edited

neolit123 May 9, 2019

neolit123 commented May 9, 2019

rosti left a comment

rosti May 9, 2019

cuericlee commented May 12, 2019

neolit123 commented May 12, 2019

neolit123 commented May 12, 2019

k8s-ci-robot commented May 12, 2019

cuericlee commented May 13, 2019

neolit123 May 13, 2019 •

edited

cuericlee May 14, 2019

cuericlee commented May 14, 2019

neolit123 May 14, 2019

neolit123 commented May 14, 2019

cuericlee commented May 19, 2019

neolit123 commented May 19, 2019

fejta-bot commented May 19, 2019

unexpected deleting of contents of mount points due to symbolic link … #77507

unexpected deleting of contents of mount points due to symbolic link … #77507

Conversation

cuericlee commented May 6, 2019 • edited

k8s-ci-robot commented May 6, 2019

cuericlee commented May 6, 2019

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

neolit123 May 7, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cuericlee commented May 7, 2019

neolit123 commented May 7, 2019

cuericlee commented May 8, 2019 • edited

Choose a reason for hiding this comment

neolit123 commented May 9, 2019

rosti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cuericlee commented May 12, 2019

neolit123 commented May 12, 2019

neolit123 commented May 12, 2019

k8s-ci-robot commented May 12, 2019

cuericlee commented May 13, 2019

neolit123 May 13, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cuericlee commented May 14, 2019

Choose a reason for hiding this comment

neolit123 commented May 14, 2019

cuericlee commented May 19, 2019

neolit123 commented May 19, 2019

fejta-bot commented May 19, 2019

cuericlee commented May 6, 2019 •

edited

neolit123 May 7, 2019 •

edited

cuericlee commented May 8, 2019 •

edited

neolit123 May 13, 2019 •

edited