Open TBBle opened 3 years ago
TBBle
commented
3 years ago Poke: This one's ready to go, I believe. Once it lands, I can then kill the same functions living in hcsshim, and then once that lands, get rid of the other copy of these functions living in a PR against containerd.
TBBle
commented
3 years ago Repoke: I think this one's still good-to-go, and I'd like to get the deduplication train moving, now that I have the second duplicate copy (in https://github.com/containerd/containerd/pull/4419) looking like it's functionally-correct.
TBBle
commented
3 years ago CI failure:
=== RUN TestVolumeMountSuccess
volmount_test.go:140: failed to format VHD: failed to format writable layer vhd: The request is not supported.
--- FAIL: TestVolumeMountSuccess (0.25s)That's interesting. There's been some suggestion that this API might not work on Windows Server LTSC2019, i.e. https://github.com/containerd/containerd/pull/4912#issuecomment-792763522, but this is the first time I've seen it fail outside https://github.com/containerd/containerd/pull/4419#issuecomment-788877519.
As it happens, I am building a Windows Server LTSC 2019 eval box (on a laptop I borrowed) so I might have a look at this while I'm using it to chase another issue that doesn't reproduce on my 20H2 desktop.
TBBle
commented
1 year ago wclayer utility, and it would make sense for that utility to match the containerd behaviour as closely as possible, I think.If I remember correctly, I introduced the hcsshim use-case because I wanted somewhere isolated to experiment with the API calls I was using for containerd, and it was also useful for fixing broken/stuck mounts. That latter use-case is what prompts me to suggest changing the hcsshim code at some point to use the #274 API, particularly since unlike volume mounts, I'm not aware of a stock Windows command-line tool for undoing Bind Filter mounts.
That said, I'm not sure if we care about any versions of Windows that predate the Bind Filter API, that might still benefit from this PR. I doubt we do for the containers case, but as general API wrappers, maybe it has value.
Closes: #176
These APIs will replace the duplicate code in https://github.com/microsoft/hcsshim/blob/master/cmd/wclayer/volumemountutils.go and https://github.com/containerd/containerd/pull/4419.
Tests require elevation in order to attach the VHD, and will skip otherwise.
I'm surprised this is the first place we needed to decode a series of UTF-16 strings from the Windows API (see
utf16ToStringArray). Perhaps there's already an implementation of this function somewhere, and I overlooked it? (Update: Turns out there was one in hcsshim, which I've copied to avoid a circular dependency, and at some point probably belongs in x/sys/windows anyway)For the tests, I pulled in
github.com/stretchr/testify/requiremostly because I didn't want to write and maintain my own implementation of "check if the array elements match in any order".If this new import is undesirable, I can hard-code the specific instances instead, now that I know the code is working.
If this new import is desirable, I could rewrite most of the other
if ! X { t.Fatal( "X wasn't") }blocks into one-liners which would be much nicer and much less repetitive.Update: WMI code talked about below was replaced using
computestoragefrom hcsshim, which put the VHD into a good-enough state that the tests can mount it successfully. Since this is only a test, a circular dependency didn't seem such a problem."WMI code talked about below"
Sadly, using WMI in the test function to partition the VHD pulls in a fair bit of random stuff. And the library I found that supports the `MSFT_Disk` and `MSFT_Partition` objects doesn't have a _go.mod_, so ends up adding a bunch of indirect dependencies. (I think that's what caused it... I'm just trusting `go mod tidy` on this.) I had started looking at implementing the Win32 API calls for disk partitioning, using `DeviceIoControl`, but decided if I was going to do that, I may as well export that as an independent package from this library, and that looked like way too much work to mix into this, with no other apparently-motiviating use-cases. And the structures are _really_ `union`y.Go-ified structures needed for
Just putting this here in case it becomes useful later, or if we want to go down that path rather than the WMI approach I'm using now. ```Go // This could be a whole new package in go-winio. But I really don't want to write // it right now. physHandle, err := windows.Open(physPath, os.O_RDWR, 0) if err != nil { return "", errors.Wrapf(err, "Open(%s) failed", physPath) } defer windows.CloseHandle(physHandle) windows.DeviceIoControl(physHandle, ) // IOCTL_DISK_SET_DRIVE_LAYOUT_EX // https://docs.microsoft.com/en-us/windows/win32/api/winioctl/ni-winioctl-ioctl_disk_set_drive_layout_ex // DRIVE_LAYOUT_INFORMATION_EX // https://docs.microsoft.com/en-us/windows/win32/api/winioctl/ns-winioctl-drive_layout_information_ex type DRIVE_LAYOUT_INFORMATION_MBR struct { Signature int32 CheckSum int32 } type DRIVE_LAYOUT_INFORMATION_GPT struct { DiskId guid.GUID StartingUsableOffset int64 UsableLength int64 MaxPartitionCount int32 } // Union of DRIVE_LAYOUT_INFORMATION_MBR and DRIVE_LAYOUT_INFORMATION_GPT type DRIVE_LAYOUT_INFORMATION_UNION [16+8+8+4]byte type IOCTL_DISK_SET_DRIVE_LAYOUT_EX struct { PartitionStyle int32 PartitionCount int32 MbrGptUnion DRIVE_LAYOUT_INFORMATION_UNION // Followed by zero or more PARTITION_INFORMATION_EX } type PARTITION_INFORMATION_MBR struct { PartitionType int8 BootIndicator bool RecognizedPartition bool HiddenSectors int32 PartitionId guid.GUID } type PARTITION_INFORMATION_GPT struct { PartitionType guid.GUID PartitionId guid.GUID Attributes int64 Name [36]uint16 } // Union of PARTITION_INFORMATION_MBR and PARTITION_INFORMATION_GPT type PARTITION_INFORMATION_UNION [16+16+8+72]byte type PARTITION_INFORMATION_EX struct { PartitionStyle int32 StartingOffset int64 PartitionLength int64 PartitionNumber int32 RewritePartition bool IsServicePartition bool MbrGptUnion PARTITION_INFORMATION_UNION } ```IOCTL_DISK_SET_DRIVE_LAYOUT_EX