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: Support for docker cp, ADD/COPY on build #34252

Merged
merged 3 commits into from Sep 15, 2017

Conversation

@gupta-ak
Contributor

gupta-ak commented Jul 25, 2017

- What I did

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

- How I did it

  1. 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.
  2. 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.
  3. 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)

image

@gupta-ak

This comment has been minimized.

Show comment
Hide comment
@gupta-ak

gupta-ak Jul 28, 2017

Contributor

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

gupta-ak commented Jul 28, 2017

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

This comment has been minimized.

Show comment
Hide comment
@gupta-ak

gupta-ak Jul 28, 2017

Contributor

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

Contributor

gupta-ak commented Jul 28, 2017

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

Show outdated Hide outdated builder/dockerfile/internals.go Outdated
Show outdated Hide outdated builder/dockerfile/internals.go Outdated
Show outdated Hide outdated builder/dockerfile/copy.go Outdated

@gupta-ak gupta-ak requested a review from AkihiroSuda as a code owner Aug 3, 2017

@gupta-ak

This comment has been minimized.

Show comment
Hide comment
@gupta-ak

gupta-ak Aug 3, 2017

Contributor

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
Contributor

gupta-ak commented Aug 3, 2017

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 gupta-ak changed the title from [WIP] LCOW: Support for docker cp, ADD/COPY on build to LCOW: Support for docker cp, ADD/COPY on build Aug 3, 2017

@gupta-ak

This comment has been minimized.

Show comment
Hide comment
@gupta-ak

gupta-ak Aug 4, 2017

Contributor

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

Contributor

gupta-ak commented Aug 4, 2017

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

@gupta-ak

This comment has been minimized.

Show comment
Hide comment
@gupta-ak

gupta-ak Aug 9, 2017

Contributor

@stevvooe PTAL + add whoever else should take a look

Contributor

gupta-ak commented Aug 9, 2017

@stevvooe PTAL + add whoever else should take a look

Show outdated Hide outdated pkg/system/path.go Outdated
Show outdated Hide outdated pkg/containerfs/archiver.go Outdated
// CopyFileWithTar emulates the behavior of the 'cp' command-line
// for a single file. It copies a regular file from path `src` to
// path `dst`, and preserves all its metadata.
func (archiver *Archiver) CopyFileWithTar(src, dst string) (err error) {

This comment has been minimized.

@dmcgowan

dmcgowan Sep 1, 2017

Member

This function is so bizarre. This is out of scope for your change since you are preserving existing behavior, but were you able to fully understand why this is necessary? I understand the directory copy with tar, but this seems so heavy.

@dmcgowan

dmcgowan Sep 1, 2017

Member

This function is so bizarre. This is out of scope for your change since you are preserving existing behavior, but were you able to fully understand why this is necessary? I understand the directory copy with tar, but this seems so heavy.

This comment has been minimized.

@gupta-ak

gupta-ak Sep 5, 2017

Contributor

I'm not exactly sure why this method exists either. It's only used for copyFile in the builder\dockerfile\copy.go, where it has a chrootarchive.Untar as the plugin untar function, and makePluginBundle in intergration-cli\fixtures\plugin, where it has the archive.Untar as the plugin untar function. It seems like it could be removed with just a regular copyfile with an option for a chroot copyfile.

@gupta-ak

gupta-ak Sep 5, 2017

Contributor

I'm not exactly sure why this method exists either. It's only used for copyFile in the builder\dockerfile\copy.go, where it has a chrootarchive.Untar as the plugin untar function, and makePluginBundle in intergration-cli\fixtures\plugin, where it has the archive.Untar as the plugin untar function. It seems like it could be removed with just a regular copyfile with an option for a chroot copyfile.

Show outdated Hide outdated daemon/graphdriver/fsdiff.go Outdated
Show outdated Hide outdated daemon/graphdriver/driver.go Outdated
Show outdated Hide outdated builder/dockerfile/copy.go Outdated
@gupta-ak

This comment has been minimized.

Show comment
Hide comment
@gupta-ak

gupta-ak Sep 5, 2017

Contributor

Updated with @dmcgowan's feedback

Contributor

gupta-ak commented Sep 5, 2017

Updated with @dmcgowan's feedback

@johnstep

This comment has been minimized.

Show comment
Hide comment
@johnstep

johnstep Sep 8, 2017

Contributor

I'm still reviewing the graph driver changes, and plan to finish by Monday. Apologies for taking longer than expected.

Contributor

johnstep commented Sep 8, 2017

I'm still reviewing the graph driver changes, and plan to finish by Monday. Apologies for taking longer than expected.

@johnstep

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

Show outdated Hide outdated daemon/graphdriver/lcow/lcow.go Outdated
Show outdated Hide outdated daemon/graphdriver/lcow/remotefs.go Outdated
Show outdated Hide outdated daemon/graphdriver/lcow/remotefs.go Outdated
logrus.Debugf("%s: VM %s did not stop succesfully: %s", title, id, err)
return nil, err
}
return d.startServiceVMIfNotRunning(id, mvdToAdd, context)

This comment has been minimized.

@johnstep

johnstep Sep 12, 2017

Contributor

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

@johnstep

johnstep Sep 12, 2017

Contributor

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

Show outdated Hide outdated daemon/graphdriver/lcow/lcow_svm.go Outdated
Show outdated Hide outdated daemon/graphdriver/lcow/lcow_svm.go Outdated
@mlaventure

This comment has been minimized.

Show comment
Hide comment
@mlaventure

mlaventure Sep 12, 2017

Contributor

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.

Contributor

mlaventure commented Sep 12, 2017

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

This comment has been minimized.

Show comment
Hide comment
@gupta-ak

gupta-ak Sep 13, 2017

Contributor

rebased + updated with @johnstep 's feedback

Contributor

gupta-ak commented Sep 13, 2017

rebased + updated with @johnstep 's feedback

gupta-ak added some commits Aug 4, 2017

Vendor containerd/continuity@22694c6
Signed-off-by: Akash Gupta <akagup@microsoft.com>
LCOW: Implemented support for docker cp + build
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>
Add LCOW behind experimental,
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)

@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft

jhowardmsft Sep 14, 2017

Contributor

Graphdriver changes LGTM

Contributor

jhowardmsft commented Sep 14, 2017

Graphdriver changes LGTM

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Sep 15, 2017

Member

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?

Member

thaJeztah commented Sep 15, 2017

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

This comment has been minimized.

Show comment
Hide comment
@dmcgowan

dmcgowan Sep 15, 2017

Member

LGTM

Member

dmcgowan commented Sep 15, 2017

LGTM

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

7 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 36850 has succeeded
Details
janky Jenkins build Docker-PRs 45490 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 5892 has succeeded
Details
vendor Jenkins build Docker-PRs-vendor 3810 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 17085 has succeeded
Details
z Jenkins build Docker-PRs-s390x 5687 has succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment