Use hcsshim osversion package for version checks, instead of docker/docker/pkg/system#2362
Use hcsshim osversion package for version checks, instead of docker/docker/pkg/system#2362thaJeztah wants to merge 1 commit intomoby:masterfrom
Conversation
|
|
||
| func endpointRequest(method, path, request string) (*hcsshim.HNSEndpoint, error) { | ||
| if windowsBuild == 14393 { | ||
| if windowsBuild == osversion.RS1 { |
There was a problem hiding this comment.
Also wondering if this should be
| if windowsBuild == osversion.RS1 { | |
| if windowsBuild < osversion.RS3 { |
(assuming RS2 is EOL / obsolete)
There was a problem hiding this comment.
Either is fine since RS2 is obsolete.
I have a slight preference to "windowsBuild == osversion.RS1" since that is what we investigated and validated.
There was a problem hiding this comment.
Per the discussion on microsoft/hcsshim#556 (comment), this should likely be more defensive and take anything < RS5
| if windowsBuild == osversion.RS1 { | |
| if windowsBuild < osversion.RS5 { |
| } | ||
| hnsresponse, err := hcsshim.HNSEndpointRequest(method, path, request) | ||
| if windowsBuild == 14393 { | ||
| if windowsBuild == osversion.RS1 { |
There was a problem hiding this comment.
Same here;
| if windowsBuild == osversion.RS1 { | |
| if windowsBuild < osversion.RS3 { |
There was a problem hiding this comment.
This should match what you do above.
So:
if windowsBuild < osversion.RS5 {
| for _, elem := range qosPolicies { | ||
| encodedPolicy, err := json.Marshal(hcsshim.QosPolicy{ | ||
| Type: "QOS", | ||
|
|
There was a problem hiding this comment.
This is a (dirty) workaround for gofmt changes between Go 1.10 and Go 1.11 and up. Adding this blank line makes it produce the same formatting on both versions (thus preventing CI linting failures if it checks for gofmt)
| hnsEndpoint.Policies = append(hnsEndpoint.Policies, paPolicy) | ||
|
|
||
| if osversion.Get().Build > 16236 { | ||
| if osversion.Get().Build >= osversion.RS3 { |
There was a problem hiding this comment.
@pradipd @nwoodmsft please double-check if this is OK (see my top comment); I suspect 16236 referred to RS2, which is not included in the constants (perhaps it should, even if it wasn't a Windows Server build?)
There's some more like these below
There was a problem hiding this comment.
I think ^ should be safe. RS3 EOL is right around the corner too, so even Build > osversion.RS3 should be okay in terms of docker engine support.
There was a problem hiding this comment.
Yes. that is correct. Thanks!
thaJeztah
force-pushed
the
improve_windows_version_handling
branch
from
4b5ac00 to
0ad66db
Compare
|
ping @pradipd @mavenugo @ddebroy (#2070 (comment)) PTAL |
thaJeztah
force-pushed
the
improve_windows_version_handling
branch
2 times, most recently
from
1f34543 to
e8cb470
Compare
|
force-pushed to trigger CI |
thaJeztah
force-pushed
the
improve_windows_version_handling
branch
from
e8cb470 to
eb73a24
Compare
|
updated to use |
|
Failure is a flaky test; |
thaJeztah
force-pushed
the
improve_windows_version_handling
branch
from
eb73a24 to
b87dbc4
Compare
thaJeztah
force-pushed
the
improve_windows_version_handling
branch
2 times, most recently
from
18db6ab to
315d4ab
Compare
|
LGTM |
|
we might have to hold off merging this one, pending the discussion on microsoft/hcsshim#581 (comment) |
thaJeztah
force-pushed
the
improve_windows_version_handling
branch
from
315d4ab to
386403d
Compare
|
Looks like microsoft/hcsshim#581 (comment) was resolved, so I rebased this; this should be ready to go now |
This replaces the hard-coded build numbers with constants that are now defined in hcsshim. Note that build 16236 is not defined in the package, and may be referring to RS2 (which was a client-only release). Checks for this version were added in commits; a9db5c5 (moby#1900), and 6681c02 (moby#2070) In a9db5c5, it is described that the intent was to allow these options only in Windows RS3 so I changed the comparisons to match RS3 (and up), or disable the option on anything older than RS3. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
thaJeztah
force-pushed
the
improve_windows_version_handling
branch
from
386403d to
b000a74
Compare
|
Obsoleted by moby/moby#43254 |
5 participants
This depends on #2361, which brings in an up-to-date version of hcsshim.This replaces the use of docker/docker/pkg/system, in favour of the same code from Microsoft/hcsshim.
With this package, hard-coded build numbers are replaced with constants that are now defined in hcsshim.
Note that build 16236 is not defined in the package, and may be referring to RS2 (which was a client-only release).
Checks for that version were added in commits;
a9db5c5 (#1900), and 6681c02 (#2070)
In a9db5c5, it is described that the intent was to allow these options only in Windows RS3 so I changed the comparisons to match RS3 (and up) or disable the option on anything older than RS3.