pkg/kubelet/server: migrate to structured logs #98643

chenyw1990 · 2021-02-01T02:59:00Z

What type of PR is this?
/kind cleanup

What this PR does / why we need it:

Which issue(s) this PR fixes:
https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/1602-structured-logging

https://github.com/kubernetes/community/blob/master/contributors/devel/sig-instrumentation/migration-to-structured-logging.md

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

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

chenyw1990 · 2021-02-01T06:41:42Z

/retest

ehashman

This looks pretty good to me, I had a few small suggestions.

/assign @serathius
/triage accepted
/priority important-longterm

I'm not sure if structured logging is targeted for 1.21 so leaving as important-longterm as opposed to soon.

ehashman · 2021-02-01T19:12:08Z

pkg/kubelet/server/server.go

@@ -145,7 +145,7 @@ func ListenAndServeKubeletServer(
 	enableDebuggingHandlers,
 	enableContentionProfiling,
 	enableSystemLogHandler bool) {
-	klog.Infof("Starting to listen on %s:%d", address, port)
+	klog.InfoS("Starting to listen server", "address", address, "port", port)


Suggested change

klog.InfoS("Starting to listen server", "address", address, "port", port)

klog.InfoS("Starting to listen", "address", address, "port", port)

Thanks for the advice, I have fixed it.

ehashman · 2021-02-01T19:12:20Z

pkg/kubelet/server/server.go

@@ -167,7 +167,7 @@ func ListenAndServeKubeletServer(

 // ListenAndServeKubeletReadOnlyServer initializes a server to respond to HTTP network requests on the Kubelet.
 func ListenAndServeKubeletReadOnlyServer(host HostInterface, resourceAnalyzer stats.ResourceAnalyzer, address net.IP, port uint, enableCAdvisorJSONEndpoints bool) {
-	klog.V(1).Infof("Starting to listen read-only on %s:%d", address, port)
+	klog.V(1).InfoS("Starting to listen read-only server", "address", address, "port", port)


Suggested change

klog.V(1).InfoS("Starting to listen read-only server", "address", address, "port", port)

klog.V(1).InfoS("Starting to listen read-only", "address", address, "port", port)

Does it make any sense that this is V(1) and the previous server is always logged? Maybe we should just always log both...

Thanks for the advice, I have fixed it. I also think V(1) is redundant, I've deleted it.

ehashman

/lgtm

serathius · 2021-02-05T14:20:35Z

pkg/kubelet/server/server.go

 			msg := fmt.Sprintf("Authorization error (user=%s, verb=%s, resource=%s, subresource=%s)", attrs.GetUser().GetName(), attrs.GetVerb(), attrs.GetResource(), attrs.GetSubresource())
-			klog.Errorf(msg, err)
+			klog.ErrorS(err, msg)


Please pass all arguments directly to call

klog.ErrorS(err, "Authorization error", "user" attrs.GetUser().GetName(), "verb", attrs.GetVerb(), "resource", attrs.GetResouce(), "subresource", attrs.GetSubresource())

serathius · 2021-02-05T14:22:08Z

pkg/kubelet/server/auth.go

@@ -109,7 +109,7 @@ func (n nodeAuthorizerAttributesGetter) GetRequestAttributes(u user.Info, r *htt
 		attrs.Subresource = "spec"
 	}

-	klog.V(5).Infof("Node request attributes: user=%#v attrs=%#v", attrs.GetUser(), attrs)
+	klog.V(5).InfoS("Node request attributes", "user", attrs.GetUser(), "attrs", attrs)


Should attrs be replaced with something like

klog.V(5).InfoS("Node request attributes"", "user", attrs.GetUser().GetName(), "verb", attrs.GetVerb(), "resource", attrs.GetResource(), "subresource", attrs.GetSubresource())

?

serathius · 2021-02-05T14:23:35Z

pkg/kubelet/server/stats/handler.go

@@ -295,7 +295,7 @@ func (h *handler) handlePodContainer(request *restful.Request, response *restful

 	pod, ok := h.provider.GetPodByName(params["namespace"], params["podName"])
 	if !ok {
-		klog.V(4).Infof("Container not found: %v", params)
+		klog.V(4).InfoS("Container not found", "params", params)


Suggested change

klog.V(4).InfoS("Container not found", "params", params)

klog.V(4).InfoS("Container not found", "pod", klog.KRef(params["namespace"], params["podName"]))

serathius · 2021-02-05T14:26:01Z

pkg/kubelet/server/stats/volume_stat_calculator.go

@@ -109,7 +109,7 @@ func (s *volumeStatCalculator) calcAndStoreStats() {
 		if err != nil {
 			// Expected for Volumes that don't support Metrics
 			if !volume.IsNotSupported(err) {
-				klog.V(4).Infof("Failed to calculate volume metrics for pod %s volume %s: %+v", format.Pod(s.pod), name, err)
+				klog.V(4).InfoS("Failed to calculate volume metrics", "pod", format.Pod(s.pod), "volumeName", name, "err", err)


I don't know what format.Pod does but we should use:

klog.KObj if s.pod is an object

klog.KRef otherwise if we have access to pod name and pod namespace

format.Pod returns a string of the format pod.Name, pod.Namespace, pod.UID
https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/util/format/pod.go#L32.

I guess then in this case it is right to go with klog.KObj?

serathius

Left some comments

serathius · 2021-02-08T12:58:11Z

Looks good, please fix test failures.

chenyw1990 · 2021-02-09T07:16:24Z

/retest

mrunalp · 2021-02-09T17:35:49Z

/approve

k8s-ci-robot · 2021-02-09T17:38:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chenyw1990, mrunalp

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/server/OWNERS~~ [mrunalp]

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

serathius · 2021-02-09T19:09:54Z

/lgtm

fejta-bot · 2021-02-09T22:28:04Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

fejta-bot · 2021-02-10T01:16:04Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

chenyw1990 · 2021-02-10T01:48:02Z

/test pull-kubernetes-e2e-kind

fejta-bot · 2021-02-10T04:25:09Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

k8s-ci-robot requested review from dchen1107 and Random-Liu February 1, 2021 02:59

k8s-ci-robot added area/kubelet 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 1, 2021

ehashman reviewed Feb 1, 2021

View reviewed changes

k8s-ci-robot assigned serathius Feb 1, 2021

chenyw1990 force-pushed the structuredLogsOfKubeletServer branch from f53424b to cd3385c Compare February 2, 2021 01:18

ehashman reviewed Feb 3, 2021

View reviewed changes

k8s-ci-robot assigned ehashman Feb 3, 2021

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

serathius reviewed Feb 5, 2021

View reviewed changes

serathius suggested changes Feb 5, 2021

View reviewed changes

This was referenced Feb 8, 2021

migrate kubelet/server/server.go logs to structured logging #98824

Closed

migrate logs to structed logging #98634

Closed

chenyw1990 force-pushed the structuredLogsOfKubeletServer branch from cd3385c to 924bace Compare February 8, 2021 11:47

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 8, 2021

pkg/kubelet/server: migrate to structured logs

db5de91

chenyw1990 force-pushed the structuredLogsOfKubeletServer branch from 924bace to db5de91 Compare February 9, 2021 07:38

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

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

k8s-ci-robot merged commit 071bdc3 into kubernetes:master Feb 10, 2021

k8s-ci-robot added this to the v1.21 milestone Feb 10, 2021

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Mar 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pkg/kubelet/server: migrate to structured logs #98643

pkg/kubelet/server: migrate to structured logs #98643

chenyw1990 commented Feb 1, 2021 •

edited by liggitt

Loading

chenyw1990 commented Feb 1, 2021

ehashman left a comment

ehashman Feb 1, 2021

chenyw1990 Feb 2, 2021

ehashman Feb 1, 2021

ehashman Feb 1, 2021

chenyw1990 Feb 2, 2021

ehashman left a comment

serathius Feb 5, 2021 •

edited

Loading

serathius Feb 5, 2021

serathius Feb 5, 2021

serathius Feb 5, 2021

adisky Feb 8, 2021

serathius Feb 9, 2021

serathius left a comment

serathius commented Feb 8, 2021

chenyw1990 commented Feb 9, 2021

mrunalp commented Feb 9, 2021

k8s-ci-robot commented Feb 9, 2021

serathius commented Feb 9, 2021

fejta-bot commented Feb 9, 2021

fejta-bot commented Feb 10, 2021

chenyw1990 commented Feb 10, 2021

fejta-bot commented Feb 10, 2021

	klog.InfoS("Starting to listen server", "address", address, "port", port)
	klog.InfoS("Starting to listen", "address", address, "port", port)

	klog.V(1).InfoS("Starting to listen read-only server", "address", address, "port", port)
	klog.V(1).InfoS("Starting to listen read-only", "address", address, "port", port)

	klog.V(4).InfoS("Container not found", "params", params)
	klog.V(4).InfoS("Container not found", "pod", klog.KRef(params["namespace"], params["podName"]))

pkg/kubelet/server: migrate to structured logs #98643

pkg/kubelet/server: migrate to structured logs #98643

Conversation

chenyw1990 commented Feb 1, 2021 • edited by liggitt Loading

chenyw1990 commented Feb 1, 2021

ehashman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehashman left a comment

Choose a reason for hiding this comment

serathius Feb 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serathius left a comment

Choose a reason for hiding this comment

serathius commented Feb 8, 2021

chenyw1990 commented Feb 9, 2021

mrunalp commented Feb 9, 2021

k8s-ci-robot commented Feb 9, 2021

serathius commented Feb 9, 2021

fejta-bot commented Feb 9, 2021

fejta-bot commented Feb 10, 2021

chenyw1990 commented Feb 10, 2021

fejta-bot commented Feb 10, 2021

chenyw1990 commented Feb 1, 2021 •

edited by liggitt

Loading

serathius Feb 5, 2021 •

edited

Loading