From 8217f8df1681c62bd463fddce345e846c01815b3 Mon Sep 17 00:00:00 2001 From: Kevin Parsons Date: Tue, 18 Apr 2023 02:34:37 -0700 Subject: [PATCH] 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. --- 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 8a7aada7b0..e4188aa347 100644 --- a/internal/uvm/scsi.go +++ b/internal/uvm/scsi.go @@ -521,19 +521,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.