Skip to content

Conversation

@cwilhit
Copy link
Contributor

@cwilhit cwilhit commented Feb 7, 2019

Signed-off-by: Craig Wilhite [email protected]

Adds documentation portion for #1606. Includes a reference for how to run with --device on Windows & explain limitations of the feature (no Hyper-V isolation, no LCOW).

@jhowardmsft FYI

> that may be removed should not be added to untrusted containers with
> `--device`.
For Windows, the format of the string passed to the `--device` option is in the form of `--device=<IdType>/<Id>`. Today, Windows only supports an IdType of `class` and the Id as a [device interface class GUID](https://docs.microsoft.com/en-us/windows-hardware/drivers/install/overview-of-device-interface-classes). For a list of container-supported device interface class GUIDs, please refer to the table defined in the [Windows container docs](https://docs.microsoft.com/en-us/virtualization/windowscontainers/deploy-containers/hardware-devices-in-containers).
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than Today, perhaps state what versions of Windows. (AFAIK, this is RS5/1809 on only).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@codecov-io
Copy link

codecov-io commented Feb 7, 2019

Codecov Report

Merging #1663 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1663      +/-   ##
==========================================
+ Coverage   56.12%   56.13%   +<.01%     
==========================================
  Files         306      306              
  Lines       20964    20931      -33     
==========================================
- Hits        11766    11749      -17     
+ Misses       8345     8334      -11     
+ Partials      853      848       -5

Copy link
Contributor

@lowenna lowenna left a comment

Choose a reason for hiding this comment

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

LGTM (not a maintainer in this repo)

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

Thanks! I left some suggestions below (feel free to amend your commits to use those). Could you also squash your commits, so that there's only a single commit in this pull request? Let me know if you need help doing so 👍

> that may be removed should not be added to untrusted containers with
> `--device`.
For Windows, the format of the string passed to the `--device` option is in the form of `--device=<IdType>/<Id>`. Beginning with Windows Server 2019 and Windows 10 October 2018 Update, Windows only supports an IdType of `class` and the Id as a [device interface class GUID](https://docs.microsoft.com/en-us/windows-hardware/drivers/install/overview-of-device-interface-classes). For a list of container-supported device interface class GUIDs, please refer to the table defined in the [Windows container docs](https://docs.microsoft.com/en-us/virtualization/windowscontainers/deploy-containers/hardware-devices-in-containers).
Copy link
Member

Choose a reason for hiding this comment

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

Trying to avoid "please" in the documentation; perhaps

Refer to the table defined in the [Windows container
documentation](https://docs.microsoft.com/en-us/virtualization/windowscontainers/deploy-containers/hardware-devices-in-containers)
for a list of container-supported device interface class GUIDs.

Also, could you wrap these to ~80 chars?

Suggested change
For Windows, the format of the string passed to the `--device` option is in the form of `--device=<IdType>/<Id>`. Beginning with Windows Server 2019 and Windows 10 October 2018 Update, Windows only supports an IdType of `class` and the Id as a [device interface class GUID](https://docs.microsoft.com/en-us/windows-hardware/drivers/install/overview-of-device-interface-classes). For a list of container-supported device interface class GUIDs, please refer to the table defined in the [Windows container docs](https://docs.microsoft.com/en-us/virtualization/windowscontainers/deploy-containers/hardware-devices-in-containers).
For Windows, the format of the string passed to the `--device` option is in
the form of `--device=<IdType>/<Id>`. Beginning with Windows Server 2019
and Windows 10 October 2018 Update, Windows only supports an IdType of
`class` and the Id as a [device interface class
GUID](https://docs.microsoft.com/en-us/windows-hardware/drivers/install/overview-of-device-interface-classes).
Refer to the table defined in the [Windows container
documentation](https://docs.microsoft.com/en-us/virtualization/windowscontainers/deploy-containers/hardware-devices-in-containers)
for a list of container-supported device interface class GUIDs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, added these fixes.

For Windows, the format of the string passed to the `--device` option is in the form of `--device=<IdType>/<Id>`. Beginning with Windows Server 2019 and Windows 10 October 2018 Update, Windows only supports an IdType of `class` and the Id as a [device interface class GUID](https://docs.microsoft.com/en-us/windows-hardware/drivers/install/overview-of-device-interface-classes). For a list of container-supported device interface class GUIDs, please refer to the table defined in the [Windows container docs](https://docs.microsoft.com/en-us/virtualization/windowscontainers/deploy-containers/hardware-devices-in-containers).

When this option is specified for a process-isolated Windows container, _all_ devices which implement the requested device interface class GUID will be made available in the container. For example, the command below will make all COM ports on the host visibile in the container.
Copy link
Member

Choose a reason for hiding this comment

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

Should it be "devices that implement" (instead of "which"?)

Also, try to avoid future tense in technical documentation; here's a quick go at changing it to remove the future tense, and wrap the paragraph to 80 chars;

Suggested change
When this option is specified for a process-isolated Windows container, _all_ devices which implement the requested device interface class GUID will be made available in the container. For example, the command below will make all COM ports on the host visibile in the container.
If this option is specified for a process-isolated Windows container, _all_
devices that implement the requested device interface class GUID are made
available in the container. For example, the command below makes all COM
ports on the host visible in the container.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are correct--I've now learned when to use 'which' vs 'that'. It always confounded me in the past.

PS C:\> docker run --device=class/86E0D1E0-8089-11D0-9CE4-08003E301F73 mcr.microsoft.com/windows/servercore:ltsc2019
```

> **Note**: `--device` may only be used on Windows with process-isolated Windows containers. This option will fail if the container
Copy link
Member

Choose a reason for hiding this comment

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

Slightly changed to wrap to 80-chars, and remove future tense;

Suggested change
> **Note**: `--device` may only be used on Windows with process-isolated Windows containers. This option will fail if the container
> **Note**: the `--device` option is only supported on process-isolated
> Windows containers. This option fails if the container isolation is `hyperv`
> or when running Linux Containers on Windows (LCOW).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

@cwilhit
Copy link
Contributor Author

cwilhit commented Feb 13, 2019

@thaJeztah Ok, incorporated the changes. This is the first time I've squashed commits before, but I believe I got it correctly. Per instructions online, I had to force push to my branch on Github.

Copy link
Contributor

@silvin-lubecki silvin-lubecki left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for this contribution @cwilhit 👍

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

spotted two markdown issues, but LGTM otherwise, so ok to merge after those are addressed (thanks!)

the form of `--device=<IdType>/<Id>`. Beginning with Windows Server 2019
and Windows 10 October 2018 Update, Windows only supports an IdType of
`class` and the Id as a [device interface class
GUID](https://docs.microsoft.com/en-us/windows-hardware/drivers/install/overview-of-device-
Copy link
Member

Choose a reason for hiding this comment

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

oh; the URL itself cannot be wrapped (it's ok for the caption, but not for the URL)

GUID](https://docs.microsoft.com/en-us/windows-hardware/drivers/install/overview-of-device-
interface-classes).
Refer to the table defined in the [Windows container
docs](https://docs.microsoft.com/en-us/virtualization/windowscontainers/deploy
Copy link
Member

Choose a reason for hiding this comment

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

same here

…mitations (no Hyper-V isolation, no LCOW).

Signed-off-by: Craig Wilhite <[email protected]>
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@thaJeztah thaJeztah merged commit e4aa87f into docker:master Mar 19, 2019
@GordonTheTurtle GordonTheTurtle added this to the 19.03.0 milestone Mar 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants