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

Migrate klog to klog.InfoS in pkg/kubelet #91624

Merged
merged 2 commits into from
Jun 4, 2020

Conversation

hase1128
Copy link
Contributor

@hase1128 hase1128 commented Jun 1, 2020

What type of PR is this?
/kind cleanup

What this PR does / why we need it:

Ref:

kubernetes/enhancements#1602
https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/1602-structured-logging

Log output before migration

I0601 19:25:53.698080   16895 kubelet_getters.go:173] status for pod nginx-pod-127.0.0.1 updated to {Running [{Initialized True 0001-01-01 00:00:00 +0000 UTC 2020-06-01 19:18:54 +0900 JST  } {Ready True 0001-01-01 00:00:00 +0000 UTC 2020-06-01 19:18:54 +0900 JST  } {ContainersReady True 0001-01-01 00:00:00 +0000 UTC 2020-06-01 19:18:54 +0900 JST  } {PodScheduled True 0001-01-01 00:00:00 +0000 UTC 2020-06-01 19:18:54 +0900 JST  }]    127.0.0.1 172.17.0.4 [{172.17.0.4}] 2020-06-01 19:18:54 +0900 JST [] [{nginx-container {nil &ContainerStateRunning{StartedAt:2020-06-01 18:27:34 +0900 JST,} nil} {nil nil nil} true 0 nginx:latest docker-pullable://nginx@sha256:6fff55753e3b34e36e24e37039ee9eae1fe38a6420d8ae16ef37c92d1eb26699 docker://c729caca14fd6b19fb1fdbf5fe436352abd31c488a134fd5e56ee9fe7dc98eaa 0xc0012c2069}] BestEffort []}

After migration

I0601 19:45:29.666416   27921 kubelet_getters.go:173] "Pod status updated" pod="default/nginx-pod-127.0.0.1" status=v1.PodStatus{Phase:"Running", Conditions:[]v1.PodCondition{v1.PodCondition{Type:"Initialized", Status:"True", LastProbeTime:v1.Time{Time:time.Time{wall:0x0, ext:0, loc:(*time.Location)(nil)}}, LastTransitionTime:v1.Time{Time:time.Time{wall:0x0, ext:63726604110, loc:(*time.Location)(0x750f720)}}, Reason:"", Message:""}, v1.PodCondition{Type:"Ready", Status:"True", LastProbeTime:v1.Time{Time:time.Time{wall:0x0, ext:0, loc:(*time.Location)(nil)}}, LastTransitionTime:v1.Time{Time:time.Time{wall:0x0, ext:63726604110, loc:(*time.Location)(0x750f720)}}, Reason:"", Message:""}, v1.PodCondition{Type:"ContainersReady", Status:"True", LastProbeTime:v1.Time{Time:time.Time{wall:0x0, ext:0, loc:(*time.Location)(nil)}}, LastTransitionTime:v1.Time{Time:time.Time{wall:0x0, ext:63726604110, loc:(*time.Location)(0x750f720)}}, Reason:"", Message:""}, v1.PodCondition{Type:"PodScheduled", Status:"True", LastProbeTime:v1.Time{Time:time.Time{wall:0x0, ext:0, loc:(*time.Location)(nil)}}, LastTransitionTime:v1.Time{Time:time.Time{wall:0x0, ext:63726604110, loc:(*time.Location)(0x750f720)}}, Reason:"", Message:""}}, Message:"", Reason:"", NominatedNodeName:"", HostIP:"127.0.0.1", PodIP:"172.17.0.4", PodIPs:[]v1.PodIP{v1.PodIP{IP:"172.17.0.4"}}, StartTime:(*v1.Time)(0xc000d74220), InitContainerStatuses:[]v1.ContainerStatus(nil), ContainerStatuses:[]v1.ContainerStatus{v1.ContainerStatus{Name:"nginx-container", State:v1.ContainerState{Waiting:(*v1.ContainerStateWaiting)(nil), Running:(*v1.ContainerStateRunning)(0xc000d74200), Terminated:(*v1.ContainerStateTerminated)(nil)}, LastTerminationState:v1.ContainerState{Waiting:(*v1.ContainerStateWaiting)(nil), Running:(*v1.ContainerStateRunning)(nil), Terminated:(*v1.ContainerStateTerminated)(nil)}, Ready:true, RestartCount:0, Image:"nginx:latest", ImageID:"docker-pullable://nginx@sha256:6fff55753e3b34e36e24e37039ee9eae1fe38a6420d8ae16ef37c92d1eb26699", ContainerID:"docker://c729caca14fd6b19fb1fdbf5fe436352abd31c488a134fd5e56ee9fe7dc98eaa", Started:(*bool)(0xc000f7f1a9)}}, QOSClass:"BestEffort", EphemeralContainerStatuses:[]v1.ContainerStatus(nil)}

@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jun 1, 2020
@k8s-ci-robot
Copy link
Contributor

Hi @hase1128. 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 needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 1, 2020
@k8s-ci-robot k8s-ci-robot requested review from matthyx and yujuhong June 1, 2020 10:47
@k8s-ci-robot k8s-ci-robot added sig/node Categorizes an issue or PR as relevant to SIG Node. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jun 1, 2020
@hase1128
Copy link
Contributor Author

hase1128 commented Jun 1, 2020

/assign @yujuhong
/assign @serathius

@yuzhiquan
Copy link
Member

/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 Jun 1, 2020
@matthyx
Copy link
Contributor

matthyx commented Jun 1, 2020

/kind cleanup
/priority important-longterm
/lgtm
/release-note-none

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. 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. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jun 1, 2020
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 1, 2020
@serathius
Copy link
Contributor

serathius commented Jun 1, 2020

Looks like there is an regression in serializing v1.Time when it is a struct subfield. Created separate issue for it kubernetes/klog#156

Not sure if intention of this log was to dump whole status (conditions is very hard to read).
I think we should consider changing this log to use status.Phase instead of status.

@hase1128
Copy link
Contributor Author

hase1128 commented Jun 2, 2020

Looks like there is an regression in serializing v1.Time when it is a struct subfield. Created separate issue for it kubernetes/klog#156

Is there any task to do in this migration PR?

Not sure if intention of this log was to dump whole status (conditions is very hard to read).
I think we should consider changing this log to use status.Phase instead of status.

I think so too. This function updates status for static pod.
I think status is updated in this line, and we can get all Pod status information correctly by GetPods function. so, I don't know whether it is necessary to output all status information to the log or not.

FYI
Here is log output in case of using status.Phase.
I would like to use status.Phase if it's accepted.

Before migration

I0602 10:07:06.146242    7665 kubelet_getters.go:173] status for pod nginx-pod-127.0.0.1 updated to Running

After migration

I0602 09:58:05.157647   23653 kubelet_getters.go:173] "Pod status updated" pod="default/nginx-pod-127.0.0.1" status="Running"

@serathius
Copy link
Contributor

I think it makes sense. Can you update the PR?
@yujuhong what's your opinion on this?

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 2, 2020
@hase1128
Copy link
Contributor Author

hase1128 commented Jun 2, 2020

I updated. (Using status.Phase)

@serathius
Copy link
Contributor

So looks like change is:
Before

I0601 19:25:53.698080   16895 kubelet_getters.go:173] status for pod nginx-pod-127.0.0.1 updated to {Running [{Initialized True 0001-01-01 00:00:00 +0000 UTC 2020-06-01 19:18:54 +0900 JST  } {Ready True 0001-01-01 00:00:00 +0000 UTC 2020-06-01 19:18:54 +0900 JST  } {ContainersReady True 0001-01-01 00:00:00 +0000 UTC 2020-06-01 19:18:54 +0900 JST  } {PodScheduled True 0001-01-01 00:00:00 +0000 UTC 2020-06-01 19:18:54 +0900 JST  }]    127.0.0.1 172.17.0.4 [{172.17.0.4}] 2020-06-01 19:18:54 +0900 JST [] [{nginx-container {nil &ContainerStateRunning{StartedAt:2020-06-01 18:27:34 +0900 JST,} nil} {nil nil nil} true 0 nginx:latest docker-pullable://nginx@sha256:6fff55753e3b34e36e24e37039ee9eae1fe38a6420d8ae16ef37c92d1eb26699 docker://c729caca14fd6b19fb1fdbf5fe436352abd31c488a134fd5e56ee9fe7dc98eaa 0xc0012c2069}] BestEffort []}

After

I0602 09:58:05.157647   23653 kubelet_getters.go:173] "Pod status updated" pod="default/nginx-pod-127.0.0.1" status="Running"

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 2, 2020
@hase1128
Copy link
Contributor Author

hase1128 commented Jun 2, 2020

So looks like change is:

correct.

@hase1128
Copy link
Contributor Author

hase1128 commented Jun 2, 2020

/retest

1 similar comment
@hase1128
Copy link
Contributor Author

hase1128 commented Jun 3, 2020

/retest

@serathius
Copy link
Contributor

ping @yujuhong

@serathius
Copy link
Contributor

Hey @Random-Liu can you approve this?

@Random-Liu
Copy link
Member

/approve
/lgtm

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hase1128, Random-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 Jun 4, 2020
@serathius
Copy link
Contributor

/retest

1 similar comment
@serathius
Copy link
Contributor

/retest

@k8s-ci-robot k8s-ci-robot merged commit 860837c into kubernetes:master Jun 4, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.19 milestone Jun 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubelet cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. release-note-none Denotes a PR that doesn't merit a release note. sig/node Categorizes an issue or PR as relevant to SIG Node. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants