kubelet: migrate pkg/kubelet/certificate to structured logging #98993
Conversation
|
Because this is updating the exit code of a Fatal log call, we need an /hold |
a0142a6 to
40a0205
Compare
Signed-off-by: SataQiu <shidaqiu2018@gmail.com>
40a0205 to
c88ef90
Compare
The |
|
/test pull-kubernetes-e2e-kind |
|
@ehashman @serathius Can we proceed with this PR? |
|
Follow the robot's instruction |
|
/test all |
| } else { | ||
| klog.Errorf("It has been %s since a valid client cert was found, but the server is not responsive. A restart may be necessary to retrieve new initial credentials.", exitAfter) | ||
| klog.ErrorS(nil, "No valid client certificate is found but the server is not responsive. A restart may be necessary to retrieve new initial credentials.", "lastCertificateAvailabilityTime", lastCertAvailable, "shutdownThreshold", exitAfter) |
There was a problem hiding this comment.
are the keys selected for messages like these considered a stable API log consumers can write extraction/monitoring around?
There was a problem hiding this comment.
Not as of beta but we're moving in that direction for GA. Right now we're trying to figure out a good (loose, non-enforced) schema that we can later enforce.
| } else { | ||
| klog.Errorf("The currently active client certificate has expired, but the server is not responsive. A restart may be necessary to retrieve new initial credentials.") | ||
| klog.ErrorS(nil, "The currently active client certificate has expired, but the server is not responsive. A restart may be necessary to retrieve new initial credentials.") |
There was a problem hiding this comment.
passing a nil error to ErrorS seems strange... should this do something like klog.ErrorS(errors.New("The currently active client...")) instead? (same question for several places above as well)
There was a problem hiding this comment.
Docs on calling ErrorS with nil: https://github.com/kubernetes/community/blob/master/contributors/devel/sig-instrumentation/migration-to-structured-logging.md#using-errors
Calling ErrorS with nil as error is semi-acceptable if there is error condition that deserves a stack trace at this origin point. For expected errors (errors that can happen during routine operations) please consider using klog.InfoS and pass error in err key instead.
I think this is an unexpected error so this is ok?
There was a problem hiding this comment.
it looks like the klog implementation gracefully omits the err=nil from the output:
kubernetes/vendor/k8s.io/klog/v2/klog.go
Lines 803 to 806 in c8fe1d9
but the structured logger always includes an err param, even if nil:
kubernetes/staging/src/k8s.io/component-base/logs/json/json.go
Lines 171 to 173 in c8fe1d9
is that expected by structured log consumers?
There was a problem hiding this comment.
Yes, that's right. We just get the log message with no named error, so it's equivalent to the current behaviour.
|
/retest |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: liggitt, 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 cleanup
What this PR does / why we need it:
Which issue(s) this PR fixes:
Ref #98976
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: