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.

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

Conversation

wallnerryan
Copy link
Contributor

Because docker added IDs to Mount() / Unmount()

Right now we choose to do nothing with the ID but add it to our plugin layer so that calls will continue to work with docker 1.12

https://clusterhq.atlassian.net/browse/FLOC-4480
#2854

@wallnerryan wallnerryan changed the title first pass at adding ID to Mount Unmount for docker 1.12 add ID to Mount / Unmount for docker 1.12 Aug 1, 2016
@@ -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

@wallrj
Copy link
Contributor

wallrj commented Aug 2, 2016

Thanks @wallnerryan

Please address or answer the comments above and merge when all the tests have passed.

Probably worth forcing the acceptance tests too....the non-loopback ones listed under the "cron"

@wallnerryan
Copy link
Contributor Author

@wallrj how come the acceptance tests under cron dont kick off automatically ?

@wallrj
Copy link
Contributor

wallrj commented Aug 2, 2016

how come the acceptance tests under cron dont kick off automatically ?

Because they're so slow.

We made a decision that it was sufficient to see the loopback acceptance tests pass .
The loopback accceptance tests use the loopback backend and therefore don't include any of the multi-node, dataset moving tests.
The full acceptance tests are run nightly on master.
And the idea is that we check the build results on master and notice any new errors.

@wallnerryan
Copy link
Contributor Author

part of combined branch #2880

@wallnerryan wallnerryan closed this Aug 3, 2016
@wallnerryan wallnerryan deleted the add-id-FLOC-4480 branch August 4, 2016 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants