fuse-overlayfs: enable shifting #195

giuseppe · 2018-07-11T08:54:27Z

FUSE overlayfs has builtin support for UID/GID shifting so that we don't need to re-create a new image when using different mappings for the user namespaces.

When the FUSE backend is used, don't create a new image and chown it, but let the FUSE backend do the remapping on the fly.

Starting with an empty /var/lib/containers/storage:

# podman pull fedora
# for i in $(seq 10)
> do
>     bin/podman --storage-opt=overlay.fuse_program=/usr/local/bin/fuse-overlayfs run  \
       --uidmap=0:$i:10000 --gidmap=0:$i:10000 --rm fedora \
        stat -c "owner is: %U" /usr/bin/ls
>     du --si --summarize /var/lib/containers/storage/
>     echo
> done

owner is: root
286M	/var/lib/containers/storage/

owner is: root
286M	/var/lib/containers/storage/

owner is: root
286M	/var/lib/containers/storage/

owner is: root
286M	/var/lib/containers/storage/

owner is: root
286M	/var/lib/containers/storage/

owner is: root
286M	/var/lib/containers/storage/

owner is: root
286M	/var/lib/containers/storage/

owner is: root
286M	/var/lib/containers/storage/

owner is: root
286M	/var/lib/containers/storage/

owner is: root
286M	/var/lib/containers/storage/

/cc @rhatdan @nalind @rhvgoyal @AkihiroSuda

rhatdan · 2018-07-12T21:40:26Z

LGTM
@nalind PTAL

giuseppe · 2018-07-16T14:51:31Z

I'll probably need to rebase this PR on top of #199

giuseppe · 2018-07-16T14:52:10Z

let's merge the other one first, as it addresses some real issues :-)

giuseppe · 2018-07-17T07:46:17Z

rebased ⬆️

rhatdan · 2018-07-17T13:08:57Z

Should supports_shifting be a flag in the storage.conf? Rather then force any mount driver to support shifting?

giuseppe · 2018-07-17T14:39:32Z

I am personally in favor of making it configurable, but I understood @nalind would prefer to keep this simpler for now. @nalind what do you think?

nalind · 2018-07-17T18:19:47Z

I don't think that making assumptions based on whether or not the mount_program option is set is going to work if we need to keep adding variations on how overlay works.

nalind · 2018-07-17T18:29:06Z

store.go

@@ -1033,16 +1038,22 @@ func (s *store) imageTopLayerForMapping(image *Image, ristore ROImageStore, read
 		}
 		rc, err := layerHomeStore.Diff("", layer.ID, &diffOptions)
 		if err != nil {
-			return nil, errors.Wrapf(err, "error reading layer %q to create an ID-mapped version of it")
+			return nil, errors.Wrapf(err, "error reading layer %q to create an ID-mapped version of it", layer.ID)


nalind · 2018-07-17T18:39:01Z

This needs tests.

How does a caller "know" which ID mappings to use when launching a container using a layer with this setting enabled as its rootfs? Previously, they'd consult the information recorded along with the layer, but we appear to be setting that information to indicate that host mappings should be used, while quietly using the global default mappings for the actual mounting.

What happens when we create a new layer that uses the new feature, using a parent layer that didn't, but which used a non-default set of ID mappings?

giuseppe · 2018-07-18T12:03:03Z

How does a caller "know" which ID mappings to use when launching a container using a layer with this setting enabled as its rootfs? Previously, they'd consult the information recorded along with the layer, but we appear to be setting that information to indicate that host mappings should be used, while quietly using the global default mappings for the actual mounting.

I thought I could propagate this information from podman, for example when starting a container that was previously created. I tried to do it, but it doesn't look nice. Where is the best place to store this information? When we mount directly the layer we don't know what mappings were used when it was created

nalind · 2018-07-18T13:33:38Z

We keep track of the mappings that a layer is created for using its UIDMap and GIDMap fields. We use that information when creating layers to set permissions in the layer, possibly reverting mappings inherited from the parent layer as part of the permissions-changing process, and then make it available to higher-level tools so that they can, in turn, pass them on to runtime tools.
CreateContainer() passes mappings that it's asked to use down to PutLayer(), and then keeps a copy of what it ends up using in the container's IDMappingOptions for the convenience of callers that don't want to deal with individual layers.

giuseppe · 2018-07-18T14:15:36Z

should I store that we are using shifting or we should allow only one mode to be used (either shifting enabled or not)?

Also, how should I pass down the information about uid/gid mappings into the driver? We add two more arguments to Get(..)?

giuseppe · 2018-07-18T20:14:00Z

test added ⬆️

giuseppe · 2018-07-25T14:01:06Z

is the new option blocking this PR?

giuseppe · 2018-07-25T20:53:42Z

I'd like to get this moving, should I add an option for enabling shifting or is it fine as the current PR does to assume it is supported by the FUSE implementation?

rhatdan · 2018-07-25T21:07:48Z

I think we should just assume the mount_program does the shifting.

rhatdan · 2018-07-25T21:14:48Z

vagrant/provision.sh

@@ -4,20 +4,6 @@ set -xe
 source /etc/os-release

 case "${ID_LIKE:-${ID:-unknown}}" in
-  debian)


Why are you removing debian testing?

rhatdan · 2018-07-25T21:15:18Z

tests/helpers.bash

@@ -2,7 +2,7 @@

 STORAGE_BINARY=${STORAGE_BINARY:-$(dirname ${BASH_SOURCE})/../containers-storage}
 TESTSDIR=${TESTSDIR:-$(dirname ${BASH_SOURCE})}
-STORAGE_DRIVER=${STORAGE_DRIVER:-vfs}
+STORAGE_DRIVER=${STORAGE_DRIVER:-overlay}


@nalind is this something that will work everywhere?

nalind · 2018-07-25T21:36:50Z

tests/helpers.bash

@@ -2,7 +2,7 @@

 STORAGE_BINARY=${STORAGE_BINARY:-$(dirname ${BASH_SOURCE})/../containers-storage}
 TESTSDIR=${TESTSDIR:-$(dirname ${BASH_SOURCE})}
-STORAGE_DRIVER=${STORAGE_DRIVER:-vfs}
+STORAGE_DRIVER=${STORAGE_DRIVER:-overlay}


I'm not okay with changing this testing default -- it trades running tests with the driver that always has to work for running them with another.

nalind · 2018-07-25T21:37:30Z

tests/idmaps.bats

+	overlay*)
+		;;
+		*)
+	skip "not supported by driver $STORAGE_DRIVER"


Indentation for these two lines is backwards.

nalind · 2018-07-25T21:38:34Z

If the mount program takes care of shifting, then we basically can never not use the option on a given layer once we've set up a layer that needs it, even if the configuration changes to no longer include using the mount program that provides the feature, because the layer essentially becomes "broken". We need to be keep track of this somehow to avoid creating a problem for callers when this happens.

…tion Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

do not create a new image with different uid/gid if the driver has support for shifting. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

giuseppe · 2018-07-26T09:53:03Z

@nalind the layer is stored without any mapping done. The translation happens only at runtime. For example, from a podman container with --uidmap 0:100000:100000:

# touch foobar foobar2
# chmod 998:999 foobar2

and from the host:

# find /var/lib/containers/storage/ -name 'foobar*' -exec stat --format="%n uid=%u gid=%g" \{\} \;
/var/lib/containers/storage/overlay/50119762b2a2366b0fa3ddaa79c7c506799a5e66d961cf53b59ac4bb78552afe/merged/foobar uid=100000 gid=100000
/var/lib/containers/storage/overlay/50119762b2a2366b0fa3ddaa79c7c506799a5e66d961cf53b59ac4bb78552afe/merged/foobar2 uid=100998 gid=100999
/var/lib/containers/storage/overlay/50119762b2a2366b0fa3ddaa79c7c506799a5e66d961cf53b59ac4bb78552afe/diff/foobar2 uid=998 gid=999
/var/lib/containers/storage/overlay/50119762b2a2366b0fa3ddaa79c7c506799a5e66d961cf53b59ac4bb78552afe/diff/foobar uid=0 gid=0

so the shifting has the opposite effect of running with a mapping enabled, are there cases when this is not enough?

nalind · 2018-07-27T14:34:59Z

We're changing what's stored in the Layer structure when we store a layer this way, in that it may no longer agree with the copy we store in the Container structure. That copy was originally added there as a convenience, to save callers from having to look up the mappings by looking up the Layer structure using the ID stored in the Container structure. I don't know how that'll impact current callers.
Leaving that aside, if the configuration is changed so that the mount helper is no longer used, we'll quietly discard the maps stored in the Container structure and mount the layer with no mappings, but if the caller is using the mapping information in the Container structure, things will break, so there needs to be an error check there.

giuseppe · 2018-07-27T15:10:17Z

yes good point, we should error out when we try to mount the same layer/container and not using shifting. I've added a patch for doing it, now I've:

# podman --storage-opt overlay.mount_program=/usr/local/bin/fuse-overlayfs run --uidmap 0:100000:100000 --name test1 fedora echo hello
hello
# podman start -a test1
unable to start container b991456592f83b96b7675b54bdde84d0249686f1eec676261051f39f74b831f8: error mounting storage for container b991456592f83b96b7675b54bdde84d0249686f1eec676261051f39f74b831f8: cannot mount layer 3160cc1b237be611c580ddb2a61076491506caf1b8f9ff0080e5a2dbaa353d1a: shifting not enabled

nalind · 2018-07-27T15:19:03Z

tests/idmaps.bats

+	createrandom "$lowermount"/file
+	storage unmount $layer
+
+        imagename=idmappedimage-shifting


Mixing tabs and spaces makes this diff look weird.

nalind · 2018-07-27T15:19:13Z

LGTM

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

giuseppe · 2018-07-27T18:42:42Z

@rhatdan tests are passing

giuseppe force-pushed the fuse-overlayfs-shifting branch 6 times, most recently from 89ca5f1 to cec74cd Compare July 11, 2018 13:44

giuseppe force-pushed the fuse-overlayfs-shifting branch from cec74cd to 45588a6 Compare July 17, 2018 07:07

nalind reviewed Jul 17, 2018

View reviewed changes

giuseppe force-pushed the fuse-overlayfs-shifting branch from 45588a6 to ce9d2bf Compare July 18, 2018 11:55

giuseppe force-pushed the fuse-overlayfs-shifting branch 6 times, most recently from 3a5c2c8 to 10b55c3 Compare July 18, 2018 20:12

giuseppe force-pushed the fuse-overlayfs-shifting branch 3 times, most recently from 8343967 to 0acf8e9 Compare July 19, 2018 08:13

rhatdan reviewed Jul 25, 2018

View reviewed changes

nalind reviewed Jul 25, 2018

View reviewed changes

giuseppe added 8 commits July 26, 2018 06:10

storage.conf: move ostree_repo and skip_mount_home to the correct sec…

9f0144d

…tion Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

overlay: use full paths to the mount program

8c814e0

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

overlay: enable support for shifting when FUSE is used

6480245

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

drivers: add new method SupportsShifting() to LayerIDMapUpdater

9bb1031

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

store: include layerID in the error message

1e60d8a

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

store: use the original image if the driver supports shifting

883ee15

do not create a new image with different uid/gid if the driver has support for shifting. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

Makefile: install ffjson

140e0b6

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

drivers: inform Mount of the mappings used by the container

1897396

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

giuseppe force-pushed the fuse-overlayfs-shifting branch from 0acf8e9 to 79d12e0 Compare July 26, 2018 04:16

nalind reviewed Jul 27, 2018

View reviewed changes

tests: add test for shifting support

06cea37

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

giuseppe force-pushed the fuse-overlayfs-shifting branch 2 times, most recently from 1b4c904 to 4ff0bde Compare July 27, 2018 15:26

shifting: raise an error if the container needs shifting

3f55e5a

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

giuseppe force-pushed the fuse-overlayfs-shifting branch from 4ff0bde to 3f55e5a Compare July 27, 2018 17:59

giuseppe mentioned this pull request Jul 27, 2018

roadmap containers/fuse-overlayfs#4

Open

rhatdan merged commit 956a197 into containers:master Jul 27, 2018

runcom mentioned this pull request Nov 9, 2018

devmapper regression #233

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fuse-overlayfs: enable shifting #195

fuse-overlayfs: enable shifting #195

giuseppe commented Jul 11, 2018

rhatdan commented Jul 12, 2018

giuseppe commented Jul 16, 2018

giuseppe commented Jul 16, 2018

giuseppe commented Jul 17, 2018

rhatdan commented Jul 17, 2018

giuseppe commented Jul 17, 2018

nalind commented Jul 17, 2018

nalind Jul 17, 2018

nalind commented Jul 17, 2018

giuseppe commented Jul 18, 2018

nalind commented Jul 18, 2018

giuseppe commented Jul 18, 2018 •

edited

Loading

giuseppe commented Jul 18, 2018

giuseppe commented Jul 25, 2018

giuseppe commented Jul 25, 2018

rhatdan commented Jul 25, 2018

rhatdan Jul 25, 2018

rhatdan Jul 25, 2018

nalind Jul 25, 2018

nalind Jul 25, 2018

nalind commented Jul 25, 2018

giuseppe commented Jul 26, 2018

nalind commented Jul 27, 2018

giuseppe commented Jul 27, 2018

nalind Jul 27, 2018

nalind commented Jul 27, 2018

giuseppe commented Jul 27, 2018

fuse-overlayfs: enable shifting #195

fuse-overlayfs: enable shifting #195

Conversation

giuseppe commented Jul 11, 2018

rhatdan commented Jul 12, 2018

giuseppe commented Jul 16, 2018

giuseppe commented Jul 16, 2018

giuseppe commented Jul 17, 2018

rhatdan commented Jul 17, 2018

giuseppe commented Jul 17, 2018

nalind commented Jul 17, 2018

nalind Jul 17, 2018

Choose a reason for hiding this comment

nalind commented Jul 17, 2018

giuseppe commented Jul 18, 2018

nalind commented Jul 18, 2018

giuseppe commented Jul 18, 2018 • edited Loading

giuseppe commented Jul 18, 2018

giuseppe commented Jul 25, 2018

giuseppe commented Jul 25, 2018

rhatdan commented Jul 25, 2018

rhatdan Jul 25, 2018

Choose a reason for hiding this comment

rhatdan Jul 25, 2018

Choose a reason for hiding this comment

nalind Jul 25, 2018

Choose a reason for hiding this comment

nalind Jul 25, 2018

Choose a reason for hiding this comment

nalind commented Jul 25, 2018

giuseppe commented Jul 26, 2018

nalind commented Jul 27, 2018

giuseppe commented Jul 27, 2018

nalind Jul 27, 2018

Choose a reason for hiding this comment

nalind commented Jul 27, 2018

giuseppe commented Jul 27, 2018

giuseppe commented Jul 18, 2018 •

edited

Loading