-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
volume: add support for non-volatile upperdir
,workdir
for overlay volumes
#12712
volume: add support for non-volatile upperdir
,workdir
for overlay volumes
#12712
Conversation
/hold |
I have also added a test to verify persistent nature of custom |
Should this be done on the --mount option and not dirty up the --volume option? |
@rhatdan That would be better but I don't think the But I'll confirm thanks. |
libpod/container_internal_linux.go
Outdated
overlayMount, err := overlay.Mount(contentDir, mountPoint, namedVol.Dest, c.RootUID(), c.RootGID(), c.runtime.store.GraphOptions()) | ||
|
||
overlayOpts = nil | ||
if upperDir != "" && workDir != "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to check that either both upperdir
and workdir
are specified, or none of them.
We cannot have just one of upperdir
or workdir
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
libpod/container_internal_linux.go
Outdated
@@ -2781,6 +2803,7 @@ func (c *Container) copyTimezoneFile(zonePath string) (string, error) { | |||
} | |||
|
|||
func (c *Container) cleanupOverlayMounts() error { | |||
//overlay.CleanupContent("/tmp/exp-overlay") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be dropped?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
d678b25
to
ff133c1
Compare
If the uppderdir and workdir are on different file systems, do you get a decent error, or should we be checking that? |
@rhatdan We get an error from Error: OCI runtime error: runc create failed: unable to start container process: error during container init: error mounting "/home/flouthoc/.local/share/containers/storage/overlay-containers/07d4e523d4a83459c7ce4187dd2d563caf74e110a2ba1bc769cb55d095fade5e/userdata/overlay/969251762/merge" to rootfs at "/data": mount /home/flouthoc/.local/share/containers/storage/overlay-containers/07d4e523d4a83459c7ce4187dd2d563caf74e110a2ba1bc769cb55d095fade5e/userdata/overlay/969251762/merge:/data (via /proc/self/fd/7), data: lowerdir=/home/flouthoc/.local/share/containers/storage/volumes/myvol/_data,upperdir=/tmp/exp-overlay/upper,workdir=/home/flouthoc/tmp/work,userxattr,context="system_u:object_r:container_file_t:s0:c267,c630": invalid argument
|
9e603f9
to
5d2652c
Compare
@flouthoc Rebase so we can continue working on this. |
@rhatdan Yes will do. I just need to update man pages for buildah changes and bump vendor again. |
5d2652c
to
90f5de5
Compare
Since we vendor-ed buildah here: #12642 I'll start re-basing this again. |
2efb5e6
to
641b09a
Compare
4a1774f
to
96bd3c0
Compare
if overlayFlag && strings.Contains(o, "upperdir") { | ||
splitOpt := strings.SplitN(o, "=", 2) | ||
if len(splitOpt) > 1 { | ||
upperDir = splitOpt[1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should you check for duplicates?
if upperDir != "" {
return ERROR
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We ignore in underlying library if upperdir is empty. But I agree user should get early error so added here.
Expect(session).Should(Exit(0)) | ||
|
||
// volume should contain only `test` not `overlay` | ||
session = podmanTest.Podman([]string{"run", "--volume", volName + ":/data:O," + overlayOpts, ALPINE, "sh", "-c", "ls /data"}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you test two volume mounts, one without overlayOpts and one without, and make sure the one without is not perment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes added another mount with just overlay
and no custom upper/workdir which tests that content is not persisted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any man page changes?
…umes Often users want their overlayed volumes to be `non-volatile` in nature that means that same `upper` dir can be re-used by one or more containers but overall of nature of volumes still have to be `overlay` so work done is still on a overlay not on the actual volume. Following PR adds support for more advanced options i.e custom `workdir` and `upperdir` for overlayed volumes. So that users can re-use `workdir` and `upperdir` across new containers as well. Usage ```console $ podman run -it -v myvol:/data:O,upperdir=/path/persistant/upper,workdir=/path/persistant/work alpine sh ``` Signed-off-by: Aditya R <[email protected]>
96bd3c0
to
e64e650
Compare
@TomSweeneyRedHat thanks added a section to docs as well. |
LGTM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: flouthoc, giuseppe 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 |
…onymous volumes Similar feature was added for named overlay volumes here: containers#12712 Following PR just mimics similar feature for anonymous volumes. Often users want their anonymous overlayed volumes to be `non-volatile` in nature that means that same `upper` dir can be re-used by one or more containers but overall of nature of volumes still have to be overlay so work done is still on a overlay not on the actual volume. Following PR adds support for more advanced options i.e custom `workdir` and `upperdir` for overlayed volumes. So that users can re-use `workdir` and `upperdir` across new containers as well. Usage ```console podman run -it -v /some/path:/data:O,upperdir=/path/persistant/upper,workdir=/path/persistant/work alpine sh ``` Signed-off-by: Aditya R <[email protected]>
Often users want their overlayed volumes to be
non-volatile
in naturethat means that same
upper
dir can be re-used by one or morecontainers but overall of nature of volumes still have to be
overlay
so work done is still on a overlay not on the actual volume.
Following PR adds support for more advanced options i.e custom
workdir
and
upperdir
for overlayed volumes. So that users can re-useworkdir
and
upperdir
across new containers as well.Usage
$ podman run -it -v myvol:/data:O,upperdir=/path/persistant/upper,workdir=/path/persistant/work alpine sh
This PR is draft till vendor is updated:
c/common
c/buidlah
Closes: #12150