Rewrite SCSI support in new package

[kevpar]

This is commit 5/6 in a chain. Recommended to review in order. If reviewing a later PR in the chain, you can view individual commits to see just what that PR changes.

• Remove UVM/container cloning functionality #1740
• gcs: Add SCSIDevice type with remove operation #1741
• Remove dependence on GetScsiUvmPath function #1742
• Rework layer handling to return a ResourceCloser  #1743
• Rewrite SCSI support in new package #1744
• Support flexible LCOW layer parsing and partitioned layers #1745

The existing SCSI implementation in internal/uvm has evolved organically
over time into what it is today. This creates unecessary difficulty when
adding new features to the code, makes it harder to maintain, and has
been a source of bugs.

Additionally, there is a significant functional issue that the current
scsi code tightly couples the idea of attaching a SCSI device to a VM,
with the use/mounting of that device inside the VM. This creates
difficulty when we want to re-use the same SCSI attachment multiple
times, especially in the future when we will need to mount multiple
partitions from a device.

This is addressed here by largely rewriting the shim's SCSI code, and
moving it to a new internal/uvm/scsi package. The new code features a
main Manager type, which delegates to attachManager and mountManager for
tracking of attachments to the VM, and mounting of devices inside the
VM, respectively. attachManager and mountManager also rely on a set of
interfaces for the actual backend implementation of interacting with a
VM. This will also allow for easier testing of the scsi package in
isolation in the future.

One consequence of this change is it is no longer possible for the
caller to request a specific UVM path for a SCSI mount. The support for
this was already kind of a sham, because if the disk was already
mounted, you would get back its existing mount path instead of the one
you wanted, so the caller already had to handle that case. Additionally,
I'm not aware of any reason why the specific location the disk is
mounted is actually relevant. Because of these reasons, and to simplify
the overall package interface, the mount path is determined by the scsi
package, using a format string passed to the Manager at creation time.

[katiewasnothere]

Could you elaborate more on the reasoning behind the "unplugger" type? There's never a time where a call to detach an attachment wouldn't also come with an unplug call right? Why not just have "unplug" be another function call on the "attacher"?

[katiewasnothere]

Are we tracking somewhere that there will need to be changes to support confidential containers as a result of this PR?

[kevpar]

Could you elaborate more on the reasoning behind the "unplugger" type? There's never a time where a call to detach an attachment wouldn't also come with an unplug call right? Why not just have "unplug" be another function call on the "attacher"?

The intent was to decouple "VM" operations from "guest" operations. Since in the future we probably want to move these parts of the code further apart, as e.g. we may have different components managing VM vs guest.

Are we tracking somewhere that there will need to be changes to support confidential containers as a result of this PR?

Not currently. Is there some specific support you're thinking of that should be tracked?

[katiewasnothere]

The intent was to decouple "VM" operations from "guest" operations. Since in the future we probably want to move these parts of the code further apart, as e.g. we may have different components managing VM vs guest.

Ah okay, that seems reasonable to me.

Not currently. Is there some specific support you're thinking of that should be tracked?

I was thinking specifically for the new bridge calls. They will need to add new policies for those calls.

[kevpar]

I was thinking specifically for the new bridge calls. They will need to add new policies for those calls.

Are you referring to the SCSIDevice resource type for "new bridge calls"? If so I agree that is something that needs looking into.

@anmaxvl what is the plan for how we keep policy up to date with bridge protocol changes?

[kevpar]

I was thinking specifically for the new bridge calls. They will need to add new policies for those calls.

Are you referring to the SCSIDevice resource type for "new bridge calls"? If so I agree that is something that needs looking into.

@anmaxvl what is the plan for how we keep policy up to date with bridge protocol changes?

I discussed offline with Maksim. We agreed there is no need to address this right now. In the future we can add a policy enforcement point for SCSIDevice removal.

[helsaawy]

Im worried this design is over-complicated and exposes a lot to the uVM client.

Now the clients have to

1. worry about calling GrantVmAccess before Add, regardless of whether the VHD was already attached (whereas before this was taken care of automatically) and is a requirement that isnt documented for the manager;
2. create the appropriate Attacher/Mounter/Unpluger even though the scsi package could have taken care of that by exposing dedicated NewBridgeManager/NewHCSManager; and
3. ensure they don't erroneously mix and match bridge and HCS (eg, an hcsMounter and a bridgeUnplugger).

I doubt the client needs to worry about the individual low-level components, since they should be dictated by the type of uVM we have, so exposing them feels unnecessary.

[katiewasnothere]

Actually, do we really need to be able to support only attaching a scsi device and not mounting it? As far as I can tell, we only do that in two places: in this runhcs code that I'm pretty sure we don't use anymore to "prepare" a disk AND in the code to create a scratch disk here.

For creating the scratch, couldn't we make use of the changes I made here #1757 to EnsureFilesystem on the device? I'm not sure if adding in a mount for the scratch formatting would slow down the whole operation or by how much, since the create scratch code does already require making a call into the UVM to run mkfs anyways and we could get that for free as part of the mount call with EnsureFilesystem.

[kevpar]

Actually, do we really need to be able to support only attaching a scsi device and not mounting it? As far as I can tell, we only do that in two places: in this runhcs code that I'm pretty sure we don't use anymore to "prepare" a disk AND in the code to create a scratch disk here.

For creating the scratch, couldn't we make use of the changes I made here #1757 to EnsureFilesystem on the device? I'm not sure if adding in a mount for the scratch formatting would slow down the whole operation or by how much, since the create scratch code does already require making a call into the UVM to run mkfs anyways and we could get that for free as part of the mount call with EnsureFilesystem.

That's a good point, but I think we should keep the ability to attach without mounting. There's a good possibility in the future we will want to do something like expose a block device directly into a container, and we would need this ability to do that (e.g. if there is no filesystem on the device).

[katiewasnothere]

Why did we move around verity code in this PR? Just cleaning?

[kevpar]

Why did we move around verity code in this PR? Just cleaning?

Yeah, it has no relation to SCSI, so didn't make sense to be in the scsi package.

[katiewasnothere]

Have you run the cri-containerd tests with these changes?

[kevpar]

Have you run the cri-containerd tests with these changes?

I ran them a bit ago, hit a bunch of unrelated failures, but mostly things seemed okay. I can do another run to see how it looks now.

[katiewasnothere]

lgtm if the cri-containerd tests pass

[kevpar]

@helsaawy I pushed a temporary commit addressing your feedback, so it is easier to view it separately.