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.

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

scheduler: contextual logging - WithName and WithValue #111155

Conversation

knelasevero
Copy link
Contributor

@knelasevero knelasevero commented Jul 14, 2022

What type of PR is this?

/kind feature

What this PR does / why we need it:

This makes kube-scheduler log output more useful.

Which issue(s) this PR fixes:

Related-to: #91633 Built on top of #110833 (Just rebased latest changes)

Special notes for your reviewer:

The basic work is in the initial commit of that WIP PR. The other commits (in that WIP PR) start with full instrumentation and then gradually reduce the runtime overhead.

I chose to create this PR already pointing to the scheduler: use WithName and WithValues commit.

I ran the perf tests a few times, and yeah, we have some overhead, but I personally think it is worth it [?]. Whole idea was to have named loggers initially. I think this is a nice first step and we can go to the other more specific improvements after this.

I rebased changes from master here, and chose to use the FailureHandler that came from there. Please let me know if I did something wrong in this rebase.

After this gets merged there's some followup work to be done (when ctx logging hits GA): #111672

Does this PR introduce a user-facing change?

Migrated kube-scheduler to use [contextual logging](https://k8s.io/docs/concepts/cluster-administration/system-logs/#contextual-logging).

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. 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-kind Indicates a PR lacks a `kind/foo` label and requires one. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jul 14, 2022
@k8s-ci-robot
Copy link
Contributor

@knelasevero: 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 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 Jul 14, 2022
@k8s-ci-robot
Copy link
Contributor

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

@k8s-ci-robot k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Jul 14, 2022
@k8s-ci-robot k8s-ci-robot added area/cloudprovider area/test kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. sig/storage Categorizes an issue or PR as relevant to SIG Storage. sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jul 14, 2022
@rikatz
Copy link
Contributor

rikatz commented Jul 14, 2022

/ok-to-test

@k8s-ci-robot 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 Jul 14, 2022
@rikatz
Copy link
Contributor

rikatz commented Jul 14, 2022

/sig scheduling

@k8s-triage-robot
Copy link

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

@knelasevero
Copy link
Contributor Author

/retest

@knelasevero
Copy link
Contributor Author

@alculquicondor I don't see those changes here. The only change in cmd/ is because of changes in the scheduler.New inside of pkg/

@alculquicondor
Copy link
Member

ah, I was confused by the changes in pkg/scheduler/apis/config/v1/default_plugins.go. I thought they had all merged in the previous PR.

@alculquicondor
Copy link
Member

any chance you can split the PRs again (and maybe a few others too)? I have very limited time to review and maybe we won't be able to merge this entire PR, but we can still make progress.

@knelasevero
Copy link
Contributor Author

Maybe someone from the community would want to take that task and push these splits of this PR forward? Since they can use this PR as reference it would be easy to anyone wanting to contribute (I can help, of course, but I may have to focus on some other things).

@alculquicondor
Copy link
Member

/help

@knelasevero
Copy link
Contributor Author

/help-wanted

@alculquicondor
Copy link
Member

Maybe it doesn't work on PRs

@pohly
Copy link
Contributor

pohly commented Feb 24, 2023

@alculquicondor: any suggestion on how to split this up? By directories?

Looking at the commit history, all commits probably can be squashed into one, i.e. there are no commits which could be merged separately.

@alculquicondor
Copy link
Member

Everything under pkg/scheduler/apis can be its own PR.

The rest is a bit unclear. Maybe it's possible to have a PR for each of the folders in pkg/scheduler/framework/plugins. Followed by a PR that does the rest.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 25, 2023
@k8s-ci-robot
Copy link
Contributor

PR needs rebase.

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.

@knelasevero
Copy link
Contributor Author

I had a look over kubernetes/enhancements#3077. Just tagging some people here to see if they want to pick this up:

@mengjiao-liu
@249043822
@Namanl2001
@ncdc
@sanwishe
@yangjunmyfm192085
@freddie400
@pravarag

Would any of you be interested in picking this one up? The work would mean basically splinting the changes of #111155 into multiple PRs, as described by @alculquicondor in #111155 (comment)

whoever picks this up can ignore the commit history here (#111155) and create new commits that make sense in each split PR.

@ncdc
Copy link
Member

ncdc commented Mar 13, 2023

I might be able to do some, but I wouldn't be able to start on anything for at least 2 weeks

@alculquicondor
Copy link
Member

code freeze is tomorrow, so I think the boat has sailed for this release.

@Namanl2001
Copy link
Member

I would like to pick this up, but unfortunately, this won't be possible to be done in a couple of days

@mengjiao-liu
Copy link
Member

I can help too, but time is too urgent before the code freeze, both for contributors and reviewers, maybe we can finish it in the next version.

@mengjiao-liu
Copy link
Member

If we start the task, we need to figure out how to split PR.
I am not quite clear about the directory structure of the scheduler, so I will first divide PR from the direction provided by @alculquicondor and then adjust it according to the actual implementation.

Please see #91633 (comment).

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Mark this PR as fresh with /remove-lifecycle stale
  • Close this PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 12, 2023
@mengjiao-liu
Copy link
Member

/sig instrumentation
/wg structured-logging
/area logging

@k8s-ci-robot k8s-ci-robot added sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. wg/structured-logging Categorizes an issue or PR as relevant to WG Structured Logging. area/logging labels Jul 10, 2023
@mengjiao-liu
Copy link
Member

Work continues in #91633 (comment)

/close

@k8s-ci-robot
Copy link
Contributor

@mengjiao-liu: Closed this PR.

In response to this:

Work continues in #91633 (comment)

/close

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cloudprovider area/logging area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. 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/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. 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/storage Categorizes an issue or PR as relevant to SIG Storage. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. wg/structured-logging Categorizes an issue or PR as relevant to WG Structured Logging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet