podman cp: support copying on tmpfs mounts #9593
Conversation
openshift-ci-robot
added
the
approved
|
@TomSweeneyRedHat FWIW, I would love to get this into 3.0 for two reasons: 1) It's a really cool thing and customers are asking for it (we can consider it a bug), 2) it will make backports much easier (there are still some bugs we need to iron out in cp). Is a backport for RHEL possible? |
vrothberg
force-pushed
the
cp-tmp
branch
4 times, most recently
from
b588b12 to
6f68c77
Compare
|
Just an FYI: in one of my test runs, I encountered a flake: "read unixpacket" has a horrible history of flakiness, usually relating to conmon: #7228, #3302. This was on my laptop, f33, conmon-2.0.26-1.fc33, 5.10.19-200.fc33. |
|
May I make a friendly suggestion? This shaves over one minute from the test's run time (126 seconds down to 58): Reason: |
Yes, I was afraid of that. I will add |
|
NOTE: we currently cannot copy tty devices from a rootless container. One I think we can merge. It's a very minor corner case and I will be adding a couple more commits to address other |
|
Updated. Thanks for the super helpful reviews! |
Just hit that in the F33 root system tests as well. |
vrothberg
force-pushed
the
cp-tmp
branch
3 times, most recently
from
2454ac0 to
8a2583b
Compare
|
Repushing. Many jobs timed out (Cc @cevich). |
vrothberg
force-pushed
the
cp-tmp
branch
2 times, most recently
from
0bbfb60 to
3e5fac7
Compare
Unfortunately, |
|
We really ought to add |
Traditionally, the path resolution for containers has been resolved on the *host*; relative to the container's mount point or relative to specified bind mounts or volumes. While this works nicely for non-running containers, it poses a problem for running ones. In that case, certain kinds of mounts (e.g., tmpfs) will not resolve correctly. A tmpfs is held in memory and hence cannot be resolved relatively to the container's mount point. A copy operation will succeed but the data will not show up inside the container. To support these kinds of mounts, we need to join the *running* container's mount namespace (and PID namespace) when copying. Note that this change implies moving the copy and stat logic into `libpod` since we need to keep the container locked to avoid race conditions. The immediate benefit is that all logic is now inside `libpod`; the code isn't scattered anymore. Further note that Docker does not support copying to tmpfs mounts. Tests have been extended to cover *both* path resolutions for running and created containers. New tests have been added to exercise the tmpfs-mount case. For the record: Some tests could be improved by using `start -a` instead of a start-exec sequence. Unfortunately, `start -a` is flaky in the CI which forced me to use the more expensive start-exec option. Signed-off-by: Valentin Rothberg <[email protected]>
|
I agree podman rm --timeout 0 -f is needed. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: giuseppe, rhatdan, vrothberg 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 |
github-actions
bot
added
the
locked - please file new issue/PR
Traditionally, the path resolution for containers has been resolved on
the host; relative to the container's mount point or relative to
specified bind mounts or volumes.
While this works nicely for non-running containers, it poses a problem
for running ones. In that case, certain kinds of mounts (e.g., tmpfs)
will not resolve correctly. A tmpfs is held in memory and hence cannot
be resolved relatively to the container's mount point. A copy operation
will succeed but the data will not show up inside the container.
To support these kinds of mounts, we need to join the running
container's mount namespace (and PID namespace) when copying.
Note that this change implies moving the copy and stat logic into
libpodsince we need to keep the container locked to avoid raceconditions. The immediate benefit is that all logic is now inside
libpod; the code isn't scattered anymore.Further note that Docker does not support copying to tmpfs mounts.
Tests have been extended to cover both path resolutions for running
and created containers. New tests have been added to exercise the
tmpfs-mount case.
Signed-off-by: Valentin Rothberg [email protected]
@containers/podman-maintainers PTAL
@edsantiago, I'd love your input especially on the test changes