This PR is also dependent upon a couple libcontainer PRs being merged and vendored. Those pull requests are:
docker-archive/libcontainer#6
docker-archive/libcontainer#7

@calfonso what are the use cases? links to docs or requirements?

Just to add some context of why we want to add a device at runtime...
Although this API is useful in general, it specifically gives us the ability to have OpenStack Nova APIs call the docker container hypervisor plugin to dynamically attach and detach a volume. In order for us to add the hypervisor plugin to OpenStack, the hypervisor plugin needs to implement certain features - including a runtime attaching/detaching of storage.

@shykes thoughts on this?

@calfonso this PR will need documentation added - both to cli.md and to the dcos/man pages - you can wait til you've had confirmation from the core devs, or use it as an opportunity to explain the feature's intentions :)

@SvenDowideit Thanks for the heads up. I've added a commit for the docs to this PR.

@calfonso you're missing the DCO on the last commit - There is a githook in the CONTRIBUTING.md that will do it for you...

I'm opposed to this. I do not believe volume attach/detach is a useful operation for a container. I feel this is the side effect of trying to shove containers into a VM model. If you want to treat a container as a VM then just use libvirt-lxc.

@ibuildthecloud would you mind elaborating a bit on the lack of usefulness? It is useful to be able to attach storage to a running container in that a service doesn't need to be terminated to attach storage.

I'm curious to see if this set of PR's (including the closed unmerged one in libcontainer) will help @jpetazzo and me do some slightly cool and dubious things with sharing mounts made from inside a container to another container

@SvenDowideit ewindisch has done a bit of research on using libseccomp to do a secure mount, which is not part of this patch set. These patches will enable sharing devices though!

@ewindisch @jpetazzo perhaps we all need to get together sometime soon :)

@SvenDowideit wait, that's totally not the same thing. This dynamically grants access to a device, using the corresponding cgroup. We were working on dynamic bind mounts (or even mounts). Of course, if you want to mount a filesystem sitting on a "real" device, that helps. But that's not what I was working on, sorry!

@jpetazzo yes, i realise its not the same thing - but its all related :)

@calfonso note that #6134 introduces language into the documentation around the security of ephemeral devices being added and not being able to be removed. It would be worth amending that documentation with the ability to 'docker devrm' devices. There would still be security risks dependent on how users manage these device bindings, but this partially addresses those concerns.

Docs essentially LGTM

ping @shykes @vieux @tianon

This feature seems useful and provides limited remediation for the security concerns noted in #6134 (devices granted may be replaced, major/minor numbers reused, etc...)

@calfonso @ewindisch @imain

I talked with the other maintainers about this and it is something that we want to get in. We are just worried about the UI for the feature. If we go the route of dev-add dev-rm` then when we want to introduce more ways to dynamically modify a container the docker commands and API will explode.

We can take what you have now with the changes to libcontainer and lxc and get these merged in but we would like it if you could propose some alternatives to the UI ( cli and API ) so that the command set does not explode when we start adding more ways to modify the container.

Does this sound ok with you?

@crosbymichael For the UI, what do you think about introducing something like the following?

docker update CONTAINER --action ACTION --args ACTION_ARGS and have the API have something similar? For device adding/removing it would look like: docker update 54b8808d05cf --action device-add --args /dev/loop0

Re merging libcontainer the lxc patch, that requires the C go patch for nsenter Mknod ( docker-archive/libcontainer#6 ) that you had requested be reimplemented in a more generic way as well, so we're working through that.

@crosbymichael after chatting with @calfonso this morning, perhaps a general foundation command like docker container CONTANIER [CONTAINER...] would be ideal here. By default it will show some info set on the provided CONTAINTER(s). The modifier flags would be something like

docker container --dev 'add=/dev/loop0' sharp_torvalds
docker container --dev 'rm=/dev/loop0' evil_stallman

And this way, the API and UI can stay generic for a number of other features.

Look okay?

@vbatts I like that, as it prevents the UI from 'exploding' and keeps the road open for additional modifications to existing containers (e.g. Dynamically add/remove links). Not too fond on the string notation of the actual action ('add=...'), they feel a bit "clunky" but don't see other options ATM.

@thaJeztah clunky, maybe, but same as existing flags like --filter and --storage-opts and --lxc-conf

@vbatts you're right. So, better clunky, but consistent than slick and inconsistent.

You have my vote FWIW :)

update or modify sounds good to me.

For an API request I was thinking about something like:

POST /containers/<name>/{update|modify}
{
    "type": "device",
    "values": {
        "add": "/dev/loop0"
    }
}

values just being a map[string]string in Go.

What do you think?

I like modify better than update. I like the API. Any other thoughts from anyone before we get started?

modify sounds fine to me. alter could be an option, but not sure if that is any better.

However, if renaming containers will be implemented in the future, I think the endpoint of the API should be;

POST /containers/(id)/modify

i.e. Use the container id, not its name. This is a bit less convenient, but consistent with other parts of the API as well.

Finally, shouldn't the API call use PUT instead of POST here? After all, we're updating, not creating.

@thaJeztah Yes, I'll use PUT

Unless @crosbymichael disagrees, because IANTM, just a friendly visitor 😄

@thaJeztah id and name can be used interchangeably so the endpoint will accept both.

PUT sounds +1

@calfonso maybe we need to change the map from map[string]string to map[string][]string so that things like below are supported.

{
    "type": "device",
    "values": {
        "add": ["/dev/loop0", "/dev/loop1"]
    }
}

ping @vieux

Maybe even one step further; to allow batch-updating multiple properties in future;

[{
    "type": "device",
    "values": {
        "add": ["/dev/loop0", "/dev/loop1"]
    }
},
{
    "type": "volume",
    "values": {
        "remove": ["/var/www"]
    }
}
]

I would personally like these features, but some of the security discussion here concerns me. dev-rm is not a good solution for the ephemeral devices problem. If you were to try to "watch" /proc or dmesg for devices getting removed and try to dev-rm them before the device number got reused, you would end up with an insecure race condition. I have yet to find any solution to this security problem. Should I ask here if anyone knows of a solution? If dev-rm is to be added, there should be strong warnings in the docs not to try to use it to deal with the ephemeral devices+device number re-use security flaw.

@timthelion - there would be a timing attack possible, but that's still better than being outright unable to remove the device nodes.

Closing this has there is a better implementation opened by the same authors.

Hi, @calfonso , May I ask you a question, How can I install docker with source code from https://github.com/calfonso/docker/tree/docker_modify which has the ability to add/remove device to running container?
I aslo seek the code about nova-docker that achieve the above operation (https://github.com/calfonso/nova-docker/tree/device_add_remove).
Can you help me?
Thanks!