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

Add --device support for Windows #1606

Merged
merged 1 commit into from Feb 6, 2019

Conversation

@jhowardmsft
Copy link
Contributor

commented Jan 7, 2019

Signed-off-by: John Howard jhoward@microsoft.com

Adds support for --device in Windows. This must take the form of: --device='class/clsid'. See https://blogs.technet.microsoft.com/virtualization/2018/08/13/bringing-device-support-to-windows-server-containers/ for more info. The server-side fix was merged in moby/moby#37638 back in November 2018.

@thaJeztah As discussed offline. Annoyingly, due to the code pattern having flag validation happening before the _ping is sent to the daemon, I had to move the validation in the Linux case to during parsing. i.e. after the server OS is known. Fortunately, that avoided the need for a global as was the option on the table before. If (at a later time), the device can be validated entirely daemon side for Linux, it will greatly simplify things.

This replaces #1290. @jterry75 FYI. You should be able to close the previous PR.

@codecov-io

This comment has been minimized.

Copy link

commented Jan 7, 2019

Codecov Report

Merging #1606 into master will decrease coverage by <.01%.
The diff coverage is 67.74%.

@@            Coverage Diff             @@
##           master    #1606      +/-   ##
==========================================
- Coverage   56.11%   56.11%   -0.01%     
==========================================
  Files         306      306              
  Lines       20909    20931      +22     
==========================================
+ Hits        11734    11746      +12     
- Misses       8328     8337       +9     
- Partials      847      848       +1
@jhowardmsft

This comment has been minimized.

Copy link
Contributor Author

commented Jan 18, 2019

@thaJeztah Could you ping some relevant folks to look at this?

@thaJeztah
Copy link
Member

left a comment

case "linux":
return validateLinuxPath(val, validDeviceMode)
case "windows":
// Windows does validation entirely server-side

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Jan 19, 2019

Member

👍 We should remove more of these validations from the client, and defer as much as possible to the daemon/api. Doing so is much more maintainable, and makes sure that the daemon is in charge as to "what's valid" and "what not" (which may differ between daemon versions). I'll start working on an epic for that

@@ -60,7 +60,7 @@ func NewCreateCommand(dockerCli command.Cli) *cobra.Command {
}

func runCreate(dockerCli command.Cli, flags *pflag.FlagSet, opts *createOptions, copts *containerOptions) error {
containerConfig, err := parse(flags, copts)
containerConfig, err := parse(flags, copts, dockerCli.ServerInfo().OSType)

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Jan 19, 2019

Member

For other reviewers; I was discussing some options with @jhowardmsft - it's a bit ugly, but we should (see my other comment) move more of these platform-specific validations out of the CLI. The cli should limit validation to "well-formedness" of values (e.g. if we expect an integer; validate it's an integer), but (if possible) nothing further than that.

@thaJeztah

This comment has been minimized.

Copy link
Member

commented Jan 19, 2019

@jhowardmsft as a follow-up, could you help write a short example for the reference documentation? https://github.com/docker/cli/blob/master/docs/reference/commandline/run.md#add-host-device-to-container---device

I'm not exactly sure what the format looks like for adding a device on Windows (or when using LCOW), so if you could show an example, that would be great! (I see there's some examples in the blog post at https://blogs.technet.microsoft.com/virtualization/2018/08/13/bringing-device-support-to-windows-server-containers/), perhaps you can take some of that

@jterry75

This comment has been minimized.

Copy link

commented Jan 23, 2019

LGTM (not a maintainer)

@thaJeztah

This comment has been minimized.

Copy link
Member

commented Jan 23, 2019

@thaJeztah

This comment has been minimized.

Copy link
Member

commented Feb 3, 2019

@jhowardmsft needs a rebase

@silvin-lubecki PTAL

Add --device support for Windows
Adds support for --device in Windows. This must take the form of:
--device='class/clsid'. See this post for more information:

https://blogs.technet.microsoft.com/virtualization/2018/08/13/bringing-device-support-to-windows-server-containers/

Signed-off-by: John Howard <jhoward@microsoft.com>

@jhowardmsft jhowardmsft force-pushed the jhowardmsft:device branch from 7ed290b to 593acf0 Feb 4, 2019

@jhowardmsft

This comment has been minimized.

Copy link
Contributor Author

commented Feb 4, 2019

@thaJeztah Yes, that's rebased now.

@jhowardmsft

This comment has been minimized.

Copy link
Contributor Author

commented Feb 6, 2019

@silvin-lubecki
Copy link
Contributor

left a comment

LGTM, thank you @jhowardmsft !
And as @thaJeztah said, it would be great to update the documentation too in a followup.

@silvin-lubecki silvin-lubecki merged commit afde31d into docker:master Feb 6, 2019

9 checks passed

ci/circleci: cross Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: shellcheck Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
ci/circleci: validate Your tests passed on CircleCI!
Details
codecov/patch 67.74% of diff hit (target 50%)
Details
codecov/project Absolute coverage decreased by -<.01% but relative coverage increased by +11.62% compared to 896ff57
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
dco-signed All commits are signed

@GordonTheTurtle GordonTheTurtle added this to the 19.03.0 milestone Feb 6, 2019

@srinathck

This comment has been minimized.

Copy link

commented Mar 27, 2019

Any update on When Docker CLI v19.03.0 will be out?

@thaJeztah

This comment has been minimized.

Copy link
Member

commented Mar 27, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.