kubeadm: Add the option to cleanup the `tmp` directory #112172

chendave · 2022-09-01T06:51:59Z

The tmp is created by kubeadm but is never removed, the
size is expected to be expanded as time goes by.

Add one bool option to cleanup the tmp dir, the flag is
off by default.

Here is what I see on my desktop,

# du -sh /etc/kubernetes/tmp
2.2G    /etc/kubernetes/tmp

Signed-off-by: Dave Chen dave.chen@arm.com

/kind feature

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

kubeadm: add the flag "--cleanup-tmp-dir" for "kubeadm reset". It will cleanup the contents of "/etc/kubernetes/tmp". The flag is off by default.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

/sig cluster-lifecycle

k8s-ci-robot · 2022-09-01T06:52:08Z

@chendave: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

pacoxu · 2022-09-01T07:24:17Z

The flag may help for some use cases. The larger part of this dir would be back-up etcd data which may be very important for users.
Other files are backup manifests and dry-run data.

chendave · 2022-09-01T07:25:13Z

cmd/kubeadm/app/cmd/reset.go

@@ -215,11 +223,16 @@ func (r *resetData) Cfg() *kubeadmapi.InitConfiguration {
 	return r.cfg
 }

-// DryRun returns the DryRun flag.
+// DryRun returns the dryRun flag.


this is irrelevant, but I'd like to clean this up, pls let me know whether you want to revert this back.

chendave · 2022-09-01T07:33:55Z

The flag may help for some use cases. The larger part of this dir would be back-up etcd data which may be very important for users.
Other files are backup manifests and dry-run data.

Yes, it might be important but also might not after tearing down the whole cluster. So, I made the flag off by default.

My understanding is that those files are created by kubeadm, and kubeadm should have the responsibility to take care of it. Leaving it as is imho is definitely not a ideal approach.
Besides, the dir occupy too large space, and it grows day by day, unless someone really needs those file or else there should be a choice for end-user to clean them up.

neolit123 · 2022-09-01T07:36:00Z

cmd/kubeadm/app/cmd/phases/reset/cleanupnode.go

@@ -103,7 +104,14 @@ func runCleanupNode(c workflow.RunData) error {
 	if certsDir != kubeadmapiv1.DefaultCertificatesDir {
 		klog.Warningf("[reset] WARNING: Cleaning a non-default certificates directory: %q\n", certsDir)
 	}
-	dirsToClean = append(dirsToClean, certsDir)
+
+	// It's possible the tmp dir is overriden by Env, but kubeadm unables to know how that Env is defined


unables -> is unable to

neolit123 · 2022-09-01T07:38:47Z

cmd/kubeadm/app/cmd/reset.go

@@ -160,6 +164,10 @@ func AddResetFlags(flagSet *flag.FlagSet, resetOptions *resetOptions) {
 		&resetOptions.dryRun, options.DryRun, resetOptions.dryRun,
 		"Don't apply any changes; just output what would be done.",
 	)
+	flagSet.BoolVar(
+		&resetOptions.cleanupTmpDir, options.CleanupTmpDir, resetOptions.cleanupTmpDir,
+		"Indicate whether reset will cleanup the temp dir",


("Cleanup the %q directory", path.Join(kubeadmconstants.KubernetesDir, kubeadmconstants.TempDirForKubeadm))

to show the user what we are cleaning

neolit123 · 2022-09-01T07:40:17Z

cmd/kubeadm/app/cmd/phases/reset/cleanupnode.go

+	tempDir := path.Join(kubeadmconstants.KubernetesDir, kubeadmconstants.TempDirForKubeadm)
+	dirsToClean = append(dirsToClean, tempDir)
+	if r.CleanupTmpDir() {
+		dirsToClean = append(dirsToClean, certsDir)


shouldn't we always append certs dir and optionally clean the tmp dir?

aha, my fault :)

neolit123

thanks for the pr @chendave

@SataQiu @pacoxu I think i am +1 to this. WDYT?

neolit123 · 2022-09-01T07:45:05Z

/release-note-edit

kubeadm: add the flag "--cleanup-tmp-dir" for "kubeadm reset". It will cleanup the contents of "/etc/kubernetes/tmp". The flag is off by default.

neolit123 · 2022-09-01T07:46:15Z

I think i am +1 to this. WDYT?

on the other hand we discussed in the past we are not going to add more flags unless really needed. today users can cleanup manually.

pacoxu · 2022-09-01T07:46:59Z

@SataQiu @pacoxu I think i am +1 to this. WDYT?

+1 for the flag.

And I prefer to add an interactively confirm for removing backup data. (And the flag name is temp-dir. I am not sure if the flag can make users aware that this is a backup dir as well.)

chendave · 2022-09-01T08:41:29Z

Thanks for @neolit123 @pacoxu for the comments!

updated to fix the comments left by @neolit123 , we can continue to discuss the interactive confirmation.

chendave · 2022-09-01T09:18:22Z

I checked interactively confirmation, and I think it is a little over-killed in this case.

We need the confirmation when reset the cluster because the user might don't know what's going to happen next, but he should know what's going on if he explicitly enabled the the flag. Do we need his confirmation again when the flag is enabled?

I am fine with add a confirmation, but I personally don't think we need it in this case.

@neolit123 @SataQiu for thoughts?

neolit123 · 2022-09-01T09:26:27Z

don't we have like one global confirmation at the beginning of reset?

I think cleaning the etcd backup does need confirmation.

chendave · 2022-09-01T09:57:12Z

Confirmation is added, here is what you will see when the flag is enabled,

[reset] Stopping the kubelet service
[reset] Unmounting mounted directories in "/var/lib/kubelet"
[cleanup-tmp-dir] Cleanup the "/etc/kubernetes/tmp" directory will clean up all backup data created by kubeadm, e.g. etcd data. Are you sure you want to proceed? [y/N]:

@pacoxu @neolit123 pls help to review it again, thanks!

neolit123 · 2022-09-01T10:03:55Z

does reset now have multiple confirmation prompts? i thought we had only one, which i think is better and simpler.

chendave · 2022-09-01T10:23:29Z

Now it looks like,

W0901 10:18:55.389827  683315 preflight.go:58] [reset] WARNING: Changes made to this host by 'kubeadm init' or 'kubeadm join' will be reverted.
W0901 10:18:55.389887  683315 preflight.go:62] [cleanup-tmp-dir] WARNING: Cleanup the "/etc/kubernetes/tmp" directory will clean up all backup data created by kubeadm, e.g. etcd data.
[reset] Are you sure you want to proceed? [y/N]:

Btw, I inherit the flag to cleanup-node phase, so that the directory could be cleanup when the phase is called. But we won't get the confirmation when this phase is called only.

pacoxu · 2022-09-01T12:39:12Z

/lgtm

neolit123 · 2022-09-01T13:10:42Z

cmd/kubeadm/app/cmd/phases/reset/preflight.go

@@ -51,8 +53,14 @@ func runPreflight(c workflow.RunData) error {
 		return errors.New("preflight phase invoked with an invalid data struct")
 	}

-	if !r.ForceReset() && !r.DryRun() {
-		klog.Warning("[reset] WARNING: Changes made to this host by 'kubeadm init' or 'kubeadm join' will be reverted.")
+	if (!r.ForceReset() && !r.DryRun()) || r.CleanupTmpDir() {


i think we can just remove handling of the cleanup temp dir flag here and printing the warning below. the flag is opt in, so users know they passed it.

make sense, I will update and pls let me know whether that is what you meant.

neolit123 · 2022-09-01T13:12:25Z

cmd/kubeadm/app/cmd/phases/reset/cleanupnode.go

 	dirsToClean = append(dirsToClean, certsDir)
+	if r.CleanupTmpDir() {
+		// It's possible the tmp dir is overridden by Env, but kubeadm is unable to know how that Env is defined


is this comment accurate though? we are using hardcoded constants to form the path.
maybe we can remove the comment entirely.

The comments is valid, since the directory for dry-run could be overridden by a env.
pls see:

kubernetes/cmd/kubeadm/app/cmd/init.go

Lines 330 to 332 in 7f3f39b

if dryRunDir, err = kubeadmconstants.CreateTempDirForKubeadm(os.Getenv("KUBEADM_INIT_DRYRUN_DIR"), "kubeadm-init-dryrun"); err != nil {

return nil, errors.Wrap(err, "couldn't create a temporary directory")

}

I am thinking we might let other cmd able to redirect the tmp directory as well, for consistency. WDYT?

the tmp dir for dryrun is not the tmp dir for backup, they are different concepts. the dry run dir ends up in /tmp, while the other tmp dir is under /kubernetes.

that env override was only added to make integration tests happy for init. preferably we should not add similar overrides for more commands.

Am I miss something? If the env is not defined, dryrun will use the /etc/kubernetes/tmp, right?

ls /etc/kubernetes/tmp/kubeadm-init-dryrun328586547 admin.conf ca.crt front-proxy-ca.key kube-scheduler.yaml apiserver.crt ca.key front-proxy-client.crt sa.key ...

kubernetes/cmd/kubeadm/app/constants/constants.go

Lines 596 to 600 in 7f3f39b

func CreateTempDirForKubeadm(kubernetesDir, dirName string) (string, error) {

tempDir := path.Join(KubernetesDir, TempDirForKubeadm)

if len(kubernetesDir) != 0 {

tempDir = path.Join(kubernetesDir, TempDirForKubeadm)

}

But anyway, now that this env is only added for integration test, I agree that comments is not necessary.

If the env is not defined, dryrun will use the /etc/kubernetes/tmp, right?

ok, you are right.

i think we can drop the comment.

neolit123 · 2022-09-02T07:24:26Z

cmd/kubeadm/app/cmd/phases/reset/preflight.go

+		if r.CleanupTmpDir() {
+			tempDir := path.Join(kubeadmconstants.KubernetesDir, kubeadmconstants.TempDirForKubeadm)
+			klog.Warningf("[cleanup-tmp-dir] WARNING: Cleanup the %q directory will clean up all backup data created by kubeadm, e.g. etcd data.", tempDir)
+		}


Suggested change

if r.CleanupTmpDir() {

tempDir := path.Join(kubeadmconstants.KubernetesDir, kubeadmconstants.TempDirForKubeadm)

klog.Warningf("[cleanup-tmp-dir] WARNING: Cleanup the %q directory will clean up all backup data created by kubeadm, e.g. etcd data.", tempDir)

}

what i meant in my previous comment is that we can drop the tmp dir cleanup warning entirely.
-cleanup-tmp-dir just means "add another extra dir to cleanup"

Do we still need the check like

if (!r.ForceReset() && !r.DryRun()) || r.CleanupTmpDir())

So that this flag will hit the confirmation below as well?

i don't think so?
if -f or -dryrun are passed the prompt will not be shown. else it will be shown independent on additional flags.

well, got you, so we don't need any change here, will update soon.

The `tmp` is created by `kubeadm` but is never removed, the size is expected to be expanded as time goes by. Add one bool option to cleanup the `tmp` dir, the flag is off by default. Signed-off-by: Dave Chen <dave.chen@arm.com>

neolit123

thanks @chendave
/lgtm
/approve

k8s-ci-robot · 2022-09-02T09:53:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chendave, feroenmmek, 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

chendave · 2022-09-02T10:06:23Z

thanks @pacoxu @neolit123 for the review and comments!

k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Sep 1, 2022

k8s-ci-robot requested review from fabriziopandini and pacoxu September 1, 2022 06:52

k8s-ci-robot added the area/kubeadm label Sep 1, 2022

chendave force-pushed the remove_tmp branch from aa26812 to cccba19 Compare September 1, 2022 07:18

chendave commented Sep 1, 2022

View reviewed changes

neolit123 reviewed Sep 1, 2022

View reviewed changes

chendave force-pushed the remove_tmp branch from cccba19 to dc32e68 Compare September 1, 2022 08:40

chendave force-pushed the remove_tmp branch from dc32e68 to 683a816 Compare September 1, 2022 09:55

chendave force-pushed the remove_tmp branch from 683a816 to 461d77f Compare September 1, 2022 10:06

chendave force-pushed the remove_tmp branch from 461d77f to 5f45f6c Compare September 1, 2022 10:20

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 1, 2022

chendave force-pushed the remove_tmp branch from 5f45f6c to 17fbfd7 Compare September 1, 2022 10:27

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 1, 2022

k8s-ci-robot assigned pacoxu Sep 1, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 1, 2022

neolit123 reviewed Sep 1, 2022

View reviewed changes

chendave force-pushed the remove_tmp branch from 17fbfd7 to 38f0529 Compare September 2, 2022 01:46

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 2, 2022

feroenmmek approved these changes Sep 2, 2022

View reviewed changes

neolit123 reviewed Sep 2, 2022

View reviewed changes

chendave force-pushed the remove_tmp branch from 38f0529 to ad48586 Compare September 2, 2022 07:31

kubeadm: Add the option to cleanup the tmp directory

57551cc

The `tmp` is created by `kubeadm` but is never removed, the size is expected to be expanded as time goes by. Add one bool option to cleanup the `tmp` dir, the flag is off by default. Signed-off-by: Dave Chen <dave.chen@arm.com>

chendave force-pushed the remove_tmp branch from ad48586 to 57551cc Compare September 2, 2022 07:58

neolit123 reviewed Sep 2, 2022

View reviewed changes

k8s-ci-robot assigned neolit123 Sep 2, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 2, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 2, 2022

k8s-ci-robot merged commit 1913c6b into kubernetes:master Sep 2, 2022

k8s-ci-robot added this to the v1.26 milestone Sep 2, 2022

chendave deleted the remove_tmp branch September 2, 2022 10:06

	if dryRunDir, err = kubeadmconstants.CreateTempDirForKubeadm(os.Getenv("KUBEADM_INIT_DRYRUN_DIR"), "kubeadm-init-dryrun"); err != nil {
	return nil, errors.Wrap(err, "couldn't create a temporary directory")
	}

	func CreateTempDirForKubeadm(kubernetesDir, dirName string) (string, error) {
	tempDir := path.Join(KubernetesDir, TempDirForKubeadm)
	if len(kubernetesDir) != 0 {
	tempDir = path.Join(kubernetesDir, TempDirForKubeadm)
	}

kubeadm: Add the option to cleanup the tmp directory #112172

kubeadm: Add the option to cleanup the tmp directory #112172

Conversation

chendave commented Sep 1, 2022 • edited by k8s-ci-robot

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Sep 1, 2022

pacoxu commented Sep 1, 2022

Choose a reason for hiding this comment

chendave commented Sep 1, 2022

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 left a comment

Choose a reason for hiding this comment

neolit123 commented Sep 1, 2022

neolit123 commented Sep 1, 2022

pacoxu commented Sep 1, 2022 • edited

chendave commented Sep 1, 2022

chendave commented Sep 1, 2022

neolit123 commented Sep 1, 2022

chendave commented Sep 1, 2022

neolit123 commented Sep 1, 2022

chendave commented Sep 1, 2022 • edited

pacoxu commented Sep 1, 2022

neolit123 Sep 1, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neolit123 Sep 1, 2022 • edited

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 Sep 2, 2022 • edited

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 left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 2, 2022

chendave commented Sep 2, 2022

kubeadm: Add the option to cleanup the `tmp` directory #112172

kubeadm: Add the option to cleanup the `tmp` directory #112172

chendave commented Sep 1, 2022 •

edited by k8s-ci-robot

pacoxu commented Sep 1, 2022 •

edited

chendave commented Sep 1, 2022 •

edited

neolit123 Sep 1, 2022 •

edited

neolit123 Sep 1, 2022 •

edited

neolit123 Sep 2, 2022 •

edited