-
Notifications
You must be signed in to change notification settings - Fork 175
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Un-hardcode list of Windows versions #166
Un-hardcode list of Windows versions #166
Conversation
@TBBle I only just realised that there's lower |
a8f4813
to
64ad6d3
Compare
Just wanted to say, I'm aware of this, but haven't been through it yet (it's a long patch). One thing that jumps out from the description: why can't we check if the container version is newer than the host OS? It should be an easier check now, since if we're working with Windows build versions, it's a straight numerical comparison, without any complications for "ltscXXXX" or "XXHY" strings. At worst, if given a build tag we don't recognise and that doesn't match the local system build version or release ID, we can bypass the test with a warning, assuming the user knows what they're doing and we are just late in an update. I'd also suggest we keep rejecting the known Windows suffixes, as it's a small list that only grows by one every six months or so, but perhaps only those that are actually still supported upstream. If useful, we could parse them out of https://mcr.microsoft.com/v2/windows/servercore/tags/list as anything in that list without Side-note: I think it'd be better to not name the bug being solved in the name of the PR, since it doesn't create the right links (i.e. this PR is linked to #138, but not #164). Edit: Looking at the description again, I realise that maybe this PR doesn't do what I expected it to do, so I'll have to actually read it before I can comment further. The description doesn't match what I had in my mind based on #138 (comment) |
We can't do that because we still refer to images using release id (1809, 20H2 and so on). If we started referring to images by their build number, you're right, it would be easy to compare them.
Yep, we can do that.
True, I did a bit different thing here. I am not doing a buildnumber -> release id lookup anywhere. Instead I ask the host system for its release id and use that to lookup image (the same way as asciidoc |
Motivation: ReleaseId is confusing. In Windows 20H2, ReleaseId is 2009 (so it doesn't match human-visible version). Even worse, ReleaseId is *also* 2009 in Windows 21H2 and we actually need to use DisplayName to distinguish between the two. Depending on how we proceed with fixing adamrehn#138 and adamrehn#166, we might completely drop ReleaseId and only operate on build numbers. This commit is not expected to change ue4-docker behavior in any way (possibly except for 21H1 that is not supported yet anyway).
I've split part of this PR into #170 so current PR becomes smaller and easier to review. |
Motivation: ReleaseId is confusing. In Windows 20H2, ReleaseId is 2009 (so it doesn't match human-visible version). Even worse, ReleaseId is *also* 2009 in Windows 21H2 and we actually need to use DisplayName to distinguish between the two. Depending on how we proceed with fixing adamrehn#138 and adamrehn#166, we might completely drop ReleaseId and only operate on build numbers. This commit is not expected to change ue4-docker behavior in any way (possibly except for 21H1 that is not supported yet anyway).
Motivation: ReleaseId is confusing. In Windows 20H2, ReleaseId is 2009 (so it doesn't match human-visible version). Even worse, ReleaseId is *also* 2009 in Windows 21H2 and we actually need to use DisplayName to distinguish between the two. Depending on how we proceed with fixing #138 and #166, we might completely drop ReleaseId and only operate on build numbers. This commit is not expected to change ue4-docker behavior in any way (possibly except for 21H1 that is not supported yet anyway).
098b5ea
to
99a5e30
Compare
Partially resolves adamrehn#138 (support for insider base image is still missing) This commit: 1. Drops the list of valid Windows versions. Instead, ue4-docker now trusts user input and will fail during image download if user enters something nonexistent 2. Drops 1603->ltsc2016, 1809->ltsc2019 and 2009->20H2 renaming. Instead, ue4-docker directly maps host OS release to Windows Server Core image tag 3. However, ue4-docker now uses advanced logic to determine host OS release. It tries to use DisplayName registry key and fallbacks to ReleaseId if DisplayName doesn't exist. This allows to properly detect 20H2 and 21H1 releases. On the negative side, some checks are lost: 1. It is no longer possible to check that container version is newer that host OS. Though is should still be rejected by Hyper-V 2. ue4-docker no longer prevents user from using suffix that collides with Windows Server Core image tags
99a5e30
to
63e911e
Compare
Okay, I've added reading tags list from https://mcr.microsoft.com/v2/windows/servercore/tags/list. But we still can't check whether target tag is newer than host system because https://mcr.microsoft.com/v2/windows/servercore/tags/list is not sorted chronologically. |
I'm not sure why you'd want them to be pre-sorted? We're only comparing host BuildID against the We know that the host BuildID either matches an X from the list (14300, 14393, 16299, 17134, 17763, 18362, 18363, 19041, 19042), aka (Windows Server 2016 Tech Preview 5, ltsc2016/1607, 1709, 1803, ltsc2019/1809, 1903, 1909, 2004, 20H2); or it's between those numbers and hence an out-of-date Insider build, or it's greater than the largest, in which case it's a current Insider build (or if it's 19043, then it's 21H1 and we're still waiting for an image to go with it, so we can pretend that's "Insider" for our purposes). I suspect the only reason Later, we can look at automatically attempting to find such "Insider" images on https://mcr.microsoft.com/v2/windows/servercore/insider/tags/list. Annoyingly, that won't work right now for Windows Server 2022 Preview on the MS Evaluation centre, as that's 10.0.20348.1, and there's no Insider container image base for that yet. It seems to take a week or so for Server Insider container images to appear after a Server Insider release, and that's out-of-scope for this work anyway. |
We need to solve two tasks:
|
On 1, we can do that with the mapping of On 2, the tag format is pretty fixed, or at least it has been for the last half-decade or so. It's either We don't care about the Sorting/ordering never matters because we can't take an inequality match on And if we want to know something is newer than the newest known version, we can always go from If we're really concerned that MS are going to change their tagging format to celebrate the second half-decade, we can grab the manifest for the named image, and check what OS version it claims. It's about a 30-line JSON file, in a fixed format, so that's not exactly a huge challenge. But that still seems like way more effort to implement and maintain than updating a table mapping tags and build IDs every six months or so (plus once every three years for an LTSC release). This latter approach might be useful for supporting both Insider and custom base images, though, so long-term perhaps it's worth implementing, or finding a library that implements it already. |
Okay, I've tried going that route, but I don't believe in it. Too much hassle with all these regexes, reverse lookups and very small benefits. We're effectively trying to reproduce Hyper-V logic that determines what images are compatible with what host OSes instead of allowing Hyper-V to do its job and fail with a loud error message if user tries to do something inappropriate. I already have 4x amount of changes compared to current PR and it's still not finished. I'm going to close this PR. |
You can take a look at what I did so far: slonopotamus@ea77722 |
Ordering matters because we have three scenarios:
Now, the question is: how to compare host version with target image tag. Currently, ue4-docker just has a hardcoded ordered list of well-known versions, uses it to determine order and completely rejects unknown tags. That doesn't scale if we want to allow insider tags, so we need to drop such logic. Now, what we have is build number of the host (good thing) and unordered list of tags from Docker registry (bad thing). We don't know which build number is inside those tags. We could try to attack this problem with regexes + well-known list, but usually regexes only increase number of problems we have. Now, let's step back and remember why we are doing all this stuff. We're doing it only to print error message that user tries to use Hyper-V in an incompatible way quicker than Hyper-V itself. Hyper-V would do that build number check anyway when ue4-docker tried to run the image. Is it worth the effort? I don't think so. |
Fully resolves #164.
Partially resolves #138 (support for insider base image is still missing)
This commit:
On the negative side, some checks are lost:
I'm marking this PR as draft because I haven't actually tested this yet and because I'd like to get some feedback before moving further. @tbbie, @adamrehn, do you have any comments?