Skip to content

node: mm-mgr: Migrate Memory Manager to contextual logging #130727

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

node: mm-mgr: Migrate Memory Manager to contextual logging #130727

wants to merge 11 commits into from

Conversation

swatisehgal
Copy link
Contributor

@swatisehgal swatisehgal commented Mar 11, 2025 •
edited
Loading

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Migrate Memory manager to contextual logging.

Which issue(s) this PR fixes:

Fixes 123037 and 130069

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Migrate Memory Manager to contextual logging

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Mar 11, 2025
@k8s-ci-robot 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 Mar 11, 2025
@swatisehgal swatisehgal force-pushed the mm-mgr-contexual-logging branch from 2682d8a to 269c6cb Compare March 11, 2025 16:34
@swatisehgal
Copy link
Contributor Author

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

@k8s-ci-robot
Copy link
Contributor

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

@k8s-ci-robot k8s-ci-robot added wg/structured-logging Categorizes an issue or PR as relevant to WG Structured Logging. area/logging labels Mar 11, 2025
@swatisehgal
Copy link
Contributor Author

/cc @bart0sh @ffromani

@ffromani
Copy link
Contributor

/triage accepted
/priority important-longterm

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Mar 11, 2025
bart0sh
bart0sh reviewed Mar 12, 2025
View reviewed changes
bart0sh
bart0sh reviewed Mar 12, 2025
View reviewed changes
bart0sh
bart0sh reviewed Mar 12, 2025
View reviewed changes
@swatisehgal
Copy link
Contributor Author

@pohly A gentle ping for your review on this one. Thanks in advance!

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 3, 2025
@swatisehgal swatisehgal force-pushed the mm-mgr-contexual-logging branch from c7989a1 to 24df696 Compare May 14, 2025 10:42
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 14, 2025
@swatisehgal
Copy link
Contributor Author

{Failed === RUN TestCmdConfigImagesList/valid:_latest_stable_Kubernetes_version_should_not_throw_the_warning_that_a_supported_etcd_version_cannot_be_found
config_test.go:46: failed CmdConfigImagesList running 'kubeadm config images list --v=1 --kubernetes-version=stable-1' with stderr output:

    	  expected: false
    	  actual: true
    
    FYI: This usually indicates that the 'SupportedEtcdVersion' map defined in 'cmd/kubeadm/app/constants/constants.go' needs to be updated.

--- FAIL: TestCmdConfigImagesList/valid:_latest_stable_Kubernetes_version_should_not_throw_the_warning_that_a_supported_etcd_version_cannot_be_found (1.06s)

=== RUN TestCmdConfigImagesList
--- FAIL: TestCmdConfigImagesList (1.06s)
}

Seems unrelated, let's retry

/test pull-kubernetes-unit-windows-master

@swatisehgal
Copy link
Contributor Author

/test pull-kubernetes-unit-windows-master

@swatisehgal
Copy link
Contributor Author

/test pull-kubernetes-unit-windows-master

unrelated failures

@swatisehgal swatisehgal force-pushed the mm-mgr-contexual-logging branch from 24df696 to 5dc4184 Compare May 19, 2025 06:17
@swatisehgal
Copy link
Contributor Author

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

@swatisehgal
Copy link
Contributor Author

@bart0sh @ffromani Could you do another round of review, I have addressed all the review comments.

bart0sh
bart0sh reviewed Jun 24, 2025
View reviewed changes
bart0sh
bart0sh reviewed Jun 24, 2025
View reviewed changes
bart0sh
bart0sh reviewed Jun 24, 2025
View reviewed changes
@swatisehgal swatisehgal force-pushed the mm-mgr-contexual-logging branch 2 times, most recently from ff272cb to 5285b9f Compare June 25, 2025 18:01
bart0sh
bart0sh reviewed Jun 26, 2025
View reviewed changes
@swatisehgal
node: mm-mgr: migrate to contextual logging
993c238 
Signed-off-by: Swati Sehgal <[email protected]>
@swatisehgal
node: mm-mgr: hack: Add memorymanager to enable log checks in CI
8aa9a9a 
Signed-off-by: Swati Sehgal <[email protected]>
@swatisehgal
node: mm-mgr: Address review comments
88f6db6 
Signed-off-by: Swati Sehgal <[email protected]>
@swatisehgal
node: mm-mgr: Update remaining instances of klog.Info -> logger.Info
122143a 
This commit will squashed with the main commit responsible for migration
to contextual logging.

Signed-off-by: Swati Sehgal <[email protected]>
@swatisehgal
node: mm-mgr: Addressing review comments: Background() to TODO()
a531d47 
This commit to be squashed into the main commit as well.

Signed-off-by: Swati Sehgal <[email protected]>
@swatisehgal
node: mm-mgr: state: Migrate to contextual logging
fdace04 
To be squashed with main commit

Signed-off-by: Swati Sehgal <[email protected]>
@swatisehgal
node: mm-mgr: state: migrate state_mem.go to contextual logging
8aa23f2 
To be squashed into main commit

Signed-off-by: Swati Sehgal <[email protected]>
@swatisehgal swatisehgal force-pushed the mm-mgr-contexual-logging branch from 5285b9f to 23e60e4 Compare June 26, 2025 11:26
@swatisehgal
node: mm-mgr: Pass logger to removeStaleState
e53ccac 
Signed-off-by: Swati Sehgal <[email protected]>
@swatisehgal
node:mm-mgr: Explore passing context to methods under Manager interface
a99de77 
We pass contexts to Start, RemoveContainer and GetAllocatableMemory.
At this stage it is not possible to cover all the methods as that would
have impact on other components e.g. Topology Manager and therefore require
changes there which is currently outside the scope of this PR.

Impact on Topology Manager is due to the fact that it defines
HintProvider interface which has Allocate, GetTopologyHints and
GetPodTopologyHints methods. Passing context to the abovementioned methods
would break the interface contract between Memory Manager not implementing
Hintprovider interface or require updating the definition of methods
under Topology Manager. The later is outside the scope for now and would
be handled when we migrate Topology Manager code base to contextual logging.

An alternative is to skip this partial migration in favor of doing it all in
one go but based on the suggestions in the review, we are passing contexts
and using those for logging purposes as much as possible without impacting
other components.

This commit is intentionally kept separate to drive conversation and can be
squahed or removed depending on reviewers' perspective.

Signed-off-by: Swati Sehgal <[email protected]>
@swatisehgal
node: mm-mgr: Pass context to AddContainer
d5fb56f 
Signed-off-by: Swati Sehgal <[email protected]>
@swatisehgal
node: mm-mgr: reduce changes to pkg/kubelet/cm package
277dd17 
Signed-off-by: Swati Sehgal <[email protected]>
@swatisehgal swatisehgal force-pushed the mm-mgr-contexual-logging branch from 23e60e4 to 277dd17 Compare June 26, 2025 11:38
@bart0sh
Copy link
Contributor

bart0sh commented Jun 30, 2025

/retest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pohly pohly pohly left review comments

@bart0sh bart0sh bart0sh left review comments

@ffromani ffromani ffromani left review comments

@Chulong-Li Chulong-Li Chulong-Li left review comments

@bobbypage bobbypage Awaiting requested review from bobbypage

@dchen1107 dchen1107 Awaiting requested review from dchen1107

Assignees
No one assigned
Labels
area/kubelet area/logging 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. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/node Categorizes an issue or PR as relevant to SIG Node. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on. wg/structured-logging Categorizes an issue or PR as relevant to WG Structured Logging.
Projects
SIG Node: code and documentation PRs
Status: Needs Reviewer
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@swatisehgal @k8s-ci-robot @ffromani @bart0sh @pohly @Chulong-Li