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.

Sign up for GitHub

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

Use osversion.Build() utility, and add a sync.Once #996

Merged
merged 2 commits into from
Apr 12, 2021
Merged

Use osversion.Build() utility, and add a sync.Once #996

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d3cbd1d
Use osversion.Build() utility where possible
thaJeztah Apr 8, 2021
bab6498
osversion.Get(): use sync.Once
thaJeztah Apr 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cmd/containerd-shim-runhcs-v1/exec_hcs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,7 @@ func (he *hcsExec) Kill(ctx context.Context, signal uint32) error {
return nil
case shimExecStateRunning:
supported := false
if osversion.Get().Build >= osversion.RS5 {
if osversion.Build() >= osversion.RS5 {
supported = he.host == nil || he.host.SignalProcessSupported()
}
var options interface{}
Expand Down
2 changes: 1 addition & 1 deletion cmd/containerd-shim-runhcs-v1/pod.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ type shimPod interface {
func createPod(ctx context.Context, events publisher, req *task.CreateTaskRequest, s *specs.Spec) (shimPod, error) {
log.G(ctx).WithField("tid", req.ID).Debug("createPod")

if osversion.Get().Build < osversion.RS5 {
if osversion.Build() < osversion.RS5 {
return nil, errors.Wrapf(errdefs.ErrFailedPrecondition, "pod support is not available on Windows versions previous to RS5 (%d)", osversion.RS5)
}

Expand Down
4 changes: 2 additions & 2 deletions cmd/containerd-shim-runhcs-v1/task_hcs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ func newHcsStandaloneTask(ctx context.Context, events publisher, req *task.Creat
owner := filepath.Base(os.Args[0])

var parent *uvm.UtilityVM
if osversion.Get().Build >= osversion.RS5 && oci.IsIsolated(s) {
if osversion.Build() >= osversion.RS5 && oci.IsIsolated(s) {
// Create the UVM parent
opts, err := oci.SpecToUVMCreateOpts(ctx, s, fmt.Sprintf("%s@vm", req.ID), owner)
if err != nil {
Expand Down Expand Up @@ -375,7 +375,7 @@ type hcsTask struct {
// host is the hosting VM for this exec if hypervisor isolated. If
// `host==nil` this is an Argon task so no UVM cleanup is required.
//
// NOTE: if `osversion.Get().Build < osversion.RS5` this will always be
// NOTE: if `osversion.Build() < osversion.RS5` this will always be
// `nil`.
host *uvm.UtilityVM

Expand Down
2 changes: 1 addition & 1 deletion cmd/runhcs/container.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,7 +307,7 @@ func createContainer(cfg *containerConfig) (_ *container, err error) {
return nil, fmt.Errorf("host container %s is not a VM host", hostID)
}
hostUniqueID = host.UniqueID
} else if vmisolated && (isSandbox || cfg.Spec.Linux != nil || osversion.Get().Build >= osversion.RS5) {
} else if vmisolated && (isSandbox || cfg.Spec.Linux != nil || osversion.Build() >= osversion.RS5) {
// This handles all LCOW, Pod Sandbox, and (Windows Xenon V2 for RS5+)
hostID = cfg.ID
newvm = true
Expand Down
2 changes: 1 addition & 1 deletion cmd/runhcs/create-scratch.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ var createScratchCommand = cli.Command{
return errors.New("'destpath' is required")
}

if osversion.Get().Build < osversion.RS5 {
if osversion.Build() < osversion.RS5 {
return errors.New("LCOW is not supported pre-RS5")
}

Expand Down
2 changes: 1 addition & 1 deletion cmd/runhcs/kill.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ signal to the init process of the "ubuntu01" container:
signalsSupported := false

// The Signal feature was added in RS5
if osversion.Get().Build >= osversion.RS5 {
if osversion.Build() >= osversion.RS5 {
if c.IsHost || c.HostID != "" {
var hostID string
if c.IsHost {
Expand Down
2 changes: 1 addition & 1 deletion cmd/runhcs/prepare-disk.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ var prepareDiskCommand = cli.Command{
return errors.New("'destpath' is required")
}

if osversion.Get().Build < osversion.RS5 {
if osversion.Build() < osversion.RS5 {
return errors.New("LCOW is not supported pre-RS5")
}

Expand Down
2 changes: 1 addition & 1 deletion computestorage/setup.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ func SetupBaseOSLayer(ctx context.Context, layerPath string, vhdHandle windows.H
//
// `options` are the options applied while processing the layer.
func SetupBaseOSVolume(ctx context.Context, layerPath, volumePath string, options OsLayerOptions) (err error) {
if osversion.Get().Build < 19645 {
if osversion.Build() < 19645 {
return errors.New("SetupBaseOSVolume is not present on builds older than 19645")
}
title := "hcsshim.SetupBaseOSVolume"
Expand Down
2 changes: 1 addition & 1 deletion internal/hcsoci/hcsdoc_wcow.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -366,7 +366,7 @@ func createWindowsContainerDocument(ctx context.Context, coi *createOptionsInter
}
v1.MappedDirectories = mounts.mdsv1
v2Container.MappedDirectories = mounts.mdsv2
if len(mounts.mpsv1) > 0 && osversion.Get().Build < osversion.RS3 {
if len(mounts.mpsv1) > 0 && osversion.Build() < osversion.RS3 {
return nil, nil, fmt.Errorf("named pipe mounts are not supported on this version of Windows")
}
v1.MappedPipes = mounts.mpsv1
Expand Down
4 changes: 2 additions & 2 deletions internal/schemaversion/schemaversion.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ func IsSupported(sv *hcsschema.Version) error {
return nil
}
if IsV21(sv) {
if osversion.Get().Build < osversion.RS5 {
if osversion.Build() < osversion.RS5 {
return fmt.Errorf("unsupported on this Windows build")
}
return nil
Expand Down Expand Up @@ -67,7 +67,7 @@ func String(sv *hcsschema.Version) string {
// requested option.
func DetermineSchemaVersion(requestedSV *hcsschema.Version) *hcsschema.Version {
sv := SchemaV10()
if osversion.Get().Build >= osversion.RS5 {
if osversion.Build() >= osversion.RS5 {
sv = SchemaV21()
}
if requestedSV != nil {
Expand Down
4 changes: 2 additions & 2 deletions internal/uvm/create.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,11 +145,11 @@ func verifyOptions(ctx context.Context, options interface{}) error {
return errors.New("PreferredRootFSTypeVHD requires at least one VPMem device")
}
}
if opts.KernelDirect && osversion.Get().Build < 18286 {
if opts.KernelDirect && osversion.Build() < 18286 {
Copy link
Contributor Author

@thaJeztah thaJeztah Apr 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: is there much overhead in the syscall that's made? (wondering if there would be benefits in setting a package-level variable to store the version in to prevent calling this repeatedly in some packages, or if that would not give (significant) benefits)

https://github.com/golang/sys/blob/700132347e0702a3e5f100fba2752993b8de9600/windows/zsyscall_windows.go#L2266-L2273

func GetVersion() (ver uint32, err error) {
	r0, _, e1 := syscall.Syscall(procGetVersion.Addr(), 0, 0, 0, 0)
	ver = uint32(r0)
	if ver == 0 {
		err = errnoErr(e1)
	}
	return
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll have to trial the overhead but one syscall will definitely be better than many 😄. You'd think we could just do this in an init, assign it to an exported global and be done with it.. Not sure if I'm missing something/some history on why we just call it over and over.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kevpar Should we move test/functional/manifest top level (or just out of test really), call Build() once in the osversion package and cache the result in an exported var and just use this everywhere? I don't have the history on why the manifest lives in test at the moment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The alternative could be to add a sync.Once in osversion.Get(); that way we don't have to add some "magic variable", and any consumer of that utility won't have to worry about it, and get the improvement "out of the box".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what I have at the moment after firing up a new branch, I like it better

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually went ahead and added that as an extra commit in this branch (can remove if it's not needed)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're too quick for me 😎. Let's see what others think, I think it should be fine

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switching to a global and sync.Once looks fine to me. I might be missing why moving manifest came up, though?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No functional reason other than it feels weird to import this thing from the test dir when needed

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I thought it was related to this somehow. I'm in favor of moving it, but as a separate PR.

return errors.New("KernelDirectBoot is not supported on builds older than 18286")
}

if opts.EnableColdDiscardHint && osversion.Get().Build < 18967 {
if opts.EnableColdDiscardHint && osversion.Build() < 18967 {
return errors.New("EnableColdDiscardHint is not supported on builds older than 18967")
}
case *OptionsWCOW:
Expand Down
2 changes: 1 addition & 1 deletion internal/uvm/create_lcow.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ func defaultLCOWOSBootFilesPath() string {
// executable files name.
func NewDefaultOptionsLCOW(id, owner string) *OptionsLCOW {
// Use KernelDirect boot by default on all builds that support it.
kernelDirectSupported := osversion.Get().Build >= 18286
kernelDirectSupported := osversion.Build() >= 18286
opts := &OptionsLCOW{
Options: newDefaultOptions(id, owner),
BootFilesPath: defaultLCOWOSBootFilesPath(),
Expand Down
2 changes: 1 addition & 1 deletion internal/uvm/network.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -512,7 +512,7 @@ func (uvm *UtilityVM) isNetworkNamespaceSupported() bool {
}

func getNetworkModifyRequest(adapterID string, requestType string, settings interface{}) interface{} {
if osversion.Get().Build >= osversion.RS5 {
if osversion.Build() >= osversion.RS5 {
return guestrequest.NetworkModifyRequest{
AdapterId: adapterID,
RequestType: requestType,
Expand Down
2 changes: 1 addition & 1 deletion internal/uvm/plan9.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ func (uvm *UtilityVM) AddPlan9(ctx context.Context, hostPath string, uvmPath str
if uvm.operatingSystem != "linux" {
return nil, errNotSupported
}
if restrict && osversion.Get().Build < osversion.V19H1 {
if restrict && osversion.Build() < osversion.V19H1 {
return nil, errors.New("single-file mappings are not supported on this build of Windows")
}
if uvmPath == "" {
Expand Down
2 changes: 1 addition & 1 deletion internal/uvm/vsmb.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ func openHostPath(path string) (windows.Handle, error) {
// To work around this, we attempt to query for FileIdInfo ourselves if on an affected build. If
// the query fails, we override the specified options to force no direct map to be used.
func forceNoDirectMap(path string) (bool, error) {
if ver := osversion.Get().Build; ver < osversion.V19H1 || ver > osversion.V20H2 {
if ver := osversion.Build(); ver < osversion.V19H1 || ver > osversion.V20H2 {
return false, nil
}
h, err := openHostPath(path)
Expand Down
2 changes: 1 addition & 1 deletion internal/wclayer/expandscratchsize.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ func ExpandScratchSize(ctx context.Context, path string, size uint64) (err error

// Manually expand the volume now in order to work around bugs in 19H1 and
// prerelease versions of Vb. Remove once this is fixed in Windows.
if build := osversion.Get().Build; build >= osversion.V19H1 && build < 19020 {
if build := osversion.Build(); build >= osversion.V19H1 && build < 19020 {
err = expandSandboxVolume(ctx, path)
if err != nil {
return err
Expand Down
28 changes: 18 additions & 10 deletions osversion/osversion_windows.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package osversion

import (
"fmt"
"sync"

"golang.org/x/sys/windows"
)
Expand All @@ -15,19 +16,26 @@ type OSVersion struct {
Build uint16
}

var (
osv OSVersion
once sync.Once
)

// Get gets the operating system version on Windows.
// The calling application must be manifested to get the correct version information.
func Get() OSVersion {
var err error
osv := OSVersion{}
osv.Version, err = windows.GetVersion()
if err != nil {
// GetVersion never fails.
panic(err)
}
osv.MajorVersion = uint8(osv.Version & 0xFF)
osv.MinorVersion = uint8(osv.Version >> 8 & 0xFF)
osv.Build = uint16(osv.Version >> 16)
once.Do(func() {
var err error
osv = OSVersion{}
osv.Version, err = windows.GetVersion()
if err != nil {
// GetVersion never fails.
panic(err)
}
osv.MajorVersion = uint8(osv.Version & 0xFF)
osv.MinorVersion = uint8(osv.Version >> 8 & 0xFF)
osv.Build = uint16(osv.Version >> 16)
})
return osv
}

Expand Down
16 changes: 8 additions & 8 deletions test/cri-containerd/container_virtual_device_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ func getGPUContainerRequestWCOW(t *testing.T, podID string, podConfig *runtime.P
func Test_RunContainer_VirtualDevice_GPU_LCOW(t *testing.T) {
requireFeatures(t, featureLCOW, featureGPU)

if osversion.Get().Build < osversion.V20H1 {
if osversion.Build() < osversion.V20H1 {
t.Skip("Requires build +20H1")
}

Expand Down Expand Up @@ -240,7 +240,7 @@ func Test_RunContainer_VirtualDevice_GPU_LCOW(t *testing.T) {
func Test_RunContainer_VirtualDevice_GPU_Multiple_LCOW(t *testing.T) {
requireFeatures(t, featureLCOW, featureGPU)

if osversion.Get().Build < osversion.V20H1 {
if osversion.Build() < osversion.V20H1 {
t.Skip("Requires build +20H1")
}

Expand Down Expand Up @@ -289,7 +289,7 @@ func Test_RunContainer_VirtualDevice_GPU_Multiple_LCOW(t *testing.T) {
func Test_RunContainer_VirtualDevice_GPU_and_NoGPU_LCOW(t *testing.T) {
requireFeatures(t, featureLCOW, featureGPU)

if osversion.Get().Build < osversion.V20H1 {
if osversion.Build() < osversion.V20H1 {
t.Skip("Requires build +20H1")
}

Expand Down Expand Up @@ -358,7 +358,7 @@ func Test_RunContainer_VirtualDevice_GPU_and_NoGPU_LCOW(t *testing.T) {
func Test_RunContainer_VirtualDevice_GPU_Multiple_Removal_LCOW(t *testing.T) {
requireFeatures(t, featureLCOW, featureGPU)

if osversion.Get().Build < osversion.V20H1 {
if osversion.Build() < osversion.V20H1 {
t.Skip("Requires build +20H1")
}

Expand Down Expand Up @@ -486,7 +486,7 @@ func Test_RunContainer_VirtualDevice_ClassGUID_WCOW_Process(t *testing.T) {
func Test_RunContainer_VirtualDevice_GPU_WCOW_Hypervisor(t *testing.T) {
requireFeatures(t, featureWCOWHypervisor, featureGPU)

if osversion.Get().Build < osversion.V20H1 {
if osversion.Build() < osversion.V20H1 {
t.Skip("Requires build +20H1")
}

Expand Down Expand Up @@ -531,7 +531,7 @@ func Test_RunContainer_VirtualDevice_GPU_WCOW_Hypervisor(t *testing.T) {
func Test_RunContainer_VirtualDevice_GPU_and_NoGPU_WCOW_Hypervisor(t *testing.T) {
requireFeatures(t, featureWCOWHypervisor, featureGPU)

if osversion.Get().Build < osversion.V20H1 {
if osversion.Build() < osversion.V20H1 {
t.Skip("Requires build +20H1")
}

Expand Down Expand Up @@ -592,7 +592,7 @@ func Test_RunContainer_VirtualDevice_GPU_and_NoGPU_WCOW_Hypervisor(t *testing.T)
func Test_RunContainer_VirtualDevice_GPU_Multiple_WCOW_Hypervisor(t *testing.T) {
requireFeatures(t, featureWCOWHypervisor, featureGPU)

if osversion.Get().Build < osversion.V20H1 {
if osversion.Build() < osversion.V20H1 {
t.Skip("Requires build +20H1")
}

Expand Down Expand Up @@ -645,7 +645,7 @@ func Test_RunContainer_VirtualDevice_GPU_Multiple_WCOW_Hypervisor(t *testing.T)
func Test_RunContainer_VirtualDevice_GPU_Multiple_Removal_WCOW_Hypervisor(t *testing.T) {
requireFeatures(t, featureWCOWHypervisor, featureGPU)

if osversion.Get().Build < osversion.V20H1 {
if osversion.Build() < osversion.V20H1 {
t.Skip("Requires build +20H1")
}

Expand Down
4 changes: 2 additions & 2 deletions test/cri-containerd/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,8 @@ const (

// Image definitions
var (
imageWindowsNanoserver = getWindowsNanoserverImage(osversion.Get().Build)
imageWindowsServercore = getWindowsServerCoreImage(osversion.Get().Build)
imageWindowsNanoserver = getWindowsNanoserverImage(osversion.Build())
imageWindowsServercore = getWindowsServerCoreImage(osversion.Build())
imageWindowsNanoserver17763 = getWindowsNanoserverImage(osversion.RS5)
imageWindowsNanoserver18362 = getWindowsNanoserverImage(osversion.V19H1)
imageWindowsNanoserver19041 = getWindowsNanoserverImage(osversion.V20H1)
Expand Down
8 changes: 4 additions & 4 deletions test/cri-containerd/runpodsandbox_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,7 @@ func Test_RunPodSandbox_MemorySize_LCOW(t *testing.T) {
func Test_RunPodSandbox_MMIO_WCOW_Process(t *testing.T) {
requireFeatures(t, featureWCOWProcess)

if osversion.Get().Build < osversion.V20H1 {
if osversion.Build() < osversion.V20H1 {
t.Skip("Requires build +20H1")
}
pullRequiredImages(t, []string{imageWindowsNanoserver})
Expand All @@ -277,7 +277,7 @@ func Test_RunPodSandbox_MMIO_WCOW_Process(t *testing.T) {
func Test_RunPodSandbox_MMIO_WCOW_Hypervisor(t *testing.T) {
requireFeatures(t, featureWCOWHypervisor)

if osversion.Get().Build < osversion.V20H1 {
if osversion.Build() < osversion.V20H1 {
t.Skip("Requires build +20H1")
}
pullRequiredImages(t, []string{imageWindowsNanoserver})
Expand All @@ -294,7 +294,7 @@ func Test_RunPodSandbox_MMIO_WCOW_Hypervisor(t *testing.T) {
func Test_RunPodSandbox_MMIO_LCOW(t *testing.T) {
requireFeatures(t, featureLCOW)

if osversion.Get().Build < osversion.V20H1 {
if osversion.Build() < osversion.V20H1 {
t.Skip("Requires build +20H1")
}
pullRequiredLcowImages(t, []string{imageLcowK8sPause})
Expand Down Expand Up @@ -917,7 +917,7 @@ func Test_RunPodSandbox_MultipleContainersSameVhd_WCOW(t *testing.T) {
requireFeatures(t, featureWCOWHypervisor)
// Prior to 19H1, we aren't able to easily create a formatted VHD, as
// HcsFormatWritableLayerVhd requires the VHD to be mounted prior the call.
if osversion.Get().Build < osversion.V19H1 {
if osversion.Build() < osversion.V19H1 {
t.Skip("Requires at least 19H1")
}

Expand Down
4 changes: 2 additions & 2 deletions test/functional/utilities/requiresbuild.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,13 @@ import (
)

func RequiresBuild(t *testing.T, b uint16) {
if osversion.Get().Build < b {
if osversion.Build() < b {
t.Skipf("Requires build %d+", b)
}
}

func RequiresExactBuild(t *testing.T, b uint16) {
if osversion.Get().Build != b {
if osversion.Build() != b {
t.Skipf("Requires exact build %d", b)
}
}