LCOW: Support most operations excluding remote filesystem by lowenna · Pull Request #33241 · moby/moby

lowenna · 2017-05-17T03:57:06Z

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

This is the start of the Linux Containers on Windows changes and implements a significant chunk of it.

This PR enables pull of a Linux image on Windows, committing a layer, basics of build, import and export. It does NOT include the remote filesystem work that is being tracked separately to allow things such as COPY or ADD to work (which need remoting to a service VM as Windows can't directly read or perform operations on a Linux filesystem).

This PR has been updated based on feedback from @crosbymichael so that when LCOW is enabled, the daemon assumes Linux-mode, and won't be able to run/see Windows containers, or see any Windows images. This update means that no API changes are required while the UX changes are being investigated separately. This is an artificial crippling with no technical reason.

(In memory of Guide Dogs for the Blind puppy-in-training "Park", front left. He sadly passed too young. 1st Jan 2016 - 16th June 2017. Rest in peace, sweet boy 🐶 💛

My two guys in the background, another former GDB puppy-in-training, "Palmetto" front-right )

lowenna · 2017-05-19T22:06:37Z

@dmcgowan @stevvooe @crosbymichael PTAL.

@johnstep @thaJeztah FYI.

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

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

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

lowenna · 2017-06-21T03:48:19Z

Update: #33757 should fix the docker-py failures currently on master affecting janky and experimental

lowenna · 2017-06-21T05:36:11Z

Short of a rebase once 33757 is merged to alleviate the docker-py failures, this is fully reviewable in its current state. @crosbymichael @stevvooe @dmcgowan PTAL

@friism, @thaJeztah FYI

lowenna · 2017-06-21T17:19:52Z

Yay, green again 💚

crosbymichael · 2017-06-21T18:22:09Z

LGTM

johnstep

LGTM

johnstep · 2017-06-21T20:06:22Z

api/server/backend/build/backend.go

 type ImageComponent interface {
 	SquashImage(from string, to string) (string, error)
-	TagImageWithReference(image.ID, reference.Named) error
+	TagImageWithReference(image.ID, string, reference.Named) error


platform string?

No, just string is correct :)

johnstep · 2017-06-21T20:20:26Z

builder/dockerfile/parser/parser.go

 func NewDefaultDirective() *Directive {
 	directive := Directive{}
 	directive.setEscapeToken(string(DefaultEscapeToken))
+	directive.setPlatformToken(runtime.GOOS)


DefaultPlatformToken instead of runtime.GOOS

Yes, fixing in a nits PR shortly

johnstep · 2017-06-21T20:28:28Z

builder/dockerfile/builder_windows.go


-var defaultShell = []string{"cmd", "/S", "/C"}
+func defaultShellForPlatform(platform string) []string {
+	if platform == "linux" {


What about moving defaultShellForPlatform to builder.go, rather than duplicating the Unix shell?

johnstep · 2017-06-21T20:39:50Z

api/server/backend/build/tag.go

-		if err := bt.imageComponent.TagImageWithReference(imageID, rt); err != nil {
+		// TODO @jhowardmsft LCOW support. Will need revisiting.
+		platform := runtime.GOOS
+		if platform == "windows" && system.LCOWSupported() {


Remove redundant platform == "windows" check since implicit in system.LCOWSupported? Since this check is duplicated in many places, it might not be worth the effort. It would be nice to have a common function.

Fixed in nits PR about to be submitted

johnstep · 2017-06-21T20:45:27Z

container/container.go

+	// we need to replace the 'env' keys where they match and append anything
+	// else.
+	//return ReplaceOrAppendEnvValues(linkedEnv, container.Config.Env)
+	foo := ReplaceOrAppendEnvValues(env, container.Config.Env)


Nit: Remove the commented out return line, and change foo := to env = .

Fixed in nits PR about to be submitted

johnstep · 2017-06-21T20:56:17Z

daemon/graphdriver/lcow/lcow.go

+	// Make sure layers are created with the correct ACL so that VMs can access them.
+	layerPath := d.dir(id)
+	logrus.Debugf("lcowdriver: create: id %s: creating layerPath %s", id, layerPath)
+	// Make sure the layers are created with the correct ACL so that VMs can access them.


Redundant comment, almost identical to comment 3 lines above.

Fixed in nits PR about to be submitted

johnstep · 2017-06-21T21:05:12Z

daemon/graphdriver/windows/windows.go

 }

+// panicIfUsedByLcow does exactly what it says.
+// TODO @jhowardmsft - this is an temporary measure for the bring-up of


Nit: "a temporary"

Fixed in nits PR about to be submitted

johnstep · 2017-06-21T21:33:57Z

daemon/info.go

+		}
+	}
+
+	// TODO @jhowardmsft LCOW support. For now, hard-code the platform shown for the driver status


Seems okay for now, but maybe also only list the corresponding driver instead of all (both) drivers?

johnstep · 2017-06-21T21:47:51Z

libcontainerd/container_windows.go

-			logrus.Warnf("libcontainerd: HasPendingUpdates() failed (container may have been killed): %s", err)
-		} else {
-			si.UpdatePending = updatePending
+		// Pending updates is only applicable for WCOW


Ha, first time I have seen "WCOW"...

johnstep · 2017-06-21T22:13:09Z

api/server/backend/build/tag.go

 	for _, rt := range bt.repoAndTags {
-		if err := bt.imageComponent.TagImageWithReference(imageID, rt); err != nil {
+		// TODO @jhowardmsft LCOW support. Will need revisiting.
+		platform := runtime.GOOS


Even if they are removed, it would be been nice to have separate functions, something like GetEffectivePlatform, IsWindowsPlatform (check if it is WCOW), and something like EnsurePlatform (make sure platform is not empty). This might seem trivial, but would allow comments and reasoning to be in one place.

Also, the request was to always be in Linux or always be in Windows mode, and I think that means platform could have been set once, globally, instead of "computed" all over the place. It was not really about crippling, but about breaking down the work into smaller phases. However, the current changes should make it easier to switch to multiple platform support, so this approach seems okay to me.

lowenna · 2017-06-21T22:42:30Z

@johnstep - spoke offline on slack, but yes, all these nits can easily be addressed in a followup. Thanks for reviewing!

GordonTheTurtle added the status/0-triage label May 17, 2017

lowenna force-pushed the jjh/multi-layerstore branch 3 times, most recently from 5507b5d to 931c67d Compare May 17, 2017 04:36

lowenna changed the title ~~IGNORE - WIP~~ LCOW: IGNORE - WIP May 17, 2017

lowenna force-pushed the jjh/multi-layerstore branch 10 times, most recently from 4c5b3fe to fc8301b Compare May 19, 2017 18:11

lowenna changed the title ~~LCOW: IGNORE - WIP~~ [WIP] LCOW: Initial implementation May 19, 2017

lowenna force-pushed the jjh/multi-layerstore branch 3 times, most recently from 64e06c6 to f694446 Compare May 31, 2017 19:00

This was referenced May 31, 2017

LCOW: Add environment variable to enable the feature #32793

Closed

LCOW: Adds platform to the layer store #32818

Closed

LCOW: Remove CommonContainer - just Container #32823

Closed

lowenna force-pushed the jjh/multi-layerstore branch 2 times, most recently from 0e77baf to 7f63c2b Compare May 31, 2017 23:33

GordonTheTurtle assigned ehazlett Jun 1, 2017

lowenna force-pushed the jjh/multi-layerstore branch 4 times, most recently from 21718fb to 88b47e2 Compare June 2, 2017 02:40

John Howard added 12 commits June 20, 2017 19:49

LCOW: Push to switch platform

a97b99e

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

LCOW: Plumb through platform on Import

3d5cec7

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

Vendor Microsoft/hcsshim @ v0.5.2

8fe9eb0

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

Vendor jhowardmsft/opengcs v0.0.3

07034a4

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

LCOW: OCI Spec and Environment for container start

f154588

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

LCOW: Create layer folders with correct ACL

ed10ac6

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

LCOW: Don't mount for linux containers either

f8aa700

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

LCOW: Coherency - ensure windowsfilter driver is not used

bdabc76

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

LCOW: Fix schemav1 pull to extract platform

b21d9ab

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

LCOW: Rework after 33454 merged which refactored daemon/builder inter…

3a09b56

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

Show what cross-build is executing

d82e26a

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

LCOW: Set correct default shell for platform in builder

fe7b4d8

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

lowenna mentioned this pull request Jun 21, 2017

Proposal: refactor image management code #33125

Open

johnstep approved these changes Jun 21, 2017

View reviewed changes

lowenna mentioned this pull request Jun 26, 2017

LCOW: Fix nits from 33241 #33826

Merged

thaJeztah mentioned this pull request Jun 26, 2017

Add a LastTagTime for images #31497

Merged

PatrickLang mentioned this pull request Jun 27, 2017

Epic: Linux Containers on Windows (LCOW) #33850

Closed

thaJeztah mentioned this pull request Jul 9, 2017

docker load -i file.tar.gz broken on master #34030

Closed

thaJeztah mentioned this pull request Oct 9, 2017

Fix regression on inspect #35144

Merged

lowenna mentioned this pull request Oct 25, 2017

LCOW: Coalesce daemon stores, allow dual LCOW and WCOW mode #34859

Merged

kolyshkin mentioned this pull request May 2, 2020

backport: No container locks on docker ps projectatomic/docker#375

Merged

This was referenced Jan 25, 2022

Remove various leftover LCOW checks #43187

Merged

pkg/system: move maxTime init() back to Chtimes code and remove use of "unsafe" #43191

Open

thaJeztah mentioned this pull request Mar 11, 2022

Remove LCOW (step 9): layer: remove layerstore.setOS(), layerstore.getOS() #43362

Merged

thaJeztah mentioned this pull request Oct 7, 2022

pkg/system: cleanup, test-fixes and improvements and minor fixes #44265

Merged

Conversation

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

Uh oh!

Uh oh!

lowenna commented May 19, 2017

Uh oh!

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

Uh oh!

Uh oh!

lowenna commented Jun 21, 2017

Uh oh!

lowenna commented Jun 21, 2017

Uh oh!

crosbymichael commented Jun 21, 2017

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnstep Jun 21, 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!

lowenna commented Jun 21, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

lowenna commented May 17, 2017 •

edited

Loading

lowenna commented Jun 21, 2017 •

edited

Loading

johnstep Jun 21, 2017 •

edited

Loading