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.

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

Expose commit memory used in WindowsMemoryUsage struct #119238

Merged
merged 2 commits into from
Jul 18, 2023
Merged

Expose commit memory used in WindowsMemoryUsage struct #119238

merged 2 commits into from
Jul 18, 2023

Conversation

kiashok
Copy link
Contributor

@kiashok kiashok commented Jul 11, 2023

What type of PR is this?

/kind feature

What this PR does / why we need it:

This PR adds a field to track the commit memory usage bytes to container stats.
In Linux, a process is charged with memory usage only based on the total amount of physical memory (working set bytes) it uses. However, on Windows, the total physical and virtual memory (commit memory) currently in use by the process, is used to account for its total memory consumption.
Because of this, auto scaling windows containers etc should rely on the commit memory used rather than the working set memory. HCS surfaces the commit memory used to hcsshim but it isn't getting tracked at the CRI layer. This PR will help to expose the total commit memory usage as part of container stats structure.

NOTE: The previous PR closed unexpectedly while rebasing to resolve conflicts. (Old PR - #117127)

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

No

CRI: expose commit memory bytes in container stats specific to Windows

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

/sig node
/sig windows

/cc @jsturtevant @marosset

@k8s-ci-robot k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jul 11, 2023
@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/windows Categorizes an issue or PR as relevant to SIG Windows. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jul 11, 2023
@k8s-ci-robot
Copy link
Contributor

Hi @kiashok. 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-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jul 11, 2023
@kiashok kiashok mentioned this pull request Jul 11, 2023
Closed
@kiashok
Copy link
Contributor Author

kiashok commented Jul 11, 2023

/cc @SergeyKanzhelev @pacoxu @furkatgofurov7

@furkatgofurov7
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 Jul 12, 2023
@bart0sh bart0sh added this to Triage in SIG Node PR Triage Jul 12, 2023
@jsturtevant
Copy link
Contributor

/triage accepted
/priority important-soon

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. 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 Jul 13, 2023
@jsturtevant
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 13, 2023
@kiashok
Copy link
Contributor Author

kiashok commented Jul 18, 2023

@SergeyKanzhelev @pacoxu Looks like another PR was merged 8 hours back that touches the proto files that this PR did causing some conflicts. The change log also needs to be modified to include this new PR. Working on these now.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 18, 2023
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 18, 2023
@kiashok
Copy link
Contributor Author

kiashok commented Jul 18, 2023

@SergeyKanzhelev @pacoxu Looks like another PR was merged 8 hours back that touches the proto files that this PR did causing some conflicts. The change log also needs to be modified to include this new PR. Working on these now.

@SergeyKanzhelev @pacoxu @markllama @jsturtevant Pushed new changes to resolve the conflict and add the new PR that was merged this morning. LGTMs have been reset. Could you please take a look?

@pacoxu
Copy link
Member

pacoxu commented Jul 18, 2023

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 18, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: d06abd3d60114e9ec311fbbd6ebbc308644645e9

@pacoxu pacoxu moved this from Triage to Done in SIG Node PR Triage Jul 18, 2023
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 18, 2023
@pacoxu
Copy link
Member

pacoxu commented Jul 18, 2023

need rebase again

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 18, 2023
@k8s-ci-robot k8s-ci-robot requested a review from pacoxu July 18, 2023 16:27
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 18, 2023
@kiashok
Copy link
Contributor Author

kiashok commented Jul 18, 2023

need rebase again

Rebased again and updated the config log to include another change that was merged last night! Could you please take a look? @pacoxu @SergeyKanzhelev @marosset @jsturtevant

SergeyKanzhelev
SergeyKanzhelev approved these changes Jul 18, 2023
View reviewed changes
Copy link
Member

@SergeyKanzhelev SergeyKanzhelev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 18, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: dc77ebf3e2b1b00ed84108025df5c75361fbaffb

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kiashok, marosset, mrunalp, pacoxu, SergeyKanzhelev

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 merged commit 6264fd9 into kubernetes:master Jul 18, 2023
12 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.28 milestone Jul 18, 2023
@kiashok kiashok mentioned this pull request Aug 10, 2023
Merged
@kiashok kiashok mentioned this pull request Oct 10, 2023
Closed
9 tasks
@jsturtevant jsturtevant mentioned this pull request Oct 11, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mrunalp mrunalp mrunalp approved these changes

@SergeyKanzhelev SergeyKanzhelev SergeyKanzhelev approved these changes

@marosset marosset marosset approved these changes

@jsturtevant jsturtevant Awaiting requested review from jsturtevant

@furkatgofurov7 furkatgofurov7 Awaiting requested review from furkatgofurov7

@pacoxu pacoxu Awaiting requested review from pacoxu

Assignees

@jsturtevant jsturtevant

@pacoxu pacoxu

@SergeyKanzhelev SergeyKanzhelev

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/feature Categorizes issue or PR as related to a new feature. 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-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. 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. sig/windows Categorizes an issue or PR as relevant to SIG Windows. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
SIG Node PR Triage
Archived in project
[sig-windows] PR tracking
Status: Done
SIG Node PR Triage
Done
Milestone
v1.28
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@kiashok @k8s-ci-robot @furkatgofurov7 @jsturtevant @pacoxu @mrunalp @SergeyKanzhelev @marosset