From d42948347a208dfd9ce76335f7b80e546f252f61 Mon Sep 17 00:00:00 2001 From: Kevin Parsons Date: Tue, 18 Apr 2023 14:59:50 -0700 Subject: [PATCH 1/2] Remove dependency on GetScsiUvmPath from driver installation Currently, when installing drivers on LCOW, we use GetScsiUvmPath to check if the VHD is already mounted, and if it is, we assume the drivers have already been installed, so we can skip doing it again. This check has a few problems: - It relies on GetScsiUvmPath, which assumes a single mount-point in the guest for a given VHD. This assumption is not safe to make in the face of future changes, where we could mount a device (or partitions on it) multiple times. - It assumes the disk has stayed attached the whole time after drivers were installed. This may be a safe assumption today, but can be fragile in the future. - It does not work in the case of a VHD containing multiple sets of drivers, or a VHD being changed/updated to newer content after first install. Again, this is safe given the current overall design today, but could break in the future. This change is still mostly a bandaid fix. Probably what is most correct is to track driver installation in something with state (the GCS) rather than using a separately invoked binary to do the in-guest install. However, this change does address the first issue above, removing the dependency on GetScsiUvmPath. I do this in the following way: - Change install-drivers to check if the overlay path exists already, and exit with a no-op if it does. This encodes the assumption that the overlay path will be consistent for a given driver set. - Change InstallDrivers in the shim to compute a V5 GUID from the VHD path, and use that as part of the overlay path given to the guest. This ensures there is a unique guest overlay path for each unique host driver VHD path. Signed-off-by: Kevin Parsons --- cmd/gcstools/installdrivers.go | 9 +++++++++ internal/devices/drivers.go | 14 +++++++------- 2 files changed, 16 insertions(+), 7 deletions(-) diff --git a/cmd/gcstools/installdrivers.go b/cmd/gcstools/installdrivers.go index 1211f8a591..e382765827 100644 --- a/cmd/gcstools/installdrivers.go +++ b/cmd/gcstools/installdrivers.go @@ -6,6 +6,7 @@ package main import ( "context" "fmt" + "io/fs" "os" "os/exec" "path/filepath" @@ -27,6 +28,14 @@ func install(ctx context.Context) error { targetOverlayPath := args[0] driver := args[1] + if _, err := os.Lstat(targetOverlayPath); err == nil { + // We assume the overlay path to be unique per set of drivers. Thus, if the path + // exists already, we have already installed these drivers, and can quit early. + return nil + } else if !errors.Is(err, fs.ErrNotExist) { + return fmt.Errorf("failed to stat overlay dir: %s: %w", targetOverlayPath, err) + } + // create an overlay mount from the driver's UVM path so we can write to the // mount path in the UVM despite having mounted in the driver originally as // readonly diff --git a/internal/devices/drivers.go b/internal/devices/drivers.go index bcf5bb0c9b..ecf2c0eaae 100644 --- a/internal/devices/drivers.go +++ b/internal/devices/drivers.go @@ -53,14 +53,9 @@ func InstallDrivers(ctx context.Context, vm *uvm.UtilityVM, share string, gpuDri return closer, execPnPInstallDriver(ctx, vm, uvmPath) } - // no need to reinstall if the driver has already been added in LCOW, return early - if _, err := vm.GetScsiUvmPath(ctx, share); err != uvm.ErrNotAttached { - return nil, err - } - // first mount driver as scsi in standard mount location uvmPathForShare := fmt.Sprintf(guestpath.LCOWGlobalMountPrefixFmt, vm.UVMMountCounter()) - closer, err = vm.AddSCSI(ctx, + mount, err := vm.AddSCSI(ctx, share, uvmPathForShare, true, @@ -70,9 +65,14 @@ func InstallDrivers(ctx context.Context, vm *uvm.UtilityVM, share string, gpuDri if err != nil { return closer, fmt.Errorf("failed to add SCSI disk to utility VM for path %+v: %s", share, err) } + closer = mount + uvmPathForShare = mount.UVMPath // construct path that the drivers will be remounted as read/write in the UVM - driverGUID, err := guid.NewV4() + + // 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: %s", share, err) } From 1dd217b51877613500216948cec52959a3a9c676 Mon Sep 17 00:00:00 2001 From: Kevin Parsons Date: Tue, 18 Apr 2023 02:34:37 -0700 Subject: [PATCH 2/2] Remove dependency on GetScsiUvmPath from WCOW-isolated mounts The WCOW-isolated SCSI mount process currently works as follows: - In resources_wcow.go, go through each mount on the OCI spec, and if it is a SCSI mount, add a mount to the UVM for it. - in hcsdoc_wcow.go, go through each mount on the OCI spec, use GetScsiUvmPath to determine the guest path it was mounted to, and add an entry to the container doc for it. This is quite hacky, as it relies on a 1:1 mapping between host VHDs and mounts in the guest, and also because it requires us to re-query information we've already been given. The SCSIMount object returned when we mounted to the guest can already tell us the guest path. This change resolves this problem by instead determing the set of guest mounts that should be added to the container doc at the time when the SCSI mounts are done, and saving it in the creation options. Then, when we construct the actual container doc, we just grab those mounts and add them in. Signed-off-by: Kevin Parsons --- internal/hcsoci/create.go | 6 ++- internal/hcsoci/hcsdoc_wcow.go | 17 ++------ internal/hcsoci/resources_wcow.go | 66 ++++++++++++++++++++----------- internal/uvm/scsi.go | 13 ------ 4 files changed, 50 insertions(+), 52 deletions(-) diff --git a/internal/hcsoci/create.go b/internal/hcsoci/create.go index ee0a7ef708..95cf4cb942 100644 --- a/internal/hcsoci/create.go +++ b/internal/hcsoci/create.go @@ -66,6 +66,8 @@ type createOptionsInternal struct { actualOwner string // Owner for the container actualNetworkNamespace string ccgState *hcsschema.ContainerCredentialGuardState // Container Credential Guard information to be attached to HCS container document + + windowsAdditionalMounts []hcsschema.MappedDirectory // Holds additional mounts based on added devices (such as SCSI). Only used for Windows v2 schema containers. } func validateContainerConfig(ctx context.Context, coi *createOptionsInternal) error { @@ -173,7 +175,7 @@ func CreateContainer(ctx context.Context, createOptions *CreateOptions) (_ cow.C return nil, nil, fmt.Errorf("container config validation failed: %s", err) } - r := resources.NewContainerResources(createOptions.ID) + r := resources.NewContainerResources(coi.ID) defer func() { if err != nil { if !coi.DoNotReleaseResourcesOnFailure { @@ -184,7 +186,7 @@ func CreateContainer(ctx context.Context, createOptions *CreateOptions) (_ cow.C if coi.HostingSystem != nil { if coi.Spec.Linux != nil { - r.SetContainerRootInUVM(fmt.Sprintf(lcowRootInUVM, createOptions.ID)) + r.SetContainerRootInUVM(fmt.Sprintf(lcowRootInUVM, coi.ID)) } else { n := coi.HostingSystem.ContainerCounter() r.SetContainerRootInUVM(fmt.Sprintf(wcowRootInUVM, strconv.FormatUint(n, 16))) diff --git a/internal/hcsoci/hcsdoc_wcow.go b/internal/hcsoci/hcsdoc_wcow.go index b094de9d41..22b042bb42 100644 --- a/internal/hcsoci/hcsdoc_wcow.go +++ b/internal/hcsoci/hcsdoc_wcow.go @@ -69,19 +69,9 @@ func createMountsConfig(ctx context.Context, coi *createOptionsInternal) (*mount } mdv2.HostPath = src } else if mount.Type == "virtual-disk" || mount.Type == "physical-disk" || mount.Type == "extensible-virtual-disk" { - mountPath := mount.Source - var err error - if mount.Type == "extensible-virtual-disk" { - _, mountPath, err = uvm.ParseExtensibleVirtualDiskPath(mount.Source) - if err != nil { - return nil, err - } - } - uvmPath, err := coi.HostingSystem.GetScsiUvmPath(ctx, mountPath) - if err != nil { - return nil, err - } - mdv2.HostPath = uvmPath + // For v2 schema containers, any disk mounts will be part of coi.additionalMounts. + // For v1 schema containers, we don't even get here, since there is no HostingSystem. + continue } else if strings.HasPrefix(mount.Source, guestpath.SandboxMountPrefix) { // Convert to the path in the guest that was asked for. mdv2.HostPath = convertToWCOWSandboxMountPath(mount.Source) @@ -97,6 +87,7 @@ func createMountsConfig(ctx context.Context, coi *createOptionsInternal) (*mount config.mdsv2 = append(config.mdsv2, mdv2) } } + config.mdsv2 = append(config.mdsv2, coi.windowsAdditionalMounts...) return &config, nil } diff --git a/internal/hcsoci/resources_wcow.go b/internal/hcsoci/resources_wcow.go index 11caf77358..0149a05331 100644 --- a/internal/hcsoci/resources_wcow.go +++ b/internal/hcsoci/resources_wcow.go @@ -19,6 +19,7 @@ import ( "github.com/Microsoft/hcsshim/internal/cmd" "github.com/Microsoft/hcsshim/internal/credentials" "github.com/Microsoft/hcsshim/internal/guestpath" + hcsschema "github.com/Microsoft/hcsshim/internal/hcs/schema2" "github.com/Microsoft/hcsshim/internal/layers" "github.com/Microsoft/hcsshim/internal/log" "github.com/Microsoft/hcsshim/internal/resources" @@ -151,35 +152,52 @@ func setupMounts(ctx context.Context, coi *createOptionsInternal, r *resources.R } } l := log.G(ctx).WithField("mount", fmt.Sprintf("%+v", mount)) - if mount.Type == "physical-disk" { - l.Debug("hcsshim::allocateWindowsResources Hot-adding SCSI physical disk for OCI mount") - scsiMount, err := coi.HostingSystem.AddSCSIPhysicalDisk(ctx, mount.Source, uvmPath, readOnly, mount.Options) - if err != nil { - return errors.Wrapf(err, "adding SCSI physical disk mount %+v", mount) - } - r.Add(scsiMount) - } else if mount.Type == "virtual-disk" { - l.Debug("hcsshim::allocateWindowsResources Hot-adding SCSI virtual disk for OCI mount") - scsiMount, err := coi.HostingSystem.AddSCSI( - ctx, - mount.Source, - uvmPath, - readOnly, - false, - mount.Options, - uvm.VMAccessTypeIndividual, + if mount.Type == "physical-disk" || mount.Type == "virtual-disk" || mount.Type == "extensible-virtual-disk" { + var ( + scsiMount *uvm.SCSIMount + err error ) - if err != nil { - return errors.Wrapf(err, "adding SCSI virtual disk mount %+v", mount) + switch mount.Type { + case "physical-disk": + l.Debug("hcsshim::allocateWindowsResources Hot-adding SCSI physical disk for OCI mount") + scsiMount, err = coi.HostingSystem.AddSCSIPhysicalDisk( + ctx, + mount.Source, + uvmPath, + readOnly, + mount.Options, + ) + case "virtual-disk": + l.Debug("hcsshim::allocateWindowsResources Hot-adding SCSI virtual disk for OCI mount") + scsiMount, err = coi.HostingSystem.AddSCSI( + ctx, + mount.Source, + uvmPath, + readOnly, + false, + mount.Options, + uvm.VMAccessTypeIndividual, + ) + case "extensible-virtual-disk": + l.Debug("hcsshim::allocateWindowsResource Hot-adding ExtensibleVirtualDisk") + scsiMount, err = coi.HostingSystem.AddSCSIExtensibleVirtualDisk( + ctx, + mount.Source, + uvmPath, + readOnly, + ) } - r.Add(scsiMount) - } else if mount.Type == "extensible-virtual-disk" { - l.Debug("hcsshim::allocateWindowsResource Hot-adding ExtensibleVirtualDisk") - scsiMount, err := coi.HostingSystem.AddSCSIExtensibleVirtualDisk(ctx, mount.Source, uvmPath, readOnly) if err != nil { - return errors.Wrapf(err, "adding SCSI EVD mount failed %+v", mount) + return fmt.Errorf("adding SCSI mount %+v: %w", mount, err) } r.Add(scsiMount) + // Compute guest mounts now, and store them, so they can be added to the container doc later. + // We do this now because we have access to the guest path through the returned mount object. + coi.windowsAdditionalMounts = append(coi.windowsAdditionalMounts, hcsschema.MappedDirectory{ + HostPath: scsiMount.UVMPath, + ContainerPath: mount.Destination, + ReadOnly: readOnly, + }) } else if strings.HasPrefix(mount.Source, guestpath.SandboxMountPrefix) { // Mounts that map to a path in the UVM are specified with a 'sandbox://' prefix. // diff --git a/internal/uvm/scsi.go b/internal/uvm/scsi.go index a7403b9234..df6ebfc22d 100644 --- a/internal/uvm/scsi.go +++ b/internal/uvm/scsi.go @@ -519,19 +519,6 @@ func (uvm *UtilityVM) allocateSCSIMount( return uvm.scsiLocations[controller][lun], false, nil } -// GetScsiUvmPath returns the guest mounted path of a SCSI drive. -// -// If `hostPath` is not mounted returns `ErrNotAttached`. -func (uvm *UtilityVM) GetScsiUvmPath(ctx context.Context, hostPath string) (string, error) { - uvm.m.Lock() - defer uvm.m.Unlock() - sm, err := uvm.findSCSIAttachment(ctx, hostPath) - if err != nil { - return "", err - } - return sm.UVMPath, err -} - // ScratchEncryptionEnabled is a getter for `uvm.encryptScratch`. // // Returns true if the scratch disks should be encrypted, false otherwise.