Skip to content

Conversation

calavera
Copy link
Contributor

Fixes #15342

/cc @cpuguy83

Signed-off-by: David Calavera david.calavera@gmail.com

@cpuguy83
Copy link
Member

lint errors on missing comments for new exported types.

@calavera calavera force-pushed the volume_driver_host_config branch from 533a2b2 to 1ba23c2 Compare August 25, 2015 07:53
@calavera calavera force-pushed the volume_driver_host_config branch 5 times, most recently from 31dbc49 to 48b1cbd Compare August 26, 2015 10:10
@cpuguy83
Copy link
Member

Definitely a +1 from me since I created the issue :)

@thaJeztah
Copy link
Member

Yup, +1 from me for changing this. Don't forget to update the docs and man-pages if this moves further.

@calavera calavera force-pushed the volume_driver_host_config branch 7 times, most recently from a1070ac to e493149 Compare August 31, 2015 07:24
@cpuguy83
Copy link
Member

Should this require backwards compat or should this part of 1.8.2 and thus modify the current API?

@cpuguy83
Copy link
Member

My thought is make it part of 1.8.x.

@thaJeztah
Copy link
Member

I'm +1 to making it part of 1.8.x, regarding it a "fix" and to reduce the exposure (yes, it's awkward doing an API change in a dot-release, but we should try to avoid people starting to rely on this)

@calavera
Copy link
Contributor Author

@cpuguy83, @thaJeztah I'm neutral, but this change is completely backwards compatible. Containers started with VolumeDriver in the config still work as expected if stopped and started with this new code.

@cpuguy83
Copy link
Member

@calavera Oh yes, makes sense. We have to support already created containers here.

@cpuguy83
Copy link
Member

I still see VolumeDriver showing up in the inspect output and not in HostConfig.
It does work, though, as VolumeDriver which was committed to the image is not used by a container created from that image.

@calavera
Copy link
Contributor Author

@cpuguy83 version numbers are hard! I'm going to add some tests for the inspect output.

@calavera calavera force-pushed the volume_driver_host_config branch from e493149 to e658ee6 Compare August 31, 2015 20:00
@calavera
Copy link
Contributor Author

I've added a couple of integration tests to prevent regressions. It turned out that I left the field in the config inspect for the next version of docker 😔

@cpuguy83
Copy link
Member

LGTM

@calavera calavera force-pushed the volume_driver_host_config branch from e658ee6 to 5655f9f Compare September 3, 2015 09:12
@calavera
Copy link
Contributor Author

calavera commented Sep 3, 2015

I've rebased this. Please, take a look.

@bboreham
Copy link
Contributor

bboreham commented Sep 3, 2015

If I understand correctly, this design still only allows one VolumeDriver per container. What happens if you want to mount one volume using Flocker and one using Convoy?

@cpuguy83
Copy link
Member

cpuguy83 commented Sep 3, 2015

@bboreham Need to use docker volume create, then bind that volume in using the name.

@cpuguy83
Copy link
Member

cpuguy83 commented Sep 3, 2015

@calavera external volume tests are failing.

@thaJeztah
Copy link
Member

@bboreham using the new volume API volume create command, this should work;

docker run -d \
   -v $(docker volume create --driver=foo):/volume-a/ \
   -v $(docker volume create --driver=bar):/volume-b/ \
   my-image

@bboreham
Copy link
Contributor

bboreham commented Sep 3, 2015

Ah, right, thanks. Wasn't clear from https://docs.docker.com/extend/plugins_volume/

@thaJeztah
Copy link
Member

@bboreham it's not released yet; the volume create command will be in the next release (docker 1.9), but you can experiment with it, by downloading a binary from https://master.dockerproject.org (there are still some bugs, so tread carefully)

@crosbymichael
Copy link
Contributor

@calavera volume test are failing after rerunning the tests.

Signed-off-by: David Calavera <david.calavera@gmail.com>
@calavera calavera force-pushed the volume_driver_host_config branch from 5655f9f to 6549d65 Compare September 4, 2015 16:45
@calavera
Copy link
Contributor Author

calavera commented Sep 4, 2015

@crosbymichael I don't know what happened with those tests, but it looks like it's passing now.

@calavera calavera added this to the 1.9.0 milestone Sep 7, 2015
@crosbymichael
Copy link
Contributor

LGTM

1 similar comment
@cpuguy83
Copy link
Member

cpuguy83 commented Sep 9, 2015

LGTM

cpuguy83 added a commit that referenced this pull request Sep 9, 2015
Move VolumeDriver to HostConfig to make containers portable.
@cpuguy83 cpuguy83 merged commit 9ca4aa4 into moby:master Sep 9, 2015
@calavera calavera deleted the volume_driver_host_config branch September 9, 2015 04:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants