Skip to content
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: Coalesce daemon stores, allow dual LCOW and WCOW mode #34859

Merged
merged 4 commits into from Jan 19, 2018

Conversation

lowenna
Copy link
Member

@lowenna lowenna commented Sep 14, 2017

Signed-off-by: John Howard jhoward@microsoft.com

Fixes #35303

@tonistiigi, @dmcgowan, @johnstep, @mlaventure, @stevvooe, @vieux, @friism This is one of the action items from the F2F and the document described in #34617.

Specifically, this re-coalesces all the remaining daemon stores which were split as part of the original LCOW implementation.

This has a hard requirement on #34642 being merged to work. It's the last commit which is of interest for the re-coalescing. With this change, this finally allows both LCOW and WCOW containers to run side-by-side. For example

PS E:\go\src\github.com\docker\docker> docker images
REPOSITORY             TAG                 IMAGE ID            CREATED             SIZE
<none>                 <none>              81661b149a91        6 hours ago         0B
committed              latest              55ad823a090c        22 hours ago        3.67MB
<none>                 <none>              e0fad00a867e        22 hours ago        3.67MB
mongo                  latest              39f5c173b5d4        26 hours ago        438MB
node                   latest              9b1a69083339        28 hours ago        973MB
swarm                  latest              2569518fadd0        30 hours ago        18.1MB
nginx                  latest              da5939581ac8        31 hours ago        155MB
alpine                 latest              76da55c8019d        33 hours ago        6.29MB
busybox                latest              54511612f1c4        38 hours ago        3.41MB
mysql                  latest              aeaed9976244        39 hours ago        532MB
registry               latest              3ebefe7c539b        41 hours ago        60MB
postgres               latest              f5b23ab70487        41 hours ago        405MB
ubuntu                 latest              8b72bba4485f        44 hours ago        175MB
redis                  latest              9813a7e8fcc0        44 hours ago        155MB
httpd                  latest              6b4e03d65aa3        47 hours ago        285MB
hello-world            latest              05a3bd381fc2        2 days ago          263kB
logstash               latest              98f8400d2944        3 days ago          1.1GB
elasticsearch          latest              d1ac13423d3c        3 days ago          820MB
microsoft/nanoserver   10.0.16289.0        6295be59ae2e        4 days ago          204MB
microsoft/nanoserver   latest              6295be59ae2e        4 days ago          204MB
PS E:\go\src\github.com\docker\docker> docker inspect -f "{{.Os}}" busybox
linux
PS E:\go\src\github.com\docker\docker> docker inspect -f "{{.Os}}" microsoft/nanoserver
windows
PS E:\go\src\github.com\docker\docker> docker run --rm busybox echo hello
hello
PS E:\go\src\github.com\docker\docker> docker run --rm microsoft/nanoserver cmd /s /c echo hello
hello
PS E:\go\src\github.com\docker\docker>

@jstarks, @PatrickLang @darrenstahlmsft FYI

@tonistiigi
Copy link
Member

Didn't go over everything yet but I think layerStore should not be changed like that. Layerstore is a content-addressable wrapper around storagedriver. Also, OS() does not belong to the layer interface. Layer interface doesn't care how it is implemented(or what platform implements it). The image could just return the correct layerstore instance(if we don't want to figure out the interface for getting the Layer instances for image rootfs atm).

@lowenna
Copy link
Member Author

lowenna commented Sep 15, 2017

@tonistiigi

You're right, there are two ways of doing this. I'm not certian either one has merit over the other.

The way I've implemented seems slightly cleaner from the perspective of the interfaces to it and the call-sites into it. Here's why:

It's only the layerstore that communicates with the graphdriver, so having a single layerstore know which graphdriver to choose based on the operating system of the layer is clean and requires no jiggling in multiple higher-level places to determine which layerstore to use. In other words, this pushes it down to the lowest possible layer keeping a clean upper surface to the layerstore. In reality, there is no need to have multiple layerstores when it's just the graphdriver that is different. Does that make sense?

@tonistiigi
Copy link
Member

This changes the layerstore Register and Create methods, and adds a new OS method to layer interface. These are all new concepts that the caller must be aware now so I'm not sure why it would be cleaner. Ideally, you could get a Layer from an image, that is already connected to a backing layerstore and caller doesn't even realize that anything is different(and no interface changes are needed). In the beginning, it can be a helper function. For registering a root layer it could also be that imagestore determines the right layerstore for a config, so the upper level is not aware how many layerstores there are.

Actually, looking at it again, I'm not sure how this version works at all. How do you make sure that the parent layer is owned by the same driver? Layerstore is basically just a tar blobstore(with complex backend) so these collisions are not uncommon at all. You could easily have same partial diffid chains for different platforms.

@lowenna
Copy link
Member Author

lowenna commented Sep 18, 2017

I assure you it works! But sure, I'll refactor again to have multiple layer stores if that is what you want. Just trying to do the right thing here after the F2F 😊

@lowenna lowenna force-pushed the jjh/singleimagestore branch 2 times, most recently from 9bc0fe9 to b326dba Compare September 19, 2017 02:08
@lowenna
Copy link
Member Author

lowenna commented Sep 19, 2017

Rebased on master (following the remote filesystem work, and updates to #34642), refactoring for multi-layerstores still in progress. Marking WIP until that is complete (ETA 2-3 days).

@lowenna lowenna changed the title [Depends on #34642] LCOW: Coalese daemon stores [WIP] [Depends on #34642] LCOW: Coalese daemon stores Sep 19, 2017
@lowenna lowenna force-pushed the jjh/singleimagestore branch 4 times, most recently from f98058f to 1b25f32 Compare September 20, 2017 01:12
@lowenna
Copy link
Member Author

lowenna commented Sep 20, 2017

@tonistiigi - Still some tidying to fix up tests for CI, but pushed a new commit which changes it back to multiple layerstores. Is this more the direction you wanted? Local verification passes on it.

@lowenna lowenna force-pushed the jjh/singleimagestore branch 5 times, most recently from 297153c to b4a583e Compare September 21, 2017 19:04
@lowenna lowenna force-pushed the jjh/singleimagestore branch 3 times, most recently from 7cf7821 to 57b8739 Compare September 21, 2017 23:11
@stevvooe
Copy link
Contributor

LGTM

My only concern is that we replace platform with os everywhere, but this seems because we are mostly dealing with areas where only os is relevant. Seeing the removal of arguments from various interfaces is a great step.

@lowenna
Copy link
Member Author

lowenna commented Jan 18, 2018

My only concern is that we replace platform with os everywhere, but this seems because we are mostly dealing with areas where only os is relevant.

That's exactly why I did it - to make it explicit that it's not a full platform, just an operating system.

@lowenna
Copy link
Member Author

lowenna commented Jan 18, 2018

@tonistiigi I'll get to these comments shortly.

Signed-off-by: John Howard <jhoward@microsoft.com>
Signed-off-by: John Howard <jhoward@microsoft.com>
@tonistiigi
Copy link
Member

LGTM

Copy link
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

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

LGTM

@cpuguy83 cpuguy83 merged commit bb6ce89 into moby:master Jan 19, 2018
rn added a commit to rn/lcow that referenced this pull request Jan 21, 2018
With:
- moby/moby#36065
- moby/moby#34859
merged there is no need for environment variables anymore

Update instructions.

Signed-off-by: Rolf Neugebauer <rolf.neugebauer@docker.com>
@rn rn mentioned this pull request Jan 21, 2018
rn added a commit to rn/lcow that referenced this pull request Jan 21, 2018
With:
- moby/moby#36065
- moby/moby#34859
merged there is no need for environment variables anymore

Update instructions.

Signed-off-by: Rolf Neugebauer <rolf.neugebauer@docker.com>
rn added a commit to rn/lcow that referenced this pull request Jan 21, 2018
With:
- moby/moby#36065
- moby/moby#34859
merged there is no need for environment variables anymore

Update instructions.

Signed-off-by: Rolf Neugebauer <rolf.neugebauer@docker.com>
@lowenna lowenna deleted the jjh/singleimagestore branch January 25, 2018 17:35
@TheDukeDK
Copy link

I have a scenario where I need to have Linux and Windows containers communicate with each other on a windows 10 box.

When will this be available? Is there a way for me to get this in alpha to try it out?

@friism
Copy link
Contributor

friism commented Feb 15, 2018

@praqmaTim I think this should be available in the latest beta: https://blog.docker.com/2018/02/docker-for-windows-18-02-with-windows-10-fall-creators-update/

cc @carlfischer1

@thaJeztah thaJeztah added the area/lcow Issues and PR's related to the experimental LCOW feature label Oct 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/lcow Issues and PR's related to the experimental LCOW feature impact/changelog platform/windows status/4-merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet