overlay, mount: add support for non-volatile `upperdir`, `workdir` for overlay volumes #3715

flouthoc · 2022-01-17T12:57:23Z

Expose MountWithOptions for overlay which allows users to pass more
verbose configuration for overlay mounts.

For instance upperdir, workerdir and in future volatile.

Allow users to specify non-volatile upper and workdir with overlay
mounts.

Usage

buildah from alpine
buildah run -v /something/lower:/test:z,O,upperdir=/somewhere/upperdir,workdir=/somwhere/workdir alpine-working-container cat /test/hello

Needed for: containers/podman#12712

flouthoc · 2022-01-17T12:59:59Z

@giuseppe @vrothberg @rhatdan @nalind @mtrmac PTAL

pkg/overlay/overlay.go

nalind · 2022-01-17T14:12:51Z

Please elaborate on the use case, and add tests.

mtrmac

Warning: I know nothing about overlay (I don’t think I have ever manually mounted one) except what I just read in the documentation, and I know even less about how c/storage uses it.

mtrmac · 2022-01-17T14:01:35Z

pkg/overlay/overlay.go

+// MountWithOptions creates a subdir of the contentDir based on the source directory
+// from the source system.  It then mounts up the source directory on to the
+// generated mount point and returns the mount point to the caller.
+// But allows api to set custom workdir, upperdir and other overlay options


FWIW – this is preexisting, and maybe I’m just ignorant. This documentation, even with the context of the overlay section of mount(8), is completely unintelligible to me. I have no idea how source/contentDir relate.

(Reading the code, apparently contentDir has a specific structure that’s not documented in this package.)

The other options are also undocumented.

… and the behavior of readonly, affecting whether $contentDir/lower is used, is completely non-obvious to me as a newcomer to the code.

pkg/overlay/overlay.go

mtrmac

Code LGTM, just documentation nits.

[NO NEW TESTS NEEDED]

Is this appropriate here?

flouthoc · 2022-01-19T11:44:05Z

Code LGTM, just documentation nits.

[NO NEW TESTS NEEDED]

Is this appropriate here?

Updated Docs.

Please elaborate on the use case, and add tests.

@nalind @mtrmac Following API is only used by podman and its much easier to test there -> containers/podman#12712 . We don't have flat tests for overlay in buildah hence i'm not entirely sure how to rig a test for this on buildah repo.

But if it does not breaks any existing buildah tests then should this be fine ?

TomSweeneyRedHat · 2022-01-20T01:10:44Z

Changes LGTM
Would like a final head nod from @nalind and @mtrmac , especially given the test status

flouthoc · 2022-01-20T05:54:51Z

@TomSweeneyRedHat I am also extending feature to buildah so we can test here as well.

Edit: @nalind @mtrmac extended buildah to support this feature to buildah as well and added a test for this.

…ed overlay Expose `MountWithOptions` for overlay which allows users to pass more verbose configuration for overlay mounts. For instance `upperdir, workerdir` and in future `volatile`. Signed-off-by: Aditya R <arajan@redhat.com>

mtrmac

Thanks for the tests!

Note that I’m just reading the diff.. I don’t know anything about the testing infrastructure, and I’ll defer to repo owners on the introduced UI.

run_linux.go

nalind

Code looks reasonable. Some nits about field names and some typos.

run_linux.go

tests/run.bats

pkg/overlay/overlay.go

nalind · 2022-01-20T15:45:00Z

pkg/overlay/overlay.go

+	// See discussion here for more context: https://github.com/containers/buildah/pull/3715#discussion_r786036959
+	// TODO: Should we address above comment and handle escaping of metacharacters like
+	// `comma`, `backslash` ,`colon` and any other specical characters
+	UpperDirOptionFragment string


Nit: the OptionFragment suffix sounds redundant.
Should probably note that if this field is set, WorkDirOptionFragment should also be set.

@nalind I decided to name both of them as fragment as they are still just fragments to overlay's options as discussed here #3715 (comment) . I named both of them in same manner as it describes the similar nature of field to the reader of the API

@nalind See that discussion, but I’m completely happy to leave naming/API design to you.

nalind · 2022-01-20T15:45:17Z

pkg/overlay/overlay.go

+	// See discussion here for more context: https://github.com/containers/buildah/pull/3715#discussion_r786036959
+	// TODO: Should we address above comment and handle escaping of metacharacters like
+	// `comma`, `backslash` ,`colon` and any other specical characters
+	WorkDirOptionFragment string


Nit: the OptionFragment suffix sounds redundant. Should probably note that if this field is set, UpperDirOptionFragment should also be set.

Reasoning for this is same as #3715 (comment)

Allow users to specify non-volatile `upper` and `workdir` with overlay mounts. Usage ```console buildah from alpine buildah run -v /something/lower:/test:z,O,upperdir=/somewhere/upperdir,workdir=/somwhere/workdir alpine-working-container cat /test/hello ``` Signed-off-by: Aditya R <arajan@redhat.com>

Signed-off-by: Aditya R <arajan@redhat.com>

rhatdan · 2022-01-21T12:36:57Z

/approve
/lgtm

openshift-ci · 2022-01-21T12:37:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flouthoc, 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:

~~OWNERS~~ [flouthoc,rhatdan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci bot added the approved label Jan 17, 2022

flouthoc mentioned this pull request Jan 17, 2022

volume: add support for non-volatile upperdir,workdir for overlay volumes containers/podman#12712

Merged

2 tasks

flouthoc force-pushed the overlay-extend-api branch from 73f29ed to 031cdaa Compare January 17, 2022 13:03

giuseppe reviewed Jan 17, 2022

View reviewed changes

pkg/overlay/overlay.go Outdated Show resolved Hide resolved

vrothberg reviewed Jan 17, 2022

View reviewed changes

pkg/overlay/overlay.go Show resolved Hide resolved

pkg/overlay/overlay.go Outdated Show resolved Hide resolved

mtrmac reviewed Jan 17, 2022

View reviewed changes

flouthoc force-pushed the overlay-extend-api branch 3 times, most recently from b4364a9 to d4d3cda Compare January 18, 2022 12:06

mtrmac reviewed Jan 18, 2022

View reviewed changes

pkg/overlay/overlay.go Outdated Show resolved Hide resolved

pkg/overlay/overlay.go Show resolved Hide resolved

pkg/overlay/overlay.go Outdated Show resolved Hide resolved

flouthoc force-pushed the overlay-extend-api branch from d4d3cda to 5a72d36 Compare January 19, 2022 11:10

flouthoc requested a review from mtrmac January 19, 2022 11:13

flouthoc force-pushed the overlay-extend-api branch from 5a72d36 to fe5ebb3 Compare January 19, 2022 11:18

mtrmac reviewed Jan 19, 2022

View reviewed changes

pkg/overlay/overlay.go Show resolved Hide resolved

pkg/overlay/overlay.go Outdated Show resolved Hide resolved

mtrmac reviewed Jan 19, 2022

View reviewed changes

flouthoc force-pushed the overlay-extend-api branch from fe5ebb3 to e92d00a Compare January 19, 2022 11:39

flouthoc force-pushed the overlay-extend-api branch from e92d00a to df7e4fe Compare January 20, 2022 09:22

flouthoc changed the title ~~overlay: add MountWithOptions to API which extends support for advanced overlay users.~~ overlay, mount: add support for non-volatile upperdir, workdir for overlay volumes Jan 20, 2022

flouthoc force-pushed the overlay-extend-api branch 3 times, most recently from 440ee51 to 57b01d4 Compare January 20, 2022 09:34

flouthoc requested review from mtrmac and nalind January 20, 2022 11:08

mtrmac reviewed Jan 20, 2022

View reviewed changes

run_linux.go Outdated Show resolved Hide resolved

run_linux.go Outdated Show resolved Hide resolved

run_linux.go Outdated Show resolved Hide resolved

flouthoc force-pushed the overlay-extend-api branch from 57b01d4 to 818bdda Compare January 20, 2022 15:09

nalind reviewed Jan 20, 2022

View reviewed changes

flouthoc force-pushed the overlay-extend-api branch from 818bdda to 03d1da1 Compare January 20, 2022 16:03

flouthoc added 2 commits January 20, 2022 21:35

vendor: bump c/common and others

685121b

Signed-off-by: Aditya R <arajan@redhat.com>

flouthoc force-pushed the overlay-extend-api branch from 03d1da1 to 685121b Compare January 20, 2022 16:06

openshift-ci bot assigned rhatdan Jan 21, 2022

openshift-ci bot added the lgtm label Jan 21, 2022

openshift-merge-robot merged commit b6f6306 into containers:main Jan 21, 2022

github-actions bot added the locked - please file new issue/PR label Sep 13, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

overlay, mount: add support for non-volatile `upperdir`, `workdir` for overlay volumes #3715

overlay, mount: add support for non-volatile `upperdir`, `workdir` for overlay volumes #3715

flouthoc commented Jan 17, 2022 •

edited

flouthoc commented Jan 17, 2022

nalind commented Jan 17, 2022

mtrmac left a comment

mtrmac Jan 17, 2022

mtrmac Jan 17, 2022

mtrmac left a comment

flouthoc commented Jan 19, 2022

TomSweeneyRedHat commented Jan 20, 2022

flouthoc commented Jan 20, 2022 •

edited

mtrmac left a comment

nalind left a comment

nalind Jan 20, 2022

flouthoc Jan 20, 2022 •

edited

mtrmac Jan 20, 2022

nalind Jan 20, 2022

flouthoc Jan 20, 2022

rhatdan commented Jan 21, 2022

openshift-ci bot commented Jan 21, 2022

overlay, mount: add support for non-volatile upperdir, workdir for overlay volumes #3715

overlay, mount: add support for non-volatile upperdir, workdir for overlay volumes #3715

Conversation

flouthoc commented Jan 17, 2022 • edited

flouthoc commented Jan 17, 2022

nalind commented Jan 17, 2022

mtrmac left a comment

Choose a reason for hiding this comment

mtrmac Jan 17, 2022

Choose a reason for hiding this comment

mtrmac Jan 17, 2022

Choose a reason for hiding this comment

mtrmac left a comment

Choose a reason for hiding this comment

flouthoc commented Jan 19, 2022

TomSweeneyRedHat commented Jan 20, 2022

flouthoc commented Jan 20, 2022 • edited

mtrmac left a comment

Choose a reason for hiding this comment

nalind left a comment

Choose a reason for hiding this comment

nalind Jan 20, 2022

Choose a reason for hiding this comment

flouthoc Jan 20, 2022 • edited

Choose a reason for hiding this comment

mtrmac Jan 20, 2022

Choose a reason for hiding this comment

nalind Jan 20, 2022

Choose a reason for hiding this comment

flouthoc Jan 20, 2022

Choose a reason for hiding this comment

rhatdan commented Jan 21, 2022

openshift-ci bot commented Jan 21, 2022

overlay, mount: add support for non-volatile `upperdir`, `workdir` for overlay volumes #3715

overlay, mount: add support for non-volatile `upperdir`, `workdir` for overlay volumes #3715

flouthoc commented Jan 17, 2022 •

edited

flouthoc commented Jan 20, 2022 •

edited

flouthoc Jan 20, 2022 •

edited