WIP Support nfs local volume mounts #2415

rhatdan · 2019-02-22T21:22:16Z

Allow users to create volumes with nfs shares.

These volumes will get mounted when the first container gets started
and then will remain mounted until the system reboots or the volume is
removed.

Signed-off-by: Daniel J Walsh [email protected]

openshift-ci-robot · 2019-02-22T21:22:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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~~ [rhatdan]

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

rhatdan · 2019-02-22T21:22:56Z

FIxes https://bugzilla.redhat.com/show_bug.cgi?id=1672157

TomSweeneyRedHat · 2019-02-22T21:27:53Z

docs/podman-volume-create.1.md

@@ -39,6 +45,8 @@ $ podman volume create myvol
 $ podman volume create

 $ podman volume create --label foo=bar myvol
+
+# podman volume create --opt type=nfs --opt o=addr=192.168.0.2,rw --opt device=:/nfsshare  mynfsvol


Should this be device=:/nfsshare or device=/nfsshare (no colon)?

Both are supported.

TomSweeneyRedHat · 2019-02-22T21:29:26Z

cmd/podman/volume_create.go

@@ -28,7 +28,8 @@ be created at.`
 		},
 		Example: `podman volume create myvol
  podman volume create
-  podman volume create --label foo=bar myvol`,
+  podman volume create --label foo=bar myvol
+  podman volume create --opt type=nfs --opt o=addr=192.168.0.2,rw --opt device=:/nfsshare  mynfsvol`,


I've been trying to keep these to three examples. Could we kill line 31 and add '--label foo=bar' to line 29?

TomSweeneyRedHat · 2019-02-22T21:30:05Z

libpod/volume.go

@@ -15,7 +23,7 @@ type VolumeConfig struct {
 	// Name of the volume
 	Name string `json:"name"`

-	Labels     map[string]string `json:"labels"`
+	Labels     map[string]string `jsno:"labels"`


s/jsno/json/

TomSweeneyRedHat · 2019-02-22T21:32:30Z

libpod/volume.go

+		}
+	}
+	return ""
+}


Should this function live in common instead?

It is only used in volume now.

TomSweeneyRedHat · 2019-02-22T21:34:17Z

Couple of nits, otherwise LGTM.

mheon · 2019-02-22T21:34:49Z

Oh dear.

We're going to need to add a state to volumes to store whether the volume is mounted (and potentially the mountpoint, for volume types without a consistent one?). That means we need to add locks back in, plus some work in the DB to add update/save functions.

Also, I really hate just storing all of this in Options... there should be an easy way of saying that a volume is sourced from NFS.

rhatdan · 2019-02-22T21:36:30Z

Why do we need to record this? The system nows. I don't think this should be recorded, if a container starts on a volume that is not mounted it will mount it.

mheon · 2019-02-22T21:41:11Z

Post-system restart? After every container using a volume has been removed?

I think we need to be able to tell if a volume is mounted. For now, I'm OK with leaving this as a mount.Mounted() check, but in the future we should be storing this in the DB so we know what the state of the system is.

rhatdan · 2019-02-22T21:45:26Z

Then you are likely to be wrong. Since a user could go in an umount the volume.

I am actually thinking about adding a podman volume mount and umount command.
But currently I think Docker just leaves these mounted until the system reboots.

rhatdan · 2019-02-23T10:22:55Z

@mheon @giuseppe @baude @vrothberg PTAL

giuseppe · 2019-02-24T08:51:43Z

LGTM

mheon · 2019-02-24T14:31:00Z

Hold this until after we cut 1.1, I don't want to land more volume work in stable before we rework more of the internals.

…

On Sun, Feb 24, 2019, 03:51 Giuseppe Scrivano ***@***.***> wrote: LGTM — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2415 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHYHCNfAet6-ic6lOOdSTphgHaVbfaAmks5vQlKggaJpZM4bKkXn> .

rhatdan · 2019-02-25T16:19:47Z

I disagee on this. This is an open bug and don't believe this needs to affect the libpod database.

mheon · 2019-02-25T16:22:24Z

At the very least I want to see a type field in the volume so we can easily determine whether this is a directory-backed volume or NFS-backed - just stuffing this in opts is not a proper solution

rhatdan · 2019-02-25T16:41:59Z

This behaviour matches Docker. These are considered local volumes. And I agree this is not the best solution, but I don't know how we are supposed to fix this unless, we have something hacking in the volumes code that looks for this type of signature and then refers to it as an local-nfs mount or something.

If we want to keep the documented behavior of Docker, then we don't want to force the users to execute different CLI.

mheon · 2019-02-25T16:44:32Z

We can retain the user interface for volume create, but parse the options before we store them and set a field in the DB telling us what the storage backend is

rh-atomic-bot · 2019-02-26T18:22:08Z

☔ The latest upstream changes (presumably #2446) made this pull request unmergeable. Please resolve the merge conflicts.

rhatdan · 2019-03-12T19:10:35Z

@mheon I would like to get this moving again. Any idea what you would like to see changed?

rhatdan · 2019-03-21T13:59:26Z

@mheon Continuing to rebase this one, waiting on you.

mheon · 2019-03-21T14:03:31Z

Most of what we need will land today or tomorrow.

mheon · 2019-03-21T14:04:20Z

libpod/volume.go

+	}
+	mountOpts := options["o"]
+	device := options["device"]
+	if options["type"] == "nfs" {


This should not be an option. Volume has a "type" field, and that is what we should be using.

Correction, I meant Driver - I think I'd count NFS mounts as a type of volume driver...

Or we should add a type field. Let's not stuff critical information into maps. Make this top-level.

We can add this under the covers, but if we want to follow the Docker CLI, we have to allow users to specify nfs in the manner.

Sure, matching their CLI is fine, but let's not let the CLI determine how we set up internal data structures

mheon · 2019-03-21T14:06:06Z

I am pretty convinced that volume locks need to come back for this. I don't want to have to deal with concurrent mount/unmount operations happening.

rhatdan · 2019-04-04T11:28:14Z

@mheon What do I need to do with this PR Now?

mheon · 2019-04-04T13:56:43Z

Largely waiting on the cleanups/reworks in #2774 to land. Unfortunately that one keeps finding other bugs.

rh-atomic-bot · 2019-04-04T18:59:10Z

☔ The latest upstream changes (presumably #2774) made this pull request unmergeable. Please resolve the merge conflicts.

TomSweeneyRedHat · 2019-04-08T17:07:03Z

@rhatdan this needs to be rebased but it looks like the PR that you were waiting on is the cause of the rebase. Perhaps we can merge this today?

rhatdan · 2019-04-12T12:47:32Z

@mheon Could you take this one over and now do it the way you envision.

mheon · 2019-04-12T13:14:06Z

Sure, I'll take it.

…

On Fri, Apr 12, 2019, 08:47 Daniel J Walsh ***@***.***> wrote: @mheon <https://github.com/mheon> Could you take this one over and now do it the way you envision. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2415 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHYHCAzprnMwhKjpJd7hQYrRsBz6VezWks5vgIBmgaJpZM4bKkXn> .

Allow users to create volumes with nfs shares. These volumes will get mounted when the first container gets started and then will remain mounted until the system reboots or the volume is removed. Signed-off-by: Daniel J Walsh <[email protected]>

rhatdan · 2019-06-08T10:42:15Z

@mheon Any update on this?

rhatdan · 2019-07-21T08:52:23Z

@mheon Any update on this?

rhatdan · 2019-08-05T18:08:47Z

@mheon ping?

mheon · 2019-08-05T18:25:49Z

#3709 is the first step. Once that lands, I can get this in. And then probably volume drivers. And then IPv6.

rh-atomic-bot · 2019-08-19T11:58:03Z

☔ The latest upstream changes (presumably #3709) made this pull request unmergeable. Please resolve the merge conflicts.

mheon · 2019-09-04T19:40:28Z

Superceded by #3931

openshift-ci-robot requested review from giuseppe and TomSweeneyRedHat February 22, 2019 21:22

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

rhatdan force-pushed the nfs branch from 1cfd337 to 09f39e6 Compare February 22, 2019 21:26

TomSweeneyRedHat reviewed Feb 22, 2019

View reviewed changes

rhatdan force-pushed the nfs branch from 09f39e6 to 0df6de4 Compare February 22, 2019 21:33

rhatdan force-pushed the nfs branch from 0df6de4 to a60d63e Compare February 22, 2019 21:35

rhatdan force-pushed the nfs branch from a60d63e to 44395cc Compare February 22, 2019 21:37

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 3, 2019

rhatdan force-pushed the nfs branch from 44395cc to 4c4df11 Compare March 4, 2019 19:14

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 4, 2019

rhatdan force-pushed the nfs branch from 4c4df11 to 88a72de Compare March 21, 2019 13:59

mheon reviewed Mar 21, 2019

View reviewed changes

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

rhatdan force-pushed the nfs branch from 88a72de to c9a5547 Compare April 12, 2019 12:46

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 12, 2019

rhatdan mentioned this pull request May 1, 2019

Named volume not mounted on container start #3046

Closed

jwhonce changed the title ~~Support nfs local volume mounts~~ WIP Support nfs local volume mounts May 6, 2019

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 6, 2019

Support nfs local volume mounts

49ee00a

Allow users to create volumes with nfs shares. These volumes will get mounted when the first container gets started and then will remain mounted until the system reboots or the volume is removed. Signed-off-by: Daniel J Walsh <[email protected]>

rhatdan force-pushed the nfs branch from c9a5547 to 49ee00a Compare May 20, 2019 17:55

mheon closed this Sep 4, 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

WIP Support nfs local volume mounts #2415

WIP Support nfs local volume mounts #2415

Conversation

rhatdan commented Feb 22, 2019

openshift-ci-robot commented Feb 22, 2019

rhatdan commented Feb 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomSweeneyRedHat commented Feb 22, 2019

mheon commented Feb 22, 2019

rhatdan commented Feb 22, 2019

mheon commented Feb 22, 2019

rhatdan commented Feb 22, 2019

rhatdan commented Feb 23, 2019

giuseppe commented Feb 24, 2019

mheon commented Feb 24, 2019 via email

rhatdan commented Feb 25, 2019

mheon commented Feb 25, 2019

rhatdan commented Feb 25, 2019

mheon commented Feb 25, 2019

rh-atomic-bot commented Feb 26, 2019

rhatdan commented Mar 12, 2019

rhatdan commented Mar 21, 2019

mheon commented Mar 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mheon commented Mar 21, 2019

rhatdan commented Apr 4, 2019

mheon commented Apr 4, 2019

rh-atomic-bot commented Apr 4, 2019

TomSweeneyRedHat commented Apr 8, 2019

rhatdan commented Apr 12, 2019

mheon commented Apr 12, 2019 via email

rhatdan commented Jun 8, 2019

rhatdan commented Jul 21, 2019

rhatdan commented Aug 5, 2019

mheon commented Aug 5, 2019

rh-atomic-bot commented Aug 19, 2019

mheon commented Sep 4, 2019