Add missing user-facing part to allow volume subpath mounts #22040

basilgello · 2024-03-14T11:19:15Z

Add missing user-facing "subpath" option to mount subpaths of named volumes.

Does this PR introduce a user-facing change?

Yes, adding "subpath" as valid mount and volume option in podman-container-create(1), podman-run(1).

Allow users to mount subpaths of named volumes via 'subpath' mount option.

The code enabling volume subpath mounts has been merged in containers#17992 but no user-facing change was made. The documentation was also not updated. Signed-off-by: Vasyl Gello <vasek.gello@gmail.com>

openshift-ci · 2024-03-14T11:19:24Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: basilgello
Once this PR has been reviewed and has the lgtm label, please assign rhatdan for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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

basilgello · 2024-03-14T11:22:05Z

After #17992 was merged, I had to add only user-facing change to fix #20661

This is a squashed commit with vendors in for easier review, I will split the relevant commits into proper PRs once I get general ACK.

rhatdan · 2024-03-14T11:35:39Z

pkg/specgenutil/volumes.go

@@ -371,6 +371,19 @@ func parseMountOptions(mountType string, args []string) (*spec.Mount, error) {
 				return nil, fmt.Errorf("host directory cannot be empty: %w", errOptionArg)
 			}
 			mnt.Source = value
+		case "subpath":
+			if setSubpath {
+				return nil, fmt.Errorf("cannot pass 'subpath' option more than once: %w", errOptionArg)


nit: This error should use %q and name like the one just below.

packit-as-a-service · 2024-03-15T15:21:08Z

Cockpit tests failed for commit dcf9dae. @martinpitt, @jelly, @mvollmer please check.

Signed-off-by: Vasyl Gello <vasek.gello@gmail.com>

packit-as-a-service · 2024-03-15T16:50:08Z

Cockpit tests failed for commit dc75dfb. @martinpitt, @jelly, @mvollmer please check.

basilgello · 2024-03-15T17:45:24Z

@giuseppe you mentioned the security implications of this features in the linked issue. I think 17992 solved them, so please review.

giuseppe · 2024-03-15T21:22:49Z

vendor/github.com/containers/common/pkg/parse/parse.go

@@ -14,7 +14,7 @@ import (

 // ValidateVolumeOpts validates a volume's options
 func ValidateVolumeOpts(options []string) ([]string, error) {
-	var foundRootPropagation, foundRWRO, foundLabelChange, bindType, foundExec, foundDev, foundSuid, foundChown, foundUpperDir, foundWorkDir, foundCopy, foundCopySymlink int
+	var foundRootPropagation, foundRWRO, foundLabelChange, bindType, foundExec, foundDev, foundSuid, foundChown, foundUpperDir, foundWorkDir, foundCopy, foundCopySymlink, foundVolumeSubpath int


these changes must go in the https://github.com/containers/common project and then revendor them

I understand that, but I need some details.

Is the correct procedure here as follows?

Move this commit into separate PR in containers/common

Wait for it to be merged

go get -u github.com/containers/common && go mod vendor in this PR

Rebase this PR with new go.{mod,sum} and vendor

yes that is correct

@giuseppe bump?

sorry, forgot to submit the review.

giuseppe · 2024-03-15T21:23:14Z

@giuseppe you mentioned the security implications of this features in the linked issue. I think 17992 solved them, so please review.

yes, these are solved with #17992

Signed-off-by: Vasyl Gello <vasek.gello@gmail.com>

packit-as-a-service · 2024-03-16T07:18:27Z

Cockpit tests failed for commit f85ce21. @martinpitt, @jelly, @mvollmer please check.

packit-as-a-service · 2024-03-16T07:45:08Z

Cockpit tests failed for commit 9b0c136. @martinpitt, @jelly, @mvollmer please check.

packit-as-a-service · 2024-03-16T07:48:51Z

Cockpit tests failed for commit 9beab8b. @martinpitt, @jelly, @mvollmer please check.

packit-as-a-service · 2024-03-16T07:57:23Z

Cockpit tests failed for commit 6b07be0. @martinpitt, @jelly, @mvollmer please check.

packit-as-a-service · 2024-03-16T07:57:24Z

Cockpit tests failed for commit efcefef. @martinpitt, @jelly, @mvollmer please check.

packit-as-a-service · 2024-03-16T08:05:12Z

Cockpit tests failed for commit 6bbfcae. @martinpitt, @jelly, @mvollmer please check.

larsks · 2024-04-06T13:06:22Z

Docker has implemented this feature using the volume-subpath mount option (see pr 45687, landed on 19-jan-2024). This is available now in Docker v26.0.0.

It's probably worth updating this pr to adopt the same option names so that the cli and api are compatible.

openshift-merge-robot · 2024-04-29T10:10:42Z

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

mheon · 2024-05-06T15:28:06Z

@basilgello Poke - are you still working on this? If not, we may take over to push this over the finish line

basilgello · 2024-05-06T17:28:45Z

Let me spliy the PR tomorrow morning @mheon Should we follow up Docker convention as larsks mentioned or the current syntax is fine?

basilgello · 2024-05-06T17:30:19Z

Also I postponded the split because I expected to get primary ACK from @giuseppe or other code owner but I did not get it to the date + several other issues on my side.

mheon · 2024-05-06T17:41:56Z

Docker convention would be preferred.

Sorry for the sudden attention, we're starting to prep for Podman 5.1 and this came up as something that could get in.

basilgello · 2024-05-06T17:44:20Z

It is OK and I needed that nudge :) I will do the split and link the derived PRs to this one.

I am using this PR in https://salsa.debian.org/neurodebian-team/neurodebian-bot for 2 months.

basilgello · 2024-05-08T15:50:58Z

@mheon I need your help :) In #22410 you added subpath option to image mounts. But Docker chose to name subpath option as volume-subpath (as larsks mentioned, just the wrong PR) Now if I change the user-facing option name for volume and mount to Docker's one, should I also rename the image mount option and its docs?

AFAIK the merged code is still in dev branch so I think it is better to rename all user-facing subpath option name to volume-subpath. WDYT?

mheon · 2024-05-08T17:05:32Z

We can also keep the existing subpath option and add volume-subpath as an alias to it, so both the current one and the Docker compatible one work

…

On Wed, May 8, 2024 at 11:51 Vasyl Gello ***@***.***> wrote: @mheon <https://github.com/mheon> I need your help :) In #22410 <#22410> you added subpath option to image mounts. But Docker chose to name subpath option as volume-subpath (as larsks mentioned, just the wrong PR <docker/cli#4331>) Now if I change the user-facing option name for volume and mount to Docker's one, should I also rename the image mount option and its docs? AFAIK the merged code is still in dev branch so I think it is better to rename all user-facing subpath option name to volume-subpath. WDYT? — Reply to this email directly, view it on GitHub <#22040 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB3AOCHJELQZRZX2JALSX7LZBJCXRAVCNFSM6AAAAABEV4W6CGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMBQHA4DSNJUGA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

Allow "subpath" in volume options

7c5c3e3

The code enabling volume subpath mounts has been merged in containers#17992 but no user-facing change was made. The documentation was also not updated. Signed-off-by: Vasyl Gello <vasek.gello@gmail.com>

openshift-ci bot added the release-note label Mar 14, 2024

rhatdan reviewed Mar 14, 2024

View reviewed changes

basilgello force-pushed the subpaths-main branch from bc819ff to dcf9dae Compare March 15, 2024 14:18

basilgello added 2 commits March 15, 2024 17:28

Allow 'subpath' in mount options

454eb68

Signed-off-by: Vasyl Gello <vasek.gello@gmail.com>

Documentation change for subpath

7a1a9e3

Signed-off-by: Vasyl Gello <vasek.gello@gmail.com>

basilgello force-pushed the subpaths-main branch from dcf9dae to dc75dfb Compare March 15, 2024 15:28

giuseppe reviewed Mar 15, 2024

View reviewed changes

basilgello force-pushed the subpaths-main branch 5 times, most recently from 6b07be0 to efcefef Compare March 16, 2024 07:13

Add tests for volume subpath mounts

6bbfcae

Signed-off-by: Vasyl Gello <vasek.gello@gmail.com>

basilgello force-pushed the subpaths-main branch from efcefef to 6bbfcae Compare March 16, 2024 07:17

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add missing user-facing part to allow volume subpath mounts #22040

Add missing user-facing part to allow volume subpath mounts #22040

basilgello commented Mar 14, 2024

openshift-ci bot commented Mar 14, 2024

basilgello commented Mar 14, 2024

rhatdan Mar 14, 2024

packit-as-a-service bot commented Mar 15, 2024

packit-as-a-service bot commented Mar 15, 2024

basilgello commented Mar 15, 2024

giuseppe Mar 15, 2024

basilgello Mar 16, 2024

giuseppe Mar 18, 2024

basilgello Mar 25, 2024

giuseppe Mar 26, 2024

giuseppe commented Mar 15, 2024

packit-as-a-service bot commented Mar 16, 2024

packit-as-a-service bot commented Mar 16, 2024

packit-as-a-service bot commented Mar 16, 2024

packit-as-a-service bot commented Mar 16, 2024

packit-as-a-service bot commented Mar 16, 2024

packit-as-a-service bot commented Mar 16, 2024

larsks commented Apr 6, 2024

openshift-merge-robot commented Apr 29, 2024

mheon commented May 6, 2024

basilgello commented May 6, 2024

basilgello commented May 6, 2024

mheon commented May 6, 2024

basilgello commented May 6, 2024 •

edited

basilgello commented May 8, 2024

mheon commented May 8, 2024 via email

Add missing user-facing part to allow volume subpath mounts #22040

Are you sure you want to change the base?

Add missing user-facing part to allow volume subpath mounts #22040

Conversation

basilgello commented Mar 14, 2024

Does this PR introduce a user-facing change?

openshift-ci bot commented Mar 14, 2024

basilgello commented Mar 14, 2024

rhatdan Mar 14, 2024

Choose a reason for hiding this comment

packit-as-a-service bot commented Mar 15, 2024

packit-as-a-service bot commented Mar 15, 2024

basilgello commented Mar 15, 2024

giuseppe Mar 15, 2024

Choose a reason for hiding this comment

basilgello Mar 16, 2024

Choose a reason for hiding this comment

giuseppe Mar 18, 2024

Choose a reason for hiding this comment

basilgello Mar 25, 2024

Choose a reason for hiding this comment

giuseppe Mar 26, 2024

Choose a reason for hiding this comment

giuseppe commented Mar 15, 2024

packit-as-a-service bot commented Mar 16, 2024

packit-as-a-service bot commented Mar 16, 2024

packit-as-a-service bot commented Mar 16, 2024

packit-as-a-service bot commented Mar 16, 2024

packit-as-a-service bot commented Mar 16, 2024

packit-as-a-service bot commented Mar 16, 2024

larsks commented Apr 6, 2024

openshift-merge-robot commented Apr 29, 2024

mheon commented May 6, 2024

basilgello commented May 6, 2024

basilgello commented May 6, 2024

mheon commented May 6, 2024

basilgello commented May 6, 2024 • edited

basilgello commented May 8, 2024

mheon commented May 8, 2024 via email

basilgello commented May 6, 2024 •

edited