LCOW: Prepare work for bind mounts by simonferquel · Pull Request #34258 · moby/moby

simonferquel · 2017-07-26T11:07:24Z

This PR modifies mounts validation logic on Windows to accept both
windows container style and linux style destination path (allowing to
run things like docker run -v c:\data:/data alpine ls /data on lcow)

- What I did

Changed a bit the validation of bind mounts specification to accept both windows and linux style destination paths on windows

- How I did it

Changed a bit some regular expressions, tried to be clever about when calling convertFromSlash

- How to verify it

Unit tests are updated accordingly

- Note

Current LCOW implementation does not seem to be able to do bind-mounts for now though. Here is the result of docker run --rm -it -v c:\users:/data busybox:

docker: Error response from daemon: container c2abff68cdfa6dcf97357154d012e37bd874fccb4ccd1517f74bbc51b3905c0b encountered an error during CreateProcess: failure in a Windows system call: Unspecified error (0x80004005) extra info: {"CommandArgs":["sh"],"WorkingDirectory":"/","Environment":{"HOSTNAME":"c2abff68cdfa","PATH":"/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin","TERM":"xterm"},"EmulateConsole":true,"CreateStdInPipe":true,"CreateStdOutPipe":true,"ConsoleSize":[30,120],"OCISpecification":{"ociVersion":"1.0.0-rc5-dev","platform":{"os":"linux","arch":"amd64"},"process":{"terminal":true,"consoleSize":{"height":30,"width":120},"user":{"uid":0,"gid":0},"args":["sh"],"env":["PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin","HOSTNAME=c2abff68cdfa","TERM=xterm"],"cwd":"/","capabilities":{"bounding":["CAP_CHOWN","CAP_DAC_OVERRIDE","CAP_FSETID","CAP_FOWNER","CAP_MKNOD","CAP_NET_RAW","CAP_SETGID","CAP_SETUID","CAP_SETFCAP","CAP_SETPCAP","CAP_NET_BIND_SERVICE","CAP_SYS_CHROOT","CAP_KILL","CAP_AUDIT_WRITE"],"effective":["CAP_CHOWN","CAP_DAC_OVERRIDE","CAP_FSETID","CAP_FOWNER","CAP_MKNOD","CAP_NET_RAW","CAP_SETGID","CAP_SETUID","CAP_SETFCAP","CAP_SETPCAP","CAP_NET_BIND_SERVICE","CAP_SYS_CHROOT","CAP_KILL","CAP_AUDIT_WRITE"],"inheritable":["CAP_CHOWN","CAP_DAC_OVERRIDE","CAP_FSETID","CAP_FOWNER","CAP_MKNOD","CAP_NET_RAW","CAP_SETGID","CAP_SETUID","CAP_SETFCAP","CAP_SETPCAP","CAP_NET_BIND_SERVICE","CAP_SYS_CHROOT","CAP_KILL","CAP_AUDIT_WRITE"],"permitted":["CAP_CHOWN","CAP_DAC_OVERRIDE","CAP_FSETID","CAP_FOWNER","CAP_MKNOD","CAP_NET_RAW","CAP_SETGID","CAP_SETUID","CAP_SETFCAP","CAP_SETPCAP","CAP_NET_BIND_SERVICE","CAP_SYS_CHROOT","CAP_KILL","CAP_AUDIT_WRITE"]}},"root":{"path":"rootfs"},"hostname":"c2abff68cdfa","mounts":[{"destination":"/proc","type":"proc","source":"proc","options":["nosuid","noexec","nodev"]},{"destination":"/dev","type":"tmpfs","source":"tmpfs","options":["nosuid","strictatime","mode=755"]},{"destination":"/dev/pts","type":"devpts","source":"devpts","options":["nosuid","noexec","newinstance","ptmxmode=0666","mode=0620","gid=5"]},{"destination":"/sys","type":"sysfs","source":"sysfs","options":["nosuid","noexec","nodev","ro"]},{"destination":"/sys/fs/cgroup","type":"cgroup","source":"cgroup","options":["ro","nosuid","noexec","nodev"]},{"destination":"/dev/mqueue","type":"mqueue","source":"mqueue","options":["nosuid","noexec","nodev"]},{"destination":"/data","source":"c:\\users"}],"linux":{"resources":{"devices":[{"allow":false,"access":"rwm"},{"allow":true,"type":"c","major":1,"minor":5,"access":"rwm"},{"allow":true,"type":"c","major":1,"minor":3,"access":"rwm"},{"allow":true,"type":"c","major":1,"minor":9,"access":"rwm"},{"allow":true,"type":"c","major":1,"minor":8,"access":"rwm"},{"allow":true,"type":"c","major":5,"minor":0,"access":"rwm"},{"allow":true,"type":"c","major":5,"minor":1,"access":"rwm"},{"allow":false,"type":"c","major":10,"minor":229,"access":"rwm"}]},"namespaces":[{"type":"mount"},{"type":"network"},{"type":"uts"},{"type":"pid"},{"type":"ipc"}],"maskedPaths":["/proc/kcore","/proc/latency_stats","/proc/timer_list","/proc/timer_stats","/proc/sched_debug"],"readonlyPaths":["/proc/asound","/proc/bus","/proc/fs","/proc/irq","/proc/sys","/proc/sysrq-trigger"]}}}.

simonferquel · 2017-07-31T07:35:54Z

Hmm this breaks many dockerfiles with volumes. I suppose we should have a different parsing mechanism for volume paths in dockerfiles (which by nature should only contain a target path and should be validated elsewise)

jstarks · 2017-07-31T21:11:07Z

@jhowardmsft PTAL

jstarks · 2017-07-31T21:12:37Z

volume/volume_windows.go

It seems to me that this validation should be context sensitive. I.e. we should only perform Windows validation when handling mounts for Windows containers, and we should only perform Linux validation when handling mounts for Linux containers.

In the latter case, it would be nice to reuse the existing Linux validation code rather than rewrite it here, although of course that means the shared Linux code would need to use path instead of filepath.

Yes, after having seen CI test failures, I agree that we must do different parsing / validation logic to handle linux containers and windows containers.
However, we can´t leverage existing linux parsing/validation completely, as the source path is still windows style, and the linux parsing code is based on the assumption that : is a forbidden character in a file path...

So, I think that after all we need a far more invasive change, and we could take the occasion to refactor a bit the volume package.

I'd like to introduce a volume.Parser interface that would expose methods to parse/validate mount specs, with a constructor taking a platform string. We would have 3 implementaions (linuxParser, windowsParser, lcowParser) that would certainly share some amount of code, instead of relying on conditional compilation and current tricks (for exemple the convertSlashes doing the exact oposite on windows and linux is SO confusing on code maintenance mode...)

simonferquel · 2017-08-01T17:37:04Z

@johnstep, @jhowardmsft, I just completely changed the approach, with a complete volume refactoring (that will require some refinements), where parsing/validation logic depends on the containerPlatform and the host OS. I also removed static compilation switches, to have an easier code reading experience (there were some dirty tricks behind compilation switches), and make it easier to share more code in the future.

simonferquel · 2017-08-03T09:18:15Z

Ok, latest CI failures where essentially stuff related to existing bugs (on certain cases, some checks were not done the same way if ParseMountSpec or ParseMountRaw were called...). I think we can discuss during the design review if we should reproduce the little bugs for the sake of behavior stability or if we want to have them fixed, at the risk of breaking compatibility

PatrickLang · 2017-08-08T20:59:51Z

I talked to @adburch and simple paths such as /data should be working in the recent insider releases such as 16257. What Windows build are you testing on?

Paths containing non-Windows allowed characters and / path separators such as /opt/usr/share/foo will need build 16263+

simonferquel · 2017-08-09T09:43:27Z

I am running 10.0.16257.1

Microsoft Windows [Version 10.0.16257.1]
(c) 2017 Microsoft Corporation. All rights reserved.

C:\Users\simon>docker run --rm -v c:\:/data busybox ls /
docker: Error response from daemon: container dcaa82082ece5fb245eee0745d5fcd02ccec0733052e88735f7d9c8f4072d5dd encountered an error during CreateProcess: failure in a Windows system call: Unspecified error (0x80004005) extra info: {"CommandArgs":["ls","/"],"WorkingDirectory":"/","Environment":{"HOSTNAME":"dcaa82082ece","PATH":"/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"},"CreateStdInPipe":true,"CreateStdOutPipe":true,"CreateStdErrPipe":true,"ConsoleSize":[0,0],"OCISpecification":{"ociVersion":"1.0.0-rc5-dev","platform":{"os":"linux","arch":"amd64"},"process":{"consoleSize":{"height":0,"width":0},"user":{"uid":0,"gid":0},"args":["ls","/"],"env":["PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin","HOSTNAME=dcaa82082ece"],"cwd":"/","capabilities":{"bounding":["CAP_CHOWN","CAP_DAC_OVERRIDE","CAP_FSETID","CAP_FOWNER","CAP_MKNOD","CAP_NET_RAW","CAP_SETGID","CAP_SETUID","CAP_SETFCAP","CAP_SETPCAP","CAP_NET_BIND_SERVICE","CAP_SYS_CHROOT","CAP_KILL","CAP_AUDIT_WRITE"],"effective":["CAP_CHOWN","CAP_DAC_OVERRIDE","CAP_FSETID","CAP_FOWNER","CAP_MKNOD","CAP_NET_RAW","CAP_SETGID","CAP_SETUID","CAP_SETFCAP","CAP_SETPCAP","CAP_NET_BIND_SERVICE","CAP_SYS_CHROOT","CAP_KILL","CAP_AUDIT_WRITE"],"inheritable":["CAP_CHOWN","CAP_DAC_OVERRIDE","CAP_FSETID","CAP_FOWNER","CAP_MKNOD","CAP_NET_RAW","CAP_SETGID","CAP_SETUID","CAP_SETFCAP","CAP_SETPCAP","CAP_NET_BIND_SERVICE","CAP_SYS_CHROOT","CAP_KILL","CAP_AUDIT_WRITE"],"permitted":["CAP_CHOWN","CAP_DAC_OVERRIDE","CAP_FSETID","CAP_FOWNER","CAP_MKNOD","CAP_NET_RAW","CAP_SETGID","CAP_SETUID","CAP_SETFCAP","CAP_SETPCAP","CAP_NET_BIND_SERVICE","CAP_SYS_CHROOT","CAP_KILL","CAP_AUDIT_WRITE"]}},"root":{"path":"rootfs"},"hostname":"dcaa82082ece","mounts":[{"destination":"/proc","type":"proc","source":"proc","options":["nosuid","noexec","nodev"]},{"destination":"/dev","type":"tmpfs","source":"tmpfs","options":["nosuid","strictatime","mode=755"]},{"destination":"/dev/pts","type":"devpts","source":"devpts","options":["nosuid","noexec","newinstance","ptmxmode=0666","mode=0620","gid=5"]},{"destination":"/sys","type":"sysfs","source":"sysfs","options":["nosuid","noexec","nodev","ro"]},{"destination":"/sys/fs/cgroup","type":"cgroup","source":"cgroup","options":["ro","nosuid","noexec","nodev"]},{"destination":"/dev/mqueue","type":"mqueue","source":"mqueue","options":["nosuid","noexec","nodev"]},{"destination":"/data","source":"c:\\"}],"linux":{"resources":{"devices":[{"allow":false,"access":"rwm"},{"allow":true,"type":"c","major":1,"minor":5,"access":"rwm"},{"allow":true,"type":"c","major":1,"minor":3,"access":"rwm"},{"allow":true,"type":"c","major":1,"minor":9,"access":"rwm"},{"allow":true,"type":"c","major":1,"minor":8,"access":"rwm"},{"allow":true,"type":"c","major":5,"minor":0,"access":"rwm"},{"allow":true,"type":"c","major":5,"minor":1,"access":"rwm"},{"allow":false,"type":"c","major":10,"minor":229,"access":"rwm"}]},"namespaces":[{"type":"mount"},{"type":"network"},{"type":"uts"},{"type":"pid"},{"type":"ipc"}],"maskedPaths":["/proc/kcore","/proc/latency_stats","/proc/timer_list","/proc/timer_stats","/proc/sched_debug"],"readonlyPaths":["/proc/asound","/proc/bus","/proc/fs","/proc/irq","/proc/sys","/proc/sysrq-trigger"]}}}.

C:\Users\simon>

dnephin · 2017-08-10T15:07:28Z

runconfig/config.go

delete commented out code and the above comment?

dnephin · 2017-08-10T15:18:15Z

volume/parser.go

I believe the return value here could be just error, it seems that false is only returned with error, so it doesn't need to have both values. It could be just ValidateVolumeName(name string) error.

I realize this was previously implemented this way, but probably doesn't hurt to fix it now that this is being refactored?

Yes I agree, I'll just have to check on error type instead of the combination of both bool and error (which is confusing)

dnephin · 2017-08-10T15:19:00Z

volume/parser.linux.go

This surprised me. There are definitely restrictions for the volume name on linux (must be at least 3 characters and start with a letter, I believe).

Is that validation happening somewhere else? Maybe we can fix that here as well?

I think I followed the codepath correctly on this one. I'll double check (the code paths were very complicated before)

dnephin · 2017-08-10T15:23:12Z

volume/parser.go

I was really surprised to find such a large interface, but after looking over the rest of the PR I see that this logic really is scattered all over so there's no easy way to reduce this interface. Hopefully over time we can refactor the callers and reduce the size of this interface.

Yes I agree. I started with 2 methods, and I progressively had to add a lot of stuff in it...

dnephin · 2017-08-10T15:24:34Z

volume/parser.lcow.go

Normally I believe underscores are used in go filenames instead of .. I'm guessing this was done because parser_linux.go would imply a linux build tag (which we don't want). Maybe parser_lcow.go and parser_linux_volume.go ?

Yes, I used dots to avoid automatic conditional compilation

dnephin · 2017-08-10T15:25:40Z

Also, I really like this approach!

Design LGTM

dnephin

couple other small comments/question on implementation

dnephin · 2017-08-16T15:03:50Z

container/container.go

Do you think it would make sense to move this default into NewParser() ?

I guess a lot of code is calling NewParser(runtime.GOOS), but it could call NewParser("") instead and handle the default case there.

NewParser() would check if the passed parameter is the empty string, and default to runtime.GOOS.

I initially forced the parameter passing to identify where the volume package was called and to try to identify where there could be problems (especially code validating mounts config where we don't yet have access to the image target OS).

dnephin · 2017-08-16T15:07:23Z

daemon/oci_linux.go

nit: move this down to line 511 so it's next to the code that uses it, instead of stranded up here near unrelated code

dnephin · 2017-08-16T15:08:56Z

runconfig/config.go

Outside of the context of this PR this comment isn't going to make any sense to the reader. I think it should be removed.

stevvooe · 2017-08-17T20:55:23Z

volume/parser.go

These are operating systems. A platform carries architecture, as well.

We'll have to be careful about how we export this model.

cc @jhowardmsft @estesp

Agreed. I suppose volume parsing logic will only be os-specific and architectures should not be taken into account. I'll rename those accordingly.

lowenna · 2017-09-07T14:19:40Z

Unfortunately for various legal complications I can't. However @johnstep now has one.

lowenna · 2017-09-07T21:06:55Z

@simonferquel

Can you squash your commits, then cherry-pick microsoft@b9abc83 from branch https://github.com/Microsoft/docker/tree/lcow-mounts.

This also depends on a revendor of HCSShim in #34771 (status 4/merge currently).

One word of caution when testing - due to a bug in opengcs (being tracked by microsoft/opengcs#131), the contents of any mapped directory will be deleted on the host on container exit currently.

Example of bind-mounting a read-only directory:

PS E:\go\src\github.com\docker\docker> docker run --rm -it -v c:\john:/target:ro busybox sh
/ # ls -l /target
total 0
-r-xr-xr-x    2 root     root            10 Sep  7 19:30 foo.bar
/ # rm /target/foo.bar
rm: remove '/target/foo.bar'? y
rm: can't remove '/target/foo.bar': Read-only file system
/ #

simonferquel · 2017-09-08T09:13:00Z

@jhowardmsft done. Effectively the final rm -rf stuff is a bit brutal

tiborvass · 2017-09-12T23:47:13Z

LGTM

lowenna · 2017-09-13T03:21:49Z

@simonferquel A few minor nits, but generally LGTM. And works :)

lowenna · 2017-09-11T17:08:27Z

container/container.go

I think we should (I have made a start in another PR) use "operatingSystem" or "os" rather than platform to indicate in the code base that it's not platform in the fuller sense os/arch/variant. Certainly not plat though.

lowenna · 2017-09-11T17:11:41Z

volume/parser_lcow_impl.go

Don't use lcow in an error message. Linux containers on Windows do not...

lowenna · 2017-09-11T17:12:41Z

volume/parser_lcow_impl.go

The file naming here seems very strange. I don't think we have a precedent for this type of naming anywhere else.

The thing is that for future-proof multi-arch/os system, this PR makes all flavors of the parser available on all OS. I also wanted file ordering make different parsers appear together (so they all have a parser_ prefix. However, if I chose to keep names as parser_linux.go / parser_windows.go it would have triggered conditional compilation, so I had to add a new suffix for avoiding it to happen.

Another workaround is - instead of _ but that is inconsistent with most of the file names in the project. Yet another is to modify the platform name so it will not match exactly, but I can't come up with a satisfactory modification.

If not for the file ordering issue, which seems minor to me, I would name them lcow_parser.go, linux_parser.go, and windows_parser.go, since that is closer to the respective type names.

@johnstep @jhowardmsft just did that. With the small number of files in this package, I suppose it is ok to have them not grouped together.

simonferquel · 2017-09-13T08:37:48Z

@jhowardmsft, I fixed the items in your comments, thanks!

lowenna · 2017-09-13T15:22:42Z

LGTM (can you squash/tidy the commits again please)

simonferquel · 2017-09-13T15:26:40Z

@jhowardmsft, just rebased and squashed

simonferquel · 2017-09-14T07:45:08Z

Errors seem related to the manifest list stuff... should I rebase once again ?

yongtang · 2017-09-14T17:15:46Z

@simonferquel If possible can you do a rebase against the master? I think the Jenkins errors has been fixed in #34837. Once the PR is rebased it should be ready to merge.

lowenna · 2017-09-14T17:56:46Z

I restarted PPC/Z. They should now pick up and apply this change on top of master.

Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>

vieux · 2017-09-14T19:34:48Z

@jhowardmsft sorry john, I didn't see your comment, I just pushed a rebase against master

GordonTheTurtle added the status/0-triage label Jul 26, 2017

simonferquel force-pushed the lcow-mounts branch 2 times, most recently from 8800647 to dad6c6d Compare July 28, 2017 08:44

jstarks reviewed Jul 31, 2017

View reviewed changes

vdemeester added status/1-design-review and removed status/0-triage labels Aug 1, 2017

vdemeester requested review from cpuguy83, johnstep and lowenna August 1, 2017 09:17

simonferquel force-pushed the lcow-mounts branch from dad6c6d to c749525 Compare August 1, 2017 17:32

simonferquel force-pushed the lcow-mounts branch 4 times, most recently from e8af7c3 to c6d2cf9 Compare August 2, 2017 13:00

PatrickLang mentioned this pull request Aug 2, 2017

Epic: Linux Containers on Windows (LCOW) #33850

Closed

simonferquel force-pushed the lcow-mounts branch from 6abd361 to 5c68181 Compare August 9, 2017 11:53

GordonTheTurtle assigned dnephin Aug 10, 2017

dnephin reviewed Aug 10, 2017

View reviewed changes

dnephin reviewed Aug 16, 2017

View reviewed changes

stevvooe reviewed Aug 17, 2017

View reviewed changes

simonferquel force-pushed the lcow-mounts branch 3 times, most recently from 7f50dae to 039450c Compare August 29, 2017 07:11

simonferquel force-pushed the lcow-mounts branch from 039450c to 81a8e70 Compare September 8, 2017 08:26

lowenna added status/2-code-review and removed status/1-design-review labels Sep 8, 2017

lowenna suggested changes Sep 13, 2017

View reviewed changes

simonferquel force-pushed the lcow-mounts branch from 360a228 to 26fd079 Compare September 13, 2017 12:02

lowenna approved these changes Sep 13, 2017

View reviewed changes

simonferquel force-pushed the lcow-mounts branch from 26fd079 to 266fea1 Compare September 13, 2017 15:26

lowenna added status/4-merge and removed status/2-code-review labels Sep 14, 2017

Volume refactoring for LCOW

e89b6e8

Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>

vieux force-pushed the lcow-mounts branch from 266fea1 to e89b6e8 Compare September 14, 2017 19:33

vieux merged commit 0300fa7 into moby:master Sep 14, 2017

lowenna mentioned this pull request Sep 14, 2017

LCOW: API: Add platform to /images/create and /build #34642

Merged

dsheets mentioned this pull request Sep 18, 2017

Mount Point Plugins Implementation #33375

Closed

thaJeztah mentioned this pull request Dec 19, 2017

Re-validate Mounts on container start #35833

Merged

johnstep mentioned this pull request Jul 16, 2018

Support file mounts on Windows 1803 and above #37474

Closed

thaJeztah added the area/lcow Issues and PR's related to the experimental LCOW feature label Oct 30, 2018

thaJeztah mentioned this pull request Mar 13, 2023

Fix pruning anon volume created from image config #45147

Merged

Conversation

simonferquel commented Jul 26, 2017

Uh oh!

simonferquel commented Jul 31, 2017

Uh oh!

jstarks commented Jul 31, 2017

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!

simonferquel commented Aug 1, 2017

Uh oh!

simonferquel commented Aug 3, 2017

Uh oh!

PatrickLang commented Aug 8, 2017

Uh oh!

simonferquel commented Aug 9, 2017

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!

dnephin commented Aug 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

lowenna commented Sep 7, 2017

Uh oh!

lowenna commented Sep 7, 2017

Uh oh!

simonferquel commented Sep 8, 2017

Uh oh!

tiborvass commented Sep 12, 2017

Uh oh!

lowenna commented Sep 13, 2017

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!

simonferquel commented Sep 13, 2017

dnephin commented Aug 10, 2017 •

edited

Loading