New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
kubeadm: Add the option to cleanup the tmp
directory
#112172
Conversation
@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 The 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. |
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. |
@@ -215,11 +223,16 @@ func (r *resetData) Cfg() *kubeadmapi.InitConfiguration { | |||
return r.cfg | |||
} | |||
|
|||
// DryRun returns the DryRun flag. | |||
// DryRun returns the dryRun flag. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is irrelevant, but I'd like to clean this up, pls let me know whether you want to revert this back.
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. |
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unables -> is unable to
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
cmd/kubeadm/app/cmd/reset.go
Outdated
@@ -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", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
("Cleanup the %q directory", path.Join(kubeadmconstants.KubernetesDir, kubeadmconstants.TempDirForKubeadm))
to show the user what we are cleaning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
tempDir := path.Join(kubeadmconstants.KubernetesDir, kubeadmconstants.TempDirForKubeadm) | ||
dirsToClean = append(dirsToClean, tempDir) | ||
if r.CleanupTmpDir() { | ||
dirsToClean = append(dirsToClean, certsDir) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't we always append certs dir and optionally clean the tmp dir?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aha, my fault :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/release-note-edit
|
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. |
Thanks for @neolit123 @pacoxu for the comments! updated to fix the comments left by @neolit123 , we can continue to discuss the interactive confirmation. |
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? |
don't we have like one global confirmation at the beginning of reset? I think cleaning the etcd backup does need confirmation. |
Confirmation is added, here is what you will see when the flag is enabled,
@pacoxu @neolit123 pls help to review it again, thanks! |
does reset now have multiple confirmation prompts? i thought we had only one, which i think is better and simpler. |
Now it looks like,
Btw, I inherit the flag to |
/lgtm |
@@ -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() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make sense, I will update and pls let me know whether that is what you meant.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this comment accurate though? we are using hardcoded constants to form the path.
maybe we can remove the comment entirely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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") | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am thinking we might let other cmd
able to redirect the tmp directory as well, for consistency. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But anyway, now that this env is only added for integration test, I agree that comments is not necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @chendave
/lgtm
/approve
[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:
Approvers can indicate their approval by writing |
thanks @pacoxu @neolit123 for the review and comments! |
The
tmp
is created bykubeadm
but is never removed, thesize is expected to be expanded as time goes by.
Add one bool option to cleanup the
tmp
dir, the flag isoff by default.
Here is what I see on my desktop,
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?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
/sig cluster-lifecycle