/hold

This is dependent on the KEP outcome.

/triage accepted
/priority important-soon

/retest

What happens if the container crashes and is restarted?

I don't really understand how this is related to this feature. What exactly are you concerned about? This feature uses cgroup of the container to set a max memory limit. I don't see how that would be impacted by restarts.

IIUC A write to tmpfs creates dirty anonymous pages (can't be flushed to disk for reclaim) attributed to the container's cgroup. If that cgroup were to get torn down for any reason, those pages are still anonymous and dirty, so they get accounted by the next-parent cgroup (I think?). If this happens, you can imagine a situation where the container tries to restart and immediately OOMs because those tmpfs files are accounted to the pod.

What I am asking is whether we have thought through those sorts of failure modes. Do we ever tear down the container cgroup? Are we confident that the accounting stays with the container? If you all say we are satisfied with the testing of that, I'm satisfied, but I wanted to call it out as an area where I know there are traps :)

@thockin
As far as I can tell, we are not using pod c groups to track memory. I looked into the pod cgroup of a pod to see if we report memory cgroup settings for the containers. I don’t think we use the chorus for tracking (memory.current is set to 0 even though container memory.current is nonzero). Memory.max for pods was set to max.

So I don’t think we are tracking anything in the pod c group.

LGTM label has been added.

/retest

@thockin I asked @ndixita her thoughts on the pod tracking. This is not a concern because we haven't been using placing tracking information into the pod cgroup.

What I am asking is whether we have thought through those sorts of failure modes. Do we ever tear down the container cgroup? Are we confident that the accounting stays with the container? If you all say we are satisfied with the testing of that, I'm satisfied, but I wanted to call it out as an area where I know there are traps :)

I took this item because of its long outstanding status as a beta feature (quote is from you).

This gate has been Beta since 1.22 - is there any reason not to just set it to GA? Seems like something any contributor can do to score some brownie points, and since it has been beta so long, I doubt we can change any behavior, even if we want to?

I am paraphrasing you here but this feature has been in beta on since 1.22. At this stage, I think any new issue related to emptyDir and tmpfs should be considered separate from this issue. This feature was about setting a size limit to node allocatable or pod limit. It didn't change anything around how the cgroups are tracked from container restarts. I think the feature works as intended and we have e2e tests that verify the limits.

@thockin

What happens if the container has requests: {memory: 1Gi}, limits: {memory: 1Gi} and the volume has a size-limit of 2Gi ? If I write to the volume, can I cause the pod to fail to start?

#128339

Yes. It looks like if a container hits an OOM limit with tmpfs than it will be stuck in an error state. I don't think this feature introduced this though.

Even if we want to say that memory backed volume must have limit set and it must be lower than container limit, we will not be able to enforce it on API side without breaking existing Pods.

So the only option would be for kubelet to override the memory-backed volume size limit to the container's limit (when defined). This may help. But it also will not guarantee we out of the "OOMLoopBackoff" as code bootstrap can be quite memory-heavy.

I would see this as to be outside of the KEP scope, but it can be a good future enhancement. @thockin do you agree?

Thanks!

/lgtm
/approve

LGTM label has been added.

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kannon92, SergeyKanzhelev, thockin

The full list of commands accepted by this bot can be found here.

The pull request process is described here