Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

scheduler: update the scheduler interface and cache methods to use contextual logging #116849

Merged
merged 1 commit into from May 29, 2023
Merged

scheduler: update the scheduler interface and cache methods to use contextual logging #116849

merged 1 commit into from May 29, 2023

Conversation

mengjiao-liu
Copy link
Member

@mengjiao-liu mengjiao-liu commented Mar 22, 2023 •
edited

What type of PR is this?

/kind feature

What this PR does / why we need it:

Update the scheduler interface and cache methods to use contextual logging.

Which issue(s) this PR fixes:

Part of #91633 (comment)

Special notes for your reviewer:

Update the interface, cache, and some of the files that call these methods.
Because these methods are called from many places and cannot be subdivided, this PR may be large.

Does this PR introduce a user-facing change?

Update the scheduler interface and cache methods to use [contextual logging](https://k8s.io/docs/concepts/cluster-administration/system-logs/#contextual-logging).

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

- [KEP]: https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/3077-contextual-logging

@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Mar 22, 2023
@mengjiao-liu
Copy link
Member Author

/test all

@k8s-ci-robot k8s-ci-robot added area/test sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Mar 22, 2023
@mengjiao-liu mengjiao-liu mentioned this pull request Mar 22, 2023
Open
@mengjiao-liu mengjiao-liu force-pushed the contextual-logging-scheduler-internal branch from 1a31ccc to 0b0dbf7 Compare March 22, 2023 12:17
@mengjiao-liu
Copy link
Member Author

/test all

@mengjiao-liu mengjiao-liu force-pushed the contextual-logging-scheduler-internal branch from 0b0dbf7 to e40b5ac Compare March 22, 2023 13:18
@mengjiao-liu
Copy link
Member Author

/test all

1 similar comment
@mengjiao-liu
Copy link
Member Author

/test all

@mengjiao-liu mengjiao-liu mentioned this pull request Mar 23, 2023
Merged
1 task
@mengjiao-liu mengjiao-liu marked this pull request as ready for review March 23, 2023 09:11
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 23, 2023
@mengjiao-liu
Copy link
Member Author

/sig instrumentation
/wg structured-logging
/area logging

@k8s-ci-robot k8s-ci-robot added the sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. label Apr 6, 2023
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 12, 2023
@mengjiao-liu mengjiao-liu force-pushed the contextual-logging-scheduler-internal branch 2 times, most recently from 7ebeb2a to 234db6b Compare May 12, 2023 14:12
kerthcet
kerthcet reviewed May 13, 2023
View reviewed changes
Copy link
Member

@kerthcet kerthcet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Several comments, I haven't review the whole codebase yet, one point I want to highlight is if not necessary to use contextual logging, we can use klog directly, like cache and eventhandler. Agree? cc @pohly @alculquicondor

pkg/scheduler/framework/interface.go Show resolved Hide resolved
pkg/scheduler/internal/cache/cache.go Outdated Show resolved Hide resolved
pkg/scheduler/internal/cache/cache.go Outdated Show resolved Hide resolved
pkg/scheduler/internal/cache/cache.go Outdated Show resolved Hide resolved
pkg/scheduler/internal/cache/interface.go Show resolved Hide resolved
pkg/scheduler/eventhandlers.go Show resolved Hide resolved
@mengjiao-liu mengjiao-liu force-pushed the contextual-logging-scheduler-internal branch 5 times, most recently from e1c3cd8 to 4b88562 Compare May 18, 2023 07:24
@mengjiao-liu
Copy link
Member Author

Pass context parameter in the interface instead of logger parameter.

@alculquicondor
Copy link
Member

/hold
for #118012

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 19, 2023
@mengjiao-liu mengjiao-liu force-pushed the contextual-logging-scheduler-internal branch 2 times, most recently from d781ace to a11edd3 Compare May 26, 2023 07:22
@mengjiao-liu
Copy link
Member Author

/hold cancel
#118012 has been merged.

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 26, 2023
@mengjiao-liu mengjiao-liu force-pushed the contextual-logging-scheduler-internal branch from a11edd3 to 0afa1b2 Compare May 26, 2023 08:39
alculquicondor
alculquicondor reviewed May 26, 2023
View reviewed changes
pkg/scheduler/internal/cache/cache_test.go Outdated Show resolved Hide resolved
pkg/scheduler/internal/cache/cache_test.go Show resolved Hide resolved
pkg/scheduler/internal/cache/node_tree.go Outdated Show resolved Hide resolved
pkg/scheduler/internal/cache/node_tree.go Outdated Show resolved Hide resolved
pkg/scheduler/schedule_one.go Outdated
@@ -320,6 +324,7 @@ func (sched *Scheduler) skipPodSchedule(fwk framework.Framework, pod *v1.Pod) bo
// during its previous scheduling cycle but before getting assumed.
isAssumed, err := sched.Cache.IsAssumedPod(pod)
if err != nil {
// TODO: pass ctx into a revised HandleError
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there an issue for this? If so, please reference TODO(#1234)
Also, does it need to be a context or can it be a logger?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there an issue for this? If so, please reference TODO(#1234)

Yes. I have added the issue number.

Also, does it need to be a context or can it be a logger?

As far as I know, the function utilruntime.HandleError is used in many components.
Do we have plans to decide whether to pass context or logger in the future? @pohly

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you open an issue to add a logger of Ctx to HandleError?

pkg/scheduler/schedule_one.go Outdated Show resolved Hide resolved
pkg/scheduler/schedule_one.go Outdated Show resolved Hide resolved
pkg/scheduler/schedule_one.go Outdated Show resolved Hide resolved
pkg/scheduler/schedule_one.go Outdated Show resolved Hide resolved
@mengjiao-liu mengjiao-liu force-pushed the contextual-logging-scheduler-internal branch from 0afa1b2 to efdfbbf Compare May 29, 2023 05:21
@mengjiao-liu mengjiao-liu force-pushed the contextual-logging-scheduler-internal branch from efdfbbf to 074900e Compare May 29, 2023 05:26
@mengjiao-liu mengjiao-liu mentioned this pull request May 29, 2023
Merged
@alculquicondor
Copy link
Member

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 29, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 956343a34a0d3646965709843da16a12419b9165

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, mengjiao-liu

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:

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 k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 29, 2023
@k8s-ci-robot k8s-ci-robot merged commit 94d0bd2 into kubernetes:master May 29, 2023
13 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.28 milestone May 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pohly pohly pohly left review comments

@alculquicondor alculquicondor alculquicondor left review comments

@kerthcet kerthcet kerthcet left review comments

@atiratree atiratree Awaiting requested review from atiratree

@denkensk denkensk Awaiting requested review from denkensk

Assignees

@logicalhan logicalhan

@alculquicondor alculquicondor

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/logging area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on. wg/structured-logging Categorizes an issue or PR as relevant to WG Structured Logging.
Projects
None yet
Milestone
v1.28
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@mengjiao-liu @k8s-ci-robot @logicalhan @alculquicondor @pohly @kerthcet