image/cache: Ignore Build and Revision on Windows#47330
image/cache: Ignore Build and Revision on Windows#47330thaJeztah merged 1 commit intomoby:masterfrom
Conversation
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 <pawel.gronowski@docker.com>
| if builderPlatform.OS == "windows" && imagePlatform.OS == builderPlatform.OS { | ||
| // OSVersion format is: | ||
| // Major.Minor.Build.Revision | ||
| builderParts := strings.Split(builderPlatform.OSVersion, ".") |
There was a problem hiding this comment.
Interesting; I thought https://github.com/microsoft/hcsshim/tree/main/osversion would have a utility for this, but it doesn't. Perhaps we should contribute one (not urgent for sure!)
Curious; do we know what isolation (hyper-v / host) is used, and if so, if we can ignore os-version altogether?
There was a problem hiding this comment.
There's one in containerd/platforms: https://github.com/containerd/containerd/blob/88fd474e4c0a3dd3865dfcbf0ca99fff12f5c7b7/platforms/defaults_windows.go#L67-L82
but it's in _windows.go file so we'd have to split the implementation into Windows and non-Windows, which would make it only testable on Windows and IMO that's too much fuss for a glorified strings.Split call 😅
There was a problem hiding this comment.
In general hyperv is the default, but even then, I'm not sure if it's okay to ignore Major/Minor versions. AFAIK there are no other images than 10.0 as of now, so can't test it.
We can always remove that check later, while the reverse might be more problematic.
There was a problem hiding this comment.
but it's in _windows.go file so we'd have to split the implementation into Windows and non-Windows,
Yeah, perhaps we should have a look at that, and discuss with the Microsoft maintainers. Agreed that it's mostly a strings.Split, but ideally there would be a (cross-platform) utility here. There's been too many oh-so-subtle things over the Years in this area, which really wants me to have a canonical source of truth for handling anything in this area.
In general hyperv is the default
On moby/moby it is. I think for containerd it may be the reverse (which should likely not be an issue for this PR, but something we need to keep in the back of our head).
I honestly don't know the compatibility matrix for Hyper-V. ISTR there were some constraints there as well (which could even be "lower versions, or compatible only"), but ... nope... really don't know.
We can always remove that check later, while the reverse might be more problematic.
Yes, agreed; not a blocker for me!
There was a problem hiding this comment.
Opened a ticket in hcsshim for further discussion;
thaJeztah
left a comment
There was a problem hiding this comment.
LGTM
(sorry, thought I already LGTM'd 🙈)
The compatibility depends on whether
hypervorprocesscontainer isolation is used.This fixes cache not being used when building images based on older Windows versions on a newer Windows host.
- How to verify it
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)