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: reset raises warnings if it cannot delete folders #85265
Conversation
87af36b
to
a33ac40
Compare
@@ -128,7 +128,7 @@ func resetConfigDir(configPathDir, pkiPathDir string) { | |||
fmt.Printf("[reset] Deleting contents of config directories: %v\n", dirsToClean) | |||
for _, dir := range dirsToClean { | |||
if err := CleanDir(dir); err != nil { | |||
klog.Warningf("[reset] Failed to remove directory: %q [%v]\n", dir, err) | |||
fmt.Printf("[reset] WARNING: Failed to delete contents of %q directory: [%v]\n", dir, err) |
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.
Reasons for using fmt.Printf
kubernetes/kubeadm#1913
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.
Should it be klog.V(1).Warningf
? [1]
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.
Not quite sure! But does klog
support klog.V(1).Warningf
?
I didn't find that usage from the code base (only klog.Warningf
could be found).
I believe the necessary warnings should be displayed by default. 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.
In klog, log levels exist and are applicable only to the "info" severity. :)
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 discussion in kubernetes/kubeadm#1913
is not clear yet. if users don't want klog output they can always pipe it to /dev/null as its printed to stderr.
let's keep the klog.Warningf for now and change all of those in a single PR once we decide after the discussion in kubernetes/kubeadm#1913
other than this LGTM
/test pull-kubernetes-node-e2e-containerd |
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 @SataQiu
/priority important-longterm
@@ -128,7 +128,7 @@ func resetConfigDir(configPathDir, pkiPathDir string) { | |||
fmt.Printf("[reset] Deleting contents of config directories: %v\n", dirsToClean) | |||
for _, dir := range dirsToClean { | |||
if err := CleanDir(dir); err != nil { | |||
klog.Warningf("[reset] Failed to remove directory: %q [%v]\n", dir, err) | |||
fmt.Printf("[reset] WARNING: Failed to delete contents of %q directory: [%v]\n", dir, err) |
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.
Should it be klog.V(1).Warningf
? [1]
cmd/kubeadm/app/cmd/reset.go
Outdated
phases.CleanDir(dir) | ||
klog.V(1).Infof("[reset] Deleting contents of %s", dir) | ||
if err := phases.CleanDir(dir); err != nil { | ||
fmt.Printf("[reset] WARNING: Failed to delete contents of %q directory: [%v]\n", dir, err) |
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.
[1]
a33ac40
to
e9526bf
Compare
/test pull-kubernetes-node-e2e-containerd |
e9526bf
to
c910730
Compare
@@ -128,7 +128,7 @@ func resetConfigDir(configPathDir, pkiPathDir string) { | |||
fmt.Printf("[reset] Deleting contents of config directories: %v\n", dirsToClean) | |||
for _, dir := range dirsToClean { | |||
if err := CleanDir(dir); err != nil { | |||
klog.Warningf("[reset] Failed to remove directory: %q [%v]\n", dir, err) | |||
klog.Warningf("[reset] Failed to delete contents of %q directory: %v\n", dir, err) |
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.
btw \n
are not needed for klog entries, they are on separate lines always.
c910730
to
b7b10fc
Compare
/test pull-kubernetes-node-e2e-containerd |
@@ -128,7 +128,7 @@ func resetConfigDir(configPathDir, pkiPathDir string) { | |||
fmt.Printf("[reset] Deleting contents of config directories: %v\n", dirsToClean) | |||
for _, dir := range dirsToClean { | |||
if err := CleanDir(dir); err != nil { | |||
klog.Warningf("[reset] Failed to remove directory: %q [%v]\n", dir, err) | |||
klog.Warningf("[reset] Failed to delete contents of %q directory: %v", dir, err) |
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.
Why we are changing this log message?
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.
Because CleanDir
doesn't remove the folder, it removes the contents of the folder.
Ref: https://github.com/kubernetes/kubernetes/blob/master/cmd/kubeadm/app/cmd/phases/reset/cleanupnode.go#L150
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.
/lgtm
/approve
let's wait for code freeze to lift.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: neolit123, SataQiu 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 |
What type of PR is this?
/kind bug
/kind feature
What this PR does / why we need it:
kubeadm: reset raises warnings if it cannot delete folders
Which issue(s) this PR fixes:
Fixes kubernetes/kubeadm#1914
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: