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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

LCOW: Support most operations excluding remote filesystem #33241

Merged
merged 27 commits into from Jun 21, 2017

Conversation

Projects
None yet
6 participants
@jhowardmsft
Contributor

jhowardmsft commented May 17, 2017

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.

park
(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 )

@jhowardmsft jhowardmsft changed the title from IGNORE - WIP to LCOW: IGNORE - WIP May 17, 2017

@jhowardmsft jhowardmsft changed the title from LCOW: IGNORE - WIP to [WIP] LCOW: Initial implementation May 19, 2017

@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft
Contributor

jhowardmsft commented May 19, 2017

@jhowardmsft jhowardmsft changed the title from [WIP] LCOW: Initial implementation to [WIP] LCOW: Pull, import, most of run and some of build Jun 2, 2017

@jhowardmsft jhowardmsft changed the title from [WIP] LCOW: Pull, import, most of run and some of build to [WIP] LCOW: Pull, import, commit, most of run and some of build Jun 2, 2017

@jhowardmsft jhowardmsft changed the title from [WIP] LCOW: Pull, import, commit, most of run and some of build to [WIP] LCOW: Pull, import, export, commit, most of run and some of build Jun 2, 2017

Show outdated Hide outdated daemon/oci_windows.go Outdated

@jhowardmsft jhowardmsft changed the title from [WIP] LCOW: Pull, import, export, commit, most of run and some of build to LCOW: Pull, import, export, most of run and some of build Jun 5, 2017

@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft

jhowardmsft Jun 5, 2017

Contributor

image

Contributor

jhowardmsft commented Jun 5, 2017

image

@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft

jhowardmsft Jun 14, 2017

Contributor

Rebased. I'm adding more commits making further progress while waiting for review as the PR isn't making any progress after removing WIP for getting on for a week after discussing offline. Also moving to code review. @crosbymichael @stevvooe @dmcgowan @thaJeztah @johnstep PTAL.

Contributor

jhowardmsft commented Jun 14, 2017

Rebased. I'm adding more commits making further progress while waiting for review as the PR isn't making any progress after removing WIP for getting on for a week after discussing offline. Also moving to code review. @crosbymichael @stevvooe @dmcgowan @thaJeztah @johnstep PTAL.

@crosbymichael

This comment has been minimized.

Show comment
Hide comment
@crosbymichael

crosbymichael Jun 14, 2017

Contributor

You should have pinged us in the PR after removing the WIP that you were ready. I didn't get an email when the PR title changes.

Contributor

crosbymichael commented Jun 14, 2017

You should have pinged us in the PR after removing the WIP that you were ready. I didn't get an email when the PR title changes.

@crosbymichael

This comment has been minimized.

Show comment
Hide comment
@crosbymichael

crosbymichael Jun 14, 2017

Contributor

Restarted the failed builds

Contributor

crosbymichael commented Jun 14, 2017

Restarted the failed builds

@@ -33,7 +35,12 @@ func NewTagger(backend ImageComponent, stdout io.Writer, names []string) (*Tagge
// TagImages creates image tags for the imageID
func (bt *Tagger) TagImages(imageID image.ID) error {
for _, rt := range bt.repoAndTags {
if err := bt.imageComponent.TagImageWithReference(imageID, rt); err != nil {
// TODO @jhowardmsft LCOW support. Will need revisiting.
platform := runtime.GOOS

This comment has been minimized.

@crosbymichael

crosbymichael Jun 14, 2017

Contributor

This can be done in a separate PR but after writing these 4 lines over 10 different times, i think this can be factored out into a function for handling platform switching

@crosbymichael

crosbymichael Jun 14, 2017

Contributor

This can be done in a separate PR but after writing these 4 lines over 10 different times, i think this can be factored out into a function for handling platform switching

This comment has been minimized.

@jhowardmsft

jhowardmsft Jun 19, 2017

Contributor

To be fair, all of these comments are markers on account of the fact of your request to cripple this PR such that the windows daemon -only- runs in either 'windows' or 'lcow' mode. If the API changes (particularly to build, run and import) are accepted to add platform, then pretty much all of these can be removed. The crippling is artificial, not technical. There is absolutely no need to factor them out - they are all temporary due to your request.

@jhowardmsft

jhowardmsft Jun 19, 2017

Contributor

To be fair, all of these comments are markers on account of the fact of your request to cripple this PR such that the windows daemon -only- runs in either 'windows' or 'lcow' mode. If the API changes (particularly to build, run and import) are accepted to add platform, then pretty much all of these can be removed. The crippling is artificial, not technical. There is absolutely no need to factor them out - they are all temporary due to your request.

This comment has been minimized.

@johnstep

johnstep Jun 21, 2017

Contributor

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.

@johnstep

johnstep Jun 21, 2017

Contributor

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.

jhowardmsft added some commits May 31, 2017

LCOW: Fix ImageCache to address right store
Signed-off-by: John Howard <jhoward@microsoft.com>
LCOW: Image exporter update
Signed-off-by: John Howard <jhoward@microsoft.com>
LCOW: Push to switch platform
Signed-off-by: John Howard <jhoward@microsoft.com>
LCOW: Plumb through platform on Import
Signed-off-by: John Howard <jhoward@microsoft.com>
Vendor Microsoft/hcsshim @ v0.5.2
Signed-off-by: John Howard <jhoward@microsoft.com>
Vendor jhowardmsft/opengcs v0.0.3
Signed-off-by: John Howard <jhoward@microsoft.com>
LCOW: OCI Spec and Environment for container start
Signed-off-by: John Howard <jhoward@microsoft.com>
LCOW: Create layer folders with correct ACL
Signed-off-by: John Howard <jhoward@microsoft.com>
LCOW: Don't mount for linux containers either
Signed-off-by: John Howard <jhoward@microsoft.com>
LCOW: Coherency - ensure windowsfilter driver is not used
Signed-off-by: John Howard <jhoward@microsoft.com>
LCOW: Fix schemav1 pull to extract platform
Signed-off-by: John Howard <jhoward@microsoft.com>
LCOW: Rework after 33454 merged which refactored daemon/builder inter鈥
鈥ace

Signed-off-by: John Howard <jhoward@microsoft.com>
Show what cross-build is executing
Signed-off-by: John Howard <jhoward@microsoft.com>
LCOW: Set correct default shell for platform in builder
Signed-off-by: John Howard <jhoward@microsoft.com>

@jhowardmsft jhowardmsft changed the title from LCOW: Pull, import, export, commit, run and some of build to LCOW: Support most operations excluding remote filesystem Jun 21, 2017

@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft

jhowardmsft Jun 21, 2017

Contributor

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

Contributor

jhowardmsft commented Jun 21, 2017

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

@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft

jhowardmsft Jun 21, 2017

Contributor

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

Contributor

jhowardmsft commented Jun 21, 2017

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

@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft

jhowardmsft Jun 21, 2017

Contributor

Yay, green again 馃挌

Contributor

jhowardmsft commented Jun 21, 2017

Yay, green again 馃挌

@crosbymichael

This comment has been minimized.

Show comment
Hide comment
@crosbymichael

crosbymichael Jun 21, 2017

Contributor

LGTM

Contributor

crosbymichael commented Jun 21, 2017

LGTM

@johnstep

LGTM

@@ -17,7 +17,7 @@ import (
// ImageComponent provides an interface for working with images
type ImageComponent interface {
SquashImage(from string, to string) (string, error)
TagImageWithReference(image.ID, reference.Named) error
TagImageWithReference(image.ID, string, reference.Named) error

This comment has been minimized.

@johnstep

johnstep Jun 21, 2017

Contributor

platform string?

@johnstep

johnstep Jun 21, 2017

Contributor

platform string?

This comment has been minimized.

@jhowardmsft

jhowardmsft Jun 26, 2017

Contributor

No, just string is correct :)

@jhowardmsft

jhowardmsft Jun 26, 2017

Contributor

No, just string is correct :)

@@ -136,6 +177,7 @@ func (d *Directive) possibleParserDirective(line string) error {
func NewDefaultDirective() *Directive {
directive := Directive{}
directive.setEscapeToken(string(DefaultEscapeToken))
directive.setPlatformToken(runtime.GOOS)

This comment has been minimized.

@johnstep

johnstep Jun 21, 2017

Contributor

DefaultPlatformToken instead of runtime.GOOS

@johnstep

johnstep Jun 21, 2017

Contributor

DefaultPlatformToken instead of runtime.GOOS

This comment has been minimized.

@jhowardmsft

jhowardmsft Jun 26, 2017

Contributor

Yes, fixing in a nits PR shortly

@jhowardmsft

jhowardmsft Jun 26, 2017

Contributor

Yes, fixing in a nits PR shortly

@@ -1,3 +1,8 @@
package dockerfile
var defaultShell = []string{"cmd", "/S", "/C"}
func defaultShellForPlatform(platform string) []string {
if platform == "linux" {

This comment has been minimized.

@johnstep

johnstep Jun 21, 2017

Contributor

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

@johnstep

johnstep Jun 21, 2017

Contributor

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

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

This comment has been minimized.

@johnstep

johnstep Jun 21, 2017

Contributor

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.

@johnstep

johnstep Jun 21, 2017

Contributor

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.

This comment has been minimized.

@jhowardmsft

jhowardmsft Jun 26, 2017

Contributor

Fixed in nits PR about to be submitted

@jhowardmsft

jhowardmsft Jun 26, 2017

Contributor

Fixed in nits PR about to be submitted

// 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)

This comment has been minimized.

@johnstep

johnstep Jun 21, 2017

Contributor

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

@johnstep

johnstep Jun 21, 2017

Contributor

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

This comment has been minimized.

@jhowardmsft

jhowardmsft Jun 26, 2017

Contributor

Fixed in nits PR about to be submitted

@jhowardmsft

jhowardmsft Jun 26, 2017

Contributor

Fixed in nits PR about to be submitted

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

This comment has been minimized.

@johnstep

johnstep Jun 21, 2017

Contributor

Redundant comment, almost identical to comment 3 lines above.

@johnstep

johnstep Jun 21, 2017

Contributor

Redundant comment, almost identical to comment 3 lines above.

This comment has been minimized.

@jhowardmsft

jhowardmsft Jun 26, 2017

Contributor

Fixed in nits PR about to be submitted

@jhowardmsft

jhowardmsft Jun 26, 2017

Contributor

Fixed in nits PR about to be submitted

@@ -149,8 +153,19 @@ func (d *Driver) Status() [][2]string {
}
}
// panicIfUsedByLcow does exactly what it says.
// TODO @jhowardmsft - this is an temporary measure for the bring-up of

This comment has been minimized.

@johnstep

johnstep Jun 21, 2017

Contributor

Nit: "a temporary"

@johnstep

johnstep Jun 21, 2017

Contributor

Nit: "a temporary"

This comment has been minimized.

@jhowardmsft

jhowardmsft Jun 26, 2017

Contributor

Fixed in nits PR about to be submitted

@jhowardmsft

jhowardmsft Jun 26, 2017

Contributor

Fixed in nits PR about to be submitted

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

This comment has been minimized.

@johnstep

johnstep Jun 21, 2017

Contributor

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

@johnstep

johnstep Jun 21, 2017

Contributor

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

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

This comment has been minimized.

@johnstep

johnstep Jun 21, 2017

Contributor

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

@johnstep

johnstep Jun 21, 2017

Contributor

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

@@ -33,7 +35,12 @@ func NewTagger(backend ImageComponent, stdout io.Writer, names []string) (*Tagge
// TagImages creates image tags for the imageID
func (bt *Tagger) TagImages(imageID image.ID) error {
for _, rt := range bt.repoAndTags {
if err := bt.imageComponent.TagImageWithReference(imageID, rt); err != nil {
// TODO @jhowardmsft LCOW support. Will need revisiting.
platform := runtime.GOOS

This comment has been minimized.

@johnstep

johnstep Jun 21, 2017

Contributor

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.

@johnstep

johnstep Jun 21, 2017

Contributor

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.

@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft

jhowardmsft Jun 21, 2017

Contributor

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

Contributor

jhowardmsft commented Jun 21, 2017

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

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

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 35179 has succeeded
Details
janky Jenkins build Docker-PRs 43788 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 4145 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 15090 has succeeded
Details
z Jenkins build Docker-PRs-s390x 3876 has succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment