KEP-1967: promote size memory backed volumes to stable #4812

kannon92 · 2024-08-28T20:38:22Z

One-line PR description: Promote 1967 to stable.

Issue link: Support to size memory backed volumes #1967

Other comments:

kannon92 · 2024-09-18T16:17:37Z

bart0sh · 2024-09-19T11:13:25Z

keps/sig-node/1967-size-memory-backed-volumes/kep.yaml

@@ -2,6 +2,7 @@ title: Size memory backed volumes
 kep-number: 1967
 authors:
  - "@derekwaynecarr"
+  - "@kannon92" #for stable promotion
 owning-sig: sig-node
 participating-sigs:
  - sig-storage


Should we also change status (line 8 9) to 'implemented' ?

Not yet. I think after the GA PR goes in in k8s/k8s.

Correct, it's usually done after it was finished.

mrunalp

/lgtm

pacoxu · 2024-09-23T09:42:05Z

Are there any ongoing issues related?

emptyDir with medium: Memory mounts a tmpfs volume without nosuid,nodev,noexec kubernetes#48912 ? but not sure if this is a feature or bug.
Would you go through the list https://github.com/kubernetes/kubernetes/issues?q=is%3Aissue+is%3Aopen+size+memory+backed+volumes ?

kannon92 · 2024-09-24T16:53:22Z

Are there any ongoing issues related?

emptyDir with medium: Memory mounts a tmpfs volume without nosuid,nodev,noexec kubernetes#48912 ? but not sure if this is a feature or bug.

Would you go through the list https://github.com/kubernetes/kubernetes/issues?q=is%3Aissue+is%3Aopen+size+memory+backed+volumes ?

Looking at these, I don't see anything blocking GA for this feature.

There was one item that was concerning in that list but the author did post a website update for this feature which I think was the right call.

Memory EmptyDir tmpfs size capped at 100% of node RAM size, not SizeLimit as specified kubernetes#126617

This is a feature for NodeSwap with EmptyDir which we are forbiding in 1.31. So this would need to be a new KEP if the community thinks this is useful.

User can easily crash an entire cluster by creating pod with memory-backed EmptyDir kubernetes#119611

This one seems to go the route of documentation. EmptyDir with Memory follows cgroup settings for memory so it is recommended to use cgroups with this feature for memory. https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/#memory-backed-emptydir documents this here.

emptyDir persistent volumes kubernetes#75378

Separate feature request. Not in scope for this feature.

emptyDir with medium: Memory mounts a tmpfs volume without nosuid,nodev,noexec kubernetes#48912

This seems to be a security feature request but any progress on this would break existing users

keps/sig-node/1967-size-memory-backed-volumes/README.md

pacoxu · 2024-09-30T09:48:21Z

Thanks for the detailed explanation.
/lgtm

/assign @johnbelamaric
for PRR review

kannon92 · 2024-10-04T16:16:00Z

/unassign @johnbelamaric
/assign @soltysh

BenTheElder

[PRR Shadow] there seem to be some formatting errors from updating the KEP template, nothing major jumped out.

keps/sig-node/1967-size-memory-backed-volumes/README.md

keps/sig-node/1967-size-memory-backed-volumes/kep.yaml

soltysh

A few comments, the biggest one around missing testing coverage listed in the doc, because it seems the functionality has the required tests.

soltysh · 2024-10-09T11:12:20Z

keps/sig-node/1967-size-memory-backed-volumes/README.md

+We have eviction tests that make sure size backed volumes cannot exceed the size limit.
+
+See [eviction test](https://github.com/kubernetes/kubernetes/blob/b2031b3cb46e946ee72eab7bda87b046db138d62/test/e2e_node/eviction_test.go#L366).
+


The entire test section is missing/not aligned with latest template. Given this is planned to go to stable it would be good to ensure we have the sufficient coverage in units and integration/e2e (or both).

soltysh · 2024-10-09T11:15:23Z

keps/sig-node/1967-size-memory-backed-volumes/README.md

 Pods that run on that node will have memory backed volumes sized based on Linux
 host default.  The sizing may not align with actual available memory for an app.

-* **Are there any tests for feature enablement/disablement?**
+#### Are there any tests for feature enablement/disablement?
+
 No, testing behavior with the feature disabled is dependent on node operating


Seems like tests exist verifying just that, see https://github.com/kubernetes/kubernetes/blob/f7fef0384ec1c666e6b68cae83a3b3c6d95aa7b0/pkg/volume/emptydir/empty_dir_test.go#L1046-L1063

soltysh · 2024-10-09T11:17:06Z

keps/sig-node/1967-size-memory-backed-volumes/README.md

 None.

-* **Were upgrade and rollback tested? Was the upgrade->downgrade->upgrade path tested?**
+#### Were upgrade and rollback tested? Was the upgrade->downgrade->upgrade path tested?
+
 I do not believe this is applicable.


Manual testing is sufficient for this functionality with a cluster which has the feature turned on and then turning it off (or downgrade). I'd hope such tests were performed to ensure the correctness of this functionality, no?

I am not sure if they were performed. This was the answer for the PRR questions around 1.22.

I don't know if the upgrade/rollback is really that relevant given that this has been on since 1.22.

There are examples across different KEPs, using kind:
https://github.com/kubernetes/enhancements/tree/master/keps/sig-network/1669-proxy-terminating-endpoints#were-upgrade-and-rollback-tested-was-the-upgrade-downgrade-upgrade-path-tested

Where you can disable a feature, and thus achieving a "fake" rollback to test it w/o going this far back. It shouldn't take more than an hour, and would definitely be good to go through to ensure it's good enough for GA.

For kubelet, the best I can do is to create kind clusters with feature gates on and feature gates off. I don't think I can toggle a feature gate on an existing cluster for kubelet. Kind doesn't run kubelet as a pod so I can't really edit the feature gate in place.

Is that sufficent?

Yeah, it's definitely better than nothing.

I pushed an update.

soltysh · 2024-10-09T11:18:14Z

keps/sig-node/1967-size-memory-backed-volumes/README.md

-* **What are the SLIs (Service Level Indicators) an operator can use to determine 
-the health of the service?**
+#### What are the SLIs (Service Level Indicators) an operator can use to determine the health of the service?
+
 This does not seem relevant to this feature.


How about the general pod startup latencies https://github.com/kubernetes/community/blob/master/sig-scalability/slos/pod_startup_latency.md ?

soltysh · 2024-10-09T11:18:55Z

keps/sig-node/1967-size-memory-backed-volumes/kep.yaml

@@ -2,6 +2,7 @@ title: Size memory backed volumes
 kep-number: 1967
 authors:
  - "@derekwaynecarr"
+  - "@kannon92" #for stable promotion
 owning-sig: sig-node
 participating-sigs:
  - sig-storage


Correct, it's usually done after it was finished.

soltysh

/approve
the PRR

k8s-ci-robot · 2024-10-09T17:48:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kannon92, mrunalp, soltysh

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:

~~keps/prod-readiness/OWNERS~~ [soltysh]
~~keps/sig-node/OWNERS~~ [mrunalp]

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

kannon92 · 2024-10-09T18:45:42Z

We have approval from sig-node and PRR. Can I get a lgtm to merge this?

mrunalp · 2024-10-09T19:08:53Z

/lgtm

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 28, 2024

k8s-ci-robot requested review from derekwaynecarr and mrunalp August 28, 2024 20:38

k8s-ci-robot added kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/node Categorizes an issue or PR as relevant to SIG Node. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 28, 2024

kannon92 mentioned this pull request Aug 28, 2024

Support to size memory backed volumes #1967

Open

12 tasks

kannon92 force-pushed the promote-size-backed-volumes-stable branch from 3d10e3f to 0a1da02 Compare September 18, 2024 15:32

k8s-ci-robot assigned mrunalp Sep 18, 2024

bart0sh reviewed Sep 19, 2024

View reviewed changes

mrunalp approved these changes Sep 19, 2024

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 19, 2024

dipesh-rawat reviewed Sep 30, 2024

View reviewed changes

keps/sig-node/1967-size-memory-backed-volumes/README.md Show resolved Hide resolved

k8s-ci-robot assigned johnbelamaric and pacoxu Sep 30, 2024

kannon92 force-pushed the promote-size-backed-volumes-stable branch from 0a1da02 to 32203b7 Compare October 3, 2024 13:15

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 3, 2024

kannon92 force-pushed the promote-size-backed-volumes-stable branch 2 times, most recently from 1d88f57 to db42c77 Compare October 4, 2024 14:57

k8s-ci-robot assigned soltysh and unassigned johnbelamaric Oct 4, 2024

BenTheElder reviewed Oct 8, 2024

View reviewed changes

keps/sig-node/1967-size-memory-backed-volumes/README.md Outdated Show resolved Hide resolved

keps/sig-node/1967-size-memory-backed-volumes/README.md Outdated Show resolved Hide resolved

keps/sig-node/1967-size-memory-backed-volumes/kep.yaml Show resolved Hide resolved

promote size memory backed volumes to stable

27d50a4

kannon92 force-pushed the promote-size-backed-volumes-stable branch from d1e8126 to 27d50a4 Compare October 8, 2024 14:22

soltysh reviewed Oct 9, 2024

View reviewed changes

add kind upgrade/downgrade for feature

db021e7

kannon92 force-pushed the promote-size-backed-volumes-stable branch from b76477f to db021e7 Compare October 9, 2024 14:58

soltysh approved these changes Oct 9, 2024

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 9, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 9, 2024

k8s-ci-robot merged commit df1832c into kubernetes:master Oct 9, 2024
4 checks passed

k8s-ci-robot added this to the v1.32 milestone Oct 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KEP-1967: promote size memory backed volumes to stable #4812

KEP-1967: promote size memory backed volumes to stable #4812

kannon92 commented Aug 28, 2024

kannon92 commented Sep 18, 2024

bart0sh Sep 19, 2024

kannon92 Sep 19, 2024

soltysh Oct 9, 2024

mrunalp left a comment

pacoxu commented Sep 23, 2024

kannon92 commented Sep 24, 2024 •

edited

Loading

pacoxu commented Sep 30, 2024

kannon92 commented Oct 4, 2024

BenTheElder left a comment

soltysh left a comment

soltysh Oct 9, 2024

kannon92 Oct 9, 2024

soltysh Oct 9, 2024

kannon92 Oct 9, 2024

soltysh Oct 9, 2024

kannon92 Oct 9, 2024

soltysh Oct 9, 2024

kannon92 Oct 9, 2024 •

edited

Loading

soltysh Oct 9, 2024

kannon92 Oct 9, 2024

soltysh Oct 9, 2024

kannon92 Oct 9, 2024

soltysh Oct 9, 2024

soltysh left a comment

k8s-ci-robot commented Oct 9, 2024

kannon92 commented Oct 9, 2024

mrunalp commented Oct 9, 2024

		We have eviction tests that make sure size backed volumes cannot exceed the size limit.

		See [eviction test](https://github.com/kubernetes/kubernetes/blob/b2031b3cb46e946ee72eab7bda87b046db138d62/test/e2e_node/eviction_test.go#L366).

KEP-1967: promote size memory backed volumes to stable #4812

KEP-1967: promote size memory backed volumes to stable #4812

Conversation

kannon92 commented Aug 28, 2024

kannon92 commented Sep 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrunalp left a comment

Choose a reason for hiding this comment

pacoxu commented Sep 23, 2024

kannon92 commented Sep 24, 2024 • edited Loading

pacoxu commented Sep 30, 2024

kannon92 commented Oct 4, 2024

BenTheElder left a comment

Choose a reason for hiding this comment

soltysh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kannon92 Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soltysh left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 9, 2024

kannon92 commented Oct 9, 2024

mrunalp commented Oct 9, 2024

kannon92 commented Sep 24, 2024 •

edited

Loading

kannon92 Oct 9, 2024 •

edited

Loading