migrate scheduling_queue.go to structured logging #98358

tanjing2020 · 2021-01-25T08:15:16Z

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:

Verify log output (scheduling_queue.go)

before
scheduling_queue.go:253] Error adding pod ns2/mpp to the scheduling queue: couldn't create key for object &{Pod:&Pod{ObjectMeta:{...CST m=+0.016562001}: <nil>
scheduling_queue.go:257] Error: pod ns2/mpp is already in the unschedulable queue.
scheduling_queue.go:262] Error: pod ns2/mpp is already in the podBackoff queue.
scheduling_queue.go:347] Unable to pop pod ns2/mpp from backoff queue despite backoff completion.
scheduling_queue.go:517] Error adding pod up to the backoff queue: couldn't create key for object &{Pod:&Pod{ObjectMeta:{...CST m=+0.018590301}: <nil>
scheduling_queue.go:524] Error adding pod to the scheduling queue: couldn't create key for object &{Pod:&Pod{ObjectMeta:{...CST m=+0.018590301}: <nil>
scheduling_queue.go:547] Error getting label selectors for pod: afp.
scheduling_queue.go:751] Pod ns2/mpp already exists in the nominated map!
scheduling_queue.go:813] About to try and schedule pod ns2/mpp
scheduling_queue.go:816] Error while retrieving next pod from scheduling queue: object was removed from heap data

after
scheduling_queue.go:253] "Error adding pod to the scheduling queue" err="couldn't create key for object &{Pod:&Pod{ObjectMeta:{...CST m=+0.018528101}: <nil>" pod="ns2/mpp"
scheduling_queue.go:257] "Error: pod is already in the unschedulable queue" pod="ns2/mpp"
scheduling_queue.go:262] "Error: pod is already in the podBackoff queue" pod="ns2/mpp"
scheduling_queue.go:347] "Unable to pop pod from backoff queue despite backoff completion" pod="ns2/mpp"
scheduling_queue.go:517] "Error adding pod to the backoff queue" err="couldn't create key for object &{Pod:&Pod{ObjectMeta:{...CST m=+0.016580101}: <nil>" pod="ns1/up"
scheduling_queue.go:524] "Error adding pod to the scheduling queue" err="couldn't create key for object &{Pod:&Pod{ObjectMeta:{...CST m=+0.016580101}: <nil>" pod="ns1/up"
scheduling_queue.go:547] "Error getting label selectors for pod" err="out is not a valid pod selector operator" pod="ns1/afp"
scheduling_queue.go:751] "Pod already exists in the nominated map" pod="ns2/mpp"
scheduling_queue.go:813] "About to try and schedule pod" pod="ns2/mpp"
scheduling_queue.go:816] "Error while retrieving next pod from scheduling queue" err="object was removed from heap data"

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-25T08:15:23Z

@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-25T08:15:24Z

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.

damemi

/ok-to-test
/lgtm

pkg/scheduler/internal/queue/scheduling_queue.go

gavinfish · 2021-01-25T15:33:22Z

Just notice this one is kind of duplicated with #98135.

damemi · 2021-01-25T21:09:42Z

Just notice this one is kind of duplicated with #98135.

Ah true, I didn't notice that.
/hold
@tanjing2020 please check out the other PR, and we may close this one as a duplicate

tanjing2020 · 2021-02-09T02:02:00Z

#98135 removed duplicate file, please review again, thanks.
@damemi @gavinfish

pkg/scheduler/internal/queue/scheduling_queue.go

gavinfish · 2021-02-10T04:12:37Z

pkg/scheduler/internal/queue/scheduling_queue.go

@@ -299,14 +291,14 @@ func (p *PriorityQueue) AddUnschedulableIfNotPresent(pInfo *framework.QueuedPodI
 	defer p.lock.Unlock()
 	pod := pInfo.Pod
 	if p.unschedulableQ.get(pod) != nil {
-		return fmt.Errorf("pod: %v is already present in unschedulable queue", nsNameForPod(pod))
+		return fmt.Errorf("pod %v is already present in unschedulable queue", klog.KObj(pod))


Suggested change

return fmt.Errorf("pod %v is already present in unschedulable queue", klog.KObj(pod))

return fmt.Errorf("Pod is already present in unschedulable queue", "pod", klog.KObj(pod))

Errorf formats according to a format specifier and returns the string as a value that satisfies error.
https://golang.org/pkg/fmt/#Errorf

I think it's better to keep the original modification.

gavinfish · 2021-02-10T04:13:59Z

pkg/scheduler/internal/queue/scheduling_queue.go

 	}

 	if _, exists, _ := p.activeQ.Get(pInfo); exists {
-		return fmt.Errorf("pod: %v is already present in the active queue", nsNameForPod(pod))
+		return fmt.Errorf("pod %v is already present in the active queue", klog.KObj(pod))


Suggested change

return fmt.Errorf("pod %v is already present in the active queue", klog.KObj(pod))

return fmt.Errorf("Pod is already present in the active queue", "pod", klog.KObj(pod))

gavinfish · 2021-02-10T04:14:25Z

pkg/scheduler/internal/queue/scheduling_queue.go

 	}
 	if _, exists, _ := p.podBackoffQ.Get(pInfo); exists {
-		return fmt.Errorf("pod %v is already present in the backoff queue", nsNameForPod(pod))
+		return fmt.Errorf("pod %v is already present in the backoff queue", klog.KObj(pod))


Suggested change

return fmt.Errorf("pod %v is already present in the backoff queue", klog.KObj(pod))

return fmt.Errorf("Pod is already present in the backoff queue", "pod", klog.KObj(pod))

damemi · 2021-02-10T16:20:42Z

pkg/scheduler/internal/queue/scheduling_queue.go

 		p.unschedulableQ.delete(pod)
 	}
 	// Delete pod from backoffQ if it is backing off
 	if err := p.podBackoffQ.Delete(pInfo); err == nil {
-		klog.Errorf("Error: pod %v is already in the podBackoff queue.", nsNameForPod(pod))
+		klog.ErrorS(err, "Error: pod is already in the podBackoff queue", "pod", klog.KObj(pod))


nit: err is always nil here

damemi

/lgtm

k8s-ci-robot · 2021-02-18T13:47:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: damemi, 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~~ [damemi]

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

lala123912 · 2021-02-19T06:43:47Z

/unhold

k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Jan 25, 2021

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

k8s-ci-robot requested review from damemi and k82cn January 25, 2021 08:16

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 25, 2021

damemi reviewed Jan 25, 2021

View reviewed changes

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 Jan 25, 2021

k8s-ci-robot assigned damemi Jan 25, 2021

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

gavinfish reviewed Jan 25, 2021

View reviewed changes

pkg/scheduler/internal/queue/scheduling_queue.go Show resolved Hide resolved

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 25, 2021

tanjing2020 force-pushed the scheduling_queue.go branch from b3c697c to af33dcd Compare January 27, 2021 03:27

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

tanjing2020 force-pushed the scheduling_queue.go branch from af33dcd to 3e096cb Compare January 27, 2021 03:36

damemi reviewed Feb 9, 2021

View reviewed changes

pkg/scheduler/internal/queue/scheduling_queue.go Show resolved Hide resolved

tanjing2020 force-pushed the scheduling_queue.go branch from 3e096cb to a5994b7 Compare February 10, 2021 01:26

k8s-ci-robot removed the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Feb 10, 2021

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 10, 2021

gavinfish reviewed Feb 10, 2021

View reviewed changes

tanjing2020 force-pushed the scheduling_queue.go branch from a5994b7 to 223ef1d Compare February 10, 2021 06:09

damemi reviewed Feb 10, 2021

View reviewed changes

migrate scheduling_queue.go to structured logging

d0eecbd

tanjing2020 force-pushed the scheduling_queue.go branch from 223ef1d to d0eecbd Compare February 18, 2021 01:57

damemi approved these changes Feb 18, 2021

View reviewed changes

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

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

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 19, 2021

k8s-ci-robot merged commit 320aaf4 into kubernetes:master Feb 19, 2021

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

tanjing2020 mentioned this pull request Mar 3, 2021

REQUEST: New membership for tanjing2020 kubernetes/org#2536

Closed

6 tasks

tanjing2020 deleted the scheduling_queue.go branch March 9, 2021 03:52

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 scheduling_queue.go to structured logging #98358

migrate scheduling_queue.go to structured logging #98358

tanjing2020 commented Jan 25, 2021 •

edited by liggitt

Loading

k8s-ci-robot commented Jan 25, 2021

k8s-ci-robot commented Jan 25, 2021

damemi left a comment

gavinfish commented Jan 25, 2021

damemi commented Jan 25, 2021

tanjing2020 commented Feb 9, 2021

gavinfish Feb 10, 2021

tanjing2020 Feb 18, 2021

gavinfish Feb 10, 2021

gavinfish Feb 10, 2021

damemi Feb 10, 2021

tanjing2020 Feb 18, 2021

damemi left a comment

k8s-ci-robot commented Feb 18, 2021

lala123912 commented Feb 19, 2021

	return fmt.Errorf("pod %v is already present in unschedulable queue", klog.KObj(pod))
	return fmt.Errorf("Pod is already present in unschedulable queue", "pod", klog.KObj(pod))

migrate scheduling_queue.go to structured logging #98358

migrate scheduling_queue.go to structured logging #98358

Conversation

tanjing2020 commented Jan 25, 2021 • edited by liggitt Loading

k8s-ci-robot commented Jan 25, 2021

k8s-ci-robot commented Jan 25, 2021

damemi left a comment

Choose a reason for hiding this comment

gavinfish commented Jan 25, 2021

damemi commented Jan 25, 2021

tanjing2020 commented Feb 9, 2021

gavinfish Feb 10, 2021

Choose a reason for hiding this comment

tanjing2020 Feb 18, 2021

Choose a reason for hiding this comment

gavinfish Feb 10, 2021

Choose a reason for hiding this comment

gavinfish Feb 10, 2021

Choose a reason for hiding this comment

damemi Feb 10, 2021

Choose a reason for hiding this comment

tanjing2020 Feb 18, 2021

Choose a reason for hiding this comment

damemi left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 18, 2021

lala123912 commented Feb 19, 2021

tanjing2020 commented Jan 25, 2021 •

edited by liggitt

Loading