Fix: do not assign an empty value to the resource (CPU or memory) if it's not defined in the container #117615

aheng-ch · 2023-04-26T12:08:39Z

What type of PR is this?

/kind bug

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?

Do not assign an empty value to the resource (CPU or memory) that not defined when stores the resources allocated to the pod in checkpoint

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

k8s-ci-robot · 2023-04-26T12:08:49Z

Hi @aheng-ch. 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.

gjkim42

Can we add some tests for this?

gjkim42 · 2023-04-26T13:55:26Z

/ok-to-test

bart0sh · 2023-04-26T14:46:07Z

pkg/kubelet/kubelet.go

+						}
+						if _, ok := allocatedResources[v1.ResourceMemory]; ok {
+							c.Resources.Requests[v1.ResourceMemory] = allocatedResources[v1.ResourceMemory]
+						}
 					}


It looks like it's a good time to wrap this duplicated code into a function.

+1. Looks good otherwise.

Updated. PTAL @bart0sh @vinaykul

bart0sh · 2023-04-26T14:49:15Z

/triage accepted
/priority important-soon
/retest
/assign @vinaykul

bart0sh · 2023-05-24T08:10:31Z

@aheng-ch please provide release note

aheng-ch · 2023-05-24T09:57:10Z

@aheng-ch please provide release note

Added

bart0sh · 2023-05-24T15:54:28Z

pkg/kubelet/kubelet.go

+			}
+			if _, ok := allocatedResources[v1.ResourceMemory]; ok {
+				c.Resources.Requests[v1.ResourceMemory] = allocatedResources[v1.ResourceMemory]
+			}


Would it make sense to set requests values from checkpoint so we don't have different data in the checkpoint and in the c.Resources.Requests? Alternatively we can save/restore data to/from checkpoint in a bit smarter way so that if memory or cpu is not requested it will not be saved and restored?

Not 100% sure it makes sense, but this difference between checkpoint and memory data is a bit concerning.

@vinaykul @SergeyKanzhelev @aheng-ch WDYT?

@bart0sh GetContainerResourceAllocation reads from checkpoint because the Requests and checkpoint values (what kubelet agreed to at a prior point) can differ. Did I miss something?

sorry, this PR somehow slipped off my radar. I was thinking that some fields with nil value can be converted to empty lists before checkpointing them, so when they're read from a checkpoint we don't need to check if they're nil.

vinaykul · 2023-07-02T03:06:54Z

/lgtm

k8s-ci-robot · 2023-07-02T03:07:01Z

LGTM label has been added.

Git tree hash: 032e9c948f3e41fd47858fb472c05bc1e7851f9b

vinaykul · 2023-07-08T16:07:40Z

/test pull-kubernetes-e2e-gce-cos-alpha-features

bart0sh · 2023-08-03T11:59:47Z

/assign @mrunalp @derekwaynecarr @dchen1107

wenzhaojie · 2023-08-22T08:34:34Z

As of now, the latest version officially released is 1.28.0. I found that as long as the feature gate of InPlacePodVPA is turned on, restarting the cluster nodes will cause the kubelet to fail to start normally. Can this pr fix this problem?

LingyanYin · 2023-09-22T20:08:07Z

We used Inplace VPA in our product, and @LastNight1997 also found this issue
We reviewed the code, and it looks good cc @Jeffwan

LastNight1997 · 2023-10-11T02:48:33Z

We've merged this PR into our branch and it looks to be working fine in our production environment.

wenzhaojie · 2023-10-11T11:16:43Z

We've merged this PR into our branch and it looks to be working fine in our production environment.

What do you mean？Does this PR no longer needed?

LastNight1997 · 2023-10-12T02:59:28Z

@wenzhaojie No, I means that this PR is working well in our environment, and suggest it can be merged.

k8s-ci-robot · 2023-10-23T21:14:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aheng-ch, mrunalp, vinaykul

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:

~~pkg/kubelet/OWNERS~~ [mrunalp]

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 added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Apr 26, 2023

k8s-ci-robot requested review from mtaufen and yujuhong April 26, 2023 12:09

bart0sh added this to Triage in SIG Node PR Triage Apr 26, 2023

gjkim42 reviewed Apr 26, 2023

View reviewed changes

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 Apr 26, 2023

bart0sh reviewed Apr 26, 2023

View reviewed changes

k8s-ci-robot assigned vinaykul Apr 26, 2023

bart0sh moved this from Triage to Needs Reviewer in SIG Node PR Triage Apr 26, 2023

aheng-ch force-pushed the checkpoint branch from 1ed8a61 to 3cc1e82 Compare May 4, 2023 14:04

aheng-ch force-pushed the checkpoint branch from a7589fb to 208cf1a Compare May 24, 2023 08:00

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels May 24, 2023

bart0sh reviewed May 24, 2023

View reviewed changes

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

vinaykul mentioned this pull request Jul 8, 2023

K8s fails to throttle pod so that it follows resized CPU limit #118371

Open

k8s-ci-robot assigned dchen1107, derekwaynecarr and mrunalp Aug 3, 2023

bart0sh moved this from Needs Reviewer to Needs Approver in SIG Node PR Triage Aug 3, 2023

LingyanYin mentioned this pull request Sep 27, 2023

In-Place Update of Pod Resources kubernetes/enhancements#1287

Open

26 tasks

LingyanYin mentioned this pull request Oct 10, 2023

REQUEST: New membership for @LingyanYin kubernetes/org#4513

Closed

9 tasks

vinaykul approved these changes Oct 15, 2023

View reviewed changes

mrunalp approved these changes Oct 23, 2023

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 23, 2023

k8s-ci-robot merged commit e1824b6 into kubernetes:master Oct 23, 2023
13 checks passed

SIG Node PR Triage automation moved this from Needs Approver to Done Oct 23, 2023

k8s-ci-robot added this to the v1.29 milestone Oct 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: do not assign an empty value to the resource (CPU or memory) if it's not defined in the container #117615

Fix: do not assign an empty value to the resource (CPU or memory) if it's not defined in the container #117615

aheng-ch commented Apr 26, 2023 •

edited

k8s-ci-robot commented Apr 26, 2023

gjkim42 left a comment

gjkim42 commented Apr 26, 2023

bart0sh Apr 26, 2023

vinaykul May 4, 2023

aheng-ch May 4, 2023

bart0sh commented Apr 26, 2023

bart0sh commented May 24, 2023

aheng-ch commented May 24, 2023

bart0sh May 24, 2023

vinaykul Jul 2, 2023

bart0sh Aug 3, 2023

vinaykul commented Jul 2, 2023

k8s-ci-robot commented Jul 2, 2023

vinaykul commented Jul 8, 2023

bart0sh commented Aug 3, 2023

wenzhaojie commented Aug 22, 2023

LingyanYin commented Sep 22, 2023

LastNight1997 commented Oct 11, 2023

wenzhaojie commented Oct 11, 2023

LastNight1997 commented Oct 12, 2023

k8s-ci-robot commented Oct 23, 2023

Fix: do not assign an empty value to the resource (CPU or memory) if it's not defined in the container #117615

Fix: do not assign an empty value to the resource (CPU or memory) if it's not defined in the container #117615

Conversation

aheng-ch commented Apr 26, 2023 • edited

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

k8s-ci-robot commented Apr 26, 2023

gjkim42 left a comment

Choose a reason for hiding this comment

gjkim42 commented Apr 26, 2023

bart0sh Apr 26, 2023

Choose a reason for hiding this comment

vinaykul May 4, 2023

Choose a reason for hiding this comment

aheng-ch May 4, 2023

Choose a reason for hiding this comment

bart0sh commented Apr 26, 2023

bart0sh commented May 24, 2023

aheng-ch commented May 24, 2023

bart0sh May 24, 2023

Choose a reason for hiding this comment

vinaykul Jul 2, 2023

Choose a reason for hiding this comment

bart0sh Aug 3, 2023

Choose a reason for hiding this comment

vinaykul commented Jul 2, 2023

k8s-ci-robot commented Jul 2, 2023

vinaykul commented Jul 8, 2023

bart0sh commented Aug 3, 2023

wenzhaojie commented Aug 22, 2023

LingyanYin commented Sep 22, 2023

LastNight1997 commented Oct 11, 2023

wenzhaojie commented Oct 11, 2023

LastNight1997 commented Oct 12, 2023

k8s-ci-robot commented Oct 23, 2023

aheng-ch commented Apr 26, 2023 •

edited