Repository navigation
Implement generic variant handling - #35
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors platform variant compatibility and ordering to use generic parsing/comparison logic (instead of generating explicit variant vectors), adds generic “natural” ordering for variants/OS versions, and consolidates Windows OS version handling into the main matching path.
Changes:
- Added a generic variant/version parser and comparison utilities, plus architecture-specific matching helpers (amd64/arm floors, arm64 v8↔v9 offset, generic “prefix+number+suffix” schemes).
- Reworked
Onlyto compute compatibility directly (Match) and sort independently (Less), avoiding unbounded vector generation for some variant schemes. - Folded Windows OS version matching into shared helpers and simplified OS feature subset logic (including
win32kfeature stripping behavior).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| variant.go | Introduces generic parsing/comparison utilities and matching helpers for versioned variants across architectures. |
| variant_test.go | Adds unit tests for variant parsing, matching helpers, and natural ordering. |
| platforms.go | Updates matcher behavior: factors OS version + OS feature subset logic into shared helpers and adjusts Windows matcher wrapping. |
| platform_windows_compat.go | Refactors Windows OS version parsing/matching to reuse the new generic parser. |
| defaults_test.go | Updates Windows MatchComparer test to explicitly wrap NewMatcher now that it returns only Matcher. |
| compare.go | Reimplements Only matching/sorting with direct compatibility checks and a natural sort-based ranking model. |
| compare_test.go | Expands test coverage for new arm64 arithmetic matching, generic variants (ppc64le/riscv64), and Only-specific ordering differences. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
harryzcy
left a comment
harryzcy
left a comment
There was a problem hiding this comment.
This is the great change overall
| // sameShapeNumbers parses hostVariant and imageVariant and, if they share | ||
| // the same prefix and suffix (differing only in their numbers), returns | ||
| // both their numeric components for comparison. | ||
| func sameShapeNumbers(hostVariant, imageVariant string) (hostNumbers, imageNumbers []int, ok bool) { |
There was a problem hiding this comment.
What's the behavior of variant with and without v prefix? are 8.3 and v8.3 treated differently? or should they get treated the same? I can see possibilities of user input error here.
It would also make sense to mandate certain pattern, like v is needed for arm64/* type. But the pattern may be different for different architecture types.
There was a problem hiding this comment.
Per the Platform Variants table, arm64 values are always v-prefixed (v8, v8.1, ...) -- 8.3 isn't a spec-conformant value to begin with, so no normalization is done: arm64VariantMatch requires both sides to parse with prefix v, so an unprefixed variant only matches itself verbatim (via the early exact-string check) and fails everything else. Fails closed rather than guessing at intent.
|
Updated per review feedback 🙇 ❤️ |
| case "arm64": | ||
| return arm64VariantMatch(c.platform.Variant, normalized.Variant) | ||
| default: | ||
| return genericVariantMatch(c.platform.Variant, normalized.Variant) |
There was a problem hiding this comment.
Would it make sense to make the switch exhaustive; or at least for known arches? OCI refers to GOARCH as a well-defined list, which we could use for this switch; https://go.dev/doc/install/source#environment
And for unknown arches, don't apply any heuristics, other than exact match?
There was a problem hiding this comment.
IMO we don't gain anything from an "exhaustive" switch here other than maintenance burden -- in my opinion, the generic matching is exactly the point of this PR, and I can't imagine a case where it's not the right answer for something we're not familiar with yet (for example, when s390x finally matures and we get a GOS390 or GOS390X variable, this should "just work" not require a PR to containerd to "fix").
There was a problem hiding this comment.
Honestly, even the v1 and v5 special cases above could just be removed -- they're not doing much more than genericVariantMatch anyways. The only reason arm64 has to be special is because it has special rules of compatibility that don't really have anything similar elsewhere (unless we wanted to somehow support the plethora of x86 features individually, but I think that'd be a mistake).
There was a problem hiding this comment.
If that level of simplification is interesting, you can see the change in b0a0a90, but my goal here wasn't actually to rock the boat that far unless maintainers think that's interesting / useful in order to simplify the code slightly more. 😄
(it's a very minor breaking change in that a linux/arm/v7 host would then consider running a linux/arm/v4 image, but that's not wrong -- just not something we've traditionally done, and I doubt there are very many <v5 images that even exist, let alone even v5)
|
@tianon @tianon-sso I opened a PR with my suggestion (for preparing for the modern |
225db75 to
1ed1b4b
Compare
|
Merged Seb's PR/idea in now 🙇 |
d628d91 to
4f82422
Compare
|
(test coverage for the whole package before this change was ~78.5% and after this change it's up to ~83.7% 😄) |
|
@dmcgowan @AkihiroSuda this one good to go? After this we may have some follow ups |
940d627 to
d1b9bf3
Compare
thaJeztah
left a comment
thaJeztah
left a comment
There was a problem hiding this comment.
still LGTM
@AkihiroSuda @dmcgowan PTAL 🤗
|
Also re-requested a review from CoPilot in case it has anything useful (but it may get into "nag" mode now 😂) |
My old variant handling was creating explicit vector arrays, which doesn't scale well (algorithmically or maintainably). This adjusts the logic to do more direct comparisons and handle "satisfiability" separate from "preferred order" which cleans up a lot of the comparisons. This also makes it trivial to write a generic means of comparing variants that works for `v4` vs `v3` the same as `power10` vs `power8` and `rva23u64` vs `rva20u64`. As part of this work, it also folds in all the Windows-specific logic into a single matcher (because that's just standard platform matching logic that's naturally keyed off `os`, not a completely separate matcher). It also genericizes the arm64 variant matching to the same vX.Y -> vX-1.Y+5 logic that Go uses. Additionally, this adds tests that covers more edge cases, like making sure that Windows is never preferred over Linux for a Linux host. As a final touch, it uses a `compare`-style interface underneath (-1, 0, 1) so that the new stdlib `cmp` package's expectations can be satisfied without much work sometime in the future. Assisted-By: "claude my eyes right out" Signed-off-by: Tianon Gravi <tianon.gravi@docker.com> Co-authored-by: Sebastiaan van Stijn <github@gone.nl>
|
(rebased) |
My old variant handling was creating explicit vector arrays, which doesn't scale well (algorithmically or maintainably).
This adjusts the logic to do more direct comparisons and handle "satisfiability" separate from "preferred order" which cleans up a lot of the comparisons.
This also makes it trivial to write a generic means of comparing variants that works for
v4vsv3the same aspower10vspower8andrva23u64vsrva20u64.As part of this work, it also folds in all the Windows-specific logic into a single matcher (because that's just standard platform matching logic that's naturally keyed off
os, not a completely separate matcher).It also genericizes the arm64 variant matching to the same vX.Y -> vX-1.Y+5 logic that Go uses.
See also https://github.com/opencontainers/image-spec/blob/v1.1.1/image-index.md#platform-variants
Assisted-By: "claude my eyes right out"