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.

Sign up for GitHub

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 ID to Mount / Unmount for docker 1.12 #2876

Closed
wants to merge 3 commits into from
Closed

add ID to Mount / Unmount for docker 1.12 #2876

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ca53d5f
first pass at adding ID to Mount Unmount for docker 1.12
Aug 1, 2016
2040fe7
fix test where ID value used as key
Aug 1, 2016
24bab0d
add default value for backward compat with docker < 1.12, add no_id t…
Aug 2, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions flocker/dockerplugin/_api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,11 +197,12 @@ def volumedriver_remove(self, Name):

@app.route("/VolumeDriver.Unmount", methods=["POST"])
@_endpoint(u"Unmount")
def volumedriver_unmount(self, Name):
def volumedriver_unmount(self, Name, ID):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've got a feeling that this is going to break with Docker < 1.12.0 which won't supply an ID.
If I'm right, then we should give it a default value and add a test that the API can can be called with and without the ID.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point. (Another reason our tests should include other docker versions)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will verify it breaks with < 1.12, then make changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

confirmed this does break.

WARN[1065] 716e7f439070882877e3f401d06edc76b6c2d59cd48fb819b8fa3d80394afa02 cleanup: Failed to umount volumes: TypeError: volumedriver_unmount() takes exactly 3 arguments (2 given)
ERRO[1065] Handler for POST /v1.22/containers/716e7f439070882877e3f401d06edc76b6c2d59cd48fb819b8fa3d80394afa02/start returned error: TypeError: volumedriver_mount() takes exactly 3 arguments (2 given)
docker: Error response from daemon: TypeError: volumedriver_mount() takes exactly 3 arguments (2 given).
root@mha-aws-demo0:/home/ubuntu# docker version
Client:
 Version:      1.10.0
 API version:  1.22
 Go version:   go1.5.3
 Git commit:   590d5108
 Built:        Thu Feb  4 19:55:25 2016
 OS/Arch:      linux/amd64

Server:
 Version:      1.10.0
 API version:  1.22
 Go version:   go1.5.3
 Git commit:   590d5108
 Built:        Thu Feb  4 19:55:25 2016
 OS/Arch:      linux/amd64

"""
The Docker container is no longer using the given volume.

:param unicode Name: The name of the volume.
:param string ID: A unique ID for caller that requested the mount

For now this does nothing. In FLOC-2755 this will release the
lease acquired for the dataset by the ``VolumeDriver.Mount``
Expand Down Expand Up @@ -313,14 +314,19 @@ def got_state(datasets):

@app.route("/VolumeDriver.Mount", methods=["POST"])
@_endpoint(u"Mount")
def volumedriver_mount(self, Name):
def volumedriver_mount(self, Name, ID):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here too.

"""
Move a volume with the given name to the current node and mount it.

Since we need to return the filesystem path we wait until the
dataset is mounted locally.

:param unicode Name: The name of the volume.
:param string ID: A unique ID for caller that requested the mount

Right now we do nothing with ID, but it can be used to track
the mount calls when multiple containers are trying to mount
the same volume.

:return: Result that includes the mountpoint.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ Maybe double check whether the return format has changed. Do we have to include the supplied ID in the json that we return from the API.
If so, then we'll have to also update the JSON schema files.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The return schema is the same, ID is only passed in the request and not the response. See here https://docs.docker.com/engine/extend/plugins_volume/

"""
Expand Down
23 changes: 18 additions & 5 deletions flocker/dockerplugin/test/test_api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
Tests for the Volumes Plugin API provided by the plugin.
"""

import random
from uuid import uuid4

from bitmath import TiB, GiB, MiB, KiB, Byte
Expand Down Expand Up @@ -115,8 +116,12 @@ def test_unmount(self):
"""
``/VolumeDriver.Unmount`` returns a successful result.
"""
unmount_id = ''.join(random.choice(
'0123456789abcdef') for n in xrange(64))
return self.assertResult(b"POST", b"/VolumeDriver.Unmount",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked at moby/moby#21015 and ID comes from https://github.com/docker/docker/blob/master/pkg/stringid/stringid.go ....and which seems to be a / the container ID.

You could do something like os.urandom(32).encode('hex') (we use that elsewhere in Flocker) which might be make the intended ID format a little clearer....although when I tested it seems to hang when there's insufficient entropy. And I guess that urandom may not be hooked up to trial's random seed option....so forget this comment :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, saw this too, tried to duplicate the stringid non cryptic id in python.

Im fine either way, the question is do we want to test multiple IDs with hypothesis or even test for malformed IDs ?

{u"Name": u"vol"}, OK, {u"Err": u""})
{u"Name": u"vol",
u"ID": unicode(unmount_id)},
OK, {u"Err": u""})

def test_create_with_profile(self):
"""
Expand Down Expand Up @@ -320,6 +325,8 @@ def test_mount(self):
"""
name = u"myvol"
dataset_id = uuid4()
mount_id = ''.join(random.choice(
'0123456789abcdef') for n in xrange(64))

# Create dataset on a different node:
d = self.flocker_client.create_dataset(
Expand All @@ -337,7 +344,7 @@ def test_mount(self):
d.addCallback(lambda _:
self.assertResult(
b"POST", b"/VolumeDriver.Mount",
{u"Name": name}, OK,
{u"Name": name, u"ID": unicode(mount_id)}, OK,
{u"Err": u"",
u"Mountpoint": u"/flocker/{}".format(dataset_id)}))
d.addCallback(lambda _: self.flocker_client.list_datasets_state())
Expand All @@ -363,6 +370,8 @@ def test_mount_timeout(self):
"""
name = u"myvol"
dataset_id = uuid4()
mount_id = ''.join(random.choice(
'0123456789abcdef') for n in xrange(64))
# Create dataset on a different node:
d = self.flocker_client.create_dataset(
self.NODE_B, int(DEFAULT_SIZE.to_Byte()),
Expand All @@ -379,7 +388,7 @@ def test_mount_timeout(self):
d.addCallback(lambda _:
self.assertResult(
b"POST", b"/VolumeDriver.Mount",
{u"Name": name}, OK,
{u"Name": name, u"ID": unicode(mount_id)}, OK,
{u"Err": u"Timed out waiting for dataset to mount.",
u"Mountpoint": u""}))
return d
Expand All @@ -392,6 +401,8 @@ def test_mount_already_exists(self):
don't have a special dataset ID.
"""
name = u"myvol"
mount_id = ''.join(random.choice(
'0123456789abcdef') for n in xrange(64))

d = self.flocker_client.create_dataset(
self.NODE_A, int(DEFAULT_SIZE.to_Byte()),
Expand All @@ -401,7 +412,7 @@ def created(dataset):
self.flocker_client.synchronize_state()
result = self.assertResult(
b"POST", b"/VolumeDriver.Mount",
{u"Name": name}, OK,
{u"Name": name, u"ID": unicode(mount_id)}, OK,
{u"Err": u"",
u"Mountpoint": u"/flocker/{}".format(
dataset.dataset_id)})
Expand All @@ -420,9 +431,11 @@ def test_unknown_mount(self):
non-existent volume.
"""
name = u"myvol"
mount_id = ''.join(random.choice(
'0123456789abcdef') for n in xrange(64))
return self.assertResult(
b"POST", b"/VolumeDriver.Mount",
{u"Name": name}, OK,
{u"Name": name, u"ID": unicode(mount_id)}, OK,
{u"Err": u"Could not find volume with given name."})

def test_path(self):
Expand Down