-
Notifications
You must be signed in to change notification settings - Fork 256
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
questions w.r.t. "osversion" package #2018
Comments
@thaJeztah @vvoland I am not sure we want to have partial cross-compat functions in hccshim. Not all OS-es need to parse OSVersion in the same way and I would incline to having windows only functionality there. Alternatively, I think containerd/platform repo should have a parser for each OS osversion_windows.go , osversion_linux.go etc . cc @kevpar |
Apologies for the rather random ticket, but I thought I'd open one for discussion.
With the containerd/platforms module now providing more options for Windows containers, we're considering using that package in a wider scope in our (Moby, BuildKit) code-base, and while looking at some changes / features, some questions popped-up. I'm opening this ticket to discuss these (looking for input / suggestions from the Microsoft and containerd maintainers).
Parsing os-version from string-format
containerd implemented a minimal
GetOsVersion
utility to parse a os-version into aOSVersion
struct https://github.com/containerd/containerd/blob/88fd474e4c0a3dd3865dfcbf0ca99fff12f5c7b7/platforms/defaults_windows.go#L67-L82While that utility is fairly trivial (it's mostly a
strings.Split
), I know there's been (sometimes very subtle) details over the Years in handling Windows versions, and it might be nice to provide a similar utility in hcsshim to provide a canonical location for parsing. Looking at containerd's function, it currently ignores any error (which may not be desirable), so when implementing, I think it'd be good to change the signature to include an error-return (which consumers can still decide to ignore depending on their use).Provide (partial) cross-platform functionality
This is a bit of a stretch, but currently the
osversion
package is Windows-only. In most cases this makes sense, as obtaining the version from the system (osversion.Get()
) requires it to run on a Windows host. Theosversion.OSVersion
struct andCheckHostAndContainerCompat
utilities should be portable though, and the same could apply to the string parsing function suggested above.With the
containerd/platforms
module potentially being used in cross-platform situations (client on Linux, runtime on Windows), I wonder if it would be useful to provide partial cross-platform functionality in the osversion package. For transparency; currently there's no use outside of Windows, but a discussion related to this code led me to looking; moby/moby#47330 (comment)Question about obtaining the host's version
This is a bit orthogonal, but I noticed that a mix of
windows.RtlGetNtVersionNumbers()
andwindows.RtlGetVersion()
is used to obtain the host's version. I recall various cases in the past where the version obtained from the system required it to be manifested, which caused some "fun" things in our CI where test-binaries are not manifested; https://github.com/moby/moby/blob/cae5d323e1f8a2b44c436f7a87bec6639841d8ec/pkg/archive/changes_test.go#L253-L257containerd's
DefaultSpec()
useswindows.RtlGetNtVersionNumbers()
https://github.com/containerd/containerd/blob/88fd474e4c0a3dd3865dfcbf0ca99fff12f5c7b7/platforms/defaults_windows.go#L30-L32
whereas
osversion.Get
useswindows.RtlGetVersion()
hcsshim/osversion/osversion_windows.go
Lines 25 to 29 in b0d91fb
The
osversion.Get
function explicitly calls out that the binary must be manifested. I'm curious if the same applies towindows.RtlGetNtVersionNumbers()
, and/or if there's a discrepancy there that could potentialy be causing issues. From the Go documentation forRtlGetNtVersionNumbers
;To add to the fun, I see in moby/moby we use
windows.GetVersion(
in some code, which looks to be "yet another" way to get the version https://github.com/moby/moby/blob/cae5d323e1f8a2b44c436f7a87bec6639841d8ec/pkg/parsers/kernel/kernel_windows.go#L38-L47The text was updated successfully, but these errors were encountered: