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

Use Microsoft/hcsshim constants and deprecate pkg/system.GetOsVersion() #39100

Merged
merged 3 commits into from
Oct 24, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 2 additions & 2 deletions cmd/dockerd/service_windows.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import (
"time"
"unsafe"

"github.com/docker/docker/pkg/system"
"github.com/Microsoft/hcsshim/osversion"
"github.com/sirupsen/logrus"
"github.com/spf13/pflag"
"golang.org/x/sys/windows"
Expand Down Expand Up @@ -171,7 +171,7 @@ func registerService() error {

// This dependency is required on build 14393 (RS1)
// it is added to the platform in newer builds
if system.GetOSVersion().Build == 14393 {
if osversion.Build() == osversion.RS1 {
depends = append(depends, "ConDrv")
}

Expand Down
33 changes: 10 additions & 23 deletions daemon/daemon_windows.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"strings"

"github.com/Microsoft/hcsshim"
"github.com/Microsoft/hcsshim/osversion"
"github.com/docker/docker/api/types"
containertypes "github.com/docker/docker/api/types/container"
"github.com/docker/docker/container"
Expand Down Expand Up @@ -126,8 +127,7 @@ func verifyPlatformContainerResources(resources *containertypes.Resources, isHyp
return warnings, fmt.Errorf("range of CPUs is from 0.01 to %d.00, as there are only %d CPUs available", sysinfo.NumCPU(), sysinfo.NumCPU())
}

osv := system.GetOSVersion()
if resources.NanoCPUs > 0 && isHyperv && osv.Build < 16175 {
Copy link
Member Author

Choose a reason for hiding this comment

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

This version was somewhere between "RS2" and "RS3", so I picked < RS3

if resources.NanoCPUs > 0 && isHyperv && osversion.Build() < osversion.RS3 {
leftoverNanoCPUs := resources.NanoCPUs % 1e9
if leftoverNanoCPUs != 0 && resources.NanoCPUs > 1e9 {
resources.NanoCPUs = ((resources.NanoCPUs + 1e9/2) / 1e9) * 1e9
Expand Down Expand Up @@ -196,14 +196,13 @@ func verifyPlatformContainerSettings(daemon *Daemon, hostConfig *containertypes.
if hostConfig == nil {
return nil, nil
}
osv := system.GetOSVersion()
hyperv := daemon.runAsHyperVContainer(hostConfig)

// On RS5, we allow (but don't strictly support) process isolation on Client SKUs.
// Prior to RS5, we don't allow process isolation on Client SKUs.
// @engine maintainers. This block should not be removed. It partially enforces licensing
// restrictions on Windows. Ping Microsoft folks if there are concerns or PRs to change this.
if !hyperv && system.IsWindowsClient() && osv.Build < 17763 {
if !hyperv && system.IsWindowsClient() && osversion.Build() < osversion.RS5 {
return warnings, fmt.Errorf("Windows client operating systems earlier than version 1809 can only run Hyper-V containers")
}

Expand All @@ -225,7 +224,7 @@ func checkSystem() error {
if osv.MajorVersion < 10 {
thaJeztah marked this conversation as resolved.
Show resolved Hide resolved
return fmt.Errorf("This version of Windows does not support the docker daemon")
}
if osv.Build < 14393 {
if osversion.Build() < osversion.RS1 {
return fmt.Errorf("The docker daemon requires build 14393 or later of Windows Server 2016 or Windows 10")
}

Expand Down Expand Up @@ -425,26 +424,15 @@ func initBridgeDriver(controller libnetwork.NetworkController, config *config.Co
winlibnetwork.NetworkName: runconfig.DefaultDaemonNetworkMode().NetworkName(),
}

var ipamOption libnetwork.NetworkOption
var subnetPrefix string

subnetPrefix := defaultNetworkSpace
if config.BridgeConfig.FixedCIDR != "" {
subnetPrefix = config.BridgeConfig.FixedCIDR
} else {
// TP5 doesn't support properly detecting subnet
Copy link
Member Author

Choose a reason for hiding this comment

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

Removing this as TP5 is really obsolete

Choose a reason for hiding this comment

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

Any idea why the original code scoped the use of defaultNetworkSpace to < 14360 only?

Copy link
Member Author

Choose a reason for hiding this comment

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

Things were still in development on the Windows side, so I think 14360 was a build where new features became available; that's been a long time back, so I'd have to check git history if there's any info about that

osv := system.GetOSVersion()
if osv.Build < 14360 {
subnetPrefix = defaultNetworkSpace
}
}

if subnetPrefix != "" {
ipamV4Conf := libnetwork.IpamConf{}
ipamV4Conf.PreferredPool = subnetPrefix
v4Conf := []*libnetwork.IpamConf{&ipamV4Conf}
v6Conf := []*libnetwork.IpamConf{}
ipamOption = libnetwork.NetworkOptionIpam("default", "", v4Conf, v6Conf, nil)
}
ipamV4Conf := libnetwork.IpamConf{PreferredPool: subnetPrefix}
v4Conf := []*libnetwork.IpamConf{&ipamV4Conf}
v6Conf := []*libnetwork.IpamConf{}
ipamOption := libnetwork.NetworkOptionIpam("default", "", v4Conf, v6Conf, nil)

_, err := controller.NewNetwork(string(runconfig.DefaultDaemonNetworkMode()), runconfig.DefaultDaemonNetworkMode().NetworkName(), "",
libnetwork.NetworkOptionGeneric(options.Generic{
Expand Down Expand Up @@ -602,7 +590,6 @@ func (daemon *Daemon) stats(c *container.Container) (*types.StatsJSON, error) {
// daemon to run in. This is only applicable on Windows
func (daemon *Daemon) setDefaultIsolation() error {
daemon.defaultIsolation = containertypes.Isolation("process")
osv := system.GetOSVersion()

// On client SKUs, default to Hyper-V. @engine maintainers. This
// should not be removed. Ping Microsoft folks is there are PRs to
Expand All @@ -626,7 +613,7 @@ func (daemon *Daemon) setDefaultIsolation() error {
daemon.defaultIsolation = containertypes.Isolation("hyperv")
}
if containertypes.Isolation(val).IsProcess() {
if system.IsWindowsClient() && osv.Build < 17763 {
if system.IsWindowsClient() && osversion.Build() < osversion.RS5 {
// On RS5, we allow (but don't strictly support) process isolation on Client SKUs.
// @engine maintainers. This block should not be removed. It partially enforces licensing
// restrictions on Windows. Ping Microsoft folks if there are concerns or PRs to change this.
Expand Down
9 changes: 5 additions & 4 deletions daemon/graphdriver/windows/windows.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,14 @@ import (
"github.com/Microsoft/go-winio/backuptar"
"github.com/Microsoft/go-winio/vhd"
"github.com/Microsoft/hcsshim"
"github.com/Microsoft/hcsshim/osversion"
"github.com/docker/docker/daemon/graphdriver"
"github.com/docker/docker/pkg/archive"
"github.com/docker/docker/pkg/containerfs"
"github.com/docker/docker/pkg/idtools"
"github.com/docker/docker/pkg/ioutils"
"github.com/docker/docker/pkg/longpath"
"github.com/docker/docker/pkg/reexec"
"github.com/docker/docker/pkg/system"
units "github.com/docker/go-units"
"github.com/pkg/errors"
"github.com/sirupsen/logrus"
Expand Down Expand Up @@ -272,7 +272,6 @@ func (d *Driver) Remove(id string) error {
// it is a transient error. Retry until it succeeds.
var computeSystems []hcsshim.ContainerProperties
retryCount := 0
osv := system.GetOSVersion()
for {
// Get and terminate any template VMs that are currently using the layer.
// Note: It is unfortunate that we end up in the graphdrivers Remove() call
Expand All @@ -294,8 +293,10 @@ func (d *Driver) Remove(id string) error {
// not required.
computeSystems, err = hcsshim.GetContainers(hcsshim.ComputeSystemQuery{})
if err != nil {
if (osv.Build < 15139) &&
Copy link
Member Author

Choose a reason for hiding this comment

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

15139 is somewhere between RS2 and RS3; from the context, it looks like the loop must be skipped on RS3 and up; I change it to an early return to make it slightly more readable

Copy link
Member

Choose a reason for hiding this comment

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

Hmmm, no I think you've changed the logic incorrectly here.

Copy link
Member Author

Choose a reason for hiding this comment

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

oh; why didn't I see this comment; I'll have to check

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, I think my logic is correct, but @lowenna @kolyshkin please double check;

(Simplified code, and replaced 15139 with RS3 for brevity)

Old situation

if err != nil {
	if (ver < RS3) && ((err ==InvalidState) || (err == AccessIsDenied)) {
		// retry
	}
	return err
}
ver error ver < RS3 ? err == InvalidState or AccessIsDenied? action
RS1 nil - - break
RS1 any return err
RS1 InvalidState retry
RS1 AccessIsDenied retry
RS3 nil - - break
RS3 any - (not evaluated) return err
RS3 InvalidState - (not evaluated) return err
RS3 AccessIsDenied - (not evaluated) return err

New situation:

if err != nil {
	if ver >= RS3 {
		// no retry, return error
	}
	if (err == InvalidState) || (err == AccessIsDenied) {
		// retry
	}
	return err
}
ver error ver >= RS3 ? err == InvalidState or AccessIsDenied? action
RS1 nil - - break
RS1 any return err
RS1 InvalidState retry
RS1 AccessIsDenied retry
RS3 nil - - break
RS3 any - (not evaluated) return err
RS3 InvalidState - (not evaluated) return err
RS3 AccessIsDenied - (not evaluated) return err

Copy link
Contributor

Choose a reason for hiding this comment

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

The new logic looks correct to me.

By the way, I found an obsoleted TODO comment just above this. I think the part of it needs to be removed, this one:

For RS3, we can remove the retries. Also

((err == hcsshim.ErrVmcomputeOperationInvalidState) || (err == hcsshim.ErrVmcomputeOperationAccessIsDenied)) {
if osversion.Build() >= osversion.RS3 {
return err
}
if (err == hcsshim.ErrVmcomputeOperationInvalidState) || (err == hcsshim.ErrVmcomputeOperationAccessIsDenied) {
if retryCount >= 500 {
break
}
Expand Down
3 changes: 2 additions & 1 deletion daemon/oci_windows.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"runtime"
"strings"

"github.com/Microsoft/hcsshim/osversion"
containertypes "github.com/docker/docker/api/types/container"
"github.com/docker/docker/container"
"github.com/docker/docker/errdefs"
Expand Down Expand Up @@ -275,7 +276,7 @@ func (daemon *Daemon) createSpecWindowsFields(c *container.Container, s *specs.S
if isHyperV {
return errors.New("device assignment is not supported for HyperV containers")
}
if system.GetOSVersion().Build < 17763 {
if osversion.Build() < osversion.RS5 {
return errors.New("device assignment requires Windows builds RS5 (17763+) or later")
}
for _, deviceMapping := range c.HostConfig.Devices {
Expand Down
7 changes: 4 additions & 3 deletions distribution/pull_v2_windows.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"strconv"
"strings"

"github.com/Microsoft/hcsshim/osversion"
"github.com/containerd/containerd/platforms"
"github.com/docker/distribution"
"github.com/docker/distribution/manifest/manifestlist"
Expand Down Expand Up @@ -65,7 +66,7 @@ func (ld *v2LayerDescriptor) open(ctx context.Context) (distribution.ReadSeekClo
}

func filterManifests(manifests []manifestlist.ManifestDescriptor, p specs.Platform) []manifestlist.ManifestDescriptor {
version := system.GetOSVersion()
version := osversion.Get()
osVersion := fmt.Sprintf("%d.%d.%d", version.MajorVersion, version.MinorVersion, version.Build)
logrus.Debugf("will prefer Windows entries with version %s", osVersion)

Expand Down Expand Up @@ -123,7 +124,7 @@ func (mbv manifestsByVersion) Swap(i, j int) {
// Fixes https://github.com/moby/moby/issues/36184.
func checkImageCompatibility(imageOS, imageOSVersion string) error {
if imageOS == "windows" {
hostOSV := system.GetOSVersion()
hostOSV := osversion.Get()
splitImageOSVersion := strings.Split(imageOSVersion, ".") // eg 10.0.16299.nnnn
if len(splitImageOSVersion) >= 3 {
if imageOSBuild, err := strconv.Atoi(splitImageOSVersion[2]); err == nil {
Expand All @@ -142,5 +143,5 @@ func formatPlatform(platform specs.Platform) string {
if platform.OS == "" {
platform = platforms.DefaultSpec()
}
return fmt.Sprintf("%s %s", platforms.Format(platform), system.GetOSVersion().ToString())
return fmt.Sprintf("%s %s", platforms.Format(platform), osversion.Get().ToString())
}
3 changes: 2 additions & 1 deletion integration-cli/docker_api_containers_windows_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"testing"

winio "github.com/Microsoft/go-winio"
"github.com/Microsoft/hcsshim/osversion"
"github.com/docker/docker/api/types"
"github.com/docker/docker/api/types/container"
"github.com/docker/docker/api/types/mount"
Expand All @@ -19,7 +20,7 @@ import (
)

func (s *DockerSuite) TestContainersAPICreateMountsBindNamedPipe(c *testing.T) {
testRequires(c, testEnv.IsLocalDaemon, DaemonIsWindowsAtLeastBuild(16299)) // Named pipe support was added in RS3
testRequires(c, testEnv.IsLocalDaemon, DaemonIsWindowsAtLeastBuild(osversion.RS3)) // Named pipe support was added in RS3

// Create a host pipe to map into the container
hostPipeName := fmt.Sprintf(`\\.\pipe\docker-cli-test-pipe-%x`, rand.Uint64())
Expand Down
13 changes: 9 additions & 4 deletions integration-cli/docker_api_images_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"strings"
"testing"

"github.com/Microsoft/hcsshim/osversion"
"github.com/docker/docker/api/types"
"github.com/docker/docker/api/types/filters"
"github.com/docker/docker/client"
Expand Down Expand Up @@ -59,10 +60,12 @@ func (s *DockerSuite) TestAPIImagesFilter(c *testing.T) {

func (s *DockerSuite) TestAPIImagesSaveAndLoad(c *testing.T) {
if runtime.GOOS == "windows" {
// Note we parse kernel.GetKernelVersion rather than osversion.Build()
// as test binaries aren't manifested, so would otherwise report build 9200.
v, err := kernel.GetKernelVersion()
assert.NilError(c, err)
build, _ := strconv.Atoi(strings.Split(strings.SplitN(v.String(), " ", 3)[2][1:], ".")[0])
if build <= 16299 {
buildNumber, _ := strconv.Atoi(strings.Split(strings.SplitN(v.String(), " ", 3)[2][1:], ".")[0])
Copy link
Member Author

Choose a reason for hiding this comment

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

renaming these as they overlap with the build package

if buildNumber <= osversion.RS3 {
c.Skip("Temporarily disabled on RS3 and older because they are too slow. See #39909")
}
}
Expand Down Expand Up @@ -139,10 +142,12 @@ func (s *DockerSuite) TestAPIImagesHistory(c *testing.T) {

func (s *DockerSuite) TestAPIImagesImportBadSrc(c *testing.T) {
if runtime.GOOS == "windows" {
// Note we parse kernel.GetKernelVersion rather than osversion.Build()
// as test binaries aren't manifested, so would otherwise report build 9200.
v, err := kernel.GetKernelVersion()
assert.NilError(c, err)
build, _ := strconv.Atoi(strings.Split(strings.SplitN(v.String(), " ", 3)[2][1:], ".")[0])
if build == 16299 {
buildNumber, _ := strconv.Atoi(strings.Split(strings.SplitN(v.String(), " ", 3)[2][1:], ".")[0])
if buildNumber == osversion.RS3 {
c.Skip("Temporarily disabled on RS3 builds")
}
}
Expand Down
19 changes: 10 additions & 9 deletions integration-cli/docker_cli_run_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"testing"
"time"

"github.com/Microsoft/hcsshim/osversion"
"github.com/docker/docker/client"
"github.com/docker/docker/integration-cli/cli"
"github.com/docker/docker/integration-cli/cli/build"
Expand Down Expand Up @@ -1880,7 +1881,7 @@ func (s *DockerSuite) TestRunBindMounts(c *testing.T) {

if testEnv.OSType == "windows" {
// Disabled prior to RS5 due to how volumes are mapped
testRequires(c, DaemonIsWindowsAtLeastBuild(17763))
testRequires(c, DaemonIsWindowsAtLeastBuild(osversion.RS5))
}

prefix, _ := getPrefixAndSlashFromDaemonPlatform()
Expand Down Expand Up @@ -3915,16 +3916,16 @@ func (s *DockerSuite) TestRunNamedVolumesFromNotRemoved(c *testing.T) {
}

func (s *DockerSuite) TestRunAttachFailedNoLeak(c *testing.T) {
// TODO @msabansal - https://github.com/moby/moby/issues/35023. Duplicate
// port mappings are not errored out on RS3 builds. Temporarily disabling
// this test pending further investigation. Note we parse kernel.GetKernelVersion
// rather than system.GetOSVersion as test binaries aren't manifested, so would
// otherwise report build 9200.
if runtime.GOOS == "windows" {
// TODO @msabansal - https://github.com/moby/moby/issues/35023. Duplicate
// port mappings are not errored out on RS3 builds. Temporarily disabling
Copy link
Member Author

Choose a reason for hiding this comment

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

This check is no longer needed, and will be removed in #39845

// this test pending further investigation. Note we parse kernel.GetKernelVersion
// rather than osversion.Build() as test binaries aren't manifested, so would
// otherwise report build 9200.
v, err := kernel.GetKernelVersion()
assert.NilError(c, err)
build, _ := strconv.Atoi(strings.Split(strings.SplitN(v.String(), " ", 3)[2][1:], ".")[0])
if build == 16299 {
buildNumber, _ := strconv.Atoi(strings.Split(strings.SplitN(v.String(), " ", 3)[2][1:], ".")[0])
if buildNumber == osversion.RS3 {
c.Skip("Temporarily disabled on RS3 builds")
}
}
Expand Down Expand Up @@ -4532,7 +4533,7 @@ func (s *DockerSuite) TestRunAddDeviceCgroupRule(c *testing.T) {

// Verifies that running as local system is operating correctly on Windows
func (s *DockerSuite) TestWindowsRunAsSystem(c *testing.T) {
testRequires(c, DaemonIsWindowsAtLeastBuild(15000))
Copy link
Member Author

Choose a reason for hiding this comment

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

This is somewhere between RS1 and RS2, so I took RS3

testRequires(c, DaemonIsWindowsAtLeastBuild(osversion.RS3))
out, _ := dockerCmd(c, "run", "--net=none", `--user=nt authority\system`, "--hostname=XYZZY", minimalBaseImage(), "cmd", "/c", `@echo %USERNAME%`)
assert.Equal(c, strings.TrimSpace(out), "XYZZY$")
}
4 changes: 3 additions & 1 deletion integration-cli/docker_cli_start_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ import (
"testing"
"time"

"github.com/Microsoft/hcsshim/osversion"

Copy link
Member Author

Choose a reason for hiding this comment

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

arf. think my IDE had goimports disabled; let me check my config

"github.com/docker/docker/integration-cli/cli"
"github.com/docker/docker/pkg/parsers/kernel"
"gotest.tools/assert"
Expand Down Expand Up @@ -196,7 +198,7 @@ func (s *DockerSuite) TestStartReturnCorrectExitCode(c *testing.T) {
v, err := kernel.GetKernelVersion()
assert.NilError(c, err)
build, _ := strconv.Atoi(strings.Split(strings.SplitN(v.String(), " ", 3)[2][1:], ".")[0])
if build < 16299 {
if build < osversion.RS3 {
c.Skip("FLAKY on Windows RS1, see #38521")
}
}
Expand Down
7 changes: 4 additions & 3 deletions libcontainerd/local/local_windows.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"time"

"github.com/Microsoft/hcsshim"
"github.com/Microsoft/hcsshim/osversion"
opengcs "github.com/Microsoft/opengcs/client"
"github.com/containerd/containerd"
"github.com/containerd/containerd/cio"
Expand Down Expand Up @@ -318,7 +319,7 @@ func (c *client) createWindows(id string, spec *specs.Spec, runtimeOptions inter
}
}
configuration.MappedDirectories = mds
if len(mps) > 0 && system.GetOSVersion().Build < 16299 { // RS3
if len(mps) > 0 && osversion.Build() < osversion.RS3 {
return errors.New("named pipe mounts are not supported on this version of Windows")
}
configuration.MappedPipes = mps
Expand All @@ -328,7 +329,7 @@ func (c *client) createWindows(id string, spec *specs.Spec, runtimeOptions inter
if configuration.HvPartition {
return errors.New("device assignment is not supported for HyperV containers")
}
if system.GetOSVersion().Build < 17763 { // RS5
if osversion.Build() < osversion.RS5 {
return errors.New("device assignment requires Windows builds RS5 (17763+) or later")
}
for _, d := range spec.Windows.Devices {
Expand Down Expand Up @@ -519,7 +520,7 @@ func (c *client) createLinux(id string, spec *specs.Spec, runtimeOptions interfa
ReadOnly: readonly,
}
// If we are 1803/RS4+ enable LinuxMetadata support by default
if system.GetOSVersion().Build >= 17134 {
if osversion.Build() >= osversion.RS4 {
md.LinuxMetadata = true
}
mds = append(mds, md)
Expand Down
3 changes: 1 addition & 2 deletions pkg/system/init_windows.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,7 @@ var (

// InitLCOW sets whether LCOW is supported or not. Requires RS5+
func InitLCOW(experimental bool) {
v := GetOSVersion()
if experimental && v.Build >= osversion.RS5 {
if experimental && osversion.Build() >= osversion.RS5 {
lcowSupported = true
}
}
Expand Down
Loading