LCOW: Support for docker cp, ADD/COPY on build by gupta-ak · Pull Request #34252 · moby/moby

gupta-ak · 2017-07-25T19:22:00Z

- What I did

Added support for LCOW container file operations likedocker cp and ADD/COPY for docker build

- How I did it

The original graphdriver interface for Get() returned a (string, error). I changed it to return a new interface RootFS defined in pkg/rootfs/rootfs.go. It has the Driver and PathDriver interface from continuity, which abstracts the file operations from the os package behind the interface. The RootFS interface also has container specific methods, such as resolving scoped paths.
I implemented the RootFS interface for non-LCOW and LCOW cases. In the non-LCOW cases, it's just a wrapper around the os package, but for LCOW, docker sends the file operations to a Linux Service VM that handles it.
After that, it was a matter of propagating the interface throughout docker.

- How to verify it

For regressions due to the interface break, I'm hoping the CI will catch them. For LCOW, I'm running @jhowardmsft 's LCOW testing script and also manually verifying that docker cp and docker build works.

- Description for the changelog

Support for docker cp and ADD/COPY in docker build

- A picture of a cute animal (not mandatory but encouraged)

gupta-ak · 2017-07-28T23:29:29Z

Example of docker cp working:

PS C:\Users\Administrator> cat '.\Documents\New folder\Dockerfile'
FROM busybox
ENV "Goldens" "Are the best dogs"
RUN export
#Hello!
PS C:\Users\Administrator> docker create -it busybox
8691c852d3bfc68201db40183d875bed2f3cba58174e30f429ef2811a12ce912
PS C:\Users\Administrator> docker cp '.\Documents\New folder\Dockerfile' 869:/root/
PS C:\Users\Administrator> docker start -ia 869
/ # cat /root/Dockerfile
FROM busybox
ENV "Goldens" "Are the best dogs"
RUN export
#Hello!/ #exit
PS C:\Users\Administrator> docker cp 869:/root/Dockerfile Dockerfile2
PS C:\Users\Administrator> cat .\Dockerfile2
FROM busybox
ENV "Goldens" "Are the best dogs"
RUN export
#Hello
PS C:\Users\Administrator>

gupta-ak · 2017-07-28T23:53:06Z

@stevvooe PTAL as well. Right now, everything is complete aside from the lcow/remotefs implementation, which mostly works, but has some reliability issues.

This PR can be split into multiple PRs (one for interface changes, one for LCOW changse, one for remotefs implementation) so if that is easier to do, I can do that as well. You would need everything to have it work, but it would probably be easier to review.

dnephin · 2017-07-31T15:56:53Z

builder/dockerfile/internals.go

+	return false
+}
+
+func validateCopySourcePath(imageSource *imageMount, origPath, platform string) error {


Please move these two helpers to below the single function where they are called.

dnephin · 2017-07-31T15:57:58Z

builder/dockerfile/internals.go

+
+func validateCopySourcePath(imageSource *imageMount, origPath, platform string) error {
+	// validate windows paths from other images
+	if imageSource == nil || platform != "windows" {


A type and constant for platform and "windows" would be nice

There's quite a few platform string that was already here. I think it's from @jhowardmsft's first LCOW PR. He's working on a platform API for the builder, so do you want the change now or in his PR? @jhowardmsft: Thoughts?

I'm not sure this makes sense at this point*. There's dozens of places outside of LCOW where platform strings are used directly without a constant. It's been a long-standing pattern like this.

E:\go\src\github.com\docker\docker [jjh/buildplatformapi]> (git grep '\"windows\"' *.go | Measure-Object).Count 262

* Sure, no objection to doing this a little further down the road, across the entire code base (ideally at a point where the fast-arriving LCOW PRs aren't going to merge conflict).

dnephin · 2017-07-31T16:00:10Z

builder/dockerfile/copy.go

+		// Work in daemon-specific filepath semantics
+		inst.dest = filepath.FromSlash(args[last])
+		separator = string(os.PathSeparator)
+	}


For the most part the path details seem to be well abstracted, but they leak out in this function.

Instead of this maybe we could have a system.HasDirectorySuffix() and system.FromSlash() or something like that?

system is already a large package, so maybe that's not the right place for it.

I added a platform tag to the copier struct. The platform was already in the dispathRequest.builder, so I just had to extract that into thecopierFromDistpatchRequest. I added a system.FromSlash() and system.Separator() to check the directory suffix.

If the copy instruction dest doesn't need the daemon/platform specific paths here, we could also just do a check like if len(infos) > 1 && !strings.HasSuffix(filepath.ToSlash(inst.dest), "/")

gupta-ak · 2017-08-03T21:53:06Z

Couple of updates.

I squashed all the commits to a single one & vendor commit.
Finished rest of remotefs implementation, so the docker code should be ready. There's some platform reliability issues I'm hitting when testing this that I'm investigating, but the docker code shouldn't change.
Addressed @dnephin's comments

gupta-ak · 2017-08-04T00:40:38Z

Now that #34170 is merged. I rebased to just my commit and the vendor.

lowenna · 2017-08-09T18:44:34Z

daemon/graphdriver/lcow/lcow_svm.go

This should switch over to github.com/Microsoft/opengcs/client now.

lowenna · 2017-08-09T19:34:20Z

daemon/graphdriver/lcow/lcow.go

This is problematic. I hit a nil pointer de-reference panic due to this change at L411. Previously, the code had an explicit nil check here. Repro was docker cp IDOfStoppedContainer:/bin/sh .

I can't repro it, but the logic in startServiceVMIfNotRunning needs some tweaking. See here:

defer func() { // Signal that start has finished, passing in the error if any. svm.signalStartFinished(err) if err == nil { return } // We added a ref to the VM, since we failed, we should delete the ref. d.terminateServiceVM(id, "error path on startServiceVMIfNotRunning", false) }() /* Some functions that can fail. */ // Start it. logrus.Debugf("lcowdriver: startServiceVmIfNotRunning: (%s) starting %s", context, svm.config.Name) if err := svm.config.StartUtilityVM(); err != nil { return nil, fmt.Errorf("failed to start service utility VM (%s): %s", context, err) }

Bascially, if any function fails before the svm.config.StartUtilityVM() part, then svm.config.Uvm will be nil, triggering a panic, so I need to guard L411 with some check.

lowenna · 2017-08-09T19:37:35Z

daemon/graphdriver/lcow/lcow.go

s/Operationg/operation

gupta-ak · 2017-08-09T20:38:34Z

@stevvooe PTAL + add whoever else should take a look

lowenna · 2017-08-09T20:55:52Z

daemon/graphdriver/lcow/lcow_svm.go

Has this comment been moved to the correct place? It used to be in the driver struct for the cache.

johnstep

I only skimmed the rest of the PR, but the LCOW graph driver changes look good!

johnstep · 2017-09-05T04:09:43Z

daemon/graphdriver/lcow/lcow.go

Nit: Why is syscall separate from the previous set of imports?

just seems like a typo

johnstep · 2017-09-12T05:01:38Z

daemon/graphdriver/lcow/remotefs.go

ContainerFS interface

johnstep · 2017-09-12T05:16:17Z

daemon/graphdriver/lcow/remotefs.go

You could move this after locking on line 103, and remove the Unlock on 105.

johnstep · 2017-09-12T05:29:09Z

daemon/graphdriver/lcow/lcow.go

Note: Seems a little odd to recurse here, but it's not a problem.

johnstep · 2017-09-12T06:22:23Z

daemon/graphdriver/lcow/lcow_svm.go

Shouldn't this function remove the mount if this is the last reference?

good catch! should be > 1

johnstep · 2017-09-12T06:24:27Z

daemon/graphdriver/lcow/lcow_svm.go

Nit: Why is io separate from the previous set of imports?

seems like a typo

mlaventure · 2017-09-12T20:00:46Z

I just got aware of this, and it's too big for me to have a proper look in a decent timeframe, but it does look like overkill. I think something is wrong if we have to re-implement the POSIX interface within moby. This would be better served by an actual filesystem driver. Any reason not to go that way? It would also allow Microsoft to change its behavior in sync with their shim without requiring changes within Docker.

gupta-ak · 2017-09-13T19:48:36Z

rebased + updated with @johnstep 's feedback

Signed-off-by: Akash Gupta <akagup@microsoft.com>

This enables docker cp and ADD/COPY docker build support for LCOW. Originally, the graphdriver.Get() interface returned a local path to the container root filesystem. This does not work for LCOW, so the Get() method now returns an interface that LCOW implements to support copying to and from the container. Signed-off-by: Akash Gupta <akagup@microsoft.com>

might not be the cleanest way, but it's definitly the way with the minimum code change. Signed-off-by: Victor Vieux <victorvieux@gmail.com>

johnstep

LGTM (based on code, will test soon)

lowenna · 2017-09-14T22:10:53Z

Graphdriver changes LGTM

thaJeztah · 2017-09-15T11:10:12Z

from #34252 (comment)

Notes from 8/25 call with Li Li, Akash, Derek, Tonis, John S, Friis, Carl

Next steps:
1 - Get 2 LGTMs on LCOW portions (@jhowardmsft, @johnstep )
2 - Get 1 LGTM from Docker on the non-LCOW portions (@dmcgowan)
3 - MSFT to work with Docker (@friism to get contact) to get graph driver test coverage via existing > end to end regression test suite

@dmcgowan @mlaventure is this one ready to go?

dmcgowan · 2017-09-15T23:46:51Z

LGTM

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

PatrickLang mentioned this pull request Jul 25, 2017

Epic: Linux Containers on Windows (LCOW) #33850

Closed

gupta-ak requested review from cpuguy83, dmcgowan, dnephin, johnstep, lowenna and tonistiigi as code owners July 28, 2017 21:59

gupta-ak force-pushed the akagup/lcow-remotefs-sandbox branch from a8c0d2e to 010670b Compare July 28, 2017 22:21

dnephin reviewed Jul 31, 2017

View reviewed changes

gupta-ak requested a review from AkihiroSuda as a code owner August 3, 2017 21:47

gupta-ak force-pushed the akagup/lcow-remotefs-sandbox branch from a9edf9a to 8340720 Compare August 3, 2017 21:48

gupta-ak changed the title ~~[WIP] LCOW: Support for docker cp, ADD/COPY on build~~ LCOW: Support for docker cp, ADD/COPY on build Aug 3, 2017

gupta-ak force-pushed the akagup/lcow-remotefs-sandbox branch 2 times, most recently from 3654690 to c4f3fef Compare August 4, 2017 00:39

lowenna added the rebuild/janky label Aug 4, 2017

GordonTheTurtle removed the rebuild/janky label Aug 4, 2017

lowenna added the rebuild/z label Aug 4, 2017

GordonTheTurtle removed the rebuild/z label Aug 4, 2017

gupta-ak force-pushed the akagup/lcow-remotefs-sandbox branch from c4f3fef to 39f436d Compare August 8, 2017 02:00

GordonTheTurtle assigned duglin Aug 9, 2017

lowenna reviewed Aug 9, 2017

View reviewed changes

daemon/graphdriver/lcow/lcow_svm.go Outdated

Copy link

Member

lowenna Aug 9, 2017 •

edited

Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should switch over to github.com/Microsoft/opengcs/client now.

lowenna reviewed Aug 9, 2017

View reviewed changes

daemon/graphdriver/lcow/lcow.go Outdated

Copy link

Member

lowenna Aug 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/Operationg/operation

lowenna reviewed Aug 9, 2017

View reviewed changes

gupta-ak force-pushed the akagup/lcow-remotefs-sandbox branch from 001deb3 to b4b4a8a Compare September 11, 2017 23:47

johnstep approved these changes Sep 12, 2017

View reviewed changes

thaJeztah added status/2-code-review and removed status/1-design-review labels Sep 12, 2017

gupta-ak force-pushed the akagup/lcow-remotefs-sandbox branch 2 times, most recently from a488760 to 4e9acdf Compare September 13, 2017 19:48

gupta-ak added 2 commits September 14, 2017 12:00

Vendor containerd/continuity@22694c6

ba13c17

Signed-off-by: Akash Gupta <akagup@microsoft.com>

gupta-ak force-pushed the akagup/lcow-remotefs-sandbox branch from 4e9acdf to 7a7357d Compare September 14, 2017 19:14

vieux added the rebuild/z label Sep 14, 2017

GordonTheTurtle removed the rebuild/z label Sep 14, 2017

Add LCOW behind experimental,

ff68674

might not be the cleanest way, but it's definitly the way with the minimum code change. Signed-off-by: Victor Vieux <victorvieux@gmail.com>

gupta-ak force-pushed the akagup/lcow-remotefs-sandbox branch from 119cbf3 to ff68674 Compare September 14, 2017 20:51

johnstep approved these changes Sep 14, 2017

View reviewed changes

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

GordonTheTurtle removed the rebuild/z label Sep 14, 2017

vieux added the rebuild/z label Sep 15, 2017

GordonTheTurtle removed the rebuild/z label Sep 15, 2017

vieux merged commit a5f9783 into moby:master Sep 15, 2017

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

thaJeztah mentioned this pull request Nov 25, 2020

Remove squash from build #34565

Open

corhere mentioned this pull request Sep 23, 2022

Remove LCOW: pkg/containerfs: drop ContainerFS abstraction #44191

Merged

thaJeztah mentioned this pull request Jun 15, 2024

builder/dockerfile: assorted linting fixes, and remove LCOW leftover #47993

Merged

Conversation

gupta-ak commented Jul 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gupta-ak commented Jul 28, 2017

Uh oh!

gupta-ak commented Jul 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gupta-ak Aug 3, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

gupta-ak commented Aug 3, 2017

Uh oh!

gupta-ak commented Aug 4, 2017

Uh oh!

lowenna Aug 9, 2017 • 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!

gupta-ak Aug 9, 2017 • 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!

gupta-ak commented Aug 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

gupta-ak Sep 13, 2017 • 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!

mlaventure commented Sep 12, 2017

Uh oh!

gupta-ak commented Sep 13, 2017

Uh oh!

johnstep left a comment

Choose a reason for hiding this comment

Uh oh!

lowenna commented Sep 14, 2017

Uh oh!

thaJeztah commented Sep 15, 2017

Uh oh!

dmcgowan commented Sep 15, 2017

Uh oh!

Reviewers

gupta-ak commented Jul 25, 2017 •

edited

Loading

gupta-ak commented Jul 28, 2017 •

edited

Loading

gupta-ak Aug 3, 2017 •

edited

Loading

lowenna Aug 9, 2017 •

edited

Loading

gupta-ak Aug 9, 2017 •

edited

Loading

gupta-ak Sep 13, 2017 •

edited

Loading