migrate default_preemption.go to structured logging #98518

tanjing2020 · 2021-01-28T07:56:56Z

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
Ref:
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?:

NONE

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

k8s-ci-robot · 2021-01-28T07:57:03Z

@tanjing2020: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

k8s-ci-robot · 2021-01-28T07:57:04Z

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

tanjing2020 · 2021-01-28T07:58:00Z

/release-note-none

pkg/scheduler/framework/plugins/defaultpreemption/default_preemption.go

ahg-g · 2021-02-01T02:58:32Z

pkg/scheduler/framework/plugins/defaultpreemption/default_preemption.go

 	pod, err := pl.podLister.Pods(pod.Namespace).Get(pod.Name)
 	if err != nil {
-		klog.Errorf("Error getting the updated preemptor pod object: %v", err)
+		klog.ErrorS(err, "Error getting the updated preemptor pod object", "pod", klog.KRef(podNamespace, podName))


Suggested change

klog.ErrorS(err, "Error getting the updated preemptor pod object", "pod", klog.KRef(podNamespace, podName))

klog.ErrorS(err, "getting the updated preemptor pod object", "pod", klog.KRef(podNamespace, podName))

ahg-g · 2021-02-01T03:05:38Z

pkg/scheduler/framework/plugins/defaultpreemption/default_preemption.go

+	for i := range candidates {
+		klog.ErrorS(errors.New("Candidate is not selected"), "Should not reach here", "candidate", candidates[i])
+	}


can't we do: klog.ErrorS(errors.New("no candidate selected"), "should not reach here", "candidates", candidates)

Thanks for your advice, I'll fix it.

can you do this one?

Yes, Of course. I didn't understand it correctly yesterday. I have revised it now.

ahg-g · 2021-02-01T03:07:20Z

pkg/scheduler/framework/plugins/defaultpreemption/default_preemption.go

 		// In this case, we should clean-up any existing nominated node name of the pod.
 		if err := util.ClearNominatedNodeName(pl.fh.ClientSet(), pod); err != nil {
-			klog.Errorf("Cannot clear 'NominatedNodeName' field of pod %v/%v: %v", pod.Namespace, pod.Name, err)
+			klog.ErrorS(err, "Cannot clear 'NominatedNodeName' field of pod", "pod", klog.KObj(pod))


Suggested change

klog.ErrorS(err, "Cannot clear 'NominatedNodeName' field of pod", "pod", klog.KObj(pod))

klog.ErrorS(err, "cannot clear 'NominatedNodeName' field of pod", "pod", klog.KObj(pod))

ahg-g · 2021-02-01T03:07:59Z

pkg/scheduler/framework/plugins/defaultpreemption/default_preemption.go

@@ -562,7 +566,7 @@ func pickOneNodeForPreemption(nodesToVictims map[string]*extenderv1.Victims) str
 		// Get earliest start time of all pods on the current node.
 		earliestStartTimeOnNode := util.GetEarliestPodStartTime(nodesToVictims[node])
 		if earliestStartTimeOnNode == nil {
-			klog.Errorf("earliestStartTime is nil for node %s. Should not reach here.", node)
+			klog.ErrorS(errors.New("EarliestStartTime is nil for node"), "Should not reach here", "node", node)


Suggested change

klog.ErrorS(errors.New("EarliestStartTime is nil for node"), "Should not reach here", "node", node)

klog.ErrorS(errors.New("earliestStartTime is nil for node"), "should not reach here", "node", node)

ahg-g · 2021-02-01T03:08:22Z

pkg/scheduler/framework/plugins/defaultpreemption/default_preemption.go

@@ -690,7 +694,7 @@ func selectVictimsOnNode(
 func PrepareCandidate(c Candidate, fh framework.Handle, cs kubernetes.Interface, pod *v1.Pod) error {
 	for _, victim := range c.Victims().Pods {
 		if err := util.DeletePod(cs, victim); err != nil {
-			klog.Errorf("Error preempting pod %v/%v: %v", victim.Namespace, victim.Name, err)
+			klog.ErrorS(err, "Error preempting pod", "pod", klog.KObj(victim))


Suggested change

klog.ErrorS(err, "Error preempting pod", "pod", klog.KObj(victim))

klog.ErrorS(err, "preempting pod", "pod", klog.KObj(victim))

ahg-g · 2021-02-01T03:08:35Z

pkg/scheduler/framework/plugins/defaultpreemption/default_preemption.go

@@ -708,7 +712,7 @@ func PrepareCandidate(c Candidate, fh framework.Handle, cs kubernetes.Interface,
 	// lets scheduler find another place for them.
 	nominatedPods := getLowerPriorityNominatedPods(fh.PreemptHandle(), pod, c.Name())
 	if err := util.ClearNominatedNodeName(cs, nominatedPods...); err != nil {
-		klog.Errorf("Cannot clear 'NominatedNodeName' field: %v", err)
+		klog.ErrorS(err, "Cannot clear 'NominatedNodeName' field")


Suggested change

klog.ErrorS(err, "Cannot clear 'NominatedNodeName' field")

klog.ErrorS(err, "cannot clear 'NominatedNodeName' field")

tanjing2020 · 2021-02-01T06:41:40Z

Thanks for your advices, I fixed everything except the first one. Please check it again, thanks.
/recc @ahg-g

lala123912 · 2021-02-03T07:56:58Z

/ok-to-test

tanjing2020 · 2021-02-03T08:28:20Z

/test pull-kubernetes-bazel-test

tanjing2020 · 2021-02-03T09:06:12Z

/test pull-kubernetes-bazel-test

tanjing2020 · 2021-02-04T01:31:02Z

/recc @ahg-g

ahg-g · 2021-02-04T01:34:11Z

/lgtm
/approve

k8s-ci-robot · 2021-02-04T01:34:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahg-g, tanjing2020

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/scheduler/OWNERS~~ [ahg-g]

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

tanjing2020 · 2021-02-04T03:33:02Z

/test pull-kubernetes-e2e-kind-ipv6

tanjing2020 · 2021-02-05T07:54:20Z

/retest

k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 28, 2021

k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Jan 28, 2021

k8s-ci-robot requested review from ahg-g and hex108 January 28, 2021 07:57

k8s-ci-robot added sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jan 28, 2021

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jan 28, 2021

ahg-g reviewed Feb 1, 2021

View reviewed changes

tanjing2020 force-pushed the change-log branch from dafcf21 to 657ec6a Compare February 1, 2021 06:29

tanjing2020 force-pushed the change-log branch from 657ec6a to f8cbf31 Compare February 2, 2021 01:34

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 2, 2021

migrate default_preemption.go to structured logging

e62cfa7

tanjing2020 force-pushed the change-log branch from f8cbf31 to e62cfa7 Compare February 2, 2021 01:42

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 Feb 3, 2021

k8s-ci-robot assigned ahg-g Feb 4, 2021

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

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

k8s-ci-robot merged commit ebe7380 into kubernetes:master Feb 4, 2021

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

tanjing2020 mentioned this pull request Mar 3, 2021

REQUEST: New membership for tanjing2020 kubernetes/org#2536

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

migrate default_preemption.go to structured logging #98518

migrate default_preemption.go to structured logging #98518

tanjing2020 commented Jan 28, 2021

k8s-ci-robot commented Jan 28, 2021

k8s-ci-robot commented Jan 28, 2021

tanjing2020 commented Jan 28, 2021

ahg-g Feb 1, 2021

ahg-g Feb 1, 2021

tanjing2020 Feb 1, 2021

ahg-g Feb 1, 2021

tanjing2020 Feb 2, 2021 •

edited

ahg-g Feb 1, 2021

ahg-g Feb 1, 2021

ahg-g Feb 1, 2021

ahg-g Feb 1, 2021

tanjing2020 commented Feb 1, 2021

lala123912 commented Feb 3, 2021

tanjing2020 commented Feb 3, 2021

tanjing2020 commented Feb 3, 2021

tanjing2020 commented Feb 4, 2021

ahg-g commented Feb 4, 2021

k8s-ci-robot commented Feb 4, 2021

tanjing2020 commented Feb 4, 2021

tanjing2020 commented Feb 5, 2021

	klog.ErrorS(err, "Error getting the updated preemptor pod object", "pod", klog.KRef(podNamespace, podName))
	klog.ErrorS(err, "getting the updated preemptor pod object", "pod", klog.KRef(podNamespace, podName))

	klog.ErrorS(err, "Cannot clear 'NominatedNodeName' field of pod", "pod", klog.KObj(pod))
	klog.ErrorS(err, "cannot clear 'NominatedNodeName' field of pod", "pod", klog.KObj(pod))

	klog.ErrorS(errors.New("EarliestStartTime is nil for node"), "Should not reach here", "node", node)
	klog.ErrorS(errors.New("earliestStartTime is nil for node"), "should not reach here", "node", node)

	klog.ErrorS(err, "Error preempting pod", "pod", klog.KObj(victim))
	klog.ErrorS(err, "preempting pod", "pod", klog.KObj(victim))

migrate default_preemption.go to structured logging #98518

migrate default_preemption.go to structured logging #98518

Conversation

tanjing2020 commented Jan 28, 2021

k8s-ci-robot commented Jan 28, 2021

k8s-ci-robot commented Jan 28, 2021

tanjing2020 commented Jan 28, 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

tanjing2020 Feb 2, 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

tanjing2020 commented Feb 1, 2021

lala123912 commented Feb 3, 2021

tanjing2020 commented Feb 3, 2021

tanjing2020 commented Feb 3, 2021

tanjing2020 commented Feb 4, 2021

ahg-g commented Feb 4, 2021

k8s-ci-robot commented Feb 4, 2021

tanjing2020 commented Feb 4, 2021

tanjing2020 commented Feb 5, 2021

tanjing2020 Feb 2, 2021 •

edited