job controller: use contextual logging #113576

sanwishe · 2022-11-03T08:14:03Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Switch the job controller code to use contextual and structured logging.

Which issue(s) this PR fixes:

Fixes #
Part of kubernetes/enhancements#3077

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 · 2022-11-03T08:14:05Z

@sanwishe: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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 · 2022-11-03T08:14:12Z

@sanwishe: 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.

yangjunmyfm192085

/release-note-none

k8s-ci-robot · 2022-11-03T08:51:28Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sanwishe
Once this PR has been reviewed and has the lgtm label, please assign soltysh for approval by writing /assign @soltysh in a comment. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

shivanshuraj1333 · 2022-11-08T14:07:52Z

/assign

k8s-ci-robot · 2022-11-08T14:08:00Z

@sanwishe: 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.

fedebongio · 2022-11-08T21:19:49Z

/remove-sig api-machinery

mimowo

Generally looks good, with some remarks:

One more klog line in tracking_utils to replace:
klog.V(4).InfoS("Expecting tracking finalizers removed", "job", jobKey, "podUIDs", deletedKeys).
a question about wrapping with LoggerWithName
a suggestion to use key for job logging identifier where available

mimowo · 2022-11-21T09:24:23Z

pkg/controller/job/job_controller.go

@@ -442,15 +451,15 @@ func (jm *Controller) updateJob(old, cur interface{}) {
 			total := time.Duration(*curADS) * time.Second
 			// AddAfter will handle total < passed
 			jm.queue.AddAfter(key, total-passed)
-			klog.V(4).Infof("job %q ActiveDeadlineSeconds updated, will rsync after %d seconds", key, total-passed)
+			klog.FromContext(ctx).V(4).Info("Job's ActiveDeadlineSeconds updated, will rsync", "job", klog.KObj(curJob), "interval", total-passed)


nit: suggesting using key instead of klog.KObj(curJob) since already at hand, and to be consistent with other places like this: klog.FromContext(ctx).Info("Enqueueing job", "job", key).

mimowo · 2022-11-21T09:27:54Z

pkg/controller/job/job_controller.go

@@ -188,8 +196,9 @@ func (jm *Controller) Run(ctx context.Context, workers int) {
 	defer jm.queue.ShutDown()
 	defer jm.orphanQueue.ShutDown()

-	klog.Infof("Starting job controller")
-	defer klog.Infof("Shutting down job controller")
+	logger := klog.LoggerWithName(klog.FromContext(ctx), "Job")


Why to wrap the logger here by klog.LoggerWithName, but not in in other places within the job_controller?

Please take out LoggerWithName. We plan to add the names one level up in the caller of this function.

dims · 2022-12-12T15:32:17Z

If you still need this PR then please rebase, if not, please close the PR

sftim · 2023-01-10T21:36:19Z

The changelog entry I suggest for this:

Migrated the Job controller (within `kube-controller-manager) to use [contextual logging](https://k8s.io/docs/concepts/cluster-administration/system-logs/#contextual-logging).

pohly · 2023-02-15T11:01:49Z

@sanwishe: ping. Can you rebase and address comments?

k8s-triage-robot · 2023-05-16T11:18:08Z

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-triage-robot · 2023-06-15T11:54:49Z

The Kubernetes project currently lacks enough active 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 rotten
Close this PR with /close
Offer to help out with Issue Triage

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

/lifecycle rotten

pohly · 2023-06-15T13:07:48Z

/remove-lifecycle stale

mengjiao-liu · 2023-06-16T06:36:42Z

PR #116910 has already done this part of the work, so we can close the PR.

mengjiao-liu · 2023-06-16T06:43:02Z

/close

k8s-ci-robot · 2023-06-16T06:43:08Z

@mengjiao-liu: Closed this PR.

In response to this:

/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.

k8s-ci-robot requested review from mwielgus and soltysh November 3, 2022 08:15

sanwishe force-pushed the contextuallogging4jobcontroller branch from 0415438 to fb1d691 Compare November 3, 2022 08:15

yangjunmyfm192085 reviewed Nov 3, 2022

View reviewed changes

sanwishe force-pushed the contextuallogging4jobcontroller branch 2 times, most recently from 2bf3cda to 5afbd11 Compare November 3, 2022 08:35

k8s-ci-robot added area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Nov 3, 2022

job controller: use contextual logging

8786ec7

sanwishe force-pushed the contextuallogging4jobcontroller branch from 5afbd11 to 8786ec7 Compare November 3, 2022 08:50

k8s-ci-robot assigned shivanshuraj1333 Nov 8, 2022

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 8, 2022

k8s-ci-robot removed the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Nov 8, 2022

pohly mentioned this pull request Nov 18, 2022

contextual logging kubernetes/enhancements#3077

Open

12 tasks

mimowo reviewed Nov 21, 2022

View reviewed changes

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 16, 2023

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jun 15, 2023

k8s-ci-robot closed this Jun 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

job controller: use contextual logging #113576

job controller: use contextual logging #113576

sanwishe commented Nov 3, 2022

k8s-ci-robot commented Nov 3, 2022

k8s-ci-robot commented Nov 3, 2022

yangjunmyfm192085 left a comment

k8s-ci-robot commented Nov 3, 2022

shivanshuraj1333 commented Nov 8, 2022

k8s-ci-robot commented Nov 8, 2022

fedebongio commented Nov 8, 2022

mimowo left a comment •

edited

Loading

mimowo Nov 21, 2022

mimowo Nov 21, 2022

pohly Feb 15, 2023

dims commented Dec 12, 2022

sftim commented Jan 10, 2023

pohly commented Feb 15, 2023

k8s-triage-robot commented May 16, 2023

k8s-triage-robot commented Jun 15, 2023

pohly commented Jun 15, 2023

mengjiao-liu commented Jun 16, 2023 •

edited

Loading

mengjiao-liu commented Jun 16, 2023

k8s-ci-robot commented Jun 16, 2023

job controller: use contextual logging #113576

job controller: use contextual logging #113576

Conversation

sanwishe commented Nov 3, 2022

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 Nov 3, 2022

k8s-ci-robot commented Nov 3, 2022

yangjunmyfm192085 left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 3, 2022

shivanshuraj1333 commented Nov 8, 2022

k8s-ci-robot commented Nov 8, 2022

fedebongio commented Nov 8, 2022

mimowo left a comment • edited Loading

Choose a reason for hiding this comment

mimowo Nov 21, 2022

Choose a reason for hiding this comment

mimowo Nov 21, 2022

Choose a reason for hiding this comment

pohly Feb 15, 2023

Choose a reason for hiding this comment

dims commented Dec 12, 2022

sftim commented Jan 10, 2023

pohly commented Feb 15, 2023

k8s-triage-robot commented May 16, 2023

k8s-triage-robot commented Jun 15, 2023

pohly commented Jun 15, 2023

mengjiao-liu commented Jun 16, 2023 • edited Loading

mengjiao-liu commented Jun 16, 2023

k8s-ci-robot commented Jun 16, 2023

mimowo left a comment •

edited

Loading

mengjiao-liu commented Jun 16, 2023 •

edited

Loading