Structured log migration for pkg/kubelet/kuberuntime/kuberuntime_gc.go #97569

jialaijun · 2020-12-29T09:43:39Z

change log functions to structured equivalent.

None

k8s-ci-robot · 2020-12-29T09:43:47Z

Welcome @jialaijun!

It looks like this is your first PR to kubernetes/kubernetes 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kubernetes has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2020-12-29T09:43:47Z

Hi @jialaijun. 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 · 2020-12-29T09:44:18Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jialaijun
To complete the pull request process, please assign derekwaynecarr after the PR has been reviewed.
You can assign the PR to them by writing /assign @derekwaynecarr in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

pkg/kubelet/OWNERS

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

jialaijun · 2020-12-30T01:18:18Z

/assign @derekwaynecarr

kikisdeliveryservice · 2021-01-22T01:10:01Z

@jialaijun can you update the PR description - this needs a release note as it's a user facing logging change.

/triage accepted
/ok-to-test

kikisdeliveryservice · 2021-01-22T01:12:29Z

pkg/kubelet/kuberuntime/kuberuntime_gc.go

@@ -357,17 +357,17 @@ func (cgc *containerGC) evictPodLogsDirectories(allSourcesReady bool) error {
 					// See https://github.com/kubernetes/kubernetes/issues/52172
 					//
 					// We only remove unhealthy symlink for dead containers
-					klog.V(5).Infof("Container %q is still running, not removing symlink %q.", containerID, logSymlink)
+					klog.V(5).InfoS("Container is still running, not removing symlink.", "containerID", containerID, "logSymlink", logSymlink)


i don't think we need the period after symlink here

QiWang19 · 2021-02-19T16:31:18Z

/priority important-soon

adisky · 2021-02-23T05:19:00Z

pkg/kubelet/kuberuntime/kuberuntime_gc.go

@@ -357,17 +357,17 @@ func (cgc *containerGC) evictPodLogsDirectories(allSourcesReady bool) error {
 					// See https://github.com/kubernetes/kubernetes/issues/52172
 					//
 					// We only remove unhealthy symlink for dead containers
-					klog.V(5).Infof("Container %q is still running, not removing symlink %q.", containerID, logSymlink)
+					klog.V(5).InfoS("Container is still running, not removing symlink.", "containerID", containerID, "logSymlink", logSymlink)


Suggested change

klog.V(5).InfoS("Container is still running, not removing symlink.", "containerID", containerID, "logSymlink", logSymlink)

klog.V(5).InfoS("Container is still running, not removing symlink", "containerID", containerID, "symlink", logSymlink)

should probably be path rather than symlink

adisky · 2021-02-23T05:19:33Z

pkg/kubelet/kuberuntime/kuberuntime_gc.go

 					continue
 				}
 			} else {
-				klog.V(4).Infof("unable to obtain container Id: %v", err)
+				klog.V(4).InfoS("unable to obtain container Id", "err", err)


Suggested change

klog.V(4).InfoS("unable to obtain container Id", "err", err)

klog.V(4).InfoS("Unable to obtain container Id", "err", err)

adisky · 2021-02-23T05:20:02Z

pkg/kubelet/kuberuntime/kuberuntime_gc.go

 			}
 			err := osInterface.Remove(logSymlink)
 			if err != nil {
 				klog.Errorf("Failed to remove container log dead symlink %q: %v", logSymlink, err)
 			} else {
-				klog.V(4).Infof("removed symlink %s", logSymlink)
+				klog.V(4).InfoS("removed symlink", "logSymlink", logSymlink)


Suggested change

klog.V(4).InfoS("removed symlink", "logSymlink", logSymlink)

klog.V(4).InfoS("Removed symlink", "symlink", logSymlink)

I suggest path rather than symlink

ehashman · 2021-03-03T00:28:19Z

/retitle Structured log migration for pkg/kubelet/kuberuntime/kuberuntime_gc.go

ehashman · 2021-03-08T22:01:46Z

@jialaijun please take a look at the PR comments and make the requested changes :)

ehashman · 2021-03-12T22:27:54Z

/close

superseded by #100196

k8s-ci-robot · 2021-03-12T22:28:04Z

@ehashman: Closed this PR.

In response to this:

/close

superseded by #100196

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.

change log functions to structured equivalent.

4da58bd

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 Dec 29, 2020

k8s-ci-robot added area/kubelet sig/node Categorizes an issue or PR as relevant to SIG Node. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Dec 29, 2020

k8s-ci-robot requested review from resouer and vishh December 29, 2020 09:44

k8s-ci-robot assigned derekwaynecarr Dec 30, 2020

pacoxu mentioned this pull request Jan 4, 2021

Structured logging kubernetes/enhancements#1602

Open

39 tasks

ehashman added this to Needs Reviewer in SIG Node PR Triage Jan 6, 2021

ehashman moved this from Needs Reviewer to Triage in SIG Node PR Triage Jan 6, 2021

kikisdeliveryservice added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Jan 22, 2021

k8s-ci-robot removed the do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. label Jan 22, 2021

kikisdeliveryservice moved this from Triage to Waiting on Author in SIG Node PR Triage Jan 22, 2021

kikisdeliveryservice reviewed Jan 22, 2021

View reviewed changes

k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Feb 19, 2021

adisky reviewed Feb 23, 2021

View reviewed changes

ehashman added this to Waiting on Author in Structured Logging Migration for Kubelet, 1.21 Mar 3, 2021

k8s-ci-robot changed the title ~~change log functions to structured equivalent.~~ Structured log migration for pkg/kubelet/kuberuntime/kuberuntime_gc.go Mar 3, 2021

ehashman mentioned this pull request Mar 4, 2021

Kubelet migration to structured logs #98976

Closed

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 Mar 5, 2021

ehashman mentioned this pull request Mar 12, 2021

Migrate remaining logs to structured logging #100196

Merged

k8s-ci-robot closed this Mar 12, 2021

SIG Node PR Triage automation moved this from Waiting on Author to Done Mar 12, 2021

Structured Logging Migration for Kubelet, 1.21 automation moved this from Waiting on Author to Done Mar 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Structured log migration for pkg/kubelet/kuberuntime/kuberuntime_gc.go #97569

Structured log migration for pkg/kubelet/kuberuntime/kuberuntime_gc.go #97569

jialaijun commented Dec 29, 2020 •

edited by puerco

k8s-ci-robot commented Dec 29, 2020

k8s-ci-robot commented Dec 29, 2020

k8s-ci-robot commented Dec 29, 2020

jialaijun commented Dec 30, 2020

kikisdeliveryservice commented Jan 22, 2021

kikisdeliveryservice Jan 22, 2021

QiWang19 commented Feb 19, 2021

adisky Feb 23, 2021

ehashman Mar 8, 2021

adisky Feb 23, 2021

adisky Feb 23, 2021

ehashman Mar 8, 2021

ehashman commented Mar 3, 2021

ehashman commented Mar 8, 2021

ehashman commented Mar 12, 2021

k8s-ci-robot commented Mar 12, 2021

	klog.V(5).InfoS("Container is still running, not removing symlink.", "containerID", containerID, "logSymlink", logSymlink)
	klog.V(5).InfoS("Container is still running, not removing symlink", "containerID", containerID, "symlink", logSymlink)

	klog.V(4).InfoS("unable to obtain container Id", "err", err)
	klog.V(4).InfoS("Unable to obtain container Id", "err", err)

	klog.V(4).InfoS("removed symlink", "logSymlink", logSymlink)
	klog.V(4).InfoS("Removed symlink", "symlink", logSymlink)

Structured log migration for pkg/kubelet/kuberuntime/kuberuntime_gc.go #97569

Structured log migration for pkg/kubelet/kuberuntime/kuberuntime_gc.go #97569

Conversation

jialaijun commented Dec 29, 2020 • edited by puerco

k8s-ci-robot commented Dec 29, 2020

k8s-ci-robot commented Dec 29, 2020

k8s-ci-robot commented Dec 29, 2020

jialaijun commented Dec 30, 2020

kikisdeliveryservice commented Jan 22, 2021

kikisdeliveryservice Jan 22, 2021

Choose a reason for hiding this comment

QiWang19 commented Feb 19, 2021

adisky Feb 23, 2021

Choose a reason for hiding this comment

ehashman Mar 8, 2021

Choose a reason for hiding this comment

adisky Feb 23, 2021

Choose a reason for hiding this comment

adisky Feb 23, 2021

Choose a reason for hiding this comment

ehashman Mar 8, 2021

Choose a reason for hiding this comment

ehashman commented Mar 3, 2021

ehashman commented Mar 8, 2021

ehashman commented Mar 12, 2021

k8s-ci-robot commented Mar 12, 2021

jialaijun commented Dec 29, 2020 •

edited by puerco