Quadlet - explicit support for read-only-tmpfs #20479
Conversation
openshift-ci
bot
added
release-note
approved
|
Ephemeral COPR build failed. @containers/packit-build please check. |
ygalblum
force-pushed
the
quadlet-read-only
branch
from
72f2e37 to
5920dc0
Compare
| @@ -521,6 +522,10 @@ This is equivalent to the Podman `--pull` option | |||
|
|
|||
| If enabled, makes the image read-only. | |||
|
|
|||
| ### `ReadOnlyTmpfs=` (defaults to `yes`) | |||
|
|
|||
| If container is running in ReadOnly is set to yes, then mount a read-write tmpfs on /dev, /dev/shm, /run, /tmp, and /var/tmp. | |||
There was a problem hiding this comment.
| If container is running in ReadOnly is set to yes, then mount a read-write tmpfs on /dev, /dev/shm, /run, /tmp, and /var/tmp. | |
| If container is running and ReadOnly is set to yes, then mount a read-write tmpfs on /dev, /dev/shm, /run, /tmp, and /var/tmp. |
Should Quadlet error out if this option is set but ReadOnlyTmpfs is not (or turned off)?
There was a problem hiding this comment.
I've rephrased the sentence.
Should Quadlet error out if this option is set but ReadOnlyTmpfs is not (or turned off)?
In general, Quadlet does not validate the values or their relationship unless needed (e.g. NetworkSubnet, NetworkGateway and KeyNetworkIPRange). Furthermore, in this specific case, Podman does not fail if --read-only-tmpfs is set while --read-only isn't. So, I don't think Quadlet should validate the values
ygalblum
force-pushed
the
quadlet-read-only
branch
from
5920dc0 to
89a6074
Compare
I've updated the commit message |
Add Quadlet key and disconnect relationship withr read-only Update and add tests Update man with new key Remove the reference to VolatileTmpfs in the man page to reduce its usage, since the same functionality can be achieved using the Tmpfs key while keeping its support to maintain backward compatibility Signed-off-by: Ygal Blum <[email protected]>
ygalblum
force-pushed
the
quadlet-read-only
branch
from
89a6074 to
76cca08
Compare
|
Worse named option ever and it gets proliferated. (It was named by me) |
openshift-ci
bot
added
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vrothberg, ygalblum 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 |
| /* !volatileTmp, disable the default tmpfs from --read-only */ | ||
| podman.add("--read-only-tmpfs=false") | ||
| if volatileTmp && !readOnly { | ||
| podman.add("--tmpfs", "/tmp:rw,size=512M,mode=1777") |
There was a problem hiding this comment.
I know this is pre-existing, but I dislike seeing stuff hardcoded like this. It would be nice to get at least the second string into a conf file somewhere. Not for now, a future consideration.
There was a problem hiding this comment.
Generally speaking, I agree. But, with this change and the removal of this key from the man page we hope people will not use it anymore. The only reason it is still there is for backward compatibility and therefore I don't think we should change any of it.
|
LGTM |
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
github-actions
bot
added
the
locked - please file new issue/PR
Add Quadlet key and disconnect relationship with read-only
Update and add tests
Update man and remove reference to VolatileTmpfs
Does this PR introduce a user-facing change?
Yes
Resolves: #20439