Repository navigation
Kube play: preserve memory-backed emptyDir data after init containers exit - #29754
devchris123 wants to merge 1 commit into
Conversation
f495722 to
07aceb4
Compare
|
[NON-BLOCKING] Packit jobs failed. @podman-container-tools/packit-jobs please check. Everyone else, feel free to ignore. |
|
Hello @magic-peach, |
|
LGTM From a code standpoint. Could you clean up the Title please a bit, at least remove your branch name? thanks @devchris123 |
Removing a one-shot init container can reduce the volume mount count to zero and unmount the host tmpfs before regular containers start. Keep memory-backed emptyDir volumes mounted through the infra container so their contents survive this transition. Infra-container volumes are normally inherited by containers joining the pod. Mark these internal lifecycle mounts as non-inheritable to avoid exposing an additional mount in every workload container. Fixes: podman-container-tools#29670 Signed-off-by: Christopher Wunder <wunder.christopher95@gmail.com>
07aceb4 to
63f29f0
Compare
|
I fixed the PR title and squash down to one commit. Thanks for your review. |
|
@TomSweeneyRedHat |
Honny1
left a comment
There was a problem hiding this comment.
Code, LGTM.
I am not convinced the approach mentioned in #24930 (comment) is better then implemented aproach. WDYT? @mheon @Luap99
|
@Honny1 There are some other problems I found with the current implemenation, which might need fixing in additional commits:
This could be done in this PR or a follow up PR, not inherently blockers I would say. |
|
This isn't the right approach. We should use an anonymous named volume using mount options to make it a tmpfs (from the command line, this would be |
|
Hello @mheon, The mounted tmpfs itself is currently tied to the container count currently using the volume. Inbetween the stop of an init container and the start of an app container the tmpfs is unmounted and data is lost. The infra container uses the existing mount/umount machinery based on usage count and keeps the mount count above 0 during the pod lifetime. E.g. it acts as a proxy for the pod lifetime. |
|
+1 to @mheon aproach. |
|
@mheon The current implementation is keeping the mount alive for the lifetime of the infra container as a proxy for the pod lifetime. Alternatively we would need to tie the lifetime of the tmpfs to the pod itself, but this requires a larger rearchitecturing. Is that what you are suggesting here? Bottom line the tmpfs mount must survive the lifetime of the pod, but at the same time not exist any longer than that. |
|
PTAL @mheon @podman-container-tools/podman-maintainers |
|
If the concern is about init containers, we should be able to paper over the cracks by mounting it manually via API, starting the pod, then unmounting it once the pod is started. Mount count will remain at 1, the volume will not be unmounted until start is complete, and we don't need to scatter untracked mounts in our filesystem |
I intentionally kept the commits as three commits for easier review. I can later squash with a commit message mentioning the issue as mentioned in the guidelines.
Problem
#29555 reported an
issue with memory-backed
emptyDirvolumes used bypodman kube play.#29565 addressed
sharing the tmpfs between regular containers that are running concurrently.
#29670 shows the
remaining case: data written by a one-shot init container is unavailable to a
regular container that starts afterward.
Root cause
A memory-backed
emptyDiris represented by an anonymous named volume. Podmanmounts a tmpfs at the volume's host-side mountpoint and bind-mounts that path
into the init container.
When a one-shot init container is removed, its volume mounts are released. If
this reduces the volume mount reference count to zero, Podman unmounts the
host-side tmpfs and its contents are lost.
Approach
The change adds an internal mount of every memory-backed
emptyDirvolume tothe pod's infra container.
Infra-container mounts are normally inherited by containers subsequently added
to the pod. That behavior supports pod-wide mounts such as:
I used an additional
NoInheritoption to bothspecgen.NamedVolumeandlibpod.ContainerNamedVolumework around this.Fixes: #29670
Related: #29555
Checklist
Ensure you have completed the following checklist for your pull request to be reviewed:
commits. (
git commit -s). (If needed, usegit commit -s --amend). The author email must matchthe sign-off email address. See CONTRIBUTING.md
for more information.
Fixes: #00000in commit message (if applicable)make validatepr(format/lint checks)Noneif no user-facing changes)Does this PR introduce a user-facing change?