Add support for mounting volumes with local driver and options by mheon · Pull Request #3931 · containers/podman

mheon · 2019-09-03T19:19:59Z

Volumes with the local driver and mount options specified are mounted using the mount command, instead of being plain directories. The volumes are mounted as the first container using them is started, and unmounted when the last container using them exits.

Given the mount/unmount behavior, we need to add a mount counter to volumes to know when to mount/unmount. This requires adding a mutable state to volumes, with associated methods in the DB to update and save the state.

Missing tests right now, but I've tested locally with tmpfs and btrfs with positive results.

openshift-ci-robot · 2019-09-03T19:20:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mheon

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [mheon]

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

mheon · 2019-09-03T19:20:25Z

@rhatdan PTAL - you said this was blocking your work on overlay volumes

TomSweeneyRedHat · 2019-09-03T21:10:37Z

I think not, but thought I'd ask. Any anticipated db upgrade issues?

mheon · 2019-09-03T21:13:14Z

The first commit should address most of them.

The bigger issue was the locks PR that went in earlier - we'll need a podman system renumber if any volumes were present when the upgrade happened.

TomSweeneyRedHat · 2019-09-03T21:25:53Z

Thanks @mheon. A quick browse and the code LGTM. The podman system renumber bit I'm assuming will be loud and proud in the appropriate release announcement and out on the Podman mailing list for starters.

baude · 2019-09-04T00:54:52Z

any reason not to volStateJSON, err = json.Marshal?

I think json.Marshal on a nil pointer makes angry errors

Oh wait, i see what you're saying. Probably will work.

This only works when changing the short decl := to an assignment =. Using the short decl := any variable on the left will be created unless already being declared in the same scope. Since volStateJSON is declared in an outer scope, it won't work unless we declare err before and change it to an assignment.

baude · 2019-09-04T01:00:54Z

lgtm other than nit question ... have you measured any perf impact to overall container running with this?

mheon · 2019-09-04T02:23:49Z

Should be 0 on containers that do not mount shared volumes.

On those with volumes requiring a full mount call, on first mount, it's pretty substantial - around 1s extra for a tmpfs, in my measurements. This about matches what I saw with Docker, though - was how I figured out they had to be doing the mount on container launch.

vrothberg

Nice work, @mheon. Just a couple of nits.

I looked into the man pages and noticed that there's no information about supported drivers. The only information I found is that the default is local.

vrothberg · 2019-09-04T11:01:53Z

We could use define.ErrNotImplemented.Error() here.

vrothberg · 2019-09-04T11:08:16Z

Can we use a constant instead of "local"?

vrothberg · 2019-09-04T11:14:51Z

Why are we changing the identifiers of the options and the driver? Prefixing with volume feels redundant since it's a VolumeConfig.

We need to discard anything already in the database. Previously we were doing absolutely no validation on these fields - now we're properly vetting their contents. We can't go back and make previous containers safe, so the next best option is to ignore them (they were all acting like volumes without options prior to this PR anyways)

vrothberg · 2019-09-04T11:42:16Z

This only works when changing the short decl := to an assignment =. Using the short decl := any variable on the left will be created unless already being declared in the same scope. Since volStateJSON is declared in an outer scope, it won't work unless we declare err before and change it to an assignment.

vrothberg · 2019-09-04T12:13:45Z

If we move it down a few lines, we can spare the defer and save a few cycles.

vrothberg · 2019-09-04T12:17:27Z

Same here: can we use a constant for "local"?

vrothberg · 2019-09-04T12:25:41Z

"error getting ..."?

vrothberg · 2019-09-04T12:35:39Z

I'd prefer calling the syscall directly. I am no mount expert, so I may be totally unaware of the difficulties.

My initial attempt tried that, but I abandoned it pretty quickly due to difficulties syncing up the defaults. Docker allows semi-arbitrary options to be passed, does almost no authentication, and pops them straight into a call to mount. We would need to reverse-engineer somewhat large tracts of the functionality of Coreutils mount to make this work properly without it.

vrothberg · 2019-09-04T12:37:29Z

Don't we need the options as well?

Oh, oops, that one's a mistake, reverting

rhatdan · 2019-09-04T19:41:13Z

I would like to see @vrothberg questions addressed, as well as squash the PRs, and add some tests. Examples in man pages would also be nice.

Looks very good.

mheon · 2019-09-05T14:28:05Z

Addressed review comments

rhatdan · 2019-09-05T16:09:59Z

@mheon can you squash your commits.

mheon · 2019-09-05T16:25:11Z

Sure

In upcoming commits, we're going to turn on the backends for these fields. Volumes with these set will act fundamentally differently from other volumes. There will probably be validation required for each field. Until now, though, we've freely allowed creation of volumes with these set - they just did nothing. So we have no idea what could be in the DB with old volumes. Change the struct tags so we don't have to worry about old, unvalidated data. We'll start fresh with new volumes. Signed-off-by: Matthew Heon <matthew.heon@pm.me>

We need to be able to track the number of times a volume has been mounted for tmpfs/nfs/etc volumes. As such, we need a mutable state for volumes. Add one, with the expected update/save methods in both states. There is backwards compat here, in that older volumes without a state will still be accepted. Signed-off-by: Matthew Heon <matthew.heon@pm.me>

mheon · 2019-09-05T17:38:05Z

Added tests

mheon · 2019-09-05T17:41:13Z

Assuming things go green, I think we're good for final review and merge

mheon · 2019-09-05T18:16:50Z

/retest

When volume options and the local volume driver are specified, the volume is intended to be mounted using the 'mount' command. Supported options will be used to volume the volume before the first container using it starts, and unmount the volume after the last container using it dies. This should work for any local filesystem, though at present I've only tested with tmpfs and btrfs. Signed-off-by: Matthew Heon <matthew.heon@pm.me>

When we fail to remove a container's SHM, that's an error, and we need to report it as such. This may be part of our lingering storage woes. Also, remove MNT_DETACH. It may be another cause of the storage removal failures. Signed-off-by: Matthew Heon <matthew.heon@pm.me>

mheon · 2019-09-05T21:41:39Z

Going green. @rhatdan @baude @vrothberg @TomSweeneyRedHat @giuseppe PTAL

vrothberg

LGTM

rhatdan · 2019-09-06T11:28:46Z

/lgtm

openshift-ci-robot requested review from jwhonce and rhatdan September 3, 2019 19:20

openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL labels Sep 3, 2019

baude reviewed Sep 4, 2019

View reviewed changes

vrothberg reviewed Sep 4, 2019

View reviewed changes

This was referenced Sep 4, 2019

WIP Support nfs local volume mounts #2415

Closed

Named volume not mounted on container start #3046

Closed

mheon force-pushed the volumes_with_options branch from 4267b38 to 29941a9 Compare September 5, 2019 14:26

mheon mentioned this pull request Sep 5, 2019

Container storage leak when the podman process is SIGKILL #3906

Closed

mheon force-pushed the volumes_with_options branch from f177d64 to 5b150b6 Compare September 5, 2019 16:27

mheon added 2 commits September 5, 2019 12:29

mheon force-pushed the volumes_with_options branch 3 times, most recently from cc80069 to f6ca089 Compare September 5, 2019 16:35

mheon force-pushed the volumes_with_options branch from 7c7e94e to d168f57 Compare September 5, 2019 17:40

mheon force-pushed the volumes_with_options branch from d168f57 to f24fa83 Compare September 5, 2019 18:04

mheon force-pushed the volumes_with_options branch 3 times, most recently from cfdd268 to 951ecb3 Compare September 5, 2019 20:18

mheon mentioned this pull request Sep 5, 2019

fix unit test using strings.Contains #3941

Merged

mheon added 2 commits September 5, 2019 17:12

mheon force-pushed the volumes_with_options branch from 951ecb3 to de9a394 Compare September 5, 2019 21:12

mheon mentioned this pull request Sep 5, 2019

Podman-remote run should wait for exit code #3934

Merged

vrothberg reviewed Sep 6, 2019

View reviewed changes

openshift-ci-robot assigned rhatdan Sep 6, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 6, 2019

openshift-merge-robot merged commit 0d8a224 into containers:master Sep 6, 2019

rh-atomic-bot mentioned this pull request Sep 6, 2019

rootless: Rearrange setup of rootless containers #3756

Merged

ryanj mentioned this pull request Sep 10, 2019

Error: podman run says the container name is already in use ryanj/poweroverinternet#10

Open

mheon added the Release Notes 1.6.0 label Sep 11, 2019

github-actions Bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 26, 2023

github-actions Bot locked as resolved and limited conversation to collaborators Sep 26, 2023

Conversation

mheon commented Sep 3, 2019

Uh oh!

openshift-ci-robot commented Sep 3, 2019

Uh oh!

mheon commented Sep 3, 2019

Uh oh!

TomSweeneyRedHat commented Sep 3, 2019

Uh oh!

mheon commented Sep 3, 2019

Uh oh!

TomSweeneyRedHat commented Sep 3, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

baude commented Sep 4, 2019

Uh oh!

mheon commented Sep 4, 2019

Uh oh!

vrothberg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhatdan commented Sep 4, 2019

Uh oh!

mheon commented Sep 5, 2019

Uh oh!

rhatdan commented Sep 5, 2019

Uh oh!

mheon commented Sep 5, 2019

Uh oh!

mheon commented Sep 5, 2019

Uh oh!

mheon commented Sep 5, 2019

Uh oh!

mheon commented Sep 5, 2019

Uh oh!

mheon commented Sep 5, 2019

Uh oh!

vrothberg left a comment

Choose a reason for hiding this comment

Uh oh!

rhatdan commented Sep 6, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!