This requires libcontainer change: docker-archive/libcontainer#209

Idea and concepts: LGTM

Just for context, we talked about this in the past and working on the UI for modifying an existing/running container.

Thanks @jamtur01, fixed the docs as per your comments.

Docs broadly LGTM ping @fredlf @ostezer @SvenDowideit

Docs LGTM once my nitpick is addressed.

Docs LGTM.
Although @imain / @calfonso it would be neat if you could replace double spacing with singles.
Thanks!

@proppy I think any container modifications.. They just didn't want to have the CLI explode with new calls that could all be lumped under 'modify'

@fredlf that was very funny :).

Hoping other features will be added soon (thinking of changing restart policy, published ports and renaming of running containers)

Hah! I'm just glad to know I'm not the only one who laughs at comma jokes!

Is this easy to do for volumes to? That would definitely be a killer feature for me with subuser.

Docs LGTM, but .... :)

the UX feels unusual (in the context of the rest of the docker cli)

docker modify --device-add /device/path CONTAINER
docker modify --device-remove /device/path CONTAINER

would seem more inline, and even more specifically

docker modify --device-add /device/path1 --device-remove /device/path2 --device-add /device/path3 CONTAINER

I don't think I agree with the concept of this. The majority of things in Docker are static once a container is launched. Actually I think basically everything is and this would be the first dynamic thing. What I don't like about this change is what process really dynamically discovers and uses new device nodes? I feel like this is a side effect of trying to run Docker in OpenStack and treating containers as VMs (and more specifically trying to get Cinder to work with Docker). VMs are generic units that you log into, start, stop, install programs, etc. Containers are more oriented towards processes. The container starts/stops with the life cycle of the process. From that perspective, if you wanted a new device node you probably have to deploy a new container with updated configuration (or ENV vars) that references the new device node.

This all feels very unnatural to me, but I realize its based on my opinion of how containers should be used. I don't like the idea of Docker supporting this use case natively.

@ibuildthecloud volumes and devices are already dynamic, in that a volume that is shared between the host and the container can be modified or deleted host side and so can a device. Personally, I need the ability to add and remove devices in subuser in order to allow users to say, plug in a webcam AFTER they have started an application which might use that webcam.

If I have a really hight trafic web server, and I'm writting logs directly to an SSD that is mounted as a device, then I might not want to restart the webserver when the SSD starts getting full.

An example of dynamic volumes: I run a wordpress server for my girlfriend. Wordpress might be somewhat more secure when one reduces write access to disk. It becomes to clean up after an attacker gets in. However, my girlfriend needs to find new themes for the websites she administrates, and needs these theme installed into wordpress somehow. It might be reasonable, to be able to dynamically add these template folders, by mounting a volume. Why should we encumber the process by making me restart wordpress every time we add a new theme and making her log out and back in again?

Basically, my point is, that I can come up with many examples where I'm using a single process system but I'd like some dynamicity.

Do you see some problem caused by this new dynamicity?

@timthelion All of your examples are dealing with things at a file level, not a block level. If you were to add a block device to expand storage you would need to format and then mount that. Mounting is not even allowed by default by Docker. The point I'm making is if your thinking about adding storage, adding a new block device is typically not useful. Imagine I'm running Apache and it logs to /var/log. I want to add storage to /var/log because it's running out of space. I can't just format and mount a new block device to /var/log2 because Apache is already configure to write to /var/log, so since I'm not restarting Apache it has to go to the same directory. Adding a block device won't do much unless I have a magical script running in the container that detects a new block device, formats it, then copies the contents of /var/log somewhere else, mount the formatted block device to /var/log, and then signal Apache to reopen the log. In the mean time you lost some logs and this requires you to be running Apache in a privileged container.

Now compare that with the more straight forward approach that works today. When I launch the apache container I do -v /host/path:/var/log and /host/path is on a LV. When I need more disk space I can just add a new PV to the VG and then just expand the LV. Now I have more disk space.

Now from a technical perspective, yes I have issues with this PR. First off the help for modify is basically useless and if you type a wrong command like modify 123 bogus value it just merrily continues with an exit status 0. But that's nit picky. The bigger problem is that changes from modify are not persisted. If I do modify 123 device-add /dev/sda1 and then restart the container, the device is gone.

We are having the same same conversation we had on #6369

Outside of the argument on whether it's useful to one person's use case or not to another person's use case, we can't support OpenStack's cinder api without this. Integration with OpenStack based upon DevCore requirements is a priority for docker, regardless of individual contributor perspective. We've rewritten the implementation to comply with the maintainers suggestions and technical direction. As I understood things, the debate over whether this should or shouldn't be implemented and merged was already water under the bridge. If this is not the case, i'd like the maintainer to revive the conversation, else we move past the 4 month old debate and focus on the technical implementation/review/merge.

@calfonso My opinions carry no weight, so I'll just let it be known that I disagree and we can continue forward.

From a technical perspective I do have the following complaints.

1. I think the syntax @SvenDowideit suggested is more consistent.
2. The help should print what options/commands are allowed.
3. An error should be printed if no or unknown commands are passed.
4. The hostConfig of the containers should be changed such that restarts maintain the changes
5. You should be able to run modify against a non-running container as the CLI does not allow you to change the hostConfig as the remote API allows you to.
6. Be careful of supporting something like modify --device-add ... --device-remove ... if you can't actually perform the operation in a transactional like fashion. If that command fails, what is the state of the container? Did the first add work or fail? If you can't do a transactional like approach then either don't allow multiple actions or have specific response codes for each commands if it failed or succeeded.

@ibuildthecloud Thank you for spending your time on such a detailed review.

Hmm, is that a real failure, or is it transitory?

@imain looks like you got HTTP 504 pulling the test image, given that your code doesn't touch anything to do with pulling images, I'd say it's not your fault.

This is now updated to use the DevicesApply() libcontainer changes.

@ibuildthecloud changing the hostConfig is an interesting situation because we don't do this when we run 'docker exec' which is another state-modifying behavior. If a container restarts, it restarts from its config and does not automatically add anything we ran with a 'docker exec' or might have otherwise been spawned inside the container. There is already a --device flag for 'docker run' which sets the devices to be used in the hostConfig and persists across restarts.

How about a compromise. I would guess there could be many people for and against changing the hostConfig depending on the use case. How about we add a flag --commit that gives the user the ability to persist the change or not.

I am in favor of allowing dynamic changes to every aspect of a container.
If the system allows it, there should be a clean way for Docker to do it.

However, I'm not a fan of a top-level "docker modify" command. A lot of
different things "modify" a container that are not part of this command.
It's not intuitive. As a user I would definitely find myself typing "docker
add-device" only to remember "oh right, that one is in modify". When we
allow mounting a volume, changing a network link, attaching key-value
annotations, resizing memory, should all that go under "modify" also?

On Thu, Oct 23, 2014 at 3:43 PM, Darren [email protected] wrote:

How about a compromise. I would guess there could be many people for and
against changing the hostConfig depending on the use case. How about we add
a flag --commit that gives the user the ability to persist the change or
not.

—
Reply to this email directly or view it on GitHub
#8348 (comment).

@shykes I agree. I was just thinking about #7468 which would conflict from a syntax standpoint. Between that proposal and this, maybe we should have a more consistent UI:

docker devices list
docker devices add
docker devices remove

Just to make sure everyone that was in the discussion then is in the discussion now... we had the same design discussion in the now replaced #6369

I'd like to make sure there is concensus around the change and that cli command sprawl is no longer an issue.

After discussing live with a few maintainers, we feel this requires making a wider UI decision - it may be a small change but it happens to be the first of a while new class of interactions (namely dynamic changes to a container). Lots of use cases and opinions out there.

Could you open a new PR with only the documentation part of this? That way we can go back and forth until we find the right design, and it won't be a waste of your time.

I'm closing this for now, not because I disagree with the implementation, but to make it clear that we need to agree on the UI first (via a separate, docs-only PR). Once that is done we can re-open, or create a new one, whichever makes most sense.

Again: everyone is in favor of adding this feature. We will get to a merge :)

@shykes thankyou for the good clear communiction.