questions w.r.t. "osversion" package

thaJeztah commented 7 months ago

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 a OSVersion struct https://github.com/containerd/containerd/blob/88fd474e4c0a3dd3865dfcbf0ca99fff12f5c7b7/platforms/defaults_windows.go#L67-L82

While 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. The osversion.OSVersion struct and CheckHostAndContainerCompat 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; https://github.com/moby/moby/pull/47330#discussion_r1478752618

Question about obtaining the host's version

This is a bit orthogonal, but I noticed that a mix of windows.RtlGetNtVersionNumbers() and windows.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-L257

// Note we parse kernel.GetKernelVersion rather than system.GetOSVersion
// as test binaries aren't manifested, so would otherwise report the wrong
// build number.
if runtime.GOOS == "windows" {
    v, err := kernel.GetKernelVersion()

containerd's DefaultSpec() uses windows.RtlGetNtVersionNumbers() https://github.com/containerd/containerd/blob/88fd474e4c0a3dd3865dfcbf0ca99fff12f5c7b7/platforms/defaults_windows.go#L30-L32

whereas osversion.Get uses windows.RtlGetVersion() https://github.com/microsoft/hcsshim/blob/b0d91fb30c4f8cc812d9e78d2fedd54c983c343a/osversion/osversion_windows.go#L25-L29

The osversion.Get function explicitly calls out that the binary must be manifested. I'm curious if the same applies to windows.RtlGetNtVersionNumbers(), and/or if there's a discrepancy there that could potentialy be causing issues. From the Go documentation for RtlGetNtVersionNumbers;

// RtlGetNtVersionNumbers returns the version of the underlying operating system,
// ignoring manifest semantics and the application compatibility layer.
func RtlGetNtVersionNumbers() (majorVersion, minorVersion, buildNumber uint32) {

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-L47

// Important - dockerd.exe MUST be manifested for this API to return
// the correct information.
dwVersion, err := windows.GetVersion()

thaJeztah commented 7 months ago

cc @kiashok @dmcgowan

kiashok commented 7 months ago

@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 .
hcsshim and containerd references for the same can be switched to use containerd/platform as well.

cc @kevpar

microsoft / hcsshim