Merge pull request #51097 from jsafrane/exec-azure

Automatic merge from submit-queue (batch tested with PRs 51105, 51097, 51110, 50843, 51107)

azure: Use VolumeHost.GetExec() to execute stuff in volume plugins

**What this PR does / why we need it**:

This PR updates Azure volume plugin to use `VolumeHost.GetExec()` to execute utilities like mkfs and lsblk instead of simple `os/exec`. This prepares the volume plugin to run these utilities in containers instead of running them on the host + makes the volume plugin more independent and less hardcoded.

See proposal in https://github.com/kubernetes/community/pull/589.

Note that this PR does **not** change place where utilities are executed - `VolumeHost.GetExec()` still leads directly to `os/exec`. It will be changed when the aforementioned proposal is merged and implemented.

To simplify testing, `/sys/bus/scsi/devices/*/model` and `.../vendor` are read using `ioutil.ReadFile` instead of `/bin/cat` in the first commit.

```release-note
NONE
```

@kubernetes/sig-storage-pr-reviews @kubernetes/sig-azure-misc

pkg/volume/azure_dd/BUILD

@@ -34,7 +34,6 @@ go_library(
         "//vendor/k8s.io/apimachinery/pkg/types:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/util/sets:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/util/wait:go_default_library",
-        "//vendor/k8s.io/utils/exec:go_default_library",
     ],
 )
 
@@ -63,7 +62,5 @@ go_test(
         "//pkg/volume/testing:go_default_library",
         "//vendor/k8s.io/api/core/v1:go_default_library",
         "//vendor/k8s.io/client-go/util/testing:go_default_library",
-        "//vendor/k8s.io/utils/exec:go_default_library",
-        "//vendor/k8s.io/utils/exec/testing:go_default_library",
     ],
 )

pkg/volume/azure_dd/attacher.go

@@ -36,7 +36,6 @@ import (
 	"k8s.io/kubernetes/pkg/volume"
 	volumeutil "k8s.io/kubernetes/pkg/volume/util"
 	"k8s.io/kubernetes/pkg/volume/util/volumehelper"
-	"k8s.io/utils/exec"
 )
 
 type azureDiskDetacher struct {
@@ -171,9 +170,7 @@ func (a *azureDiskAttacher) WaitForAttach(spec *volume.Spec, devicePath string,
 	newDevicePath := ""
 
 	err = wait.Poll(1*time.Second, timeout, func() (bool, error) {
-		exe := exec.New()
-
-		if newDevicePath, err = findDiskByLun(lun, io, exe); err != nil {
+		if newDevicePath, err = findDiskByLun(lun, io); err != nil {
 			return false, fmt.Errorf("azureDisk - WaitForAttach ticker failed node (%s) disk (%s) lun(%v) err(%s)", nodeName, diskName, lun, err)
 		}
 
@@ -182,7 +179,7 @@ func (a *azureDiskAttacher) WaitForAttach(spec *volume.Spec, devicePath string,
 			// the curent sequence k8s uses for unformated disk (check-disk, mount, fail, mkfs.extX) hangs on
 			// Azure Managed disk scsi interface. this is a hack and will be replaced once we identify and solve
 			// the root case on Azure.
-			formatIfNotFormatted(newDevicePath, *volumeSource.FSType)
+			formatIfNotFormatted(newDevicePath, *volumeSource.FSType, a.plugin.host.GetExec(a.plugin.GetPluginName()))
 			return true, nil
 		}
 

pkg/volume/azure_dd/azure_common.go

@@ -21,7 +21,6 @@ import (
 	"io/ioutil"
 	"os"
 	"path"
-	"regexp"
 	"strconv"
 	libstrings "strings"
 
@@ -35,7 +34,6 @@ import (
 	"k8s.io/kubernetes/pkg/util/mount"
 	"k8s.io/kubernetes/pkg/util/strings"
 	"k8s.io/kubernetes/pkg/volume"
-	"k8s.io/utils/exec"
 )
 
 const (
@@ -158,6 +156,7 @@ type ioHandler interface {
 	ReadDir(dirname string) ([]os.FileInfo, error)
 	WriteFile(filename string, data []byte, perm os.FileMode) error
 	Readlink(name string) (string, error)
+	ReadFile(filename string) ([]byte, error)
 }
 
 //TODO: check if priming the iscsi interface is actually needed
@@ -176,6 +175,10 @@ func (handler *osIOHandler) Readlink(name string) (string, error) {
 	return os.Readlink(name)
 }
 
+func (handler *osIOHandler) ReadFile(filename string) ([]byte, error) {
+	return ioutil.ReadFile(filename)
+}
+
 // exclude those used by azure as resource and OS root in /dev/disk/azure
 func listAzureDiskPath(io ioHandler) []string {
 	azureDiskPath := "/dev/disk/azure/"
@@ -209,13 +212,13 @@ func scsiHostRescan(io ioHandler) {
 	}
 }
 
-func findDiskByLun(lun int, io ioHandler, exe exec.Interface) (string, error) {
+func findDiskByLun(lun int, io ioHandler) (string, error) {
 	azureDisks := listAzureDiskPath(io)
-	return findDiskByLunWithConstraint(lun, io, exe, azureDisks)
+	return findDiskByLunWithConstraint(lun, io, azureDisks)
 }
 
 // finds a device mounted to "current" node
-func findDiskByLunWithConstraint(lun int, io ioHandler, exe exec.Interface, azureDisks []string) (string, error) {
+func findDiskByLunWithConstraint(lun int, io ioHandler, azureDisks []string) (string, error) {
 	var err error
 	sys_path := "/sys/bus/scsi/devices"
 	if dirs, err := io.ReadDir(sys_path); err == nil {
@@ -237,18 +240,30 @@ func findDiskByLunWithConstraint(lun int, io ioHandler, exe exec.Interface, azur
 			if lun == l {
 				// find the matching LUN
 				// read vendor and model to ensure it is a VHD disk
-				vendor := path.Join(sys_path, name, "vendor")
-				model := path.Join(sys_path, name, "model")
-				out, err := exe.Command("cat", vendor, model).CombinedOutput()
+				vendorPath := path.Join(sys_path, name, "vendor")
+				vendorBytes, err := io.ReadFile(vendorPath)
 				if err != nil {
-					glog.V(4).Infof("azure disk - failed to cat device vendor and model, err: %v", err)
+					glog.Errorf("failed to read device vendor, err: %v", err)
 					continue
 				}
-				matched, err := regexp.MatchString("^MSFT[ ]{0,}\nVIRTUAL DISK[ ]{0,}\n$", libstrings.ToUpper(string(out)))
-				if err != nil || !matched {
-					glog.V(4).Infof("azure disk - doesn't match VHD, output %v, error %v", string(out), err)
+				vendor := libstrings.TrimSpace(string(vendorBytes))
+				if libstrings.ToUpper(vendor) != "MSFT" {
+					glog.V(4).Infof("vendor doesn't match VHD, got %s", vendor)
 					continue
 				}
+
+				modelPath := path.Join(sys_path, name, "model")
+				modelBytes, err := io.ReadFile(modelPath)
+				if err != nil {
+					glog.Errorf("failed to read device model, err: %v", err)
+					continue
+				}
+				model := libstrings.TrimSpace(string(modelBytes))
+				if libstrings.ToUpper(model) != "VIRTUAL DISK" {
+					glog.V(4).Infof("model doesn't match VHD, got %s", model)
+					continue
+				}
+
 				// find a disk, validate name
 				dir := path.Join(sys_path, name, "block")
 				if dev, err := io.ReadDir(dir); err == nil {
@@ -270,8 +285,8 @@ func findDiskByLunWithConstraint(lun int, io ioHandler, exe exec.Interface, azur
 	return "", err
 }
 
-func formatIfNotFormatted(disk string, fstype string) {
-	notFormatted, err := diskLooksUnformatted(disk)
+func formatIfNotFormatted(disk string, fstype string, exec mount.Exec) {
+	notFormatted, err := diskLooksUnformatted(disk, exec)
 	if err == nil && notFormatted {
 		args := []string{disk}
 		// Disk is unformatted so format it.
@@ -283,9 +298,8 @@ func formatIfNotFormatted(disk string, fstype string) {
 			args = []string{"-E", "lazy_itable_init=0,lazy_journal_init=0", "-F", disk}
 		}
 		glog.Infof("azureDisk - Disk %q appears to be unformatted, attempting to format as type: %q with options: %v", disk, fstype, args)
-		runner := exec.New()
-		cmd := runner.Command("mkfs."+fstype, args...)
-		_, err := cmd.CombinedOutput()
+
+		_, err := exec.Run("mkfs."+fstype, args...)
 		if err == nil {
 			// the disk has been formatted successfully try to mount it again.
 			glog.Infof("azureDisk - Disk successfully formatted (mkfs): %s - %s %s", fstype, disk, "tt")
@@ -300,12 +314,10 @@ func formatIfNotFormatted(disk string, fstype string) {
 	}
 }
 
-func diskLooksUnformatted(disk string) (bool, error) {
+func diskLooksUnformatted(disk string, exec mount.Exec) (bool, error) {
 	args := []string{"-nd", "-o", "FSTYPE", disk}
-	runner := exec.New()
-	cmd := runner.Command("lsblk", args...)
 	glog.V(4).Infof("Attempting to determine if disk %q is formatted using lsblk with args: (%v)", disk, args)
-	dataOut, err := cmd.CombinedOutput()
+	dataOut, err := exec.Run("lsblk", args...)
 	if err != nil {
 		glog.Errorf("Could not determine if disk %q is formatted (%v)", disk, err)
 		return false, err

pkg/volume/azure_dd/azure_common_test.go

@@ -19,11 +19,9 @@ package azure_dd
 import (
 	"fmt"
 	"os"
+	"strings"
 	"testing"
 	"time"
-
-	"k8s.io/utils/exec"
-	fakeexec "k8s.io/utils/exec/testing"
 )
 
 type fakeFileInfo struct {
@@ -107,27 +105,18 @@ func (handler *fakeIOHandler) Readlink(name string) (string, error) {
 	return "/dev/azure/disk/sda", nil
 }
 
+func (handler *fakeIOHandler) ReadFile(filename string) ([]byte, error) {
+	if strings.HasSuffix(filename, "vendor") {
+		return []byte("Msft    \n"), nil
+	}
+	if strings.HasSuffix(filename, "model") {
+		return []byte("Virtual Disk \n"), nil
+	}
+	return nil, fmt.Errorf("unknown file")
+}
+
 func TestIoHandler(t *testing.T) {
-	var fcmd fakeexec.FakeCmd
-	fcmd = fakeexec.FakeCmd{
-		CombinedOutputScript: []fakeexec.FakeCombinedOutputAction{
-			// cat
-			func() ([]byte, error) {
-				return []byte("Msft    \nVirtual Disk \n"), nil
-			},
-			// cat
-			func() ([]byte, error) {
-				return []byte("Msft    \nVirtual Disk \n"), nil
-			},
-		},
-	}
-	fake := fakeexec.FakeExec{
-		CommandScript: []fakeexec.FakeCommandAction{
-			func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) },
-			func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) },
-		},
-	}
-	disk, err := findDiskByLun(lun, &fakeIOHandler{}, &fake)
+	disk, err := findDiskByLun(lun, &fakeIOHandler{})
 	// if no disk matches lun, exit
 	if disk != "/dev/"+devName || err != nil {
 		t.Errorf("no data disk found: disk %v err %v", disk, err)