Port Windows device support from containerd

[adamrehn]

(Contributes to the high-level goal of supporting Windows containers, as outlined in #28.)

This change adds support for exposing host devices to Windows containers using the --device flag with nerdctl run. The code itself is directly ported from containerd, where it was first contributed by @TBBle in #6618 (relevant diff lines here), and subsequently modified by @thaJeztah to use strings.Cut() in commit eaedadbe. My only unique contribution here is to update the command reference and adorn the --device flag with the little blue square which denotes Windows support. 😆

It's worth noting that I haven't added any unit tests to cover this functionality. Most of the existing Windows unit tests appear to run containers and check their output, but I don't know of a decent way to test this without requiring that a GPU be present on the underlying host system. I can see that Windows unit tests are currently run as part of the Cirrus CI workflow, but I suspect that the cloud VMs being used for the tests do not have GPUs, and any test requiring one would fail. If a simpler kind of unit test would be desirable (e.g. one that just tests the parsing behaviour for device IDType://ID values without actually running a container) then I would certainly be happy to write one.

[fahedouch]

LGTM for implementation. but is it possible to add an integration test with a special device ( e.g NUL device) ?

[adamrehn]

So looking at the Devices in Containers on Windows page, I see the following devices can be exposed to containers:

Device Type	Interface Class GUID
GPIO	916EF1CB-8426-468D-A6F7-9AE8076881B3
I2C Bus	A11EE3C6-8421-4202-A3E7-B91FF90188E4
COM Port	86E0D1E0-8089-11D0-9CE4-08003E301F73
SPI Bus	DCDE6AF9-6610-4285-828F-CAAF78C424CC
DirectX GPU Acceleration	See GPU acceleration docs

Perhaps a COM port might be the best option? I don't think cloud VMs typically have one by default, but if we're able to modify the VM image used for the Cirrus CI workflow then we could potentially install a third-party driver that provides a virtual COM port and use that?

(It's also worth noting that those docs are quite out of date compared to what's supported by newer versions of Windows, so experimentation might potentially reveal other types of devices that could be used instead. After all, that page still references the old IDType/ID syntax rather than the newer IDType://ID syntax, and makes no mention of exposing GPUs via their LocationPath, which was evidently introduced in HCS schema version 2.2 with the release of Windows 10 version 1903.)

[fahedouch]

After all, that page still references the old IDType/ID syntax rather than the newer IDType://ID syntax

IDType://ID is just a ctr format choice to introduce devices.

cirrus base image is windows-2019-core-for-containers. We fill all the requirement to run one of these devices. I am wondering is there a built-in/specific device in the above list

but if we're able to modify the VM image used for the Cirrus CI workflow then we could potentially install a third-party driver that provides a virtual COM port and use that

SGTM, if there is a way to do it from .cirrus.yml file

[adamrehn]

IDType://ID is just a ctr format choice to introduce devices.

Oops, my mistake! I had incorrectly assumed that this was an HCS API convention due to seeing that format used within the hcsshim codebase, but looking again now, I can see that the only place it's actually used is within unit tests related to containerd (hence the use of the containerd/ctr format convention).

cirrus base image is windows-2019-core-for-containers. We fill all the requirement to run one of these devices. I am wondering is there a built-in/specific device in the above list

So looking at the Cirrus docs for the compute_engine_instance execution environment, it seems that the VM that gets launched is a Google Compute Engine instance (and the Packer template you linked also confirms this). According to the Google Cloud docs, GCE instances should have four COM ports by default, which are used for the instance's serial console. If one of these built-in COM ports can be exposed to Windows containers then we should be able to use that for testing, without the need for a virtual COM port driver. I'll run some tests with a GCE instance and report back.

[TBBle]

To clarify, I was trying to standardise to IDType://ID everywhere as since the initial class/GUID syntax was introduced, we've had other IDTypes than class (in HCSv2 onwards) created that may have / in the ID, so this gives a less-ambiguous parse. It also lets us easily reject accidental use of POSIX paths, i.e. using Linux device syntax with a WCOW container is never incorrectly accepted. (And reading back over my own notes, this also avoids future conflict with CDI's naming convention, by total luck.)

As it happens, Docker had not yet had a release supporting anything except class as an IDType, so we took the opportunity to mark IDType/ID as deprecated, and only have one supported value for IDType in that syntax: class for back-compat. There was literally no tooling in existence that attempted to expose other IDType values except an old fork of containerd by one of the MS team, which is where the IDType://ID syntax was introduced. (Which is how it ended up in the hcsshim unit tests where I first saw it; they were run against that containerd fork.)

So I'd lean towards avoiding supporting class/X if possible, and only support IDType://ID for clis. That's what I did with ctr, as it had no existing --device support for Windows. Because we could get class/GUID from k8s-directx-device-plugin and the existing docker cli, both moby and the containerd-cri backends needed to support it.

It's a good call that now that Docker v.23.00 is released, the MS docs can probably be updated to prefer the new syntax, and possibly demonstrate some of the other IDTypes, although the others tend to rely on system-local details that you look up with pnputil. class itself is marked as legacy IDType, but the replacement name escapes me. (I'd have to look it up in the containerd unit tests, to be honest...)

Note that there was no matching change in the Docker cli AFAIR because it passes the string wholesale through the API, so all the parsing is on the moby/moby side.

So anyway, the current code seems fine to me on this basis (since I wrote an earlier version...), just giving some background as to why, and giving context if you decide you want Docker-legacy-compat as well. (In which case, consider slurping code from either containerd-cri, or moby, since both do the same splitting but also recognise class/GUID explicitly.

---

For integration tests, you could consider the trick I used in containerd (or moby... I forget which) which is that when activating the DirectX device class, even if no dedicated GPU is present, a magic host-bind-mount is created. That's where this nonsense came from, and proved able to run integration tests on the random Azure and GitHub Actions VMs that containerd test suites get run on.

If you check C:\windows\System32\containers\devices.def you might find another device class that also does something detectable when used.

We also had a discussion somewhere (probably in that PR's history, but it might have been in the matching moby/moby PR; whenever it was that MS's container device expert noticed what I was up to and suggested we find a not-relying-on-undocumented-internal-behaviour solution -- Edit: Found the thread.) about using pnputil or similar to detect mapped device classes or individual devices. However, pnputil isn't present in nanoserver images, from memory. So we filed the host-mount trick as "good enough" to merge, since it was already true for every released Windows version we cared about, and replacing it was still a "someday" idea for that team.

There's also this little toy I was putting together to use in integration tests of this code, before I realised the directory-mount trick would be simpler. Something like that, in a dedicated image, might be usable and extendable for all of the various things that need to build integration tests for Windows devices, perhaps. But I don't have time to work on it at the (long) moment.

If you want to look at other IDType values for tests, I believe that the containerd-cri test suite's TestWithDevices contained every variation I could find, including links to where I found them. However, these were parse-only tests, since as I mentioned a few of the IDTypes depend on system-specific details, e.g. the PCI routing path to the device to be mapped into the container.

[adamrehn]

@TBBle thanks for the additional context and advice about testing this functionality!

So I performed some tests on a GCE instance, and I was indeed able to expose the COM ports to a Windows container. The only problem was that the chgport command I was using to test for their presence isn't available in the Windows Nanoserver image that's used for the unit tests, and the other alternatives I could find required PowerShell. As a workaround, I attempted to simply bind-mount C:\Windows\System32\chgport.exe from the host system, but unfortunately that failed due to #759.

In the end, I decided that the simplest option was to follow @TBBle's example by specifying the device interface class for GPUs and testing for the presence of the C:\Windows\System32\HostDriverStore directory inside the container. Obviously this isn't ideal given that Microsoft might decide to change the path in the future (breaking both this unit test and the one in containerd itself), but it's the best we can do without adding files to the test image or waiting for bind-mount support to function correctly. Later on down the track, I suspect that a better test would be using device-util.exe from the hcsshim repository to test for the presence of the \Device\DxgKrnl entry in the Object Manager namespace for the container's Silo, since my understanding is that the path for that particular mount cannot be changed out from under us without Microsoft making fundamental changes to the way that WDDM works.

[fahedouch]

Thanks

[fahedouch]

CI is failing after 5 retires :( . I believe that cause is not related to your changes

[adamrehn]

Yeah, the CI failures all appear to be rootless Linux tests, and the changes in this PR only affect Windows hosts, so I guess something else must be going on there.

[AkihiroSuda]

Why incompatible?

[TBBle]

I didn't notice that line earlier, but now I look, every test in this file appear to be DockerIncompatible, and some appear to be needlessly-so (like this one and TestRunProcessIsolated, which are both trivially "run container with Docker-compatible command-line, and check stdout" tests). Is there something in the underlying test suite that makes running against a WIndows Docker build inherently fail?

container_run_user_windows_test.go doesn't seem to have this limitation though, so perhaps this is a long-ago copy-and-paste oversight that has kept propagating from a test which did use Docker-incompatible features?

[adamrehn]

Yeah, I wasn't 100% sure if that line was actually necessary. I included it because it was present in TestRunProcessIsolated, despite the absence of any Docker-incompatible flags. I figured that the same compatibility constraints apply to the new test as the existing test, but if that line is erroneous in the existing test then I've effectively just duplicated that mistake.