Windows: Block pulling uplevel images by lowenna · Pull Request #36327 · moby/moby

lowenna · 2018-02-15T21:20:47Z

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

Fixes #36184. This change gives a decent error message about image incompatibility early, rather than blindly continuing to download the image, try to extract it, and then fail with an extremely misleading error:

Before this change, on an RS1 host, pulling a 1709/RS3 image:

PS E:\go\src\github.com\docker\docker> docker pull microsoft/nanoserver:1709
1709: Pulling from microsoft/nanoserver
407ada6e90de: Extracting [==================================================>]  81.04MB/81.04MB
3cb15ab58a2c: Download complete
failed to register layer: re-exec error: exit status 1: output: ProcessUtilityVMImage C:\control\windowsfilter\21c033dbff11f1084e2ef67c041c491fd04be6aea0eb6d622067f2136b6b48b4\UtilityVM: The system cannot find the path specified.
PS E:\go\src\github.com\docker\docker>

After this change:

PS E:\go\src\github.com\docker\docker> docker pull microsoft/nanoserver:1709
1709: Pulling from microsoft/nanoserver
a Windows version 10.0.16299-based image is incompatible with a 10.0.14393 host
PS E:\go\src\github.com\docker\docker>

@johnstep @darrenstahlmsft @thaJeztah PTAL

darstahl

LGTM with minor nit

darstahl · 2018-02-15T21:32:26Z

distribution/pull_v2_windows.go

Might want to remove this TODO, as we don't want to filter greater versions here anymore as the returned error would be just that there is no matching image in the manifest, rather than an incompatible version.

Done, update pushed.

cpuguy83 · 2018-02-16T21:08:42Z

Seems ok to do this, but I'm not familiar with this part of the codebase.

cpuguy83 · 2018-02-16T21:06:41Z

distribution/config.go

Naming is hard

DecodeConfig and return a struct with said config?

Yes, was also not too happy with four separate values being returned

lowenna · 2018-02-21T22:57:08Z

@thaJeztah @cpuguy83 Went back to the original implementation which was just RootFS and added a new method to get the platform pieces.

lowenna · 2018-02-22T16:57:08Z

@johnstep as discussed offline. PTAL

codecov · 2018-02-22T19:09:16Z

Codecov Report

❗ No coverage uploaded for pull request base (master@00c1c60). Click here to learn what that means.
The diff coverage is 0%.

@@            Coverage Diff            @@
##             master   #36327   +/-   ##
=========================================
  Coverage          ?   34.63%           
=========================================
  Files             ?      614           
  Lines             ?    45526           
  Branches          ?        0           
=========================================
  Hits              ?    15767           
  Misses            ?    27698           
  Partials          ?     2061

Impacted Files	Coverage Δ
plugin/backend_linux.go	`4.16% <0%> (ø)`
distribution/push_v2.go	`25.07% <0%> (ø)`
plugin/blobstore.go	`8.79% <0%> (ø)`
distribution/pull_v2_unix.go	`0% <0%> (ø)`
distribution/pull_v2.go	`10.53% <0%> (ø)`
distribution/config.go	`0% <0%> (ø)`
plugin/manager.go	`25.88% <0%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 00c1c60...8390883. Read the comment docs.

johnstep

LGTM

johnstep · 2018-02-16T19:37:32Z

distribution/pull_v2_windows.go

I'm not sure I like this name; there could be an updated older version that is technically newer. :) Also, I usually think of "is" functions returning a bool, but not sure. What about checkImageCompatibility or something?

Sure, checkImageCompatibility. Update pushed.

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

johnstep · 2018-02-27T05:39:43Z

@thaJeztah @cpuguy83 Will one of you take another look? It looks good to me.

thaJeztah · 2018-02-26T21:42:04Z

distribution/pull_v2_windows.go

+			if imageOSBuild, err := strconv.Atoi(splitImageOSVersion[2]); err == nil {
+				if imageOSBuild > int(hostOSV.Build) {
+					errMsg := fmt.Sprintf("a Windows version %s.%s.%s-based image is incompatible with a %s host", splitImageOSVersion[0], splitImageOSVersion[1], splitImageOSVersion[2], hostOSV.ToString())
+					logrus.Debugf(errMsg)


Isn't this already logged somewhere (as it's also returned as an error?)

thaJeztah

LGTM

dmcgowan

LGTM

Following the addition of Hyper-V isolation, it should be possible for Windows hosts to run container images with older/newer OS versions given a Windows host with the RS5 updates and above. This patch includes the following changes: - [distribution/pull_v2] enable pulling images newer than the host on RS5+ - [daemon/create] add version checks which prevent running newer/older unless using Hyper-V isolation Previously, pulling a Windows image with a newer OS version than the host was prevented by moby#36327, whose behavior is preserved on pre-RS5 hosts. Compatibility requirements were [refernced from Microsoft](https://learn.microsoft.com/en-us/virtualization/windowscontainers/deploy-containers/version-compatibility?tabs=windows-server-2022%2Cwindows-10#windows-server-host-os-compatibility). Signed-off-by: Nashwan Azhari <nazhari@cloudbasesolutions.com>

Following the addition of Hyper-V isolation, it should be possible for Windows hosts to run container images with older/newer OS versions given a Windows host with the RS5 updates and above. This patch includes the following changes: - [distribution/pull_v2] enable pulling images newer than the host on RS5+ - [daemon/create] add version checks which prevent running newer/older unless using Hyper-V isolation Previously, pulling a Windows image with a newer OS version than the host was prevented by moby#36327, whose behavior is preserved on pre-RS5 hosts. Signed-off-by: Nashwan Azhari <nazhari@cloudbasesolutions.com>

Following the addition of Hyper-V isolation, it should be possible for Windows hosts to run container images with older/newer OS versions given a Windows host with the RS5 updates and above. This patch includes the following changes: - [distribution/pull_v2] enable pulling images newer than the host on RS5+ - [daemon/create] add version checks which prevent running newer/older images unless using Hyper-V isolation Previously, pulling a Windows image with a newer OS version than the host was prevented by moby#36327, whose behavior is preserved on pre-RS5 hosts. Signed-off-by: Nashwan Azhari <nazhari@cloudbasesolutions.com>

lowenna requested a review from cpuguy83 as a code owner February 15, 2018 21:20

GordonTheTurtle added the status/0-triage label Feb 15, 2018

lowenna mentioned this pull request Feb 15, 2018

docker pull of image built on 18.02 fails on 17.06 #36184

Closed

darstahl approved these changes Feb 15, 2018

View reviewed changes

lowenna force-pushed the jjh/block-pulling-uplevel branch from 4b9e0ed to 1d67540 Compare February 15, 2018 21:42

vdemeester added status/1-design-review and removed status/0-triage labels Feb 16, 2018

vdemeester requested review from dmcgowan, johnstep, thaJeztah and tonistiigi February 16, 2018 10:02

cpuguy83 reviewed Feb 16, 2018

View reviewed changes

lowenna force-pushed the jjh/block-pulling-uplevel branch from 1d67540 to 8a9a94e Compare February 21, 2018 22:55

lowenna added rebuild/experimental labels Feb 22, 2018

GordonTheTurtle removed rebuild/experimental labels Feb 22, 2018

johnstep approved these changes Feb 26, 2018

View reviewed changes

Windows: Block pulling uplevel images

8390883

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

lowenna added status/2-code-review and removed status/1-design-review labels Feb 26, 2018

lowenna force-pushed the jjh/block-pulling-uplevel branch from 8a9a94e to 8390883 Compare February 26, 2018 20:36

lowenna added the rebuild/experimental label Feb 26, 2018

GordonTheTurtle removed the rebuild/experimental label Feb 26, 2018

This was referenced Feb 27, 2018

LCOW: Autoselect platform #36418

Closed

LCOW - Change platform parser directive to FROM statement flag #35089

Merged

thaJeztah reviewed Feb 27, 2018

View reviewed changes

thaJeztah approved these changes Feb 27, 2018

View reviewed changes

GordonTheTurtle assigned johnstep Mar 2, 2018

dmcgowan approved these changes Mar 5, 2018

View reviewed changes

johnstep merged commit 3e1505e into moby:master Mar 5, 2018

lowenna deleted the jjh/block-pulling-uplevel branch March 8, 2018 17:45

RajeetN mentioned this pull request Mar 26, 2018

There was an error during download.Failed to download container image microsoft/service-fabric-issues#752

Closed

JoshEbersol mentioned this pull request Nov 14, 2018

Windows: unhelpful error message when pulling an up-level, multi-arch image #38199

Closed

iskiselev mentioned this pull request Mar 12, 2020

Docker Desktop, KB4532693 breaks SysWOW64 support in process isolation #40525

Closed

aznashwan mentioned this pull request Nov 17, 2022

windows: daemon: allow pulling/running of newer images under Hyper-V. #44489

Open

Conversation

lowenna commented Feb 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

darstahl left a comment

Choose a reason for hiding this comment

Uh oh!

darstahl Feb 15, 2018 • 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 Feb 15, 2018

Choose a reason for hiding this comment

Uh oh!

cpuguy83 commented Feb 16, 2018

Uh oh!

cpuguy83 Feb 16, 2018

Choose a reason for hiding this comment

Uh oh!

cpuguy83 Feb 16, 2018

Choose a reason for hiding this comment

Uh oh!

thaJeztah Feb 19, 2018

Choose a reason for hiding this comment

Uh oh!

lowenna commented Feb 21, 2018

Uh oh!

lowenna commented Feb 22, 2018

Uh oh!

codecov bot commented Feb 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

johnstep left a comment

Choose a reason for hiding this comment

Uh oh!

johnstep Feb 16, 2018

Choose a reason for hiding this comment

Uh oh!

lowenna Feb 26, 2018

Choose a reason for hiding this comment

Uh oh!

johnstep commented Feb 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah Feb 26, 2018

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

dmcgowan left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

lowenna commented Feb 15, 2018 •

edited

Loading

darstahl Feb 15, 2018 •

edited

Loading

codecov bot commented Feb 22, 2018 •

edited

Loading

johnstep commented Feb 27, 2018 •

edited

Loading