Fixup some issues with plugin refcounting #35265
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cpuguy83
force-pushed
the
32609_defreference_voldriver_on_error
branch
4 times, most recently
from
e043e01 to
f22eb1a
Compare
|
For some reason tests work fine running manually but when run from the makefile it's not. |
cpuguy83
force-pushed
the
32609_defreference_voldriver_on_error
branch
2 times, most recently
from
b1c6b50 to
b26e068
Compare
In some circumstances we were not properly releasing plugin references, leading to failures in removing a plugin with no way to recover other than restarting the daemon. 1. If volume create fails (in the driver) 2. If a driver validation fails (should be rare) 3. If trying to get a plugin that does not match the passed in capability Ideally the test for 1 and 2 would just be a unit test, however the plugin interfaces are too complicated as `plugingetter` relies on github.com/pkg/plugin/Client (a concrete type), which will require spinning up services from within the unit test... it just wouldn't be a unit test at this point. I attempted to refactor this a bit, but since both libnetwork and swarmkit are reliant on `plugingetter` as well, this would not work. This really requires a re-write of the lower-level plugin management to decouple these pieces. Signed-off-by: Brian Goff <[email protected]>
cpuguy83
force-pushed
the
32609_defreference_voldriver_on_error
branch
from
b26e068 to
3816b51
Compare
|
Good to go now. |
boaz0
reviewed
Oct 25, 2017
| @@ -1,6 +1,8 @@ | |||
| package plugingetter | |||
|
|
|||
| import "github.com/docker/docker/pkg/plugins" | |||
| import ( | |||
| "github.com/docker/docker/pkg/plugins" | |||
There was a problem hiding this comment.
Just that goimports changed it.
boaz0
reviewed
Oct 25, 2017
| return installPath | ||
| } | ||
|
|
||
| func asVolumeDriver(cfg *plugin.Config) { |
There was a problem hiding this comment.
Love this idea, learned something new! 👍 💯
|
ping @vieux @tiborvass PTAL ❤️ |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In some circumstances we were not properly releasing plugin references,
leading to failures in removing a plugin with no way to recover other
than restarting the daemon.
Ideally the test for 1 and 2 would just be a unit test, however the
plugin interfaces are too complicated as
plugingetterrelies ongithub.com/pkg/plugin/Client (a concrete type), which will require
spinning up services from within the unit test... it just wouldn't be a
unit test at this point.
I attempted to refactor this a bit, but since both libnetwork and
swarmkit are reliant on
plugingetteras well, this would not work.This really requires a re-write of the lower-level plugin management to
decouple these pieces.
Fixes #32609