unittests: Fixes unit tests for Windows (part 3)

Currently, there are some unit tests that are failing on Windows due to various reasons: - paths not properly joined (filepath.Join should be used). - Proxy Mode IPVS not supported on Windows. - DeadlineExceeded can occur when trying to read data from an UDP socket. This can be used to detect whether the port was closed or not. - In Windows, with long file name support enabled, file names can have up to 32,767 characters. In this case, the error windows.ERROR_FILENAME_EXCED_RANGE will be encountered instead. - files not closed, which means that they cannot be removed / renamed. - time.Now() is not as precise on Windows, which means that 2 consecutive calls may return the same timestamp. - path.Base() will return the same path. filepath.Base() should be used instead. - path.Join() will always join the paths with a / instead of the OS specific separator. filepath.Join() should be used instead.
yibozhuang · Oct 21, 2022 · 9f95b7b · 9f95b7b
1 parent 83415e5
commit 9f95b7b
Show file tree

Hide file tree

Showing 14 changed files with 131 additions and 50 deletions.
diff --git a/pkg/kubelet/cm/devicemanager/manager_test.go b/pkg/kubelet/cm/devicemanager/manager_test.go
@@ -81,8 +81,8 @@ func tmpSocketDir() (socketDir, socketName, pluginSocketName string, err error)
 	if err != nil {
 		return
 	}
-	socketName = socketDir + "/server.sock"
-	pluginSocketName = socketDir + "/device-plugin.sock"
+	socketName = filepath.Join(socketDir, "server.sock")
+	pluginSocketName = filepath.Join(socketDir, "device-plugin.sock")
 	os.MkdirAll(socketDir, 0755)
 	return
 }

diff --git a/pkg/kubelet/pluginmanager/pluginwatcher/plugin_watcher_test.go b/pkg/kubelet/pluginmanager/pluginwatcher/plugin_watcher_test.go
@@ -20,6 +20,7 @@ import (
 	"flag"
 	"fmt"
 	"os"
+	"path/filepath"
 	"sync"
 	"testing"
 	"time"
@@ -32,8 +33,6 @@ import (
 )
 
 var (
-	socketDir string
-
 	supportedVersions = []string{"v1beta1", "v1beta2"}
 )
 
@@ -44,18 +43,17 @@ func init() {
 	flag.Set("alsologtostderr", fmt.Sprintf("%t", true))
 	flag.StringVar(&logLevel, "logLevel", "6", "test")
 	flag.Lookup("v").Value.Set(logLevel)
+}
 
+func initTempDir(t *testing.T) string {
+	// Creating a different directory. os.RemoveAll is not atomic enough;
+	// os.MkdirAll can get into an "Access Denied" error on Windows.
 	d, err := os.MkdirTemp("", "plugin_test")
 	if err != nil {
-		panic(fmt.Sprintf("Could not create a temp directory: %s", d))
+		t.Fatalf("Could not create a temp directory %s: %v", d, err)
 	}
 
-	socketDir = d
-}
-
-func cleanup(t *testing.T) {
-	require.NoError(t, os.RemoveAll(socketDir))
-	os.MkdirAll(socketDir, 0755)
+	return d
 }
 
 func waitForRegistration(
@@ -106,13 +104,14 @@ func retryWithExponentialBackOff(initialDuration time.Duration, fn wait.Conditio
 }
 
 func TestPluginRegistration(t *testing.T) {
-	defer cleanup(t)
+	socketDir := initTempDir(t)
+	defer os.RemoveAll(socketDir)
 
 	dsw := cache.NewDesiredStateOfWorld()
-	newWatcher(t, dsw, wait.NeverStop)
+	newWatcher(t, socketDir, dsw, wait.NeverStop)
 
 	for i := 0; i < 10; i++ {
-		socketPath := fmt.Sprintf("%s/plugin-%d.sock", socketDir, i)
+		socketPath := filepath.Join(socketDir, fmt.Sprintf("plugin-%d.sock", i))
 		pluginName := fmt.Sprintf("example-plugin-%d", i)
 
 		p := NewTestExamplePlugin(pluginName, registerapi.DevicePlugin, socketPath, supportedVersions...)
@@ -139,16 +138,17 @@ func TestPluginRegistration(t *testing.T) {
 }
 
 func TestPluginRegistrationSameName(t *testing.T) {
-	defer cleanup(t)
+	socketDir := initTempDir(t)
+	defer os.RemoveAll(socketDir)
 
 	dsw := cache.NewDesiredStateOfWorld()
-	newWatcher(t, dsw, wait.NeverStop)
+	newWatcher(t, socketDir, dsw, wait.NeverStop)
 
 	// Make 10 plugins with the same name and same type but different socket path;
 	// all 10 should be in desired state of world cache
 	pluginName := "dep-example-plugin"
 	for i := 0; i < 10; i++ {
-		socketPath := fmt.Sprintf("%s/plugin-%d.sock", socketDir, i)
+		socketPath := filepath.Join(socketDir, fmt.Sprintf("plugin-%d.sock", i))
 		p := NewTestExamplePlugin(pluginName, registerapi.DevicePlugin, socketPath, supportedVersions...)
 		require.NoError(t, p.Serve("v1beta1", "v1beta2"))
 
@@ -164,14 +164,15 @@ func TestPluginRegistrationSameName(t *testing.T) {
 }
 
 func TestPluginReRegistration(t *testing.T) {
-	defer cleanup(t)
+	socketDir := initTempDir(t)
+	defer os.RemoveAll(socketDir)
 
 	dsw := cache.NewDesiredStateOfWorld()
-	newWatcher(t, dsw, wait.NeverStop)
+	newWatcher(t, socketDir, dsw, wait.NeverStop)
 
 	// Create a plugin first, we are then going to remove the plugin, update the plugin with a different name
 	// and recreate it.
-	socketPath := fmt.Sprintf("%s/plugin-reregistration.sock", socketDir)
+	socketPath := filepath.Join(socketDir, "plugin-reregistration.sock")
 	pluginName := "reregister-plugin"
 	p := NewTestExamplePlugin(pluginName, registerapi.DevicePlugin, socketPath, supportedVersions...)
 	require.NoError(t, p.Serve("v1beta1", "v1beta2"))
@@ -206,12 +207,13 @@ func TestPluginReRegistration(t *testing.T) {
 }
 
 func TestPluginRegistrationAtKubeletStart(t *testing.T) {
-	defer cleanup(t)
+	socketDir := initTempDir(t)
+	defer os.RemoveAll(socketDir)
 
 	plugins := make([]*examplePlugin, 10)
 
 	for i := 0; i < len(plugins); i++ {
-		socketPath := fmt.Sprintf("%s/plugin-%d.sock", socketDir, i)
+		socketPath := filepath.Join(socketDir, fmt.Sprintf("plugin-%d.sock", i))
 		pluginName := fmt.Sprintf("example-plugin-%d", i)
 
 		p := NewTestExamplePlugin(pluginName, registerapi.DevicePlugin, socketPath, supportedVersions...)
@@ -224,7 +226,7 @@ func TestPluginRegistrationAtKubeletStart(t *testing.T) {
 	}
 
 	dsw := cache.NewDesiredStateOfWorld()
-	newWatcher(t, dsw, wait.NeverStop)
+	newWatcher(t, socketDir, dsw, wait.NeverStop)
 
 	var wg sync.WaitGroup
 	for i := 0; i < len(plugins); i++ {
@@ -252,7 +254,7 @@ func TestPluginRegistrationAtKubeletStart(t *testing.T) {
 	}
 }
 
-func newWatcher(t *testing.T, desiredStateOfWorldCache cache.DesiredStateOfWorld, stopCh <-chan struct{}) *Watcher {
+func newWatcher(t *testing.T, socketDir string, desiredStateOfWorldCache cache.DesiredStateOfWorld, stopCh <-chan struct{}) *Watcher {
 	w := NewWatcher(socketDir, desiredStateOfWorldCache)
 	require.NoError(t, w.Start(stopCh))
 

diff --git a/pkg/kubelet/pluginmanager/reconciler/reconciler_test.go b/pkg/kubelet/pluginmanager/reconciler/reconciler_test.go
@@ -19,6 +19,7 @@ package reconciler
 import (
 	"fmt"
 	"os"
+	"path/filepath"
 	"testing"
 	"time"
 
@@ -187,7 +188,7 @@ func Test_Run_Positive_Register(t *testing.T) {
 	stopChan := make(chan struct{})
 	defer close(stopChan)
 	go reconciler.Run(stopChan)
-	socketPath := fmt.Sprintf("%s/plugin.sock", socketDir)
+	socketPath := filepath.Join(socketDir, "plugin.sock")
 	pluginName := fmt.Sprintf("example-plugin")
 	p := pluginwatcher.NewTestExamplePlugin(pluginName, registerapi.DevicePlugin, socketPath, supportedVersions...)
 	require.NoError(t, p.Serve("v1beta1", "v1beta2"))
@@ -233,7 +234,7 @@ func Test_Run_Positive_RegisterThenUnregister(t *testing.T) {
 	defer close(stopChan)
 	go reconciler.Run(stopChan)
 
-	socketPath := fmt.Sprintf("%s/plugin.sock", socketDir)
+	socketPath := filepath.Join(socketDir, "plugin.sock")
 	pluginName := fmt.Sprintf("example-plugin")
 	p := pluginwatcher.NewTestExamplePlugin(pluginName, registerapi.DevicePlugin, socketPath, supportedVersions...)
 	require.NoError(t, p.Serve("v1beta1", "v1beta2"))
@@ -289,7 +290,7 @@ func Test_Run_Positive_ReRegister(t *testing.T) {
 	defer close(stopChan)
 	go reconciler.Run(stopChan)
 
-	socketPath := fmt.Sprintf("%s/plugin2.sock", socketDir)
+	socketPath := filepath.Join(socketDir, "plugin2.sock")
 	pluginName := fmt.Sprintf("example-plugin2")
 	p := pluginwatcher.NewTestExamplePlugin(pluginName, registerapi.DevicePlugin, socketPath, supportedVersions...)
 	require.NoError(t, p.Serve("v1beta1", "v1beta2"))

diff --git a/pkg/proxy/apis/config/validation/validation_test.go b/pkg/proxy/apis/config/validation/validation_test.go
@@ -427,6 +427,11 @@ func TestValidateKubeProxyConfiguration(t *testing.T) {
 	}
 
 	for name, testCase := range testCases {
+		if runtime.GOOS == "windows" && testCase.config.Mode == kubeproxyconfig.ProxyModeIPVS {
+			// IPVS is not supported on Windows.
+			t.Log("Skipping test on Windows: ", name)
+			continue
+		}
 		t.Run(name, func(t *testing.T) {
 			errs := Validate(&testCase.config)
 			if len(testCase.expectedErrs) != len(errs) {
@@ -694,9 +699,11 @@ func TestValidateKubeProxyConntrackConfiguration(t *testing.T) {
 func TestValidateProxyMode(t *testing.T) {
 	newPath := field.NewPath("KubeProxyConfiguration")
 	successCases := []kubeproxyconfig.ProxyMode{""}
+	expectedNonExistentErrorMsg := "must be iptables,ipvs or blank (blank means the best-available proxy [currently iptables])"
 
 	if runtime.GOOS == "windows" {
 		successCases = append(successCases, kubeproxyconfig.ProxyModeKernelspace)
+		expectedNonExistentErrorMsg = "must be kernelspace or blank (blank means the most-available proxy [currently kernelspace])"
 	} else {
 		successCases = append(successCases, kubeproxyconfig.ProxyModeIPTables, kubeproxyconfig.ProxyModeIPVS)
 	}
@@ -717,7 +724,7 @@ func TestValidateProxyMode(t *testing.T) {
 		},
 		"invalid mode non-existent": {
 			mode:         kubeproxyconfig.ProxyMode("non-existing"),
-			expectedErrs: field.ErrorList{field.Invalid(newPath.Child("ProxyMode"), "non-existing", "must be iptables,ipvs or blank (blank means the best-available proxy [currently iptables])")},
+			expectedErrs: field.ErrorList{field.Invalid(newPath.Child("ProxyMode"), "non-existing", expectedNonExistentErrorMsg)},
 		},
 	}
 	for _, testCase := range testCases {

diff --git a/pkg/proxy/winkernel/hns_test.go b/pkg/proxy/winkernel/hns_test.go
@@ -132,11 +132,11 @@ func TestGetEndpointByIpAddressAndName(t *testing.T) {
 		t.Errorf("%v does not match %v", endpoint.ip, Endpoint.IpConfigurations[0].IpAddress)
 	}
 
-	endpoint, err = hns.getEndpointByName(Endpoint.Name)
+	endpoint2, err := hns.getEndpointByName(Endpoint.Name)
 	if err != nil {
 		t.Error(err)
 	}
-	diff := cmp.Diff(endpoint, Endpoint)
+	diff := cmp.Diff(endpoint, endpoint2)
 	if diff != "" {
 		t.Errorf("getEndpointByName(%s) returned a different endpoint. Diff: %s ", Endpoint.Name, diff)
 	}

diff --git a/pkg/proxy/winkernel/proxier_test.go b/pkg/proxy/winkernel/proxier_test.go
@@ -138,18 +138,19 @@ func NewFakeProxier(syncPeriod time.Duration, minSyncPeriod time.Duration, clust
 		networkType: networkType,
 	}
 	proxier := &Proxier{
-		serviceMap:          make(proxy.ServiceMap),
-		endpointsMap:        make(proxy.EndpointsMap),
-		clusterCIDR:         clusterCIDR,
-		hostname:            testHostName,
-		nodeIP:              nodeIP,
-		serviceHealthServer: healthcheck.NewFakeServiceHealthServer(),
-		network:             *hnsNetworkInfo,
-		sourceVip:           sourceVip,
-		hostMac:             macAddress,
-		isDSR:               false,
-		hns:                 newFakeHNS(),
-		endPointsRefCount:   make(endPointsReferenceCountMap),
+		serviceMap:            make(proxy.ServiceMap),
+		endpointsMap:          make(proxy.EndpointsMap),
+		clusterCIDR:           clusterCIDR,
+		hostname:              testHostName,
+		nodeIP:                nodeIP,
+		serviceHealthServer:   healthcheck.NewFakeServiceHealthServer(),
+		network:               *hnsNetworkInfo,
+		sourceVip:             sourceVip,
+		hostMac:               macAddress,
+		isDSR:                 false,
+		hns:                   newFakeHNS(),
+		endPointsRefCount:     make(endPointsReferenceCountMap),
+		forwardHealthCheckVip: true,
 	}
 
 	serviceChanges := proxy.NewServiceChangeTracker(proxier.newServiceInfo, v1.IPv4Protocol, nil, proxier.serviceMapChange)

diff --git a/pkg/routes/const_other.go b/pkg/routes/const_other.go
@@ -0,0 +1,26 @@
+//go:build !windows
+// +build !windows
+
+/*
+Copyright 2022 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package routes
+
+import "syscall"
+
+const (
+	fileNameTooLong = syscall.ENAMETOOLONG
+)
diff --git a/pkg/routes/const_windows.go b/pkg/routes/const_windows.go
@@ -0,0 +1,23 @@
+/*
+Copyright 2022 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package routes
+
+import "golang.org/x/sys/windows"
+
+const (
+	fileNameTooLong = windows.ERROR_FILENAME_EXCED_RANGE
+)
diff --git a/pkg/routes/logs.go b/pkg/routes/logs.go
@@ -20,7 +20,6 @@ import (
 	"net/http"
 	"os"
 	"path"
-	"syscall"
 
 	"github.com/emicklei/go-restful/v3"
 )
@@ -63,7 +62,7 @@ func logFileListHandler(req *restful.Request, resp *restful.Response) {
 func logFileNameIsTooLong(filePath string) bool {
 	_, err := os.Stat(filePath)
 	if err != nil {
-		if e, ok := err.(*os.PathError); ok && e.Err == syscall.ENAMETOOLONG {
+		if e, ok := err.(*os.PathError); ok && e.Err == fileNameTooLong {
 			return true
 		}
 	}

diff --git a/pkg/routes/logs_test.go b/pkg/routes/logs_test.go
@@ -19,11 +19,13 @@ package routes
 import (
 	"fmt"
 	"os"
+	"path/filepath"
 	"testing"
 )
 
 func TestPreCheckLogFileNameLength(t *testing.T) {
-	oversizeFileName := fmt.Sprintf("%0256s", "a")
+	// In windows, with long file name support enabled, file names can have up to 32,767 characters.
+	oversizeFileName := fmt.Sprintf("%032768s", "a")
 	normalFileName := fmt.Sprintf("%0255s", "a")
 
 	// check file with oversize name.
@@ -37,11 +39,19 @@ func TestPreCheckLogFileNameLength(t *testing.T) {
 	}
 
 	// check file with normal name which does exist.
-	_, err := os.Create(normalFileName)
+	dir, err := os.MkdirTemp("", "logs")
+	if err != nil {
+		t.Fatal("failed to create temp dir")
+	}
+	defer os.RemoveAll(dir)
+
+	normalFileName = filepath.Join(dir, normalFileName)
+	f, err := os.Create(normalFileName)
 	if err != nil {
 		t.Error("failed to create test file")
 	}
 	defer os.Remove(normalFileName)
+	defer f.Close()
 	if logFileNameIsTooLong(normalFileName) {
 		t.Error("failed to check normal filename")
 	}

diff --git a/pkg/scheduler/internal/queue/scheduling_queue_test.go b/pkg/scheduler/internal/queue/scheduling_queue_test.go
@@ -1957,8 +1957,14 @@ func TestMoveAllToActiveOrBackoffQueue_PreEnqueueChecks(t *testing.T) {
 			ctx, cancel := context.WithCancel(context.Background())
 			defer cancel()
 			q := NewTestQueue(ctx, newDefaultQueueSort())
-			for _, podInfo := range tt.podInfos {
+			for i, podInfo := range tt.podInfos {
 				q.AddUnschedulableIfNotPresent(podInfo, q.schedulingCycle)
+				// NOTE: On Windows, time.Now() is not as precise, 2 consecutive calls may return the same timestamp,
+				// resulting in 0 time delta / latency. This will cause the pods to be backed off in a random
+				// order, which would cause this test to fail, since the expectation is for them to be backed off
+				// in a certain order.
+				// See: https://github.com/golang/go/issues/8687
+				podInfo.Timestamp = podInfo.Timestamp.Add(time.Duration((i - len(tt.podInfos))) * time.Millisecond)
 			}
 			q.MoveAllToActiveOrBackoffQueue(TestEvent, tt.preEnqueueCheck)
 			var got []string

diff --git a/pkg/util/removeall/removeall_test.go b/pkg/util/removeall/removeall_test.go
@@ -20,6 +20,7 @@ import (
 	"errors"
 	"os"
 	"path"
+	"path/filepath"
 	"strings"
 	"testing"
 
@@ -33,7 +34,7 @@ type fakeMounter struct {
 
 // IsLikelyNotMountPoint overrides mount.FakeMounter.IsLikelyNotMountPoint for our use.
 func (f *fakeMounter) IsLikelyNotMountPoint(file string) (bool, error) {
-	name := path.Base(file)
+	name := filepath.Base(file)
 	if strings.HasPrefix(name, "mount") {
 		return false, nil
 	}