Migrate pkg/kubelet/winstats to contextual logging #131001

Chulong-Li · Mar 23, 2025

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Migrate pkg/kubelet/winstats to contextual logging

Which issue(s) this PR fixes:

Relates to #130069

Does this PR introduce a user-facing change?

Migrate pkg/kubelet/winstats to contextual logging

k8s-ci-robot · Mar 23, 2025

Hi @Chulong-Li. 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.

Chulong-Li · Mar 23, 2025

/wg structured-logging
/area logging
/kind cleanup
/cc @kubernetes/wg-structured-logging-reviews

k8s-ci-robot · Mar 23, 2025

@Chulong-Li: GitHub didn't allow me to request PR reviews from the following users: kubernetes/wg-structured-logging-reviews.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/wg structured-logging
/area logging
/kind cleanup
/cc @kubernetes/wg-structured-logging-reviews

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.

Chulong-Li · Mar 23, 2025

/cc @bart0sh

HirazawaUi · Mar 23, 2025

/ok-to-test

HirazawaUi · Mar 23, 2025

pkg/kubelet/winstats/cpu_topology_test.go

+
+	cadvisorapi "github.com/google/cadvisor/info/v1"
+	"github.com/stretchr/testify/assert"
+	"k8s.io/klog/v2/ktesting"


A super nit: please group the imports by organization. We generally refer to the following convention:

Imports are organized in groups, with blank lines between them. The standard library packages are always in the first group.

ref: https://go.dev/wiki/CodeReviewComments#imports

Although the lint in the k/k repo won’t mind it :)

@HirazawaUi Thanks for comments! That's a good point. Should we update it like below?

import ( "testing" "unsafe" cadvisorapi "github.com/google/cadvisor/info/v1" "github.com/stretchr/testify/assert" "k8s.io/klog/v2/ktesting" )

Yes, that's quite right.

@HirazawaUi Have updated the order of imports for all related files to follow the standard convention. Could you please take another look?

HirazawaUi · Mar 23, 2025

pkg/kubelet/winstats/network_stats_test.go

@@ -25,6 +25,7 @@ import (

 	cadvisorapi "github.com/google/cadvisor/info/v1"
 	"github.com/stretchr/testify/assert"
+	"k8s.io/klog/v2/ktesting"


Same as above.

Updated as well

Chulong-Li · Mar 23, 2025

/retest

Chulong-Li · Mar 23, 2025

/retest

bart0sh · Mar 23, 2025

@Chulong-Li Thank you for the PR! It looks good to me from the first brief look. Please, update lint configurations in hacks/ thanks!

k8s-ci-robot · Mar 24, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Chulong-Li
Once this PR has been reviewed and has the lgtm label, please assign dchen1107 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/OWNERS

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

Chulong-Li · Mar 24, 2025

@bart0sh Thanks for the reminder! Almost missed this part. I've just updated the lint configurations in hack folder. Please take another look. 😊

Chulong-Li · Mar 24, 2025

Squashed the commits for simplicity

Chulong-Li · Mar 24, 2025

/retest

Chulong-Li · Mar 25, 2025

/retest

HirazawaUi · Mar 25, 2025

The failure of the pull-kubernetes-e2e-capz-windows-master job does not seem to be caused by the current PR. This job has a very low success rate and is not a "Required" test job, so you don’t need to repeatedly test it.

bart0sh · Mar 25, 2025

/skip pull-kubernetes-e2e-capz-windows-master

bart0sh · Mar 25, 2025

pkg/kubelet/winstats/cpu_topology.go

@@ -233,7 +234,7 @@ func convertWinApiToCadvisorApi(buffer []byte) (int, int, []cadvisorapi.Node, er
 			}

 		default:
-			klog.V(4).Infof("Not using Windows CPU relationship type: %d", info.Relationship)


It's interesting that this non-structured logging was here. Theoretically it should raise lint errors as all pkg/kubelet is supposed to use structured logging.

We have a gap with linting: it doesn't run for Windows-only code like this one here. Might be worth closing?

cc @BenTheElder @aojea

We also don't for e.g. GOOS=arm64, right?

We have some very specific linting for "does it typecheck" that targets all platforms, with a custom tool.

I'd be a bit concerned about the cost to golangci-lint for all platforms, but I suppose we're more likely to have build tagged files for linux,windows generally speaking.

It's definitely gradual and a question of "bang for the buck". My expectation is that we have more Windows-only code than arm64-only code.

I had a brief look by setting GOOS=windows in the script. There are several valid reports where Windows code is not following conventions that are enforced when building for Linux. I wonder whether SIG Windows is aware.

Anyway, I'm not going to pursue this further. If I did, I would have to fix all those issues...

bart0sh · Mar 25, 2025

pkg/kubelet/winstats/cpu_topology_test.go

@@ -312,7 +315,8 @@ func Test_convertWinApiToCadvisorApi(t *testing.T) {
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			numOfCores, numOfSockets, nodes, err := convertWinApiToCadvisorApi(tt.buffer)
+			logger, _ := ktesting.NewTestContext(t)


nit: this can be done at the beginning of the function, before the main loop.

@bart0sh Thanks for this comment! Let me update it. BTW, do we have any best practices for when we should put it at the beginning of the function vs after the t.Run()?

In my opinion it should be done in the beginning of the function because it's a test(not a test case) context.

@bart0sh That make senses. Thanks for explanation!

bart0sh · Mar 25, 2025

@Chulong-Li Please, rebase on top of the latest master branch. This can potentially fix CI job failures.

Chulong-Li · Mar 25, 2025

The failure of the pull-kubernetes-e2e-capz-windows-master job does not seem to be caused by the current PR. This job has a very low success rate and is not a "Required" test job, so you don’t need to repeatedly test it.

@HirazawaUi Thanks for this information! This kind of failure should be transient.

Update the order of imports to follow the standard convention Quick update on import order for cadvisor_windows.go Update the hack files Update contextual logging in Test_convertWinApiToCadvisorApi

Chulong-Li · Mar 25, 2025

@bart0sh Thanks for all these comments! I've just updated and rebased the code. Hope it would fix that check failure issue. Could you please take another look?

Chulong-Li · Mar 25, 2025

/retest

bart0sh · Mar 26, 2025

/retest

bart0sh · Mar 26, 2025

/lgtm

/assign @mrunalp @SergeyKanzhelev
for approval

k8s-ci-robot · Mar 26, 2025

LGTM label has been added.

Git tree hash: b15b96feb9252e70a8081fbb6eed46da6cf02db3

k8s-ci-robot · Mar 26, 2025

@Chulong-Li: The following test 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-e2e-capz-windows-master	`0673e9c`	link	false	`/test pull-kubernetes-e2e-capz-windows-master`

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 · May 3, 2025

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-ci-robot requested review from andrewsykim and dchen1107 March 23, 2025 07:30

github-project-automation bot added this to SIG Node: code and documentation PRs Mar 23, 2025

github-project-automation bot moved this to Triage in SIG Node: code and documentation PRs Mar 23, 2025

k8s-ci-robot requested a review from bart0sh March 23, 2025 07:31

Chulong-Li mentioned this pull request Mar 23, 2025

Migrate Kubelet codebase to contextual logging #130069

Open

49 tasks

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 Mar 23, 2025

HirazawaUi reviewed Mar 23, 2025

View reviewed changes

bart0sh moved this from Triage to Needs Reviewer in SIG Node: code and documentation PRs Mar 23, 2025

Chulong-Li force-pushed the winstats-contextual-logging branch from d7578e6 to 2667b18 Compare March 24, 2025 14:22

bart0sh reviewed Mar 25, 2025

View reviewed changes

Migrate pkg/kubelet/winstats to contextual logging

0673e9c

Update the order of imports to follow the standard convention Quick update on import order for cadvisor_windows.go Update the hack files Update contextual logging in Test_convertWinApiToCadvisorApi

Chulong-Li force-pushed the winstats-contextual-logging branch from 2667b18 to 0673e9c Compare March 25, 2025 13:12

k8s-ci-robot assigned mrunalp and SergeyKanzhelev Mar 26, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 26, 2025

bart0sh moved this from Needs Reviewer to Needs Approver in SIG Node: code and documentation PRs Mar 26, 2025

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

Search code, repositories, users, issues, pull requests...

Migrate pkg/kubelet/winstats to contextual logging #131001

Are you sure you want to change the base?

Migrate pkg/kubelet/winstats to contextual logging #131001

Uh oh!

Conversation

Chulong-Li commented Mar 23, 2025

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Does this PR introduce a user-facing change?

Uh oh!

k8s-ci-robot commented Mar 23, 2025

Uh oh!

Chulong-Li commented Mar 23, 2025

Uh oh!

k8s-ci-robot commented Mar 23, 2025

Uh oh!

Chulong-Li commented Mar 23, 2025

Uh oh!

HirazawaUi commented Mar 23, 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!

Chulong-Li commented Mar 23, 2025

Uh oh!

Chulong-Li commented Mar 23, 2025

Uh oh!

bart0sh commented Mar 23, 2025

Uh oh!

k8s-ci-robot commented Mar 24, 2025

Uh oh!

Chulong-Li commented Mar 24, 2025

Uh oh!

Chulong-Li commented Mar 24, 2025

Uh oh!

Chulong-Li commented Mar 24, 2025

Uh oh!

Chulong-Li commented Mar 25, 2025

Uh oh!

HirazawaUi commented Mar 25, 2025

Uh oh!

bart0sh commented Mar 25, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bart0sh commented Mar 25, 2025

Uh oh!

Chulong-Li commented Mar 25, 2025

Uh oh!

Chulong-Li commented Mar 25, 2025

Uh oh!

Chulong-Li commented Mar 25, 2025

Uh oh!