New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LCOW: Prepare work for bind mounts #34258

Merged
merged 1 commit into from Sep 14, 2017

Conversation

@simonferquel
Contributor

simonferquel commented Jul 26, 2017

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

This comment has been minimized.

Show comment
Hide comment
@simonferquel

simonferquel Jul 31, 2017

Contributor

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)

Contributor

simonferquel commented Jul 31, 2017

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

This comment has been minimized.

Show comment
Hide comment
@jstarks

jstarks Jul 31, 2017

Contributor
Contributor

jstarks commented Jul 31, 2017

Show outdated Hide outdated volume/volume_windows.go Outdated
@simonferquel

This comment has been minimized.

Show comment
Hide comment
@simonferquel

simonferquel Aug 1, 2017

Contributor

@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.

Contributor

simonferquel commented Aug 1, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@simonferquel

simonferquel Aug 3, 2017

Contributor

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

Contributor

simonferquel commented Aug 3, 2017

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

This comment has been minimized.

Show comment
Hide comment
@PatrickLang

PatrickLang Aug 8, 2017

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+

PatrickLang commented Aug 8, 2017

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

This comment has been minimized.

Show comment
Hide comment
@simonferquel

simonferquel Aug 9, 2017

Contributor

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>
Contributor

simonferquel commented Aug 9, 2017

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>
Show outdated Hide outdated runconfig/config.go Outdated
Show outdated Hide outdated volume/parser.go Outdated
Show outdated Hide outdated volume/parser.linux.go Outdated
Show outdated Hide outdated volume/parser.go Outdated
Show outdated Hide outdated volume/parser.lcow.go Outdated
@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Aug 10, 2017

Member

Also, I really like this approach!

Design LGTM

Member

dnephin commented Aug 10, 2017

Also, I really like this approach!

Design LGTM

@dnephin

couple other small comments/question on implementation

Show outdated Hide outdated container/container.go Outdated
Show outdated Hide outdated daemon/oci_linux.go Outdated
Show outdated Hide outdated runconfig/config.go Outdated
Show outdated Hide outdated volume/parser.go Outdated
@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft

jhowardmsft Sep 5, 2017

Contributor

There's something wrong in the end-to-end here. Debugged it to the point of where runc is attempting to start the process (container) in the utility VM, and it's passing a Windows path through incorrectly.

image

Command was a simple docker run -it --rm -v c:\john:/john busybox sh

Edit: verified by changing to -v c:\john:/target that it's still the source being passed through (just in case /john was being normalised incorrectly to c:\john)

Contributor

jhowardmsft commented Sep 5, 2017

There's something wrong in the end-to-end here. Debugged it to the point of where runc is attempting to start the process (container) in the utility VM, and it's passing a Windows path through incorrectly.

image

Command was a simple docker run -it --rm -v c:\john:/john busybox sh

Edit: verified by changing to -v c:\john:/target that it's still the source being passed through (just in case /john was being normalised incorrectly to c:\john)

@simonferquel

This comment has been minimized.

Show comment
Hide comment
@simonferquel

simonferquel Sep 6, 2017

Contributor

I don't have a working linuxkit anymore to test. Could you check if the / conversion happen before calling HCS ? (by looking at dockerd logs in debug mode, I think you'll see the json payload sent to hcs)

Contributor

simonferquel commented Sep 6, 2017

I don't have a working linuxkit anymore to test. Could you check if the / conversion happen before calling HCS ? (by looking at dockerd logs in debug mode, I think you'll see the json payload sent to hcs)

@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft

jhowardmsft Sep 7, 2017

Contributor

@simonferquel I have it working!!!! There's some extra code you're going to need. I only have it hacked up at the moment, and a revendor of HCSShim needed. I'll tidy and you can cherry-pick the commits across hopefully tomorrow. In the meantime, can you squash you current commits to make the overall PR clean. Thanks! :)

Contributor

jhowardmsft commented Sep 7, 2017

@simonferquel I have it working!!!! There's some extra code you're going to need. I only have it hacked up at the moment, and a revendor of HCSShim needed. I'll tidy and you can cherry-pick the commits across hopefully tomorrow. In the meantime, can you squash you current commits to make the overall PR clean. Thanks! :)

@simonferquel

This comment has been minimized.

Show comment
Hide comment
@simonferquel

simonferquel Sep 7, 2017

Contributor

@jhowardmsft, is there any chance you could share your working linux image privately? I am currently unable to test this pr end to end...

Contributor

simonferquel commented Sep 7, 2017

@jhowardmsft, is there any chance you could share your working linux image privately? I am currently unable to test this pr end to end...

@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft

jhowardmsft Sep 7, 2017

Contributor

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

Contributor

jhowardmsft commented Sep 7, 2017

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

@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft

jhowardmsft Sep 7, 2017

Contributor

@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
/ #
Contributor

jhowardmsft commented Sep 7, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@simonferquel

simonferquel Sep 8, 2017

Contributor

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

Contributor

simonferquel commented Sep 8, 2017

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

@tiborvass

This comment has been minimized.

Show comment
Hide comment
@tiborvass

tiborvass Sep 12, 2017

Collaborator

LGTM

Collaborator

tiborvass commented Sep 12, 2017

LGTM

@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft

jhowardmsft Sep 13, 2017

Contributor

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

Contributor

jhowardmsft commented Sep 13, 2017

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

Show outdated Hide outdated container/container.go Outdated
Show outdated Hide outdated volume/parser_lcow_impl.go Outdated
Show outdated Hide outdated volume/parser_lcow_impl.go Outdated
@simonferquel

This comment has been minimized.

Show comment
Hide comment
@simonferquel

simonferquel Sep 13, 2017

Contributor

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

Contributor

simonferquel commented Sep 13, 2017

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

@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft

jhowardmsft Sep 13, 2017

Contributor

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

Contributor

jhowardmsft commented Sep 13, 2017

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

@simonferquel

This comment has been minimized.

Show comment
Hide comment
@simonferquel

simonferquel Sep 13, 2017

Contributor

@jhowardmsft, just rebased and squashed

Contributor

simonferquel commented Sep 13, 2017

@jhowardmsft, just rebased and squashed

@simonferquel

This comment has been minimized.

Show comment
Hide comment
@simonferquel

simonferquel Sep 14, 2017

Contributor

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

Contributor

simonferquel commented Sep 14, 2017

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

@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Sep 14, 2017

Member

@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.

Member

yongtang commented Sep 14, 2017

@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.

@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft

jhowardmsft Sep 14, 2017

Contributor

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

Contributor

jhowardmsft commented Sep 14, 2017

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

Volume refactoring for LCOW
Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>
@vieux

This comment has been minimized.

Show comment
Hide comment
@vieux

vieux Sep 14, 2017

Collaborator

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

Collaborator

vieux commented Sep 14, 2017

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

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

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 36848 has succeeded
Details
janky Jenkins build Docker-PRs 45488 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 5890 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 17083 has succeeded
Details
z Jenkins build Docker-PRs-s390x 5671 has succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment