Add --device support for Windows #37638

jterry75 · 2018-08-14T15:26:06Z

Implements the --device forwarding for Windows daemons. This maps the physical
device into the container at runtime.

Ex:

docker run --device="class/"

Signed-off-by: Justin Terry (VM) juterry@microsoft.com

@jhowardmsft - FYI

lowenna

Couple of minor nits for strings. Code LGTM

daemon/oci_windows.go

lowenna · 2018-08-14T16:39:10Z

@johnstep PTAL

codecov · 2018-08-14T17:35:54Z

Codecov Report

Merging #37638 into master will decrease coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #37638      +/-   ##
==========================================
- Coverage   36.12%   36.08%   -0.04%     
==========================================
  Files         610      610              
  Lines       45244    45234      -10     
==========================================
- Hits        16343    16323      -20     
- Misses      26660    26669       +9     
- Partials     2241     2242       +1

jterry75 · 2018-08-16T16:13:06Z

@johnstep - All CI has passed except experimental which failed due to Swarm (unrelated in any way to this change that I can tell). Mind taking another look?

johnstep · 2018-08-16T17:01:04Z

I cannot find the failed build on Jenkins, after it was restarted, but it might have been one of the known flaky tests., possibly DockerSwarmSuite.TestSwarmClusterRotateUnlockKey in the list here: #37306.

jterry75 · 2018-11-05T19:22:23Z

@jhowardmsft @johnstep - I made a mistake in the vendor release on HCSShim for v0.7.9-1 that I have now corrected. But I have no code changes here to restart the builds. Can you do that for me?

lowenna · 2018-11-06T16:33:59Z

@jterry75 Restarted, but unfortunately needs another rebase

jterry75 · 2018-11-06T16:53:32Z

I am going to have to make a new release of hcsshim. I didn't realize you had an outstanding merge. Will rebase this AM

jterry75 · 2018-11-07T19:35:47Z

@jhowardmsft - z failed with: FAIL: docker_api_swarm_test.go:296: DockerSwarmSuite.TestAPISwarmLeaderElection which is unrelated to this change. windowsRS5-process failed with: ERROR: Failed 'ERROR: Failed to load c:\baseimages\windowsservercore.tar into daemon under test' at 11/07/2018 00:01:17 again unrelated. Are we good with this change?

lowenna · 2018-11-08T19:36:12Z

RS5 CI is hawked at the moment. Investigating why. Restarting the other jobs - failures are unrelated.

lowenna

LGTM

lowenna · 2018-11-08T19:38:34Z

@johnstep PTAL

johnstep

Looks good, but see a couple comments. Thanks!

libcontainerd/client_local_windows.go

Implements the --device forwarding for Windows daemons. This maps the physical device into the container at runtime. Ex: docker run --device="class/<clsid>" <image> <cmd> Signed-off-by: Justin Terry (VM) <juterry@microsoft.com>

jterry75 · 2018-11-21T23:33:28Z

@johnstep - PTAL. After adding the errors checks you requested we do need the len(spec.Windows.Devices) > 0 check so this should pass CI now.

johnstep · 2018-11-26T05:48:00Z

Looks good.

thaJeztah · 2018-11-27T23:35:42Z

vendor/github.com/Microsoft/hcsshim/interface.go

+// AssignedDevice represents a device that has been directly assigned to a container
+//
+// NOTE: Support added in RS5
+type AssignedDevice = schema1.AssignedDevice


I just noticed the -1 in this release, and wondered why; that's when I noticed this was added in v0.7.9-1 https://github.com/Microsoft/hcsshim/blob/v0.7.9-1/interface.go#L20-L23, but;

not present in v0.7.10 https://github.com/Microsoft/hcsshim/blob/v0.7.10/interface.go

not present in v0.7.11 https://github.com/Microsoft/hcsshim/blob/v0.7.11/interface.go

not present in v0.7.12 https://github.com/Microsoft/hcsshim/blob/v0.7.12/interface.go

added again in v0.7.12-1 https://github.com/Microsoft/hcsshim/blob/v0.7.12-1/interface.go#L20-L23

not present in v0.7.13 https://github.com/Microsoft/hcsshim/blob/v0.7.13/interface.go

added again in v0.7.14 https://github.com/Microsoft/hcsshim/blob/v0.7.14/interface.go#L20-L23

present in all 0.8.x versions

Curious; was it not possible to either update to (e.g.) a new v0.7.x version (v0.7.14), or update to v0.8.x? (I assumed the hcsshim repository used SemVer, so updating to newer patch releases wouldn't break things)

Unfortunately hcsshim was not using SymVer correctly at the time of these releases. The minimal change to docker was to make this spot fix to the v0.7.9 release + this one commit. This change is already part of master in the v0.8.x releases as you see and the v0.8.x releases are properly using SymVer from now on. When we update docker to one of those releases (much bigger change) we will no longer need this spot fix.

Well, strictly it's still 0.x, so breaking changes are allowed 😅

My concern was that if there's bugfixes in hcsshim, and more recent (0.7.x) versions have breaking changes, we wouldn't be able to update to those versions, unless those fixes are backported to (e.g.) v0.7.12-2.

Are there any breaking changes between v0.7.12 and v0.7.14 that currently prevent us from updating to the latest 0.7.x version? microsoft/hcsshim@v0.7.9...v0.7.14

If there are; are those changes addressable in the moby codebase?

@jhowardmsft - Is likely better equip to answer that question. My assumption is that if we need a v0.7 release we will have to carry this fix as you say. But hopefully when @jhowardmsft is ready we will just move to the v0.8.x releases. I am not aware of any breaking changes for the v1 API's that Docker uses

GordonTheTurtle added the status/0-triage label Aug 14, 2018

jterry75 mentioned this pull request Aug 14, 2018

Add --device support for Windows docker/cli#1290

Closed

lowenna approved these changes Aug 14, 2018

View reviewed changes

daemon/oci_windows.go Outdated Show resolved Hide resolved

daemon/oci_windows.go Outdated Show resolved Hide resolved

lowenna added status/2-code-review rebuild/experimental and removed status/0-triage labels Aug 16, 2018

GordonTheTurtle removed the rebuild/experimental label Aug 16, 2018

GordonTheTurtle assigned johnstep Aug 29, 2018

yongtang added the rebuild/experimental label Sep 9, 2018

GordonTheTurtle removed the rebuild/experimental label Sep 9, 2018

lowenna added the rebuild/* label Nov 6, 2018

GordonTheTurtle removed the rebuild/* label Nov 6, 2018

lowenna added rebuild/powerpc labels Nov 8, 2018

GordonTheTurtle removed rebuild/powerpc labels Nov 8, 2018

lowenna approved these changes Nov 8, 2018

View reviewed changes

lowenna added the rebuild/powerpc label Nov 14, 2018

GordonTheTurtle removed the rebuild/powerpc label Nov 14, 2018

johnstep approved these changes Nov 15, 2018

View reviewed changes

libcontainerd/client_local_windows.go Show resolved Hide resolved

libcontainerd/client_local_windows.go Show resolved Hide resolved

Add --device support for Windows

b2d9986

Implements the --device forwarding for Windows daemons. This maps the physical device into the container at runtime. Ex: docker run --device="class/<clsid>" <image> <cmd> Signed-off-by: Justin Terry (VM) <juterry@microsoft.com>

lowenna added status/4-merge rebuild/powerpc and removed status/2-code-review labels Nov 26, 2018

GordonTheTurtle removed rebuild/powerpc labels Nov 26, 2018

yongtang added rebuild/powerpc labels Nov 27, 2018

GordonTheTurtle removed rebuild/powerpc labels Nov 27, 2018

yongtang added the rebuild/powerpc label Nov 27, 2018

GordonTheTurtle removed the rebuild/powerpc label Nov 27, 2018

vdemeester merged commit 6fa1498 into moby:master Nov 27, 2018

jterry75 deleted the devices_windows branch November 27, 2018 23:02

thaJeztah added the impact/changelog label Nov 27, 2018

thaJeztah reviewed Nov 27, 2018

View reviewed changes

thaJeztah mentioned this pull request Nov 30, 2018

[WIP] overlay2: fix for Linux 4.19 (metacopy=off) #38296

Closed

lowenna mentioned this pull request Jan 7, 2019

Add --device support for Windows docker/cli#1606

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add --device support for Windows #37638

Add --device support for Windows #37638

jterry75 commented Aug 14, 2018

lowenna left a comment

lowenna commented Aug 14, 2018

codecov bot commented Aug 14, 2018 •

edited

Loading

jterry75 commented Aug 16, 2018

johnstep commented Aug 16, 2018

jterry75 commented Nov 5, 2018

lowenna commented Nov 6, 2018

jterry75 commented Nov 6, 2018

jterry75 commented Nov 7, 2018

lowenna commented Nov 8, 2018

lowenna left a comment

lowenna commented Nov 8, 2018

johnstep left a comment

jterry75 commented Nov 21, 2018

johnstep commented Nov 26, 2018

thaJeztah Nov 27, 2018 •

edited

Loading

jterry75 Nov 27, 2018

thaJeztah Nov 28, 2018

jterry75 Nov 28, 2018

Add --device support for Windows #37638

Add --device support for Windows #37638

Conversation

jterry75 commented Aug 14, 2018

lowenna left a comment

Choose a reason for hiding this comment

lowenna commented Aug 14, 2018

codecov bot commented Aug 14, 2018 • edited Loading

Codecov Report

jterry75 commented Aug 16, 2018

johnstep commented Aug 16, 2018

jterry75 commented Nov 5, 2018

lowenna commented Nov 6, 2018

jterry75 commented Nov 6, 2018

jterry75 commented Nov 7, 2018

lowenna commented Nov 8, 2018

lowenna left a comment

Choose a reason for hiding this comment

lowenna commented Nov 8, 2018

johnstep left a comment

Choose a reason for hiding this comment

jterry75 commented Nov 21, 2018

johnstep commented Nov 26, 2018

thaJeztah Nov 27, 2018 • edited Loading

Choose a reason for hiding this comment

jterry75 Nov 27, 2018

Choose a reason for hiding this comment

thaJeztah Nov 28, 2018

Choose a reason for hiding this comment

jterry75 Nov 28, 2018

Choose a reason for hiding this comment

codecov bot commented Aug 14, 2018 •

edited

Loading

thaJeztah Nov 27, 2018 •

edited

Loading