LCOW: Coalesce daemon stores, allow dual LCOW and WCOW mode by lowenna · Pull Request #34859 · moby/moby

lowenna · 2017-09-14T23:49:05Z

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

@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 · 2017-09-15T01:45:10Z

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 · 2017-09-15T16:52:06Z

@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 · 2017-09-15T17:24:58Z

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 · 2017-09-18T03:34:56Z

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 · 2017-09-19T02:10:01Z

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 · 2017-09-20T02:00:29Z

@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 · 2018-01-18T16:48:55Z

Ping @carlfischer1 @friism

tonistiigi

I think some code is still here that can be removed. I'd like more type safety than passing os/platform as a string and the reuse of layer interface described in earlier comments but these can be follow up.

Please also remove my handle from the commit message. I get spammed on every rebase.

tonistiigi · 2018-01-18T18:32:27Z

daemon/build.go

Fixed (and removed your handle)

tonistiigi · 2018-01-18T18:59:51Z

layer/layer.go

Where is this used? The only place I see is in the image delete where this info is already known. Same for rwlayer and test mocks.

Ah, indeed, you are correct. Good catch. I've removed it, and updated image delete to use it's knowledge of the operating system instead. See commit c94d34f addressing it.

tonistiigi · 2018-01-18T19:00:24Z

layer/layer.go

Where is this used?

Removed now.

stevvooe · 2018-01-18T20:04:47Z

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 · 2018-01-18T20:26:58Z

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 · 2018-01-18T20:27:38Z

@tonistiigi I'll get to these comments shortly.

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

tonistiigi · 2018-01-19T01:06:07Z

LGTM

cpuguy83

LGTM

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>

TheDukeDK · 2018-02-15T06:27:12Z

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 · 2018-02-15T14:30:40Z

@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

lowenna requested review from cpuguy83, dnephin, johnstep and tonistiigi as code owners September 14, 2017 23:49

GordonTheTurtle added the status/0-triage label Sep 14, 2017

tonistiigi mentioned this pull request Sep 15, 2017

add testcase in layer/empty_test.go with Platform(),Metadata() #34849

Merged

lowenna force-pushed the jjh/singleimagestore branch from 36c5e5e to 910fb8c Compare September 15, 2017 17:36

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

lowenna changed the title ~~[Depends on #34642] LCOW: Coalese daemon stores~~ [WIP] [Depends on #34642] LCOW: Coalese daemon stores Sep 19, 2017

lowenna force-pushed the jjh/singleimagestore branch 4 times, most recently from f98058f to 1b25f32 Compare September 20, 2017 01:12

lowenna force-pushed the jjh/singleimagestore branch 5 times, most recently from 297153c to b4a583e Compare September 21, 2017 19:04

lowenna requested a review from vdemeester as a code owner September 21, 2017 19:04

lowenna mentioned this pull request Sep 21, 2017

Add platform to /image/json endpoint #34924

Closed

lowenna force-pushed the jjh/singleimagestore branch 3 times, most recently from 7cf7821 to 57b8739 Compare September 21, 2017 23:11

tonistiigi reviewed Jan 18, 2018

View reviewed changes

Address feedback from Tonis

0cba774

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

lowenna force-pushed the jjh/singleimagestore branch from fb586ca to 0cba774 Compare January 18, 2018 20:31

Remove OS() from layer interface

c94d34f

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

lowenna added the rebuild/powerpc label Jan 18, 2018

GordonTheTurtle removed the rebuild/powerpc label Jan 18, 2018

lowenna added the rebuild/experimental label Jan 19, 2018

GordonTheTurtle removed the rebuild/experimental label Jan 19, 2018

lowenna added status/4-merge and removed status/2-code-review labels Jan 19, 2018

cpuguy83 approved these changes Jan 19, 2018

View reviewed changes

cpuguy83 merged commit bb6ce89 into moby:master Jan 19, 2018

rn mentioned this pull request Jan 21, 2018

Assorted updates linuxkit/lcow#6

Merged

StefanScherer mentioned this pull request Jan 21, 2018

Add LCOW scenario StefanScherer/docker-windows-box#26

Merged

carlfischer1 mentioned this pull request Jan 22, 2018

docker images does not list all local images docker/for-win#1591

Closed

thaJeztah added platform/windows impact/changelog labels Jan 22, 2018

stebet mentioned this pull request Jan 22, 2018

Mixed Windows and Linux containers docker/compose#3987

Closed

thaJeztah mentioned this pull request Jan 25, 2022

Remove various leftover LCOW checks #43187

Merged

thaJeztah mentioned this pull request Aug 9, 2022

daemon: remove some legacy fallbacks, and small refactor #43940

Merged

Conversation

lowenna commented Sep 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tonistiigi commented Sep 15, 2017

Uh oh!

lowenna commented Sep 15, 2017

Uh oh!

tonistiigi commented Sep 15, 2017

Uh oh!

lowenna commented Sep 18, 2017

Uh oh!

lowenna commented Sep 19, 2017

Uh oh!

lowenna commented Sep 20, 2017

Uh oh!

lowenna commented Jan 18, 2018

Uh oh!

tonistiigi left a comment

Choose a reason for hiding this comment

Uh oh!

tonistiigi Jan 18, 2018

Choose a reason for hiding this comment

Uh oh!

lowenna Jan 18, 2018

Choose a reason for hiding this comment

Uh oh!

tonistiigi Jan 18, 2018

Choose a reason for hiding this comment

Uh oh!

lowenna Jan 18, 2018

Choose a reason for hiding this comment

Uh oh!

tonistiigi Jan 18, 2018

Choose a reason for hiding this comment

Uh oh!

lowenna Jan 18, 2018

Choose a reason for hiding this comment

Uh oh!

stevvooe commented Jan 18, 2018

Uh oh!

lowenna commented Jan 18, 2018

Uh oh!

lowenna commented Jan 18, 2018

Uh oh!

tonistiigi commented Jan 19, 2018

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

TheDukeDK commented Feb 15, 2018

Uh oh!

friism commented Feb 15, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

lowenna commented Sep 14, 2017 •

edited

Loading