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 [email protected]

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.

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!

Replicate relevant mutations to the in-memory ACID store. Readers will then be able to query container state without locking. Signed-off-by: Fabio Kung <[email protected]> (cherry picked from commit eed4c7b) Signed-off-by: Kir Kolyshkin <[email protected]> Conflicts: - daemon/daemon.go: missing moby#33241 - container/container_unix.go: missing moby#32336 - container/container_windows.go, daemon/container_operations.go: minor conflicts Signed-off-by: Kir Kolyshkin <[email protected]>

Container queries are now served from the consistent in-memory db, and don't need to grab a lock on every container being listed. Signed-off-by: Fabio Kung <[email protected]> (cherry picked from commit 8e425eb) Signed-off-by: Kir Kolyshkin <[email protected]> Conflicts: - daemon/list.go: missing github.com/moby/pull/33241

Reuse existing structures and rely on json serialization to deep copy Container objects. Also consolidate all "save" operations on container.CheckpointTo, which now both saves a serialized json to disk, and replicates state to the ACID in-memory store. Signed-off-by: Fabio Kung <[email protected]> (cherry picked from commit edad527) Signed-off-by: Kir Kolyshkin <[email protected]> Conflicts: - daemon/container_operations.go: missing moby#30117 - daemon/daemon.go: missing moby#32821 - daemon/list.go: missing moby#27557, moby#33241 etc.

Signed-off-by: Fabio Kung <[email protected]> (cherry picked from commit 9134e87) Signed-off-by: Kir Kolyshkin <[email protected]> Conflicts: - container/container.go: missing moby#33241, moby#32687 Signed-off-by: Kir Kolyshkin <[email protected]>

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

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

johnstep merged commit 930e689 into moby:master Jun 21, 2017

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

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

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

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

johnstep Jun 21, 2017 •

edited

Loading