Don't limit the size on /run for systemd based containers #7346
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rhatdan 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 |
openshift-ci-robot
added
the
approved
|
I'm a little uncomfortable with this given that we don't set a memory limit by default. Maybe cap at 1gb or so by default on systems with >2gb of ram and no memory limit set? |
|
tmpfs already have this limit set and users can currently max out memory on /dev/shm, So this is just eliminating a potential bug, for not much advantage. tmpfs is controlled by kernel to be 50% of available memory, in cgroup or in host, depending on which one is lower. |
|
Does this line need to change too? https://github.com/containers/podman/blob/master/pkg/spec/storage.go#L174 |
|
We should not be changing anything in pkg/spec without a very good reason |
|
Can there be tests added? |
|
@TomSweeneyRedHat What do you want me to test? That the size is not set? |
|
@rhatdan my thinking was to set above and below the old limit and make sure that the value was set appropriately. |
We had a customer incident where they ran out of space on /run. If you don't specify size, it will be still limited to 50% or memory available in the cgroup the container is running in. If the cgroup is unlimited then the /run will be limited to 50% of the total memory on the system. Also /run is mounted on the host as exec, so no reason for us to mount it noexec. Signed-off-by: Daniel J Walsh <[email protected]>
|
LGTM |
| @@ -88,17 +88,11 @@ func parseVolumes(volumeFlag, mountFlag, tmpfsFlag []string, addReadOnlyTmpfs bo | |||
| if _, ok := unifiedVolumes[dest]; ok { | |||
| continue | |||
| } | |||
| localOpts := options | |||
| if dest == "/run" { | |||
| localOpts = append(localOpts, "noexec", "size=65536k") | |||
There was a problem hiding this comment.
I may have missed context here, but if I understand @TomSweeneyRedHat's suggestion, should that have been to remove only the size= portion, but not the noexec?
There was a problem hiding this comment.
I mentioned this in the PR
Also /run is mounted on the host as exec, so no reason for us to mount it noexec.
There was a problem hiding this comment.
Oh, it's in the commit message. Sorry, I missed that.
/lgtm
github-actions
bot
added
the
locked - please file new issue/PR
We had a customer incident where they ran out of space on /run.
If you don't specify size, it will be still limited to 50% or memory
available in the cgroup the container is running in. If the cgroup is
unlimited then the /run will be limited to 50% of the total memory
on the system.
Signed-off-by: Daniel J Walsh [email protected]