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

Fixup some issues with plugin refcounting #35265

Merged
merged 1 commit into from Nov 7, 2017

Conversation

Projects
None yet
6 participants
@cpuguy83
Contributor

cpuguy83 commented Oct 20, 2017

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.

Fixes #32609

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Oct 20, 2017

Contributor

For some reason tests work fine running manually but when run from the makefile it's not.

Contributor

cpuguy83 commented Oct 20, 2017

For some reason tests work fine running manually but when run from the makefile it's not.

Fixup some issues with plugin refcounting
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 <cpuguy83@gmail.com>
@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Oct 22, 2017

Contributor

Good to go now.

Contributor

cpuguy83 commented Oct 22, 2017

Good to go now.

@@ -1,6 +1,8 @@
package plugingetter
import "github.com/docker/docker/pkg/plugins"
import (
"github.com/docker/docker/pkg/plugins"

This comment has been minimized.

@boaz1337

boaz1337 Oct 25, 2017

Member

Any reason for this change?

@boaz1337

boaz1337 Oct 25, 2017

Member

Any reason for this change?

This comment has been minimized.

@cpuguy83

cpuguy83 Oct 25, 2017

Contributor

Just that goimports changed it.

@cpuguy83

cpuguy83 Oct 25, 2017

Contributor

Just that goimports changed it.

return installPath
}
func asVolumeDriver(cfg *plugin.Config) {

This comment has been minimized.

@boaz1337

boaz1337 Oct 25, 2017

Member

Love this idea, learned something new! 👍 💯

@boaz1337

boaz1337 Oct 25, 2017

Member

Love this idea, learned something new! 👍 💯

@thaJeztah thaJeztah added this to backlog in maintainers-session Nov 2, 2017

@vieux vieux self-assigned this Nov 2, 2017

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Nov 2, 2017

Member

ping @vieux @tiborvass PTAL ❤️

Member

thaJeztah commented Nov 2, 2017

ping @vieux @tiborvass PTAL ❤️

@thaJeztah thaJeztah removed this from backlog in maintainers-session Nov 2, 2017

@vieux

vieux approved these changes Nov 2, 2017

LGTM

@vdemeester

LGTM 🐮

@vieux vieux merged commit 5745a85 into moby:master Nov 7, 2017

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 37469 has succeeded
Details
janky Jenkins build Docker-PRs 46160 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 6554 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 17742 has succeeded
Details
z Jenkins build Docker-PRs-s390x 6354 has succeeded
Details

@cpuguy83 cpuguy83 deleted the cpuguy83:32609_defreference_voldriver_on_error branch Nov 15, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment