Migrate pkg/kubelet/kuberuntime to structured logging #99970

krzysiekg · 2021-03-09T01:12:20Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Migrate pkg/kubelet/kuberuntime to structured logging

Which issue(s) this PR fixes:

Ref #98976

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

/sig node
/sig instrumentation
/priority important-soon

k8s-ci-robot · 2021-03-09T01:12:28Z

Hi @krzysiekg. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

shubheksha · 2021-03-09T11:19:12Z

pkg/kubelet/kuberuntime/kuberuntime_container_linux.go

 			continue
 		}

 		sizeString, err := v1helper.HugePageUnitSizeFromByteSize(pageSize.Value())
 		if err != nil {
-			klog.Warningf("pageSize is invalid: %v", err)
+			klog.InfoS("Hugepage size is invalid", "object", resourceObj, "err", err)


I wouldn't add Hugepage here

shubheksha · 2021-03-09T11:19:48Z

pkg/kubelet/kuberuntime/kuberuntime_container_windows.go

@@ -97,7 +97,7 @@ func (m *kubeGenericRuntimeManager) generateWindowsContainerConfig(container *v1
 	if wc.Resources.CpuCount > 0 {
 		if wc.Resources.CpuMaximum > 0 {
 			wc.Resources.CpuMaximum = 0
-			klog.Warningf("Mutually exclusive options: CPUCount priority > CPUMaximum priority on Windows Server Containers. CPUMaximum should be ignored")
+			klog.InfoS("Mutually exclusive options CPUCount and CPUMaximum present, CPUMaximum value ignored")


Why has the error message been modified here?

I was trying to simplify the message somehow, but I doesn't look good to me today. It's probably best to leave the original message intact. I reverted my changes.

shubheksha · 2021-03-09T11:48:20Z

/triage accepted

troy0820 · 2021-03-09T23:50:44Z

/ok-to-test

krzysiekg · 2021-03-10T08:06:23Z

/retest

krzysiekg · 2021-03-10T10:41:07Z

/retest

krzysiekg · 2021-03-10T18:28:29Z

/retest

kikisdeliveryservice

Apologies wrong PR..

krzysiekg · 2021-03-12T13:48:28Z

/assign @derekwaynecarr
/retest

ehashman · 2021-03-13T02:22:54Z

/skip

mrunalp · 2021-03-16T20:05:20Z

pkg/kubelet/kuberuntime/kuberuntime_sandbox.go

@@ -38,15 +38,15 @@ func (m *kubeGenericRuntimeManager) createPodSandbox(pod *v1.Pod, attempt uint32
 	podSandboxConfig, err := m.generatePodSandboxConfig(pod, attempt)
 	if err != nil {
 		message := fmt.Sprintf("GeneratePodSandboxConfig for pod %q failed: %v", format.Pod(pod), err)


Update the message as well.

I updated the messages. I also changed one more message that didn't have matching klog.ErrorS call to make all messages in this method consistent.

mrunalp · 2021-03-16T20:05:31Z

pkg/kubelet/kuberuntime/kuberuntime_sandbox.go

@@ -38,15 +38,15 @@ func (m *kubeGenericRuntimeManager) createPodSandbox(pod *v1.Pod, attempt uint32
 	podSandboxConfig, err := m.generatePodSandboxConfig(pod, attempt)
 	if err != nil {
 		message := fmt.Sprintf("GeneratePodSandboxConfig for pod %q failed: %v", format.Pod(pod), err)
-		klog.Error(message)
+		klog.ErrorS(err, "Failed to generate sandbox config for pod", "pod", klog.KObj(pod))
 		return "", message, err
 	}

 	// Create pod logs directory
 	err = m.osInterface.MkdirAll(podSandboxConfig.LogDirectory, 0755)
 	if err != nil {
 		message := fmt.Sprintf("Create pod log directory for pod %q failed: %v", format.Pod(pod), err)


Update the message as well.

mrunalp · 2021-03-16T20:05:47Z

pkg/kubelet/kuberuntime/kuberuntime_sandbox.go

@@ -58,14 +58,14 @@ func (m *kubeGenericRuntimeManager) createPodSandbox(pod *v1.Pod, attempt uint32
 			return "", message, err
 		}
 		if runtimeHandler != "" {
-			klog.V(2).Infof("Running pod %s with RuntimeHandler %q", format.Pod(pod), runtimeHandler)
+			klog.V(2).InfoS("Running pod with runtime handler", "pod", klog.KObj(pod), "runtimeHandler", runtimeHandler)
 		}
 	}

 	podSandBoxID, err := m.runtimeService.RunPodSandbox(podSandboxConfig, runtimeHandler)
 	if err != nil {
 		message := fmt.Sprintf("CreatePodSandbox for pod %q failed: %v", format.Pod(pod), err)


Update the message as well.

ehashman · 2021-03-17T00:48:50Z

Hi @krzysiekg, can you squash your commits? I think this is good to go otherwise

ehashman

/lgtm

pacoxu · 2021-03-17T05:46:19Z

/skip
/retest

k8s-ci-robot · 2021-03-17T07:07:40Z

@krzysiekg: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-azure-file-windows	`629d5ab`	link	`/test pull-kubernetes-e2e-azure-file-windows`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

krzysiekg · 2021-03-17T08:17:29Z

/skip

mrunalp · 2021-03-17T17:29:50Z

/approve

k8s-ci-robot · 2021-03-17T17:32:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ehashman, krzysiekg, 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/OWNERS~~ [mrunalp]

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 · 2021-03-17T17:32:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ehashman, krzysiekg, 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/OWNERS~~ [mrunalp]

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 dims and Random-Liu March 9, 2021 01:12

k8s-ci-robot added the area/kubelet label Mar 9, 2021

ehashman added this to Needs Reviewer in Structured Logging Migration for Kubelet, 1.21 Mar 9, 2021

ehashman added this to Triage in SIG Node PR Triage Mar 9, 2021

shubheksha reviewed Mar 9, 2021

View reviewed changes

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Mar 9, 2021

shubheksha moved this from Needs Reviewer to Waiting on Author in Structured Logging Migration for Kubelet, 1.21 Mar 9, 2021

krzysiekg force-pushed the structured_logging_pkg_kubelet_kuberuntime branch from c52ec01 to 4ab5fec Compare March 9, 2021 18:24

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 9, 2021

troy0820 moved this from Waiting on Author to Needs Reviewer in Structured Logging Migration for Kubelet, 1.21 Mar 9, 2021

ehashman mentioned this pull request Mar 10, 2021

Kubelet migration to structured logs #98976

Closed

kikisdeliveryservice reviewed Mar 11, 2021

View reviewed changes

k8s-ci-robot assigned derekwaynecarr Mar 12, 2021

mrunalp reviewed Mar 16, 2021

View reviewed changes

mrunalp moved this from Needs Approver to Waiting on Author in SIG Node PR Triage Mar 16, 2021

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

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

Migrate pkg/kubelet/kuberuntime to structured logging

629d5ab

krzysiekg force-pushed the structured_logging_pkg_kubelet_kuberuntime branch from 016bfd0 to 629d5ab Compare March 17, 2021 00:56

ehashman reviewed Mar 17, 2021

View reviewed changes

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

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

ehashman moved this from Waiting on Author to Needs Approver in SIG Node PR Triage Mar 17, 2021

mrunalp approved these changes Mar 17, 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 17, 2021

k8s-ci-robot merged commit 862aa6d into kubernetes:master Mar 17, 2021

SIG Node PR Triage automation moved this from Needs Approver to Done Mar 17, 2021

Structured Logging Migration for Kubelet, 1.21 automation moved this from Needs Approver to Done Mar 17, 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

rphillips mentioned this pull request Jun 24, 2021

add rphillips to sig-node-reviewers #103159

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate pkg/kubelet/kuberuntime to structured logging #99970

Migrate pkg/kubelet/kuberuntime to structured logging #99970

krzysiekg commented Mar 9, 2021 •

edited by liggitt

k8s-ci-robot commented Mar 9, 2021

shubheksha Mar 9, 2021

krzysiekg Mar 9, 2021

shubheksha Mar 9, 2021

krzysiekg Mar 9, 2021

shubheksha commented Mar 9, 2021

troy0820 commented Mar 9, 2021

krzysiekg commented Mar 10, 2021

krzysiekg commented Mar 10, 2021

krzysiekg commented Mar 10, 2021

kikisdeliveryservice left a comment •

edited

krzysiekg commented Mar 12, 2021

ehashman commented Mar 13, 2021

mrunalp Mar 16, 2021

krzysiekg Mar 17, 2021

mrunalp Mar 16, 2021

mrunalp Mar 16, 2021

ehashman commented Mar 17, 2021

ehashman left a comment

pacoxu commented Mar 17, 2021

k8s-ci-robot commented Mar 17, 2021

krzysiekg commented Mar 17, 2021

mrunalp commented Mar 17, 2021

k8s-ci-robot commented Mar 17, 2021

k8s-ci-robot commented Mar 17, 2021

Migrate pkg/kubelet/kuberuntime to structured logging #99970

Migrate pkg/kubelet/kuberuntime to structured logging #99970

Conversation

krzysiekg commented Mar 9, 2021 • edited by liggitt

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

k8s-ci-robot commented Mar 9, 2021

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

shubheksha commented Mar 9, 2021

troy0820 commented Mar 9, 2021

krzysiekg commented Mar 10, 2021

krzysiekg commented Mar 10, 2021

krzysiekg commented Mar 10, 2021

kikisdeliveryservice left a comment • edited

Choose a reason for hiding this comment

krzysiekg commented Mar 12, 2021

ehashman commented Mar 13, 2021

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 commented Mar 17, 2021

ehashman left a comment

Choose a reason for hiding this comment

pacoxu commented Mar 17, 2021

k8s-ci-robot commented Mar 17, 2021

krzysiekg commented Mar 17, 2021

mrunalp commented Mar 17, 2021

k8s-ci-robot commented Mar 17, 2021

k8s-ci-robot commented Mar 17, 2021

krzysiekg commented Mar 9, 2021 •

edited by liggitt

kikisdeliveryservice left a comment •

edited