Execute the Run function of kubelet, Remove invalid comments and remove run function #110691
Conversation
k8s-ci-robot
added
release-note
|
/cc @endocrimes @dims |
There was a problem hiding this comment.
/cc @pohly
Or we modify to the way as kube-apiserver?
such as:
cmd/kubelet/app/server.go
Outdated
| if err != nil { | ||
| klog.ErrorS(err, "Failed to run kubelet") | ||
| } | ||
| return err |
There was a problem hiding this comment.
This seems odd. Why is the caller not printing the error when RunE returns it?
There was a problem hiding this comment.
I think the code is just made a judgment and did not print,
if err := command.Execute(); err != nil {
return 1
}
return 0
But I noticed that this code has been modified again recently.
@yxxhero, I noticed that the annotation describes
// kubelet uses a config file and does its own special
// parsing of flags and that config file. It initializes
// logging after it is done with that. Therefore it does
// not use cli.Run like other, simpler commands.
Is your modification correct?
There was a problem hiding this comment.
That comment should have been removed in 7a5af81 because clearly now kubelet does use cli.Run.
Originally, that comment was correct. InitLogs had to be called after flag parsing, and kubelet therefore couldn't use cli.run.
But that restriction has been lifted when allowing reconfiguration of the flush interval in 18bf5d2
There was a problem hiding this comment.
7a5af81 cannot be backported to 1.24. In 1.24, adding error printing to
kubernetes/cmd/kubelet/kubelet.go
Line 56 in 2a550bc
There was a problem hiding this comment.
Yeah I think you are right, Let me take a look,thanks
Yes, making the main function more like in other commands would be the preferred solution. I don't remember why that wasn't already done. |
yangjunmyfm192085
force-pushed
the
fixlogprint
branch
2 times, most recently
from
be4f779
to
1162ba3
Compare
yangjunmyfm192085
changed the title
There was a problem hiding this comment.
Due to the following modification, the issue that the main branch cannot print the log has been solved.
This pr is only used to delete invalid logs.
In the 1.24 branch I will take pohly's suggestion and resubmit pr to fix
7a5af81
| // kubelet uses a config file and does its own special | ||
| // parsing of flags and that config file. It initializes | ||
| // logging after it is done with that. Therefore it does | ||
| // not use cli.Run like other, simpler commands. |
There was a problem hiding this comment.
Can you also eliminate the run function? It serves no purpose anymore.
There was a problem hiding this comment.
And remove command.SetGlobalNormalizationFunc(cliflag.WordSepNormalizeFunc)?
That is done by cli.Run:
There was a problem hiding this comment.
Let me update
There was a problem hiding this comment.
I have updated. could you please take a look again, thanks? @pohly
yangjunmyfm192085
force-pushed
the
fixlogprint
branch
from
1162ba3
to
842abe9
Compare
k8s-ci-robot
added
size/S
yangjunmyfm192085
changed the title
|
/retest |
k8s-ci-robot
added
the
lgtm
k8s-ci-robot
added
triage/accepted
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mrunalp, pohly, yangjunmyfm192085 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 |
k8s-ci-robot
added
the
approved
What type of PR is this?
/kind bug
What this PR does / why we need it:
Run the Run function, when there is an error exit, print the error log
Which issue(s) this PR fixes:
Fixes #110690
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: