refactor drivers util methods

internal/controller/drivers/doc.go

@@ -0,0 +1,14 @@
+//go:build windows
+
+// Package drivers provides utility methods for installing drivers into
+// Linux or Windows utility VMs (UVMs).
+//
+// These utility methods are used by 'containerd-shim-runhcs-v1' as well as
+// V2 shims to install the drivers.
+//
+// For LCOW guests, driver installation is performed by executing the
+// 'install-drivers' binary inside the guest via [ExecGCSInstallDriver].
+//
+// For WCOW guests, driver installation is performed by invoking 'pnputil'
+// inside the UVM via [ExecPnPInstallDriver].
+package drivers

internal/controller/drivers/drivers_lcow.go

@@ -0,0 +1,131 @@
+//go:build windows
+
+package drivers
+
+import (
+	"context"
+	"errors"
+	"fmt"
+	"io"
+	"net"
+
+	"github.com/Microsoft/go-winio/pkg/guid"
+
+	"github.com/Microsoft/hcsshim/internal/cmd"
+	"github.com/Microsoft/hcsshim/internal/guestpath"
+)
+
+var errNoExecOutput = errors.New("failed to get any pipe output")
+
+// guest is the UVM instance in which the driver will be installed.
+type guest interface {
+	ExecInUVM(ctx context.Context, request *cmd.CmdProcessRequest) (int, error)
+}
+
+// ExecGCSInstallDriver installs a driver into the UVM by running 'install-drivers'
+// inside the guest. hostPath is the host VHD path and guestPath is the
+// SCSI-mounted location inside the UVM. Returns an error if installation fails,
+// along with any stderr output from the guest process.
+func ExecGCSInstallDriver(ctx context.Context, guest guest, hostPath string, guestPath string) error {
+	driverReadWriteDir, err := getDriverWorkDir(hostPath)
+	if err != nil {
+		return fmt.Errorf("failed to create a guid path for driver %+v: %w", hostPath, err)
+	}
+
+	p, l, err := cmd.CreateNamedPipeListener()
+	if err != nil {
+		return err
+	}
+	defer l.Close()
+
+	var stderrOutput string
+	errChan := make(chan error)
+
+	go readAllPipeOutput(l, errChan, &stderrOutput)
+
+	args := []string{
+		"/bin/install-drivers",
+		driverReadWriteDir,
+		guestPath,
+	}
+	req := &cmd.CmdProcessRequest{
+		Args:   args,
+		Stderr: p,
+	}
+
+	// A call to `ExecInUvm` may fail in the following ways:
+	// - The process runs and exits with a non-zero exit code. In this case we need to wait on the output
+	//   from stderr so we can log it for debugging.
+	// - There's an error trying to run the process. No need to wait for stderr logs.
+	// - There's an error copying IO. No need to wait for stderr logs.
+	//
+	// Since we cannot distinguish between the cases above, we should always wait to read the stderr output.
+	exitCode, execErr := guest.ExecInUVM(ctx, req)
+
+	// wait to finish parsing stdout results
+	select {
+	case err := <-errChan:
+		if err != nil && !errors.Is(err, errNoExecOutput) {
+			return fmt.Errorf("failed to get stderr output from command %s: %w", guestPath, err)
+		}
+	case <-ctx.Done():
+		return fmt.Errorf("timed out waiting for the console output from installing driver %s: %w", guestPath, ctx.Err())
+	}
+
+	if execErr != nil {
+		return fmt.Errorf("%w: failed to install driver %s in uvm with exit code %d: %v", execErr, guestPath, exitCode, stderrOutput)
+	}
+	return nil
+}
+
+// getDriverWorkDir returns the deterministic guest path used as the overlayfs
+// root for a driver installation. 'install-drivers' uses the read-only SCSI VHD
+// as the lower layer and uses this directory for the upper, work, and content
+// directories, giving depmod/modprobe a writable view.
+//
+// If the directory already exists, 'install-drivers' skips reinstallation.
+// The path is derived from a v5 UUID seeded with the host VHD path,
+// ensuring a stable mapping across reboots.
+func getDriverWorkDir(hostPath string) (string, error) {
+	// 914aadc8-f700-4365-8016-ddad0a9d406d. Random GUID chosen for namespace.
+	ns := guid.GUID{
+		Data1: 0x914aadc8,
+		Data2: 0xf700,
+		Data3: 0x4365,
+		Data4: [8]byte{0x80, 0x16, 0xdd, 0xad, 0x0a, 0x9d, 0x40, 0x6d},
+	}
+
+	driverGUID, err := guid.NewV5(ns, []byte(hostPath))
+	if err != nil {
+		return "", err
+	}
+
+	return fmt.Sprintf(guestpath.LCOWGlobalDriverPrefixFmt, driverGUID.String()), nil
+}
+
+// readAllPipeOutput is a helper function that connects to a listener and attempts to
+// read the connection's entire output. Resulting output is returned as a string
+// in the `result` param. The `errChan` param is used to propagate an errors to
+// the calling function.
+func readAllPipeOutput(l net.Listener, errChan chan<- error, result *string) {
+	defer close(errChan)
+	c, err := l.Accept()
+	if err != nil {
+		errChan <- fmt.Errorf("failed to accept named pipe: %w", err)
+		return
+	}
+	bytes, err := io.ReadAll(c)
+	if err != nil {
+		errChan <- err
+		return
+	}
+
+	*result = string(bytes)
+
+	if len(*result) == 0 {
+		errChan <- errNoExecOutput
+		return
+	}
+
+	errChan <- nil
+}

internal/controller/drivers/drivers_wcow.go

@@ -0,0 +1,63 @@
+//go:build windows
+
+package drivers
+
+import (
+	"context"
+	"fmt"
+
+	"github.com/Microsoft/hcsshim/internal/cmd"
+	"github.com/Microsoft/hcsshim/internal/log"
+	"github.com/Microsoft/hcsshim/internal/logfields"
+	"github.com/Microsoft/hcsshim/internal/uvm"
+	"github.com/Microsoft/hcsshim/internal/winapi"
+
+	"github.com/sirupsen/logrus"
+)
+
+const (
+	uvmPnpExePath                  = "C:\\Windows\\System32\\pnputil.exe"
+	pnputilNoMoreItemsErrorMessage = `driver not ranked higher than existing driver in UVM.
+										if drivers were not previously present in the UVM, this
+										is an expected race and can be ignored.`
+)
+
+// createPnPInstallDriverCommand creates a pnputil command to add and install drivers
+// present in `driverUVMPath` and all subdirectories.
+func createPnPInstallDriverCommand(driverUVMPath string) []string {
+	dirFormatted := fmt.Sprintf("%s/*.inf", driverUVMPath)
+	args := []string{
+		"cmd",
+		"/c",
+		uvmPnpExePath,
+		"/add-driver",
+		dirFormatted,
+		"/subdirs",
+		"/install",
+	}
+	return args
+}
+
+// ExecPnPInstallDriver makes the calls to exec in the uvm the pnp command
+// that installs a driver previously mounted into the uvm.
+func ExecPnPInstallDriver(ctx context.Context, vm *uvm.UtilityVM, driverDir string) error {
+	args := createPnPInstallDriverCommand(driverDir)
+	cmdReq := &cmd.CmdProcessRequest{
+		Args: args,
+	}
+	exitCode, err := vm.ExecInUVM(ctx, cmdReq)
+	if err != nil && exitCode != winapi.ERROR_NO_MORE_ITEMS {
+		return fmt.Errorf("failed to install driver %s in uvm with exit code %d: %w", driverDir, exitCode, err)
+	} else if exitCode == winapi.ERROR_NO_MORE_ITEMS {
+		// As mentioned in `pnputilNoMoreItemsErrorMessage`, this exit code comes from pnputil
+		// but is not necessarily an error
+		log.G(ctx).WithFields(logrus.Fields{
+			logfields.UVMID: vm.ID(),
+			"driver":        driverDir,
+			"error":         pnputilNoMoreItemsErrorMessage,
+		}).Warn("expected version of driver may not have been installed")
+	}
+
+	log.G(ctx).WithField("added drivers", driverDir).Debug("installed drivers")
+	return nil
+}

internal/controller/vm/vm.go

@@ -252,7 +252,7 @@ func (c *Manager) ExecIntoHost(ctx context.Context, request *shimdiag.ExecProces
 		Stdout:   request.Stdout,
 		Stderr:   request.Stderr,
 	}
-	return c.guest.ExecIntoUVM(ctx, cmdReq)
+	return c.guest.ExecInUVM(ctx, cmdReq)
 }
 
 // DumpStacks dumps the GCS stacks associated with the VM

internal/devices/assigned_devices.go

@@ -5,14 +5,17 @@ package devices
 
 import (
 	"context"
+	"errors"
 	"fmt"
+	"io"
+	"net"
 	"path/filepath"
 	"strconv"
+	"strings"
 
 	"github.com/Microsoft/hcsshim/internal/cmd"
 	"github.com/Microsoft/hcsshim/internal/log"
 	"github.com/Microsoft/hcsshim/internal/uvm"
-	"github.com/pkg/errors"
 )
 
 // AddDevice is the api exposed to oci/hcsoci to handle assigning a device on a WCOW UVM
@@ -43,7 +46,7 @@ func AddDevice(ctx context.Context, vm *uvm.UtilityVM, idType, deviceID string,
 	if uvm.IsValidDeviceType(idType) {
 		vpci, err = vm.AssignDevice(ctx, deviceID, index, "")
 		if err != nil {
-			return vpci, nil, errors.Wrapf(err, "failed to assign device %s of type %s to pod %s", deviceID, idType, vm.ID())
+			return vpci, nil, fmt.Errorf("failed to assign device %s of type %s to pod %s: %w", deviceID, idType, vm.ID(), err)
 		}
 		vmBusInstanceID := vm.GetAssignedDeviceVMBUSInstanceID(vpci.VMBusGUID)
 		log.G(ctx).WithField("vmbus id", vmBusInstanceID).Info("vmbus instance ID")
@@ -77,7 +80,7 @@ func getChildrenDeviceLocationPaths(ctx context.Context, vm *uvm.UtilityVM, vmBu
 	}
 	exitCode, err := vm.ExecInUVM(ctx, cmdReq)
 	if err != nil {
-		return nil, errors.Wrapf(err, "failed to find devices with exit code %d", exitCode)
+		return nil, fmt.Errorf("failed to find devices with exit code %d: %w", exitCode, err)
 	}
 
 	// wait to finish parsing stdout results
@@ -120,3 +123,32 @@ func GetDeviceInfoFromPath(rawDevicePath string) (string, uint16) {
 	// otherwise, just use default index and full device ID given
 	return rawDevicePath, 0
 }
+
+// readCsPipeOutput is a helper function that connects to a listener and reads
+// the connection's comma separated output until done. resulting comma separated
+// values are returned in the `result` param. The `errChan` param is used to
+// propagate an errors to the calling function.
+func readCsPipeOutput(l net.Listener, errChan chan<- error, result *[]string) {
+	defer close(errChan)
+	c, err := l.Accept()
+	if err != nil {
+		errChan <- fmt.Errorf("failed to accept named pipe: %w", err)
+		return
+	}
+	bytes, err := io.ReadAll(c)
+	if err != nil {
+		errChan <- err
+		return
+	}
+
+	elementsAsString := strings.TrimSuffix(string(bytes), "\n")
+	elements := strings.Split(elementsAsString, ",")
+	*result = append(*result, elements...)
+
+	if len(*result) == 0 {
+		errChan <- errors.New("failed to get any pipe output")
+		return
+	}
+
+	errChan <- nil
+}

internal/devices/drivers.go

@@ -5,12 +5,9 @@ package devices
 
 import (
 	"context"
-	"errors"
 	"fmt"
 
-	"github.com/Microsoft/go-winio/pkg/guid"
-	"github.com/Microsoft/hcsshim/internal/cmd"
-	"github.com/Microsoft/hcsshim/internal/guestpath"
+	"github.com/Microsoft/hcsshim/internal/controller/drivers"
 	"github.com/Microsoft/hcsshim/internal/log"
 	"github.com/Microsoft/hcsshim/internal/resources"
 	"github.com/Microsoft/hcsshim/internal/uvm"
@@ -51,7 +48,7 @@ func InstallDrivers(ctx context.Context, vm *uvm.UtilityVM, share string) (close
 		}
 		// attempt to install even if the driver has already been installed before so we
 		// can guarantee the device is ready for use afterwards
-		return closer, execPnPInstallDriver(ctx, vm, uvmPath)
+		return closer, drivers.ExecPnPInstallDriver(ctx, vm, uvmPath)
 	}
 
 	// first mount driver as scsi in standard mount location
@@ -69,63 +66,6 @@ func InstallDrivers(ctx context.Context, vm *uvm.UtilityVM, share string) (close
 	closer = mount
 	uvmPathForShare := mount.GuestPath()
 
-	// construct path that the drivers will be remounted as read/write in the UVM
-
-	// 914aadc8-f700-4365-8016-ddad0a9d406d. Random GUID chosen for namespace.
-	ns := guid.GUID{Data1: 0x914aadc8, Data2: 0xf700, Data3: 0x4365, Data4: [8]byte{0x80, 0x16, 0xdd, 0xad, 0x0a, 0x9d, 0x40, 0x6d}}
-	driverGUID, err := guid.NewV5(ns, []byte(share))
-	if err != nil {
-		return closer, fmt.Errorf("failed to create a guid path for driver %+v: %w", share, err)
-	}
-	uvmReadWritePath := fmt.Sprintf(guestpath.LCOWGlobalDriverPrefixFmt, driverGUID.String())
-
 	// install drivers using gcs tool `install-drivers`
-	return closer, execGCSInstallDriver(ctx, vm, uvmPathForShare, uvmReadWritePath)
-}
-
-func execGCSInstallDriver(ctx context.Context, vm *uvm.UtilityVM, driverDir string, driverReadWriteDir string) error {
-	p, l, err := cmd.CreateNamedPipeListener()
-	if err != nil {
-		return err
-	}
-	defer l.Close()
-
-	var stderrOutput string
-	errChan := make(chan error)
-
-	go readAllPipeOutput(l, errChan, &stderrOutput)
-
-	args := []string{
-		"/bin/install-drivers",
-		driverReadWriteDir,
-		driverDir,
-	}
-	req := &cmd.CmdProcessRequest{
-		Args:   args,
-		Stderr: p,
-	}
-
-	// A call to `ExecInUvm` may fail in the following ways:
-	// - The process runs and exits with a non-zero exit code. In this case we need to wait on the output
-	//   from stderr so we can log it for debugging.
-	// - There's an error trying to run the process. No need to wait for stderr logs.
-	// - There's an error copying IO. No need to wait for stderr logs.
-	//
-	// Since we cannot distinguish between the cases above, we should always wait to read the stderr output.
-	exitCode, execErr := vm.ExecInUVM(ctx, req)
-
-	// wait to finish parsing stdout results
-	select {
-	case err := <-errChan:
-		if err != nil && !errors.Is(err, noExecOutputErr) {
-			return fmt.Errorf("failed to get stderr output from command %s: %w", driverDir, err)
-		}
-	case <-ctx.Done():
-		return fmt.Errorf("timed out waiting for the console output from installing driver %s: %w", driverDir, ctx.Err())
-	}
-
-	if execErr != nil {
-		return fmt.Errorf("%w: failed to install driver %s in uvm with exit code %d: %v", execErr, driverDir, exitCode, stderrOutput)
-	}
-	return nil
+	return closer, drivers.ExecGCSInstallDriver(ctx, vm, share, uvmPathForShare)
 }