kubelet: migrate pkg/kubelet/certificate to structured logging #98993

SataQiu · 2021-02-11T10:12:17Z

What type of PR is this?

/kind cleanup

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?

Migrate `pkg/kubelet/certificate` to structured logging
ACTION REQUIRED: Exit code changed from 255 to 1

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

pkg/kubelet/certificate/transport.go

ehashman · 2021-02-11T22:55:24Z

Because this is updating the exit code of a Fatal log call, we need an ACTION REQUIRED: release note.

/hold

pkg/kubelet/certificate/bootstrap/bootstrap.go

pkg/kubelet/certificate/transport.go

Signed-off-by: SataQiu <shidaqiu2018@gmail.com>

SataQiu · 2021-02-18T01:47:00Z

Because this is updating the exit code of a Fatal log call, we need an ACTION REQUIRED: release note.

/hold

The ACTION REQUIRED was added to the release note.
And all the suggestions were applied.

SataQiu · 2021-02-18T12:20:42Z

/test pull-kubernetes-e2e-kind

SataQiu · 2021-02-26T03:50:20Z

@ehashman @serathius Can we proceed with this PR?

SataQiu · 2021-03-01T03:25:11Z

Follow the robot's instruction
/assign @liggitt

ehashman

/hold cancel
/lgtm

SataQiu · 2021-03-03T09:36:25Z

kindly ping @liggitt
@ehashman had already left LGTM.

SataQiu · 2021-03-03T09:36:35Z

/test all

liggitt · 2021-03-03T15:34:48Z

pkg/kubelet/certificate/transport.go

 					} 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)


are the keys selected for messages like these considered a stable API log consumers can write extraction/monitoring around?

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.

liggitt · 2021-03-03T15:36:31Z

pkg/kubelet/certificate/transport.go

 					} 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.")


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)

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?

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

if err != nil {

b.WriteByte(' ')

b.WriteString(fmt.Sprintf("err=%q", err.Error()))

}

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

func handleError(err error) zap.Field {

return zap.NamedError("err", err)

}

is that expected by structured log consumers?

Yes, that's right. We just get the log message with no named error, so it's equivalent to the current behaviour.

ehashman · 2021-03-04T00:30:48Z

/retest

liggitt · 2021-03-04T19:23:04Z

/approve

k8s-ci-robot · 2021-03-04T19:23:29Z

[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:

~~pkg/kubelet/certificate/OWNERS~~ [liggitt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot requested review from smarterclayton and sttts February 11, 2021 10:12

k8s-ci-robot added area/kubelet sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/node Categorizes an issue or PR as relevant to SIG Node. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Feb 11, 2021

endocrimes reviewed Feb 11, 2021

View reviewed changes

pkg/kubelet/certificate/transport.go Outdated Show resolved Hide resolved

kikisdeliveryservice added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Feb 11, 2021

k8s-ci-robot removed the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Feb 11, 2021

kikisdeliveryservice added this to Waiting on Author in SIG Node PR Triage Feb 11, 2021

kikisdeliveryservice added the triage/accepted Indicates an issue or PR is ready to be actively worked on. label Feb 11, 2021

k8s-ci-robot removed the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Feb 11, 2021

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 11, 2021

adisky reviewed Feb 15, 2021

View reviewed changes

pkg/kubelet/certificate/bootstrap/bootstrap.go Outdated Show resolved Hide resolved

pkg/kubelet/certificate/bootstrap/bootstrap.go Outdated Show resolved Hide resolved

pkg/kubelet/certificate/transport.go Outdated Show resolved Hide resolved

k8s-ci-robot added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Feb 18, 2021

SataQiu force-pushed the structlog-kubelet-20210211 branch from a0142a6 to 40a0205 Compare February 18, 2021 01:37

migrate to structured logging

c88ef90

Signed-off-by: SataQiu <shidaqiu2018@gmail.com>

SataQiu force-pushed the structlog-kubelet-20210211 branch from 40a0205 to c88ef90 Compare February 18, 2021 01:40

k8s-ci-robot assigned liggitt Mar 1, 2021

ehashman reviewed Mar 3, 2021

View reviewed changes

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 3, 2021

ehashman added this to Needs Approver in Structured Logging Migration for Kubelet, 1.21 Mar 3, 2021

k8s-ci-robot assigned ehashman Mar 3, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 3, 2021

liggitt reviewed Mar 3, 2021

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 4, 2021

ehashman moved this from Needs Approver to Done in Structured Logging Migration for Kubelet, 1.21 Mar 4, 2021

ehashman mentioned this pull request Mar 4, 2021

Kubelet migration to structured logs #98976

Closed

k8s-ci-robot merged commit 508a883 into kubernetes:master Mar 4, 2021

SIG Node PR Triage automation moved this from Waiting on Author to Done Mar 4, 2021

k8s-ci-robot added this to the v1.21 milestone Mar 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubelet: migrate pkg/kubelet/certificate to structured logging #98993

kubelet: migrate pkg/kubelet/certificate to structured logging #98993

SataQiu commented Feb 11, 2021 •

edited

ehashman commented Feb 11, 2021

SataQiu commented Feb 18, 2021

SataQiu commented Feb 18, 2021

SataQiu commented Feb 26, 2021

SataQiu commented Mar 1, 2021

ehashman left a comment

SataQiu commented Mar 3, 2021

SataQiu commented Mar 3, 2021

liggitt Mar 3, 2021

ehashman Mar 4, 2021

liggitt Mar 3, 2021 •

edited

ehashman Mar 4, 2021

liggitt Mar 4, 2021

ehashman Mar 4, 2021

ehashman commented Mar 4, 2021

liggitt commented Mar 4, 2021

k8s-ci-robot commented Mar 4, 2021

	if err != nil {
	b.WriteByte(' ')
	b.WriteString(fmt.Sprintf("err=%q", err.Error()))
	}

	func handleError(err error) zap.Field {
	return zap.NamedError("err", err)
	}

kubelet: migrate pkg/kubelet/certificate to structured logging #98993

kubelet: migrate pkg/kubelet/certificate to structured logging #98993

Conversation

SataQiu commented Feb 11, 2021 • edited

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.:

ehashman commented Feb 11, 2021

SataQiu commented Feb 18, 2021

SataQiu commented Feb 18, 2021

SataQiu commented Feb 26, 2021

SataQiu commented Mar 1, 2021

ehashman left a comment

Choose a reason for hiding this comment

SataQiu commented Mar 3, 2021

SataQiu commented Mar 3, 2021

liggitt Mar 3, 2021

Choose a reason for hiding this comment

ehashman Mar 4, 2021

Choose a reason for hiding this comment

liggitt Mar 3, 2021 • edited

Choose a reason for hiding this comment

ehashman Mar 4, 2021

Choose a reason for hiding this comment

liggitt Mar 4, 2021

Choose a reason for hiding this comment

ehashman Mar 4, 2021

Choose a reason for hiding this comment

ehashman commented Mar 4, 2021

liggitt commented Mar 4, 2021

k8s-ci-robot commented Mar 4, 2021

SataQiu commented Feb 11, 2021 •

edited

liggitt Mar 3, 2021 •

edited