Skip to content
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

Open
thaJeztah opened this issue Feb 6, 2024 · 2 comments
Open

questions w.r.t. "osversion" package #2018

thaJeztah opened this issue Feb 6, 2024 · 2 comments

Comments

@thaJeztah
Copy link
Contributor

thaJeztah commented Feb 6, 2024

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; 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() 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()

// Get gets the operating system version on Windows.
// The calling application must be manifested to get the correct version information.
func Get() OSVersion {
once.Do(func() {
v := *windows.RtlGetVersion()

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
Copy link
Contributor Author

cc @kiashok @dmcgowan

@kiashok
Copy link
Contributor

kiashok commented Feb 6, 2024

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants