Add support for passing device into docker driver #3512
Conversation
client/driver/docker.go
Outdated
| if len(driverConfig.Devices) > 0 { | ||
| var devices []docker.Device | ||
| for _, device := range driverConfig.Devices { | ||
| if device.HostPath != "" { |
There was a problem hiding this comment.
Do we need to check this since validate will fail if HostPath is empty?
client/driver/docker_test.go
Outdated
| if _, err := driver.Prestart(ctx.ExecCtx, task); err != nil { | ||
| t.Fatalf("unexpected error:%v", err) | ||
| } | ||
|
|
There was a problem hiding this comment.
Can we start and stat the file to make sure it exists? A lot of tests do this with the waitForExist helper and then inspecting the created container:
nomad/client/driver/docker_test.go
Line 786 in 22400bf
There was a problem hiding this comment.
I validated that the hostConfig from docker inspect output showed the mounted device. Didn't do a stat of the device file. All the existing tests were reading things off the output of docker inspect so I did the same, this should verify that the device mounting did work.
| for _, c := range dev.CgroupPermissions { | ||
| ch := string(c) | ||
| if ch != "r" && ch != "w" && ch != "m" { | ||
| return fmt.Errorf("invalid cgroup permission string: %q", dev.CgroupPermissions) |
There was a problem hiding this comment.
There was a problem hiding this comment.
I made this validation logic after reading https://docs.docker.com/engine/reference/commandline/run/#add-host-device-to-container-device. Also, note this sentence at the end of that documentation: Note: --device cannot be safely used with ephemeral devices. Block devices that may be removed should not be added to untrusted containers with --device. The documentation you linked above for --device-cgroup-rule for dynamic devices, which looks like something else. I could be missing something though.
We can create another follow up ticket to add configuration for --device-cgroup-rule without needing --device if this is something people need.
preetapan
force-pushed
the
b-docker-device-option
branch
from
ef24c1d
to
8e70fd8
Compare
|
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
Fixes #2938