Structured Logging migration: modify eviction part logs of kubelet. #99032

yangjunmyfm192085 · 2021-02-12T15:35:49Z

Signed-off-by: JunYang yang.jun22@zte.com.cn

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Structured Logging migration: modify eviction part logs of kubelet.
xref:
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

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Migrate `pkg/kubelet/(eviction)` to structured logging

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

k8s-ci-robot · 2021-02-12T15:35:57Z

Hi @yangjunmyfm192085. 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.

SergeyKanzhelev · 2021-02-17T18:29:29Z

/ok-to-test
/priority important-soon
/triage accepted

yangjunmyfm192085 · 2021-02-17T20:26:49Z

/test pull-kubernetes-e2e-gce-100-performance

SergeyKanzhelev · 2021-02-17T20:38:46Z

pkg/kubelet/eviction/eviction_manager.go

@@ -196,7 +196,7 @@ func (m *managerImpl) Start(diskInfoProvider DiskInfoProvider, podFunc ActivePod
 	go func() {
 		for {
 			if evictedPods := m.synchronize(diskInfoProvider, podFunc); evictedPods != nil {
-				klog.Infof("eviction manager: pods %s evicted, waiting for pod to be cleaned up", format.Pods(evictedPods))
+				klog.InfoS("Eviction manager: pods evicted, waiting for pod to be cleaned up", "pods", format.Pods(evictedPods))


is it expected that on line 168 the klog.KObj(attrs.Pod) used and here is format.Pods is used?

I want to use klog.KObj(attrs.Pod), But I think evictedPods(multiple pods) is not work here,so I keep it not change.

@serathius what should we be doing in the case of multiple pods like this? Do you think this is ok?

I think we should migrate it to klog.KObj to support Json and make changes in formatting easier. We didn't add specific methods for multiple pods as we weren't sure how often this case will came up.

This is the first case using, so for now let's just implement a simple for loop.

SergeyKanzhelev · 2021-02-17T20:39:49Z

pkg/kubelet/eviction/eviction_manager.go

 		m.synchronize(diskInfoProvider, podFunc)
 	}
 	if m.config.KernelMemcgNotification {
 		for _, threshold := range m.config.Thresholds {
 			if threshold.Signal == evictionapi.SignalMemoryAvailable || threshold.Signal == evictionapi.SignalAllocatableMemoryAvailable {
 				notifier, err := NewMemoryThresholdNotifier(threshold, m.config.PodCgroupRoot, &CgroupNotifierFactory{}, thresholdHandler)
 				if err != nil {
-					klog.Warningf("eviction manager: failed to create memory threshold notifier: %v", err)
+					klog.InfoS("Eviction manager: failed to create memory threshold notifier", "err", err)


why warning is replaced with Informational?

I modified it according to the following rules
https://github.com/kubernetes/community/blob/master/contributors/devel/sig-instrumentation/migration-to-structured-logging.md

hm, interesting. thank you!

SergeyKanzhelev · 2021-02-17T20:40:40Z

pkg/kubelet/eviction/eviction_manager.go

 		return nil
 	}

 	if m.clock.Since(m.thresholdsLastUpdated) > notifierRefreshInterval {
 		m.thresholdsLastUpdated = m.clock.Now()
 		for _, notifier := range m.thresholdNotifiers {
 			if err := notifier.UpdateThreshold(summary); err != nil {
-				klog.Warningf("eviction manager: failed to update %s: %v", notifier.Description(), err)
+				klog.InfoS("Eviction manager: failed to update notifier", "notifier", notifier.Description(), "err", err)


warning replaced by Informational?

I modified it according to the following rules
https://github.com/kubernetes/community/blob/master/contributors/devel/sig-instrumentation/migration-to-structured-logging.md

This is correct---"warning" log messages are being deprecated.

SergeyKanzhelev · 2021-02-17T20:41:43Z

pkg/kubelet/eviction/eviction_manager.go

@@ -296,7 +296,7 @@ func (m *managerImpl) synchronize(diskInfoProvider DiskInfoProvider, podFunc Act
 	// node conditions report true if it has been observed within the transition period window
 	nodeConditions = nodeConditionsObservedSince(nodeConditionsLastObservedAt, m.config.PressureTransitionPeriod, now)
 	if len(nodeConditions) > 0 {
-		klog.V(3).Infof("eviction manager: node conditions - transition period not met: %v", nodeConditions)
+		klog.V(3).InfoS("Eviction manager: node conditions - transition period not met", "nodeCondition", nodeConditions)


how the nodeConditions would be formatted? Will %v be applied by default?

I modified it according to the following rules
https://github.com/kubernetes/community/blob/master/contributors/devel/sig-instrumentation/migration-to-structured-logging.md

SergeyKanzhelev · 2021-02-17T20:42:20Z

pkg/kubelet/eviction/eviction_manager.go

@@ -336,36 +336,36 @@ func (m *managerImpl) synchronize(diskInfoProvider DiskInfoProvider, podFunc Act
 	if !foundAny {
 		return nil
 	}
-	klog.Warningf("eviction manager: attempting to reclaim %v", resourceToReclaim)
+	klog.InfoS("Eviction manager: attempting to reclaim", "resource", resourceToReclaim)


again, warning replaced with informational. I will not comment further on this

Thak you very much.
I modified it according to the following rules,Help me see if it works?

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

ehashman

This is great, thanks, just had one question.

/cc @serathius

ehashman · 2021-02-19T18:36:26Z

pkg/kubelet/eviction/eviction_manager.go

@@ -196,7 +196,7 @@ func (m *managerImpl) Start(diskInfoProvider DiskInfoProvider, podFunc ActivePod
 	go func() {
 		for {
 			if evictedPods := m.synchronize(diskInfoProvider, podFunc); evictedPods != nil {
-				klog.Infof("eviction manager: pods %s evicted, waiting for pod to be cleaned up", format.Pods(evictedPods))
+				klog.InfoS("Eviction manager: pods evicted, waiting for pod to be cleaned up", "pods", format.Pods(evictedPods))


@serathius what should we be doing in the case of multiple pods like this? Do you think this is ok?

ehashman · 2021-02-19T18:36:53Z

pkg/kubelet/eviction/eviction_manager.go

 		return nil
 	}

 	if m.clock.Since(m.thresholdsLastUpdated) > notifierRefreshInterval {
 		m.thresholdsLastUpdated = m.clock.Now()
 		for _, notifier := range m.thresholdNotifiers {
 			if err := notifier.UpdateThreshold(summary); err != nil {
-				klog.Warningf("eviction manager: failed to update %s: %v", notifier.Description(), err)
+				klog.InfoS("Eviction manager: failed to update notifier", "notifier", notifier.Description(), "err", err)


This is correct---"warning" log messages are being deprecated.

serathius · 2021-02-20T10:14:29Z

pkg/kubelet/eviction/eviction_manager.go

 		return nil
 	}

 	// rank the running pods for eviction for the specified resource
 	rank(activePods, statsFunc)

-	klog.Infof("eviction manager: pods ranked for eviction: %s", format.Pods(activePods))
+	klog.InfoS("Eviction manager: pods ranked for eviction", "pods", format.Pods(activePods))


Same us above

serathius · 2021-02-20T10:14:48Z

pkg/kubelet/eviction/eviction_manager.go

 			return
 		case <-ticker.C():
 			for i, pod := range pods {
 				if !podCleanedUpFunc(pod) {
 					break
 				}
 				if i == len(pods)-1 {
-					klog.Infof("eviction manager: pods %s successfully cleaned up", format.Pods(pods))
+					klog.InfoS("Eviction manager: pods successfully cleaned up", "pods", format.Pods(pods))


pkg/kubelet/eviction/eviction_manager.go

pkg/kubelet/eviction/helpers.go

serathius · 2021-02-20T16:37:04Z

/lgtm
/assign @klueska

yangjunmyfm192085 · 2021-02-20T17:20:35Z

/test pull-kubernetes-e2e-kind

Signed-off-by: JunYang <yang.jun22@zte.com.cn>

yangjunmyfm192085 · 2021-03-03T07:00:04Z

/test pull-kubernetes-unit

mrunalp · 2021-03-04T18:58:54Z

/approve

k8s-ci-robot · 2021-03-04T18:59:05Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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 added needs-priority Indicates a PR lacks a `priority/foo` label and requires one. 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 12, 2021

k8s-ci-robot requested review from dchen1107 and mrunalp February 12, 2021 15:36

ehashman added this to Triage in SIG Node PR Triage Feb 12, 2021

SergeyKanzhelev moved this from Triage to Needs Reviewer in SIG Node PR Triage Feb 17, 2021

SergeyKanzhelev reviewed Feb 17, 2021

View reviewed changes

ehashman reviewed Feb 19, 2021

View reviewed changes

serathius reviewed Feb 20, 2021

View reviewed changes

pkg/kubelet/eviction/eviction_manager.go Outdated Show resolved Hide resolved

serathius reviewed Feb 20, 2021

View reviewed changes

pkg/kubelet/eviction/helpers.go Outdated Show resolved Hide resolved

serathius reviewed Feb 20, 2021

View reviewed changes

pkg/kubelet/eviction/helpers.go Outdated Show resolved Hide resolved

yangjunmyfm192085 force-pushed the run-test18 branch 6 times, most recently from 328beab to af0b4c9 Compare February 20, 2021 16:35

k8s-ci-robot assigned klueska and serathius Feb 20, 2021

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

Structured Logging migration: modify eviction part logs of kubelet.

af0b4c9

Signed-off-by: JunYang <yang.jun22@zte.com.cn>

ehashman moved this from Needs Reviewer to Needs Approver in SIG Node PR Triage Mar 2, 2021

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

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

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

This was referenced Mar 4, 2021

kubelet: migrate pkg/kubelet/eviction to structured logging #99029

Closed

Kubelet migration to structured logs #98976

Closed

k8s-ci-robot merged commit 14abaa2 into kubernetes:master Mar 4, 2021

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

QiWang19 mentioned this pull request Mar 5, 2021

Refactor pods format to return ObjRef slice #99799

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Structured Logging migration: modify eviction part logs of kubelet. #99032

Structured Logging migration: modify eviction part logs of kubelet. #99032

yangjunmyfm192085 commented Feb 12, 2021 •

edited

k8s-ci-robot commented Feb 12, 2021

SergeyKanzhelev commented Feb 17, 2021

yangjunmyfm192085 commented Feb 17, 2021

SergeyKanzhelev Feb 17, 2021

yangjunmyfm192085 Feb 18, 2021

ehashman Feb 19, 2021

serathius Feb 20, 2021

SergeyKanzhelev Feb 17, 2021

yangjunmyfm192085 Feb 18, 2021

SergeyKanzhelev Feb 19, 2021

SergeyKanzhelev Feb 17, 2021

yangjunmyfm192085 Feb 18, 2021 •

edited

ehashman Feb 19, 2021

SergeyKanzhelev Feb 17, 2021

yangjunmyfm192085 Feb 18, 2021

SergeyKanzhelev Feb 17, 2021

yangjunmyfm192085 Feb 18, 2021

ehashman left a comment

ehashman Feb 19, 2021

ehashman Feb 19, 2021

serathius Feb 20, 2021

serathius Feb 20, 2021

serathius commented Feb 20, 2021

yangjunmyfm192085 commented Feb 20, 2021

yangjunmyfm192085 commented Mar 3, 2021

mrunalp commented Mar 4, 2021

k8s-ci-robot commented Mar 4, 2021

Structured Logging migration: modify eviction part logs of kubelet. #99032

Structured Logging migration: modify eviction part logs of kubelet. #99032

Conversation

yangjunmyfm192085 commented Feb 12, 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.:

k8s-ci-robot commented Feb 12, 2021

SergeyKanzhelev commented Feb 17, 2021

yangjunmyfm192085 commented Feb 17, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yangjunmyfm192085 Feb 18, 2021 • edited

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

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 commented Feb 20, 2021

yangjunmyfm192085 commented Feb 20, 2021

yangjunmyfm192085 commented Mar 3, 2021

mrunalp commented Mar 4, 2021

k8s-ci-robot commented Mar 4, 2021

yangjunmyfm192085 commented Feb 12, 2021 •

edited

yangjunmyfm192085 Feb 18, 2021 •

edited