chore(kubelet): migrate stats to contextual logging #130259

liangyuanpeng · 2025-02-19T07:02:05Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

partof #130069

/assign @bart0sh

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

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

k8s-ci-robot · 2025-02-19T07:02:16Z

Hi @liangyuanpeng. 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-sigs/prow repository.

k8s-ci-robot · 2025-02-19T07:02:36Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: liangyuanpeng
Once this PR has been reviewed and has the lgtm label, please assign dims for approval. For more information see the 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:

hack/OWNERS
pkg/kubelet/stats/OWNERS

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

bart0sh · 2025-02-19T08:16:23Z

pkg/kubelet/stats/cri_stats_provider_windows.go

@@ -53,17 +54,19 @@ func (s networkStats) GetHNSEndpointStats(endpointName string) (*hnslib.HNSEndpo
 func (p *criStatsProvider) listContainerNetworkStats() (map[string]*statsapi.NetworkStats, error) {
 	networkStatsProvider := newNetworkStatsProvider(p)

+	ctx := context.TODO()


It would be nice to have an explanation comment for every usage of context.TODO() explaining that this is a temporary solution and when it must be changed to a permanent one.

Thanks, updated.

The same here - please pass context from the upper level, thanks!

bart0sh · 2025-02-19T08:19:09Z

/ok-to-test
/triage accepted
/priority important-longterm
/wg structured-logging
/area logging

bart0sh · 2025-02-20T08:40:19Z

@liangyuanpeng please, fix CI failures, thanks!

HirazawaUi · 2025-02-23T13:35:33Z

pkg/kubelet/stats/cadvisor_stats_provider.go

@@ -516,10 +519,11 @@ func getCadvisorContainerInfo(ca cadvisor.Interface) (map[string]cadvisorapiv2.C
 		Recursive: true,
 	})
 	if err != nil {
+		logger := klog.FromContext(context.TODO())


You can have ListPodCPUAndMemoryStats() accept a context and pass it to getCadvisorContainerInfo(), which would avoid having to declare context.TODO() here.

HirazawaUi · 2025-02-23T13:45:16Z

pkg/kubelet/stats/cri_stats_provider.go

@@ -448,8 +452,9 @@ func (p *criStatsProvider) ImageFsDevice(ctx context.Context) (string, error) {
 // fsID. If any error occurs, this function logs the error and returns
 // nil.
 func (p *criStatsProvider) getFsInfo(fsID *runtimeapi.FilesystemIdentifier) (*cadvisorapiv2.FsInfo, error) {
+	logger := klog.FromContext(context.TODO())


getFsInfo() can accept the context passed from ImageFsStats(), thereby avoiding the need to declare context.TODO() here.

HirazawaUi · 2025-02-23T13:52:28Z

pkg/kubelet/stats/provider.go

 	// Rootfs and imagefs doesn't make sense for raw cgroup.
-	s := cadvisorInfoToContainerStats(cgroupName, info, nil, nil)
+	s := cadvisorInfoToContainerStats(ctx, cgroupName, info, nil, nil)


The context passed from the upper layer can be used here, thereby avoiding the need to declare context.TODO() in this scope.

The context passed from the upper layer can be used here

No upper layer here, This PR wan't change the parameter of method GetCgroupStats, because this would involve modifying the interface.

HirazawaUi · 2025-02-23T13:55:33Z

pkg/kubelet/stats/cri_stats_provider.go

@@ -505,8 +510,9 @@ func (p *criStatsProvider) addPodNetworkStats(
 		return
 	}

+	logger := klog.FromContext(context.TODO())


Declaring ctx := context.TODO() at the top of the function is a better practice. This ensures future developers are aware that the function may need to handle Context, particularly if cancellation logic or request scoped values might be added later.

Signed-off-by: Lan <[email protected]>

bart0sh · 2025-02-26T14:51:31Z

pkg/kubelet/stats/cadvisor_stats_provider.go

@@ -362,11 +364,15 @@ func buildPodRef(containerLabels map[string]string) statsapi.PodReference {

 // isPodManagedContainer returns true if the cinfo container is managed by a Pod
 func isPodManagedContainer(cinfo *cadvisorapiv2.ContainerInfo) bool {
+	// Use context.TODO() because we currently do not have a proper context to pass in.
+	// Replace this with an appropriate context when refactoring this function to accept a context parameter.
+	ctx := context.TODO()


Why not? There should be a context on the upper level, I believe.

bart0sh · 2025-02-26T15:01:22Z

pkg/kubelet/stats/cri_stats_provider_windows.go

+
+	// Use context.TODO() because we currently do not have a proper context to pass in.
+	// Replace this with an appropriate context when refactoring this function to accept a context parameter.
+	ctx := context.TODO()


context.TODO() should not be used in local functions. It should be passed from the upper level.

k8s-ci-robot · 2025-02-26T15:35:33Z

@liangyuanpeng: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubernetes-typecheck	`c439550`	link	true	`/test pull-kubernetes-typecheck`
pull-kubernetes-e2e-capz-windows-master	`c439550`	link	false	`/test pull-kubernetes-e2e-capz-windows-master`
pull-kubernetes-verify	`c439550`	link	true	`/test pull-kubernetes-verify`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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-sigs/prow repository. I understand the commands that are listed here.

k8s-ci-robot · 2025-03-25T06:13:01Z

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-sigs/prow repository.

k8s-triage-robot · 2025-06-24T22:07:51Z

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

bart0sh · 2025-06-30T14:15:45Z

@liangyuanpeng Are you going to work on this PR? If so, please rebase and address review comments. If not, please let me know and I'll create new PR for this component.

k8s-ci-robot assigned bart0sh Feb 19, 2025

k8s-ci-robot added needs-priority Indicates a PR lacks a `priority/foo` label and requires one. 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 Feb 19, 2025

k8s-ci-robot requested review from justaugustus and yujuhong February 19, 2025 07:02

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 Feb 19, 2025

liangyuanpeng force-pushed the clogging_kubelet_stats branch from 8aebf04 to 61c94fe Compare February 19, 2025 07:17

liangyuanpeng mentioned this pull request Feb 19, 2025

Migrate Kubelet codebase to contextual logging #130069

Open

50 tasks

bart0sh reviewed Feb 19, 2025

View reviewed changes

HirazawaUi reviewed Feb 23, 2025

View reviewed changes

liangyuanpeng force-pushed the clogging_kubelet_stats branch from 61c94fe to a8a0c0a Compare February 26, 2025 14:04

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 26, 2025

chore(kubelet): migrate stats to contextual logging

c439550

Signed-off-by: Lan <[email protected]>

liangyuanpeng force-pushed the clogging_kubelet_stats branch from a8a0c0a to c439550 Compare February 26, 2025 14:30

bart0sh reviewed Feb 26, 2025

View reviewed changes

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

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

chore(kubelet): migrate stats to contextual logging #130259

Are you sure you want to change the base?

chore(kubelet): migrate stats to contextual logging #130259

Conversation

liangyuanpeng commented Feb 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

Uh oh!

k8s-ci-robot commented Feb 19, 2025

Uh oh!

k8s-ci-robot commented Feb 19, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bart0sh commented Feb 19, 2025

Uh oh!

bart0sh commented Feb 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Feb 26, 2025

Uh oh!

k8s-ci-robot commented Mar 25, 2025

Uh oh!

k8s-triage-robot commented Jun 24, 2025

Uh oh!

bart0sh commented Jun 30, 2025

Uh oh!

Uh oh!

liangyuanpeng commented Feb 19, 2025 •

edited

Loading