Add `rootless_storage_path` directive to storage.conf by QiWang19 · Pull Request #529 · containers/storage

QiWang19 · 2020-02-12T17:17:49Z

This allows rootless admins to setup alternative
paths to content in the homedir.

Rootless users on NFS homedirs will not be allowed to run
podman, if an admin wants to setup alternative directory say
in /var/tmp on local storage, they could configure the storage.conf
file and then all users would automatically get storage in /var/tmp.

Signed-off-by: Daniel J Walsh dwalsh@redhat.com
Signed-off-by: Qi Wang qiwan@redhat.com

rhatdan · 2020-02-13T11:00:06Z

@QiWang19 Is this supposed to replace #518?

We need to have a PR for podman that proves that this works. Or at least experiment with it to make sure it works correctly.

rhatdan · 2020-02-18T19:36:25Z

@QiWang19 this needs a rebase.

rhatdan

LGTM

rhatdan · 2020-02-19T12:37:19Z

@TomSweeneyRedHat @nalind @vrothberg PTAL

TomSweeneyRedHat · 2020-02-19T14:48:49Z

+**rootless_storage_path**="$HOME/.local/share/containers/storage"
+  Storage path for rootless users. By default the graphroot for rootless users
+is set to `$XDG_DATA_HOME/containers/storage`, if XDG_DATA_HOME is set.
+Otherwise the `$HOME/.local/share/containers/storage` is used.  This field can


"Otherwise the" -> "Otherwise"

TomSweeneyRedHat · 2020-02-19T14:50:55Z

+    * `$USER` => Replaced by the users name
+
+  A common use case for this field is `NFS home directories`, which do not work
+with rootless container storage.


"do not work" -> "does not work"
or do we want to soften it a bit with something like "which is not currently supported with rootless container storage"?

TomSweeneyRedHat · 2020-02-19T14:54:28Z

+**rootless_storage_path**="$HOME/.local/share/containers/storage"
+  Storage path for rootless users. By default the graphroot for rootless users
+is set to `$XDG_DATA_HOME/containers/storage`, if XDG_DATA_HOME is set.
+Otherwise the `$HOME/.local/share/containers/storage` is used.  This field can


https://github.com/containers/storage/pull/529/files#diff-81a17eae8a51582de5ca93d298bf0162R18 says it uses "$HOME/.config/containers/storage". Is $HOME/.local/share/containers/storage used?

Fixed. The path is not used, the default is set to "".

TomSweeneyRedHat · 2020-02-19T18:39:35Z


+# Storage path for rootless users
+#
+# rootless_storage_path = "$HOME/.config/containers/storage"


Maybe I've over code-reviewed today, but the man page seems to say this should be $HOME/.local/share/containers/storage?

Are they talking about no need to specify rootless_storage_path = "$HOME/.config/containers/storage" in storage.conf? I need ask @rhatdan

Yes Tom is correct this should be something like

# rootless_storage_path = "/var/tmp/$USER/containers/storage"

/var/tmp? not $HOME/.config/containers/storage? also change thoses man pages to /var/tmp/$USER/containers/storage?

Let's talk today. If we are showing the default then it should be
$HOME/.local/share/containers/storage

TomSweeneyRedHat

LGTM

edsantiago

Sorry; I hadn't seen this earlier.

edsantiago · 2020-02-24T17:51:34Z

 	// GraphRoot is the filesystem path under which we will store the
 	// contents of layers, images, and containers.
 	GraphRoot string `json:"root,omitempty"`
+	// RooltessStoragePath is the storage path for rootless users


edsantiago · 2020-02-24T17:55:14Z

 			if storageOpts.GraphRoot == "" {
 				storageOpts.GraphRoot = defaultRootlessGraphRoot
+			} else if storageOpts.RootlessStoragePath != "" {
+				rootlessStoragePath := strings.Replace(storageOpts.RootlessStoragePath, "$HOME", homedir.Get(), -1)


Oh, ouch. This sort of string replacement isn't as simple as that: for instance, what if someone writes $HOMEDIR or $USERNAME? What you probably want here is something closer to:

look for dollar sign followed by any number of letters if those letters are HOME, UID, or USER, replace as needed otherwise, throw an error

edsantiago · 2020-02-24T17:57:42Z

  container storage graph dir (default: "/var/lib/containers/storage")
  Default directory to store all writable content created by container storage programs.

+**rootless_storage_path**="$HOME/.local/share/containers/storage"


What is the RHS of the = supposed to be? In all other options it's simply an empty string "". Why is this one different? Especially why is it misleadingly different? The = here implies that it's a default, but right below it says otherwise.

the value of rootless_storage_path will not be used unless the user overwrites it in storage.conf. I think it can be described as "". But I'n not sure. I need ask @rhatdan

edsantiago · 2020-02-24T17:59:24Z

+    * `$UID`  => Replaced by the users UID
+    * `$USER` => Replaced by the users name
+
+  A common use case for this field is `NFS home directories`, which is not currently supported with rootless container storage.


This is confusing. Perhaps something more like "A common use case for this field is to provide a local storage directory when user home directories are NFS-mounted (podman does not support container storage over NFS)."

This allows rootless admins to setup alternative paths to content in the homedir. Rootless users on NFS homedirs will not be allowed to run podman, if an admin wants to setup alternative directory say in /var/tmp on local storage, they could configure the storage.conf file and then all users would automatically get storage in /var/tmp. Signed-off-by: Daniel J Walsh <dwalsh@redhat.com> Signed-off-by: Qi Wang <qiwan@redhat.com>

edsantiago · 2020-02-25T19:29:43Z

LGTM. Thank you for addressing my concerns.

QiWang19 force-pushed the rootless-storage-path branch from df33f73 to 1c1be21 Compare February 13, 2020 17:03

QiWang19 mentioned this pull request Feb 13, 2020

test rootless_storage_path from storage.conf containers/podman#5202

Merged

rhatdan mentioned this pull request Feb 17, 2020

[WIP] Add rootless_storage_path directive to storage.conf #518

Closed

QiWang19 force-pushed the rootless-storage-path branch from 1c1be21 to 82e34e8 Compare February 18, 2020 20:14

rhatdan approved these changes Feb 19, 2020

View reviewed changes

TomSweeneyRedHat reviewed Feb 19, 2020

View reviewed changes

QiWang19 force-pushed the rootless-storage-path branch from 82e34e8 to a4f0b3a Compare February 19, 2020 18:18

TomSweeneyRedHat reviewed Feb 19, 2020

View reviewed changes

rhatdan mentioned this pull request Feb 20, 2020

Add storage options to make it easy to setup storage for rootless. containers/common#30

Closed

QiWang19 force-pushed the rootless-storage-path branch from a4f0b3a to 843211d Compare February 20, 2020 20:27

QiWang19 force-pushed the rootless-storage-path branch from 843211d to 4d559c6 Compare February 20, 2020 20:33

TomSweeneyRedHat reviewed Feb 20, 2020

View reviewed changes

edsantiago requested changes Feb 24, 2020

View reviewed changes

QiWang19 force-pushed the rootless-storage-path branch from 4d559c6 to e4f65e7 Compare February 24, 2020 19:42

edsantiago approved these changes Feb 25, 2020

View reviewed changes

rhatdan merged commit 67c7e82 into containers:master Feb 26, 2020

brianv0 mentioned this pull request Feb 26, 2020

Explore use of Namespaced File Capabilities to support rootless on NFS/GPFS #539

Closed

Conversation

QiWang19 commented Feb 12, 2020

Uh oh!

rhatdan commented Feb 13, 2020

Uh oh!

rhatdan commented Feb 18, 2020

Uh oh!

rhatdan left a comment

Choose a reason for hiding this comment

Uh oh!

rhatdan commented Feb 19, 2020

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 Feb 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

TomSweeneyRedHat left a comment

Choose a reason for hiding this comment

Uh oh!

edsantiago 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!

edsantiago commented Feb 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rhatdan Feb 19, 2020 •

edited

Loading