From 1378215f4379639dec0a683edb717cac0751e6d6 Mon Sep 17 00:00:00 2001 From: Harsh Rawat Date: Tue, 24 Mar 2026 02:43:55 +0530 Subject: [PATCH] refactor drivers util methods In order to install device drivers, the following workflow is followed- - Make the device driver available within the UVM via SCSI or VSMB. - Use `install-drivers` for LCOW and `pnputils` for WCOW to install the driver Presently, everything was clustered together in `internal/devices` package. The utility methods to perform step 2 above are not tied to devices per se and belong to the `drivers`. Since we can easily re-use these methods, we are refactoring them into `internal/controller/drivers` where they exist as utility methods and are used in both new and old shims. Signed-off-by: Harsh Rawat --- internal/controller/drivers/doc.go | 14 +++ internal/controller/drivers/drivers_lcow.go | 131 ++++++++++++++++++++ internal/controller/drivers/drivers_wcow.go | 63 ++++++++++ internal/controller/vm/vm.go | 2 +- internal/devices/assigned_devices.go | 38 +++++- internal/devices/drivers.go | 66 +--------- internal/devices/pnp.go | 125 ------------------- internal/vm/guestmanager/manager.go | 8 +- 8 files changed, 251 insertions(+), 196 deletions(-) create mode 100644 internal/controller/drivers/doc.go create mode 100644 internal/controller/drivers/drivers_lcow.go create mode 100644 internal/controller/drivers/drivers_wcow.go delete mode 100644 internal/devices/pnp.go diff --git a/internal/controller/drivers/doc.go b/internal/controller/drivers/doc.go new file mode 100644 index 0000000000..8aea411c1f --- /dev/null +++ b/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 diff --git a/internal/controller/drivers/drivers_lcow.go b/internal/controller/drivers/drivers_lcow.go new file mode 100644 index 0000000000..bb2713c740 --- /dev/null +++ b/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 +} diff --git a/internal/controller/drivers/drivers_wcow.go b/internal/controller/drivers/drivers_wcow.go new file mode 100644 index 0000000000..ead7da980d --- /dev/null +++ b/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 +} diff --git a/internal/controller/vm/vm.go b/internal/controller/vm/vm.go index e4c8c42736..967b9a5cff 100644 --- a/internal/controller/vm/vm.go +++ b/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 diff --git a/internal/devices/assigned_devices.go b/internal/devices/assigned_devices.go index 97db77e0d6..8e1bd3bb22 100644 --- a/internal/devices/assigned_devices.go +++ b/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 +} diff --git a/internal/devices/drivers.go b/internal/devices/drivers.go index f1c4e3f340..248e523760 100644 --- a/internal/devices/drivers.go +++ b/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) } diff --git a/internal/devices/pnp.go b/internal/devices/pnp.go deleted file mode 100644 index 71bdd7643f..0000000000 --- a/internal/devices/pnp.go +++ /dev/null @@ -1,125 +0,0 @@ -//go:build windows -// +build windows - -package devices - -import ( - "context" - "fmt" - "io" - "net" - "strings" - - "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/pkg/errors" - "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.` -) - -var noExecOutputErr = errors.New("failed to get any pipe output") - -// 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 errors.Wrapf(err, "failed to install driver %s in uvm with exit code %d", driverDir, exitCode) - } 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 -} - -// 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 <- errors.Wrapf(err, "failed to accept named pipe") - 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 <- noExecOutputErr - return - } - - errChan <- 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 <- errors.Wrapf(err, "failed to accept named pipe") - return - } - bytes, err := io.ReadAll(c) - if err != nil { - errChan <- err - return - } - - *result = string(bytes) - - if len(*result) == 0 { - errChan <- noExecOutputErr - return - } - - errChan <- nil -} diff --git a/internal/vm/guestmanager/manager.go b/internal/vm/guestmanager/manager.go index 1db0b4da7a..7144aaa872 100644 --- a/internal/vm/guestmanager/manager.go +++ b/internal/vm/guestmanager/manager.go @@ -34,8 +34,8 @@ type Manager interface { DumpStacks(ctx context.Context) (string, error) // DeleteContainerState removes persisted state for the container identified by `cid` from the guest. DeleteContainerState(ctx context.Context, cid string) error - // ExecIntoUVM executes commands specified in the requests in the utility VM. - ExecIntoUVM(ctx context.Context, request *cmd.CmdProcessRequest) (int, error) + // ExecInUVM executes commands specified in the requests in the utility VM. + ExecInUVM(ctx context.Context, request *cmd.CmdProcessRequest) (int, error) } var _ Manager = (*Guest)(nil) @@ -85,7 +85,7 @@ func (gm *Guest) DeleteContainerState(ctx context.Context, cid string) error { return nil } -// ExecIntoUVM executes commands specified in the requests in the utility VM. -func (gm *Guest) ExecIntoUVM(ctx context.Context, request *cmd.CmdProcessRequest) (int, error) { +// ExecInUVM executes commands specified in the requests in the utility VM. +func (gm *Guest) ExecInUVM(ctx context.Context, request *cmd.CmdProcessRequest) (int, error) { return cmd.ExecInUvm(ctx, gm.gc, request) }