From 347be7f928da6332cbd1d5017817639d11c68da4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Gronowski?= Date: Mon, 5 Feb 2024 17:38:00 +0100 Subject: [PATCH 1/3] image/cache: Ignore Build and Revision on Windows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The compatibility depends on whether `hyperv` or `process` container isolation is used. This fixes cache not being used when building images based on older Windows versions on a newer Windows host. Signed-off-by: Paweł Gronowski (cherry picked from commit 91ea04089b75f3c56e60e33e27daeb0d494d79f5) Signed-off-by: Paweł Gronowski --- image/cache/cache.go | 4 +- image/cache/compare.go | 27 +++++++++++++ image/cache/compare_test.go | 80 +++++++++++++++++++++++++++++++++++++ 3 files changed, 109 insertions(+), 2 deletions(-) diff --git a/image/cache/cache.go b/image/cache/cache.go index ee89a171e2958..b4f551a016aa4 100644 --- a/image/cache/cache.go +++ b/image/cache/cache.go @@ -6,7 +6,6 @@ import ( "reflect" "strings" - "github.com/containerd/containerd/platforms" containertypes "github.com/docker/docker/api/types/container" "github.com/docker/docker/dockerversion" "github.com/docker/docker/image" @@ -255,11 +254,12 @@ func getLocalCachedImage(imageStore image.Store, imgID image.ID, config *contain OSFeatures: img.OSFeatures, Variant: img.Variant, } + // Discard old linux/amd64 images with empty platform. if imgPlatform.OS == "" && imgPlatform.Architecture == "" { continue } - if !platforms.OnlyStrict(platform).Match(imgPlatform) { + if !comparePlatform(platform, imgPlatform) { continue } diff --git a/image/cache/compare.go b/image/cache/compare.go index d438b65be72e8..c13b11a06d8c4 100644 --- a/image/cache/compare.go +++ b/image/cache/compare.go @@ -1,7 +1,11 @@ package cache // import "github.com/docker/docker/image/cache" import ( + "strings" + + "github.com/containerd/containerd/platforms" "github.com/docker/docker/api/types/container" + ocispec "github.com/opencontainers/image-spec/specs-go/v1" ) // TODO: Remove once containerd image service directly uses the ImageCache and @@ -10,6 +14,29 @@ func CompareConfig(a, b *container.Config) bool { return compare(a, b) } +func comparePlatform(builderPlatform, imagePlatform ocispec.Platform) bool { + // On Windows, only check the Major and Minor versions. + // The Build and Revision compatibility depends on whether `process` or + // `hyperv` isolation used. + // + // Fixes https://github.com/moby/moby/issues/47307 + if builderPlatform.OS == "windows" && imagePlatform.OS == builderPlatform.OS { + // OSVersion format is: + // Major.Minor.Build.Revision + builderParts := strings.Split(builderPlatform.OSVersion, ".") + imageParts := strings.Split(imagePlatform.OSVersion, ".") + + if len(builderParts) >= 3 && len(imageParts) >= 3 { + // Keep only Major & Minor. + builderParts[0] = imageParts[0] + builderParts[1] = imageParts[1] + imagePlatform.OSVersion = strings.Join(builderParts, ".") + } + } + + return platforms.Only(builderPlatform).Match(imagePlatform) +} + // compare two Config struct. Do not container-specific fields: // - Image // - Hostname diff --git a/image/cache/compare_test.go b/image/cache/compare_test.go index 939e99f050706..0ed163f91c806 100644 --- a/image/cache/compare_test.go +++ b/image/cache/compare_test.go @@ -1,11 +1,15 @@ package cache // import "github.com/docker/docker/image/cache" import ( + "runtime" "testing" + "github.com/containerd/containerd/platforms" "github.com/docker/docker/api/types/container" "github.com/docker/docker/api/types/strslice" "github.com/docker/go-connections/nat" + "gotest.tools/v3/assert" + is "gotest.tools/v3/assert/cmp" ) // Just to make life easier @@ -124,3 +128,79 @@ func TestCompare(t *testing.T) { } } } + +func TestPlatformCompare(t *testing.T) { + for _, tc := range []struct { + name string + builder platforms.Platform + image platforms.Platform + expected bool + }{ + { + name: "same os and arch", + builder: platforms.Platform{Architecture: "amd64", OS: runtime.GOOS}, + image: platforms.Platform{Architecture: "amd64", OS: runtime.GOOS}, + expected: true, + }, + { + name: "same os different arch", + builder: platforms.Platform{Architecture: "amd64", OS: runtime.GOOS}, + image: platforms.Platform{Architecture: "arm64", OS: runtime.GOOS}, + expected: false, + }, + { + name: "same os smaller host variant", + builder: platforms.Platform{Variant: "v7", Architecture: "arm", OS: runtime.GOOS}, + image: platforms.Platform{Variant: "v8", Architecture: "arm", OS: runtime.GOOS}, + expected: false, + }, + { + name: "same os higher host variant", + builder: platforms.Platform{Variant: "v8", Architecture: "arm", OS: runtime.GOOS}, + image: platforms.Platform{Variant: "v7", Architecture: "arm", OS: runtime.GOOS}, + expected: true, + }, + { + // Test for https://github.com/moby/moby/issues/47307 + name: "different build and revision", + builder: platforms.Platform{Architecture: "amd64", OS: "windows", OSVersion: "10.0.22621"}, + image: platforms.Platform{Architecture: "amd64", OS: "windows", OSVersion: "10.0.17763.5329"}, + expected: true, + }, + { + name: "different revision", + builder: platforms.Platform{Architecture: "amd64", OS: "windows", OSVersion: "10.0.17763.1234"}, + image: platforms.Platform{Architecture: "amd64", OS: "windows", OSVersion: "10.0.17763.5329"}, + expected: true, + }, + { + name: "different major", + builder: platforms.Platform{Architecture: "amd64", OS: "windows", OSVersion: "11.0.17763.5329"}, + image: platforms.Platform{Architecture: "amd64", OS: "windows", OSVersion: "10.0.17763.5329"}, + expected: false, + }, + { + name: "different minor same osver", + builder: platforms.Platform{Architecture: "amd64", OS: "windows", OSVersion: "10.0.17763.5329"}, + image: platforms.Platform{Architecture: "amd64", OS: "windows", OSVersion: "10.1.17763.5329"}, + expected: false, + }, + { + name: "different arch same osver", + builder: platforms.Platform{Architecture: "arm64", OS: "windows", OSVersion: "10.0.17763.5329"}, + image: platforms.Platform{Architecture: "amd64", OS: "windows", OSVersion: "10.0.17763.5329"}, + expected: false, + }, + } { + tc := tc + // OSVersion comparison is only performed by containerd platform + // matcher if built on Windows. + if (tc.image.OSVersion != "" || tc.builder.OSVersion != "") && runtime.GOOS != "windows" { + continue + } + + t.Run(tc.name, func(t *testing.T) { + assert.Check(t, is.Equal(comparePlatform(tc.builder, tc.image), tc.expected)) + }) + } +} From a51e65b33450f6f4f65ce3e9641db5eedefdd0a2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Gronowski?= Date: Tue, 6 Feb 2024 14:25:47 +0100 Subject: [PATCH 2/3] image/cache: Use Platform from ocispec MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Paweł Gronowski (cherry picked from commit 2c01d53d9692250a7d1b18742f1e100e43615586) Signed-off-by: Paweł Gronowski --- image/cache/compare_test.go | 42 ++++++++++++++++++------------------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/image/cache/compare_test.go b/image/cache/compare_test.go index 0ed163f91c806..8d6ce735e2a6a 100644 --- a/image/cache/compare_test.go +++ b/image/cache/compare_test.go @@ -4,10 +4,10 @@ import ( "runtime" "testing" - "github.com/containerd/containerd/platforms" "github.com/docker/docker/api/types/container" "github.com/docker/docker/api/types/strslice" "github.com/docker/go-connections/nat" + ocispec "github.com/opencontainers/image-spec/specs-go/v1" "gotest.tools/v3/assert" is "gotest.tools/v3/assert/cmp" ) @@ -132,63 +132,63 @@ func TestCompare(t *testing.T) { func TestPlatformCompare(t *testing.T) { for _, tc := range []struct { name string - builder platforms.Platform - image platforms.Platform + builder ocispec.Platform + image ocispec.Platform expected bool }{ { name: "same os and arch", - builder: platforms.Platform{Architecture: "amd64", OS: runtime.GOOS}, - image: platforms.Platform{Architecture: "amd64", OS: runtime.GOOS}, + builder: ocispec.Platform{Architecture: "amd64", OS: runtime.GOOS}, + image: ocispec.Platform{Architecture: "amd64", OS: runtime.GOOS}, expected: true, }, { name: "same os different arch", - builder: platforms.Platform{Architecture: "amd64", OS: runtime.GOOS}, - image: platforms.Platform{Architecture: "arm64", OS: runtime.GOOS}, + builder: ocispec.Platform{Architecture: "amd64", OS: runtime.GOOS}, + image: ocispec.Platform{Architecture: "arm64", OS: runtime.GOOS}, expected: false, }, { name: "same os smaller host variant", - builder: platforms.Platform{Variant: "v7", Architecture: "arm", OS: runtime.GOOS}, - image: platforms.Platform{Variant: "v8", Architecture: "arm", OS: runtime.GOOS}, + builder: ocispec.Platform{Variant: "v7", Architecture: "arm", OS: runtime.GOOS}, + image: ocispec.Platform{Variant: "v8", Architecture: "arm", OS: runtime.GOOS}, expected: false, }, { name: "same os higher host variant", - builder: platforms.Platform{Variant: "v8", Architecture: "arm", OS: runtime.GOOS}, - image: platforms.Platform{Variant: "v7", Architecture: "arm", OS: runtime.GOOS}, + builder: ocispec.Platform{Variant: "v8", Architecture: "arm", OS: runtime.GOOS}, + image: ocispec.Platform{Variant: "v7", Architecture: "arm", OS: runtime.GOOS}, expected: true, }, { // Test for https://github.com/moby/moby/issues/47307 name: "different build and revision", - builder: platforms.Platform{Architecture: "amd64", OS: "windows", OSVersion: "10.0.22621"}, - image: platforms.Platform{Architecture: "amd64", OS: "windows", OSVersion: "10.0.17763.5329"}, + builder: ocispec.Platform{Architecture: "amd64", OS: "windows", OSVersion: "10.0.22621"}, + image: ocispec.Platform{Architecture: "amd64", OS: "windows", OSVersion: "10.0.17763.5329"}, expected: true, }, { name: "different revision", - builder: platforms.Platform{Architecture: "amd64", OS: "windows", OSVersion: "10.0.17763.1234"}, - image: platforms.Platform{Architecture: "amd64", OS: "windows", OSVersion: "10.0.17763.5329"}, + builder: ocispec.Platform{Architecture: "amd64", OS: "windows", OSVersion: "10.0.17763.1234"}, + image: ocispec.Platform{Architecture: "amd64", OS: "windows", OSVersion: "10.0.17763.5329"}, expected: true, }, { name: "different major", - builder: platforms.Platform{Architecture: "amd64", OS: "windows", OSVersion: "11.0.17763.5329"}, - image: platforms.Platform{Architecture: "amd64", OS: "windows", OSVersion: "10.0.17763.5329"}, + builder: ocispec.Platform{Architecture: "amd64", OS: "windows", OSVersion: "11.0.17763.5329"}, + image: ocispec.Platform{Architecture: "amd64", OS: "windows", OSVersion: "10.0.17763.5329"}, expected: false, }, { name: "different minor same osver", - builder: platforms.Platform{Architecture: "amd64", OS: "windows", OSVersion: "10.0.17763.5329"}, - image: platforms.Platform{Architecture: "amd64", OS: "windows", OSVersion: "10.1.17763.5329"}, + builder: ocispec.Platform{Architecture: "amd64", OS: "windows", OSVersion: "10.0.17763.5329"}, + image: ocispec.Platform{Architecture: "amd64", OS: "windows", OSVersion: "10.1.17763.5329"}, expected: false, }, { name: "different arch same osver", - builder: platforms.Platform{Architecture: "arm64", OS: "windows", OSVersion: "10.0.17763.5329"}, - image: platforms.Platform{Architecture: "amd64", OS: "windows", OSVersion: "10.0.17763.5329"}, + builder: ocispec.Platform{Architecture: "arm64", OS: "windows", OSVersion: "10.0.17763.5329"}, + image: ocispec.Platform{Architecture: "amd64", OS: "windows", OSVersion: "10.0.17763.5329"}, expected: false, }, } { From d10756f356a4c67c6ed933692a17b66ff605d36f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Gronowski?= Date: Tue, 6 Feb 2024 16:10:23 +0100 Subject: [PATCH 3/3] image/cache: Require Major and Minor match for Windows OSVersion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The platform comparison was backported from the branch that vendors containerd 1.7. In this branch the vendored containerd version is older and doesn't have the same comparison logic for Windows specific OSVersion. Require both major and minor components of Windows OSVersion to match. Signed-off-by: Paweł Gronowski (cherry picked from commit b3888ed89951e568c1bf33b5223e3e515019e87f) Signed-off-by: Paweł Gronowski --- image/cache/compare.go | 7 +++++++ image/cache/compare_test.go | 6 ------ 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/image/cache/compare.go b/image/cache/compare.go index c13b11a06d8c4..7633b0e966403 100644 --- a/image/cache/compare.go +++ b/image/cache/compare.go @@ -26,6 +26,13 @@ func comparePlatform(builderPlatform, imagePlatform ocispec.Platform) bool { builderParts := strings.Split(builderPlatform.OSVersion, ".") imageParts := strings.Split(imagePlatform.OSVersion, ".") + // Major and minor must match. + for i := 0; i < 2; i++ { + if len(builderParts) > i && len(imageParts) > i && builderParts[i] != imageParts[i] { + return false + } + } + if len(builderParts) >= 3 && len(imageParts) >= 3 { // Keep only Major & Minor. builderParts[0] = imageParts[0] diff --git a/image/cache/compare_test.go b/image/cache/compare_test.go index 8d6ce735e2a6a..7cffbdad1bf27 100644 --- a/image/cache/compare_test.go +++ b/image/cache/compare_test.go @@ -193,12 +193,6 @@ func TestPlatformCompare(t *testing.T) { }, } { tc := tc - // OSVersion comparison is only performed by containerd platform - // matcher if built on Windows. - if (tc.image.OSVersion != "" || tc.builder.OSVersion != "") && runtime.GOOS != "windows" { - continue - } - t.Run(tc.name, func(t *testing.T) { assert.Check(t, is.Equal(comparePlatform(tc.builder, tc.image), tc.expected)) })