From d3237b72587fb0be5d0a9f9a2719fa0115d17948 Mon Sep 17 00:00:00 2001 From: Claudiu Belu Date: Thu, 15 Dec 2022 16:03:37 +0000 Subject: [PATCH] unittests: Fixes unit tests for Windows (part 7) Currently, there are some unit tests that are failing on Windows due to various reasons: - if a powershell command that could return an array (e.g.: Get-Disk) would return an array of only one element, powershell will in fact return that object directly, and **not** an array containing that element. In a few cases, these commands are used and their output is converted to json, after which they're unmarshalled in golang, with the expectation that the unmarshalled data to be an array. If it's not an array, we get an error. - when mounting Block Devices, Windows expects the given source to be a Disk Number, not a path. - for rbd_windows_test.go, we should start with Disk Number 0, which exists on all hosts. - if a Disk has multiple volumes, Get-Volume doesn't return the volumes in the same order. This can result in various assertions failing. - the pkg/volume/rbd/rdb_test.TestPlugin test expects that mounter.MountSensitive is called when attacher.MountDevice is called. The Windows attacher doesn't currently make that call. --- pkg/volume/local/local_test.go | 29 +++++++++++-------- pkg/volume/rbd/rbd_test.go | 9 ++---- pkg/volume/rbd/rbd_unix_test.go | 3 +- pkg/volume/rbd/rbd_windows_test.go | 25 ++++------------ .../vsphere_volume_util_windows.go | 4 +++ .../src/k8s.io/mount-utils/mount_windows.go | 18 ++++-------- 6 files changed, 37 insertions(+), 51 deletions(-) diff --git a/pkg/volume/local/local_test.go b/pkg/volume/local/local_test.go index 0a956aaf654..8d549ae7c15 100644 --- a/pkg/volume/local/local_test.go +++ b/pkg/volume/local/local_test.go @@ -136,30 +136,40 @@ func getPersistentPlugin(t *testing.T) (string, volume.PersistentVolumePlugin) { } func getDeviceMountablePluginWithBlockPath(t *testing.T, isBlockDevice bool) (string, volume.DeviceMountableVolumePlugin) { - tmpDir, err := utiltesting.MkTmpdir("localVolumeTest") - if err != nil { - t.Fatalf("can't make a temp dir: %v", err) + var ( + source string + err error + ) + + if isBlockDevice && runtime.GOOS == "windows" { + // On Windows, block devices are referenced by the disk number, which is validated by the mounter, + source = "0" + } else { + source, err = utiltesting.MkTmpdir("localVolumeTest") + if err != nil { + t.Fatalf("can't make a temp dir: %v", err) + } } plugMgr := volume.VolumePluginMgr{} var pathToFSType map[string]hostutil.FileType if isBlockDevice { pathToFSType = map[string]hostutil.FileType{ - tmpDir: hostutil.FileTypeBlockDev, + source: hostutil.FileTypeBlockDev, } } - plugMgr.InitPlugins(ProbeVolumePlugins(), nil /* prober */, volumetest.NewFakeKubeletVolumeHostWithMounterFSType(t, tmpDir, nil, nil, pathToFSType)) + plugMgr.InitPlugins(ProbeVolumePlugins(), nil /* prober */, volumetest.NewFakeKubeletVolumeHostWithMounterFSType(t, source, nil, nil, pathToFSType)) plug, err := plugMgr.FindDeviceMountablePluginByName(localVolumePluginName) if err != nil { - os.RemoveAll(tmpDir) + os.RemoveAll(source) t.Fatalf("Can't find the plugin by name") } if plug.GetPluginName() != localVolumePluginName { t.Errorf("Wrong name: %s", plug.GetPluginName()) } - return tmpDir, plug + return source, plug } func getTestVolume(readOnly bool, path string, isBlock bool, mountOptions []string) *volume.Spec { @@ -244,11 +254,6 @@ func TestInvalidLocalPath(t *testing.T) { } func TestBlockDeviceGlobalPathAndMountDevice(t *testing.T) { - // Skip tests that fail on Windows, as discussed during the SIG Testing meeting from January 10, 2023 - if runtime.GOOS == "windows" { - t.Skip("Skipping test that fails on Windows") - } - // Block device global mount path testing tmpBlockDir, plug := getDeviceMountablePluginWithBlockPath(t, true) defer os.RemoveAll(tmpBlockDir) diff --git a/pkg/volume/rbd/rbd_test.go b/pkg/volume/rbd/rbd_test.go index 2e0b608ddad..b11246666ce 100644 --- a/pkg/volume/rbd/rbd_test.go +++ b/pkg/volume/rbd/rbd_test.go @@ -359,11 +359,6 @@ type testcase struct { } func TestPlugin(t *testing.T) { - // Skip tests that fail on Windows, as discussed during the SIG Testing meeting from January 10, 2023 - if runtime.GOOS == "windows" { - t.Skip("Skipping test that fails on Windows") - } - tmpDir, err := utiltesting.MkTmpdir("rbd_test") if err != nil { t.Fatalf("error creating temp dir: %v", err) @@ -374,10 +369,10 @@ func TestPlugin(t *testing.T) { t.Fatal(err) } - expectedDevicePath := "/dev/rbd1" + expectedDevicePath := "/dev/rbd0" if runtime.GOOS == "windows" { // Windows expects Disk Numbers. - expectedDevicePath = "1" + expectedDevicePath = "0" } podUID := uuid.NewUUID() diff --git a/pkg/volume/rbd/rbd_unix_test.go b/pkg/volume/rbd/rbd_unix_test.go index 4c7877be013..1c935be80f0 100644 --- a/pkg/volume/rbd/rbd_unix_test.go +++ b/pkg/volume/rbd/rbd_unix_test.go @@ -26,9 +26,10 @@ import ( func (fake *fakeDiskManager) AttachDisk(b rbdMounter) (string, error) { fake.mutex.Lock() defer fake.mutex.Unlock() - fake.rbdMapIndex++ devicePath := fmt.Sprintf("/dev/rbd%d", fake.rbdMapIndex) fake.rbdDevices[devicePath] = true + // Increment rbdMapIndex afterwards, so we can start from rbd0. + fake.rbdMapIndex++ return devicePath, nil } diff --git a/pkg/volume/rbd/rbd_windows_test.go b/pkg/volume/rbd/rbd_windows_test.go index 208b332493c..a915dc30de8 100644 --- a/pkg/volume/rbd/rbd_windows_test.go +++ b/pkg/volume/rbd/rbd_windows_test.go @@ -20,42 +20,29 @@ limitations under the License. package rbd import ( - "fmt" - "os/exec" "strconv" - "strings" + + "k8s.io/mount-utils" ) func (fake *fakeDiskManager) AttachDisk(b rbdMounter) (string, error) { fake.mutex.Lock() defer fake.mutex.Unlock() - fake.rbdMapIndex++ - // Windows expects Disk Numbers. - volIds, err := listVolumesOnDisk(strconv.Itoa(fake.rbdMapIndex)) + // Windows expects Disk Numbers. We start with rbdMapIndex 0, referring to the first Disk. + volIds, err := mount.ListVolumesOnDisk(strconv.Itoa(fake.rbdMapIndex)) if err != nil { return "", err } fake.rbdDevices[volIds[0]] = true devicePath := strconv.Itoa(fake.rbdMapIndex) + fake.rbdMapIndex++ return devicePath, nil } -// listVolumesOnDisk - returns back list of volumes(volumeIDs) in the disk (requested in diskID) on Windows. -func listVolumesOnDisk(diskID string) (volumeIDs []string, err error) { - cmd := fmt.Sprintf("(Get-Disk -DeviceId %s | Get-Partition | Get-Volume).UniqueId", diskID) - output, err := exec.Command("powershell", "/c", cmd).CombinedOutput() - if err != nil { - return []string{}, fmt.Errorf("error list volumes on disk. cmd: %s, output: %s, error: %v", cmd, string(output), err) - } - - volumeIds := strings.Split(strings.TrimSpace(string(output)), "\r\n") - return volumeIds, nil -} - func getLoggedSource(devicePath string) (string, error) { // Windows mounter is mounting based on the Disk's Unique ID. - volIds, err := listVolumesOnDisk(devicePath) + volIds, err := mount.ListVolumesOnDisk(devicePath) if err != nil { return "", err } diff --git a/pkg/volume/vsphere_volume/vsphere_volume_util_windows.go b/pkg/volume/vsphere_volume/vsphere_volume_util_windows.go index b9f2b6c590b..ae85c73f431 100644 --- a/pkg/volume/vsphere_volume/vsphere_volume_util_windows.go +++ b/pkg/volume/vsphere_volume/vsphere_volume_util_windows.go @@ -40,6 +40,10 @@ func verifyDevicePath(path string) (string, error) { klog.V(4).Infof("Found vSphere disk attached with disk number %v", path) return path, nil } + // NOTE: If a powershell command that would return an array (e.g.: Get-Disk) would return an array of + // one element, powershell will in fact return that object directly, and **not an array containing + // that elemenent, which means piping it to ConvertTo-Json would not result in array as expected below. + // The following syntax forces it to always be an array. cmd := exec.Command("powershell", "/c", "Get-Disk | Select Number, SerialNumber | ConvertTo-JSON") output, err := cmd.Output() if err != nil { diff --git a/staging/src/k8s.io/mount-utils/mount_windows.go b/staging/src/k8s.io/mount-utils/mount_windows.go index 7b2a2d1a15e..4f2b7aee873 100644 --- a/staging/src/k8s.io/mount-utils/mount_windows.go +++ b/staging/src/k8s.io/mount-utils/mount_windows.go @@ -299,26 +299,20 @@ func (mounter *SafeFormatAndMount) formatAndMountSensitive(source string, target } klog.V(4).Infof("diskMount: Disk successfully formatted, disk: %q, fstype: %q", source, fstype) - volumeIds, err := listVolumesOnDisk(source) + volumeIds, err := ListVolumesOnDisk(source) if err != nil { return err } driverPath := volumeIds[0] - target = NormalizeWindowsPath(target) - output, err := mounter.Exec.Command("cmd", "/c", "mklink", "/D", target, driverPath).CombinedOutput() - if err != nil { - klog.Errorf("mklink(%s, %s) failed: %v, output: %q", target, driverPath, err, string(output)) - return err - } - klog.V(2).Infof("formatAndMount disk(%s) fstype(%s) on(%s) with output(%s) successfully", driverPath, fstype, target, string(output)) - return nil + return mounter.MountSensitive(driverPath, target, fstype, options, sensitiveOptions) } // ListVolumesOnDisk - returns back list of volumes(volumeIDs) in the disk (requested in diskID). -func listVolumesOnDisk(diskID string) (volumeIDs []string, err error) { - cmd := fmt.Sprintf("(Get-Disk -DeviceId %s | Get-Partition | Get-Volume).UniqueId", diskID) +func ListVolumesOnDisk(diskID string) (volumeIDs []string, err error) { + // If a Disk has multiple volumes, Get-Volume may not return items in the same order. + cmd := fmt.Sprintf("(Get-Disk -DeviceId %s | Get-Partition | Get-Volume | Sort-Object -Property UniqueId).UniqueId", diskID) output, err := exec.Command("powershell", "/c", cmd).CombinedOutput() - klog.V(4).Infof("listVolumesOnDisk id from %s: %s", diskID, string(output)) + klog.V(4).Infof("ListVolumesOnDisk id from %s: %s", diskID, string(output)) if err != nil { return []string{}, fmt.Errorf("error list volumes on disk. cmd: %s, output: %s, error: %v", cmd, string(output), err) }