Migrate pkg/kubelet/cm/memorymanager to structured logging #99974

knabben · 2021-03-09T01:56:49Z

What type of PR is this?

/kind feature
/sig instrumentation
/sig node

What this PR does / why we need it:

Migrate pkg/kubelet/cm/memorymanager to structured logging

Which issue(s) this PR fixes:

Refs #98976

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

pkg/kubelet/cm/memorymanager/memory_manager.go

pkg/kubelet/cm/memorymanager/policy_static.go

adisky · 2021-03-09T11:02:48Z

pkg/kubelet/cm/memorymanager/policy_static.go

 			return false
 		}

 		if len(nodeState1.MemoryMap) != len(nodeState2.MemoryMap) {
-			klog.Errorf("[memorymanager] node states memory map have different length: %d != %d", len(nodeState1.MemoryMap), len(nodeState2.MemoryMap))
+			klog.ErrorS(nil, "[memorymanager] Node states memory map have different length.", "len(state1.MemoryMap)", len(nodeState1.MemoryMap), "len(state2.MemoryMap)", len(nodeState2.MemoryMap))


Suggested change

klog.ErrorS(nil, "[memorymanager] Node states memory map have different length.", "len(state1.MemoryMap)", len(nodeState1.MemoryMap), "len(state2.MemoryMap)", len(nodeState2.MemoryMap))

klog.ErrorS(nil, "[memorymanager] Node states memory map have different length", "len1", len(nodeState1.MemoryMap), "len2", len(nodeState2.MemoryMap))

not sure here, keys could be state1 or state2 or lengthState1 lengthState2

i have this doubt, maybe x1 and x2 is enough for the context

adisky · 2021-03-09T11:05:47Z

pkg/kubelet/cm/memorymanager/policy_static.go

 			return false
 		}

 		if nodeState1.NumberOfAssignments != nodeState2.NumberOfAssignments {
-			klog.Errorf("[memorymanager] node states number of assignments are different: %d != %d", nodeState1.NumberOfAssignments, nodeState2.NumberOfAssignments)
+			klog.ErrorS(nil,"[memorymanager] Node states number of assignments are different.", "state1.NumberOfAssignments", nodeState1.NumberOfAssignments, "state2.NumberOfAssignments", nodeState2.NumberOfAssignments)


Suggested change

klog.ErrorS(nil,"[memorymanager] Node states number of assignments are different.", "state1.NumberOfAssignments", nodeState1.NumberOfAssignments, "state2.NumberOfAssignments", nodeState2.NumberOfAssignments)

klog.ErrorS(nil,"[memorymanager] Node states number of assignments are different.", "assignment1", nodeState1.NumberOfAssignments, "assignment2", nodeState2.NumberOfAssignments)

adisky · 2021-03-09T11:06:47Z

/triage accepted
/priority important-soon

lala123912 · 2021-03-09T10:03:15Z

pkg/kubelet/cm/memorymanager/fake_memory_manager.go

 	return NewPolicyNone()
 }

 func (m *fakeManager) Allocate(pod *v1.Pod, container *v1.Container) error {
-	klog.Infof("[fake memorymanager] Allocate (pod: %s, container: %s", pod.Name, container.Name)
+	klog.InfoS("[fake memorymanager] Allocate", "podName", pod.Name, "container", container.Name)


use klog.KRef() instead. https://github.com/kubernetes/community/blob/master/contributors/devel/sig-instrumentation/migration-to-structured-logging.md#use-klogkobj-and-klogkref-for-kubernetes-objects

I think these are only strings

pkg/kubelet/cm/memorymanager/fake_memory_manager.go

pkg/kubelet/cm/memorymanager/memory_manager.go

pkg/kubelet/cm/memorymanager/fake_memory_manager.go

pkg/kubelet/cm/memorymanager/policy_static.go

pkg/kubelet/cm/memorymanager/state/state_checkpoint.go

kikisdeliveryservice

this is a user facing change, please update pr description so this gets a release note. :)

note added :)

kikisdeliveryservice

Couple things but q: if we are deleting [memorymanager] should it also be deleted from fmt.Errorf("[memorymanager] failed to get the default NUMA affinity, no NUMA nodes with enough memory is available") in the same file (pkg/kubelet/cm/memorymanager/policy_static.go )?
cc: @ehashman

pkg/kubelet/cm/memorymanager/memory_manager.go

pkg/kubelet/cm/memorymanager/policy_static.go

knabben · 2021-03-12T02:53:09Z

/retest

kikisdeliveryservice · 2021-03-12T19:12:32Z

/test pull-kubernetes-unit

ehashman

One left :)

pkg/kubelet/cm/memorymanager/policy_static.go

ehashman · 2021-03-12T21:44:32Z

/milestone v1.21

knabben · 2021-03-13T15:19:15Z

/retest

pkg/kubelet/cm/memorymanager/policy_static.go

pkg/kubelet/cm/memorymanager/state/state_mem.go

pkg/kubelet/cm/memorymanager/memory_manager.go

pkg/kubelet/cm/memorymanager/policy_static.go

kikisdeliveryservice · 2021-03-14T23:53:02Z

pkg/kubelet/cm/memorymanager/memory_manager.go

@@ -228,13 +228,13 @@ func (m *manager) RemoveContainer(containerID string) error {
 	// if error appears it means container entry already does not exist under the container map
 	podUID, containerName, err := m.containerMap.GetContainerRef(containerID)
 	if err != nil {
-		klog.Warningf("[memorymanager] Failed to get container %s from container map error: %v", containerID, err)
+		klog.ErrorS(err, "Failed to get container from container map", "containerID", containerID)


You have quite a few Warningf->ErrorS - shouldn't they be InfoS??

Can you please check throughout

I think is correct to migrate Warningf to Info, but in this case should not be this an error log level?

moving it to klog.InfoS(..., "err", err), just looks odd to me

InfoS is the correct level for warnings. See the migration guide: https://github.com/kubernetes/community/blob/master/contributors/devel/sig-instrumentation/migration-to-structured-logging.md#change-log-functions-to-structured-equivalent

@ehashman yes, the point is the Warning is already being wrong here.. but changed according to the migration guide

pkg/kubelet/cm/memorymanager/state/state_checkpoint.go

pkg/kubelet/cm/memorymanager/policy_static.go

pkg/kubelet/cm/memorymanager/state/state_checkpoint.go

pkg/kubelet/cm/memorymanager/state/state_mem.go

kikisdeliveryservice · 2021-03-15T21:36:53Z

Thanks for all of our work on this @knabben and for updating with changes so quickly 👍

/lgtm

k8s-ci-robot · 2021-03-16T20:39:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: knabben, 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/cm/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 ehashman and matthyx March 9, 2021 01:56

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

adisky reviewed Mar 9, 2021

View reviewed changes

lala123912 reviewed Mar 9, 2021

View reviewed changes

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

knabben force-pushed the sl-memorymanager branch from d8fb9d3 to 286b6fe Compare March 9, 2021 13:33

ehashman mentioned this pull request Mar 10, 2021

Kubelet migration to structured logs #98976

Closed

ehashman suggested changes Mar 10, 2021

View reviewed changes

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

SergeyKanzhelev added this to Triage in SIG Node CI/Test Board Mar 10, 2021

knabben force-pushed the sl-memorymanager branch from 6ea6650 to d2a8932 Compare March 10, 2021 20:39

kikisdeliveryservice previously requested changes Mar 11, 2021

View reviewed changes

k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Mar 11, 2021

k8s-ci-robot removed the release-note-none Denotes a PR that doesn't merit a release note. label Mar 11, 2021

knabben force-pushed the sl-memorymanager branch from d2a8932 to 0126fb7 Compare March 12, 2021 00:18

kikisdeliveryservice reviewed Mar 12, 2021

View reviewed changes

knabben force-pushed the sl-memorymanager branch from 0126fb7 to 62f7787 Compare March 12, 2021 01:35

ehashman reviewed Mar 12, 2021

View reviewed changes

pkg/kubelet/cm/memorymanager/policy_static.go Outdated Show resolved Hide resolved

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

knabben force-pushed the sl-memorymanager branch from 62f7787 to 3eb25ba Compare March 13, 2021 14:26

kikisdeliveryservice reviewed Mar 14, 2021

View reviewed changes

knabben force-pushed the sl-memorymanager branch from 73e73e4 to 362c520 Compare March 15, 2021 12:24

shubheksha suggested changes Mar 15, 2021

View reviewed changes

pkg/kubelet/cm/memorymanager/policy_static.go Outdated Show resolved Hide resolved

pkg/kubelet/cm/memorymanager/state/state_checkpoint.go Outdated Show resolved Hide resolved

pkg/kubelet/cm/memorymanager/state/state_mem.go Outdated Show resolved Hide resolved

Memory manager structured logging

b078a4f

knabben force-pushed the sl-memorymanager branch from ee996c2 to b078a4f Compare March 15, 2021 19:51

kikisdeliveryservice moved this from Waiting on Author to Needs Approver in Structured Logging Migration for Kubelet, 1.21 Mar 15, 2021

k8s-ci-robot assigned kikisdeliveryservice Mar 15, 2021

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

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

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

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

k8s-ci-robot merged commit e5309ef into kubernetes:master Mar 16, 2021

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

Migrate pkg/kubelet/cm/memorymanager to structured logging #99974

Migrate pkg/kubelet/cm/memorymanager to structured logging #99974

knabben commented Mar 9, 2021 •

edited by liggitt

adisky Mar 9, 2021

adisky Mar 9, 2021

knabben Mar 9, 2021

adisky Mar 9, 2021

adisky commented Mar 9, 2021

lala123912 Mar 9, 2021

knabben Mar 9, 2021

kikisdeliveryservice left a comment

kikisdeliveryservice left a comment •

edited

knabben commented Mar 12, 2021

kikisdeliveryservice commented Mar 12, 2021

ehashman left a comment

ehashman commented Mar 12, 2021

knabben commented Mar 13, 2021

kikisdeliveryservice Mar 14, 2021

knabben Mar 15, 2021

knabben Mar 15, 2021

ehashman Mar 15, 2021

knabben Mar 15, 2021

kikisdeliveryservice commented Mar 15, 2021

k8s-ci-robot commented Mar 16, 2021

	klog.ErrorS(nil, "[memorymanager] Node states memory map have different length.", "len(state1.MemoryMap)", len(nodeState1.MemoryMap), "len(state2.MemoryMap)", len(nodeState2.MemoryMap))
	klog.ErrorS(nil, "[memorymanager] Node states memory map have different length", "len1", len(nodeState1.MemoryMap), "len2", len(nodeState2.MemoryMap))

	klog.ErrorS(nil,"[memorymanager] Node states number of assignments are different.", "state1.NumberOfAssignments", nodeState1.NumberOfAssignments, "state2.NumberOfAssignments", nodeState2.NumberOfAssignments)
	klog.ErrorS(nil,"[memorymanager] Node states number of assignments are different.", "assignment1", nodeState1.NumberOfAssignments, "assignment2", nodeState2.NumberOfAssignments)

Migrate pkg/kubelet/cm/memorymanager to structured logging #99974

Migrate pkg/kubelet/cm/memorymanager to structured logging #99974

Conversation

knabben 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?

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

adisky commented Mar 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kikisdeliveryservice left a comment

Choose a reason for hiding this comment

kikisdeliveryservice left a comment • edited

Choose a reason for hiding this comment

knabben commented Mar 12, 2021

kikisdeliveryservice commented Mar 12, 2021

ehashman left a comment

Choose a reason for hiding this comment

ehashman commented Mar 12, 2021

knabben 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

Choose a reason for hiding this comment

kikisdeliveryservice commented Mar 15, 2021

k8s-ci-robot commented Mar 16, 2021

knabben commented Mar 9, 2021 •

edited by liggitt

kikisdeliveryservice left a comment •

edited