node: cm: migrate container manager to contextual logging #134985

swatisehgal · 2025-10-30T18:48:04Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

This commit migrates the container manager package to use contextual logging.

This follows the contextual logging migration pattern where:
- Logger is passed from the boundary down to implementations
- klog.TODO() is used at call sites where context is not yet available
- Functions with context.Context extract logger via klog.FromContext(ctx)
- klog.InfoS/ErrorS/V().InfoS calls are replaced with logger.Info/Error/V().Info
- Sub-managers receive proper logger context from their callers

Some call sites still use klog.TODO() as placeholders, with TODO
comments indicating these will be replaced with proper contextual loggers
as the migration continues upward through the call stack.

Test/fake implementations use klog.Background() which is the appropriate
choice for test code where no real context is available.

Which issue(s) this PR is related to:

#130069

Does this PR introduce a user-facing change?

NONE

This commit migrates the container manager package to use contextual logging. This follows the contextual logging migration pattern where: - Logger is passed from the boundary down to implementations - klog.TODO() is used at call sites where context is not yet available - Functions with context.Context extract logger via klog.FromContext(ctx) - klog.InfoS/ErrorS/V().InfoS calls are replaced with logger.Info/Error/V().Info - Sub-managers receive proper logger context from their callers Some call sites still use klog.TODO() as placeholders, with TODO comments indicating these will be replaced with proper contextual loggers as the migration continues upward through the call stack. Test/fake implementations use klog.Background() which is the appropriate choice for test code where no real context is available. Mock file updates (pkg/kubelet/cm/testing/mocks.go) were done manually due to mockery tool incompatibility with Go 1.25 and will be committed separately with an explanation. Key changes: 1. PodContainerManager and ContainerManager interface updates: - PodContainerManager.EnsureExists: add logger parameter - PodContainerManager.Destroy: add logger parameter - PodContainerManager.ReduceCPULimits: add logger parameter - PodContainerManager.SetPodCgroupConfig: add logger parameter - ContainerManager.UpdateQOSCgroups: add logger parameter - Update all implementations (Linux, Windows, stub, noop, fake) 2. Helper function updates: - GetKubeletContainer: add logger parameter (Linux and unsupported platforms) - Update cmd/kubelet/app/server.go to pass logger to GetKubeletContainer - Comment out type assertion in helpers.go due to signature change 3. Cgroup manager contextual logging: - CgroupManager interface methods updated to accept logger: * Destroy: add logger parameter * ReduceCPULimits: add logger parameter * SetCgroupConfig: add logger parameter - Update cgroupCommon and unsupportedCgroupManager implementations - Migrate klog.InfoS/V().InfoS calls to logger.Info/V().Info 4. Container manager implementation updates: - Extract logger from context in NewContainerManager() and Start() - Pass logger to sub-managers (deviceManager, topologyManager) - Update DRA manager initialization to use logger from context - Migrate klog.InfoS/ErrorS to logger.Info/Error throughout 5. Call sites updated with TODO comments: - pkg/kubelet/kubelet.go: UpdateQOSCgroups, EnsureExists - pkg/kubelet/kubelet_pods.go: UpdateQOSCgroups, ReduceCPULimits, Destroy - pkg/kubelet/kuberuntime/kuberuntime_manager.go: SetPodCgroupConfig - pkg/kubelet/kuberuntime/kuberuntime_manager_test.go: test expectations Most call sites currently use klog.TODO() as placeholders, with TODO comments indicating these will be replaced with proper contextual loggers as the migration continues upward through the call stack. Test/fake implementations use klog.Background() which is the appropriate choice for test code where no real context is available. Mock file updates (pkg/kubelet/cm/testing/mocks.go) were done manually due to mockery tool incompatibility with Go 1.25 and will be committed separately with an explanation. Signed-off-by: Swati Sehgal <swsehgal@redhat.com>

ffromani · 2025-10-31T06:19:44Z

/triage accepted
/priority important-longterm

Autogenerated by running: ``` ./hack/update-mocks.sh ``` Signed-off-by: Swati Sehgal <swsehgal@redhat.com>

Signed-off-by: Swati Sehgal <swsehgal@redhat.com>

swatisehgal · 2025-10-31T10:07:22Z

@ffromani @bart0sh PTAL,this is ready for review!

ffromani

Few inline comments, but overall LGTM

like in other instances, I for one I'm not completely happy with the context/logger call chain, but I think is better to complete the porting and iterate later rather than dive into details at this stage. The current changeset is still an improvement.

pkg/kubelet/cm/cgroup_manager_linux.go

pkg/kubelet/cm/container_manager_linux.go

pkg/kubelet/cm/helpers.go

ffromani · 2025-10-31T14:44:04Z

pkg/kubelet/cm/qos_container_manager_linux.go

+	Start(context.Context, func() v1.ResourceList, ActivePodsFunc) error
 	GetQOSContainersInfo() QOSContainersInfo
-	UpdateCgroups() error
+	UpdateCgroups(logger klog.Logger) error


not sure but this can probably use a context? we can also fix later

I don't have a strong opinion here. I chose klog.Logger here because:

The method doesn't need context for cancellation or deadlines - it's a simple synchronous update operation

It maintains consistency with the current pattern where we're passing loggers to methods that only need them for logging

We can keep it as-is with klog.Logger for now since it works, but I'm open to changing it to context if you prefer that approach.

I was under the impression that UpdateCgroups ultimately needs a Context as it ends up writing on sysfs through vendored packages; but if the context plumbing was not done yet, there's no ne need to do it now so we can leave as-is and we can re-evaluate later.

pkg/kubelet/kubelet_pods.go

Signed-off-by: Swati Sehgal <swsehgal@redhat.com>

ffromani · 2025-11-01T07:54:53Z

/lgtm

my comments have been addressed. Thanks!

ffromani · 2025-11-01T07:55:01Z

/retest

k8s-ci-robot · 2025-11-01T07:55:01Z

LGTM label has been added.

Git tree hash: 34ca225f4a8acee745394a03c36f906fdbecad13

k8s-ci-robot · 2025-11-01T08:27:11Z

@swatisehgal: 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	`f2d075e`	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.

bart0sh · 2025-11-01T10:20:53Z

/lgtm

/assign @ffromani @SergeyKanzhelev
for approval

bart0sh · 2025-11-01T11:00:21Z

/skip

ffromani

/approve

SergeyKanzhelev · 2025-11-03T16:03:38Z

/approve

k8s-ci-robot · 2025-11-03T16:03:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ffromani, SergeyKanzhelev, swatisehgal

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:

~~cmd/kubelet/OWNERS~~ [SergeyKanzhelev]
~~pkg/kubelet/OWNERS~~ [SergeyKanzhelev]
~~test/e2e_node/OWNERS~~ [SergeyKanzhelev,ffromani]

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

github-project-automation bot added this to SIG Node: code and documentation PRs and SIG Node CI/Test Board Oct 30, 2025

github-project-automation bot moved this to Triage in SIG Node CI/Test Board Oct 30, 2025

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

k8s-ci-robot requested review from ffromani and mtaufen October 30, 2025 18:48

node: cm: update mocks for contextual logging migration

67280d4

Autogenerated by running: ``` ./hack/update-mocks.sh ``` Signed-off-by: Swati Sehgal <swsehgal@redhat.com>

swatisehgal force-pushed the node-cm-ctx-logging-migration branch from b002ba7 to 67280d4 Compare October 31, 2025 06:50

node: cm: fix linter failures

f225705

Signed-off-by: Swati Sehgal <swsehgal@redhat.com>

swatisehgal force-pushed the node-cm-ctx-logging-migration branch from fc26905 to f225705 Compare October 31, 2025 08:14

bart0sh mentioned this pull request Oct 31, 2025

Migrate Kubelet codebase to contextual logging #130069

Open

49 tasks

ffromani reviewed Oct 31, 2025

View reviewed changes

Address review comments

f2d075e

Signed-off-by: Swati Sehgal <swsehgal@redhat.com>

swatisehgal force-pushed the node-cm-ctx-logging-migration branch from c1ecefa to f2d075e Compare October 31, 2025 17:42

k8s-ci-robot assigned ffromani Nov 1, 2025

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

bart0sh moved this from Triage to Needs Approver in SIG Node: code and documentation PRs Nov 1, 2025

k8s-ci-robot assigned SergeyKanzhelev and bart0sh Nov 1, 2025

ffromani reviewed Nov 1, 2025

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 3, 2025

k8s-ci-robot merged commit 4e84997 into kubernetes:master Nov 3, 2025
15 checks passed

k8s-ci-robot added this to the v1.35 milestone Nov 3, 2025

github-project-automation bot moved this from Needs Approver to Done in SIG Node: code and documentation PRs Nov 3, 2025

github-project-automation bot moved this from Triage to Done in SIG Node CI/Test Board Nov 3, 2025

node: cm: migrate container manager to contextual logging #134985

node: cm: migrate container manager to contextual logging #134985

Conversation

swatisehgal commented Oct 30, 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 is related to:

Does this PR introduce a user-facing change?

Uh oh!

ffromani commented Oct 31, 2025

Uh oh!

swatisehgal commented Oct 31, 2025

Uh oh!

ffromani left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ffromani Oct 31, 2025

Choose a reason for hiding this comment

Uh oh!

swatisehgal Oct 31, 2025

Choose a reason for hiding this comment

Uh oh!

ffromani Nov 1, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ffromani commented Nov 1, 2025

Uh oh!

ffromani commented Nov 1, 2025

Uh oh!

k8s-ci-robot commented Nov 1, 2025

Uh oh!

k8s-ci-robot commented Nov 1, 2025

Uh oh!

bart0sh commented Nov 1, 2025

Uh oh!

bart0sh commented Nov 1, 2025

Uh oh!

ffromani left a comment

Choose a reason for hiding this comment

Uh oh!

SergeyKanzhelev commented Nov 3, 2025

Uh oh!

k8s-ci-robot commented Nov 3, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

swatisehgal commented Oct 30, 2025 •

edited

Loading