Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Volume Mount Point management functions wrapped nicely for Go #187

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

TBBle
Copy link
Contributor

@TBBle TBBle commented Jan 2, 2021

Closes: #176

These APIs will replace the duplicate code in https://github.com/microsoft/hcsshim/blob/master/cmd/wclayer/volumemountutils.go and containerd/containerd#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/require mostly 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 computestorage from 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 uniony.

Go-ified structures needed for IOCTL_DISK_SET_DRIVE_LAYOUT_EX

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.

		// 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
		}

pkg/volmount/getvolumepathnamesforvolumename.go Outdated Show resolved Hide resolved
pkg/volmount/getvolumepathnamesforvolumename.go Outdated Show resolved Hide resolved
pkg/volmount/getvolumepathnamesforvolumename.go Outdated Show resolved Hide resolved
pkg/volmount/volmount_test.go Outdated Show resolved Hide resolved
pkg/volmount/volmount_test.go Outdated Show resolved Hide resolved
pkg/volmount/volmount_test.go Show resolved Hide resolved
@TBBle TBBle force-pushed the VolumeMountPoint-APIs branch 2 times, most recently from fae2e60 to 8b3cf83 Compare January 12, 2021 13:59
@TBBle
Copy link
Contributor Author

TBBle commented Jan 22, 2021

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
Copy link
Contributor Author

TBBle commented Apr 5, 2021

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 containerd/containerd#4419) looking like it's functionally-correct.

Fixes: microsoft#176

These provide Go-typed wrappers around the Win32 APIs backing them,
protecting the user from unsafe pointers and UTF-16 conversions.

utf16ToStringArray adapted from ConvertStringSetToSlice in hcsshim

Signed-off-by: Paul "TBBle" Hampson <Paul.Hampson@Pobox.com>
These are a separate commit, because in order to implement the tests, a
bunch of new modules were pulled into go.mod.

And the tests only run as Administrator anyway.

On the plus side, the tests exercise a few APIs in the vhd package:
- CreateVirtualDisk
- AttachVirtualDisk
- DetachVirtualDisk
- GetVirtualDiskPhysicalPath

And in future, these tests could also exercise the DecodeReparsePoint
API too, since it should be 1:1 with GetVolumeNameForVolumeMountPoint.

Signed-off-by: Paul "TBBle" Hampson <Paul.Hampson@Pobox.com>
This trivially demonstrates the difference between DecodeReparsePoint
and GetVolumeNameForVolumeMountPoint.

Signed-off-by: Paul "TBBle" Hampson <Paul.Hampson@Pobox.com>
@TBBle
Copy link
Contributor Author

TBBle commented Apr 23, 2021

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. containerd/containerd#4912 (comment), but this is the first time I've seen it fail outside containerd/containerd#4419 (comment).

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.

bufferlength := uint32(50) // "A reasonable size for the buffer" per the documentation.
buffer := make([]uint16, bufferlength)

if err = windows.GetVolumeNameForVolumeMountPoint(targetP, &buffer[0], bufferlength); err != nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That should be err :=, I think, for idiomatic reasons.

@TBBle
Copy link
Contributor Author

TBBle commented Feb 6, 2023

#274 should supplant the use-case for this PR, I think. This PR was to unify two extant users, one of which (in a containerd PR) is being replaced with use of the #274 API, and the other is in hcsshim's 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrap Volume Mount Point management functions nicely for Go
2 participants