From 6dcc37964bb51e1e38acb81d97ab2e9f2fc1f9fe Mon Sep 17 00:00:00 2001 From: Brian Goff Date: Wed, 30 Nov 2016 16:02:45 -0500 Subject: [PATCH] Refcount graphdriver plugins properly Adds 2 new methods to v2 plugin `Acquire` and `Release` which allow refcounting directly at the plugin level instead of just the store. Since a graphdriver is initialized exactly once, and is really managed by a separate object, it didn't really seem right to call `getter.Get()` to refcount graphdriver plugins. On shutdown it was particularly weird where we'd either need to keep a driver reference in daemon, or keep a reference to the pluggin getter in the layer store, and even then still store extra details on if the graphdriver is a plugin or not. Instead the plugin proxy itself will handle calling the neccessary refcounting methods directly on the plugin object. Also adds a new interface in `plugingetter` to account for these new functions which are not going to be implemented by v1 plugins. Changes terms `plugingetter.CREATE` and `plugingetter.REMOVE` to `ACQUIRE` and `RELEASE` respectively, which seems to be better adjectives for what we're doing. Signed-off-by: Brian Goff Upstream-commit: f29bbd16f5d2bb82d815ea59f8ef85fe59384c89 Component: engine --- components/engine/daemon/graphdriver/plugin.go | 6 +++--- components/engine/daemon/graphdriver/proxy.go | 15 +++++++++++++++ components/engine/pkg/plugingetter/getter.go | 15 +++++++++++---- components/engine/plugin/v2/plugin.go | 14 ++++++++++++++ components/engine/volume/drivers/extpoint.go | 4 ++-- 5 files changed, 45 insertions(+), 9 deletions(-) diff --git a/components/engine/daemon/graphdriver/plugin.go b/components/engine/daemon/graphdriver/plugin.go index 5e519ad255..6a79c29681 100644 --- a/components/engine/daemon/graphdriver/plugin.go +++ b/components/engine/daemon/graphdriver/plugin.go @@ -22,10 +22,10 @@ func lookupPlugin(name, home string, opts []string, pg plugingetter.PluginGetter if err != nil { return nil, fmt.Errorf("Error looking up graphdriver plugin %s: %v", name, err) } - return newPluginDriver(name, home, opts, pl.Client()) + return newPluginDriver(name, home, opts, pl) } -func newPluginDriver(name, home string, opts []string, c pluginClient) (Driver, error) { - proxy := &graphDriverProxy{name, c} +func newPluginDriver(name, home string, opts []string, pl plugingetter.CompatPlugin) (Driver, error) { + proxy := &graphDriverProxy{name, pl.Client(), pl} return proxy, proxy.Init(filepath.Join(home, name), opts) } diff --git a/components/engine/daemon/graphdriver/proxy.go b/components/engine/daemon/graphdriver/proxy.go index 45aeba1737..713124b442 100644 --- a/components/engine/daemon/graphdriver/proxy.go +++ b/components/engine/daemon/graphdriver/proxy.go @@ -6,11 +6,13 @@ import ( "io" "github.com/docker/docker/pkg/archive" + "github.com/docker/docker/pkg/plugingetter" ) type graphDriverProxy struct { name string client pluginClient + p plugingetter.CompatPlugin } type graphDriverRequest struct { @@ -35,6 +37,12 @@ type graphDriverInitRequest struct { } func (d *graphDriverProxy) Init(home string, opts []string) error { + if !d.p.IsV1() { + if cp, ok := d.p.(plugingetter.CountedPlugin); ok { + // always acquire here, it will be cleaned up on daemon shutdown + cp.Acquire() + } + } args := &graphDriverInitRequest{ Home: home, Opts: opts, @@ -167,6 +175,13 @@ func (d *graphDriverProxy) GetMetadata(id string) (map[string]string, error) { } func (d *graphDriverProxy) Cleanup() error { + if !d.p.IsV1() { + if cp, ok := d.p.(plugingetter.CountedPlugin); ok { + // always release + defer cp.Release() + } + } + args := &graphDriverRequest{} var ret graphDriverResponse if err := d.client.Call("GraphDriver.Cleanup", args, &ret); err != nil { diff --git a/components/engine/pkg/plugingetter/getter.go b/components/engine/pkg/plugingetter/getter.go index c568184b3c..b8096b96b2 100644 --- a/components/engine/pkg/plugingetter/getter.go +++ b/components/engine/pkg/plugingetter/getter.go @@ -5,10 +5,10 @@ import "github.com/docker/docker/pkg/plugins" const ( // LOOKUP doesn't update RefCount LOOKUP = 0 - // CREATE increments RefCount - CREATE = 1 - // REMOVE decrements RefCount - REMOVE = -1 + // ACQUIRE increments RefCount + ACQUIRE = 1 + // RELEASE decrements RefCount + RELEASE = -1 ) // CompatPlugin is a abstraction to handle both v2(new) and v1(legacy) plugins. @@ -19,6 +19,13 @@ type CompatPlugin interface { IsV1() bool } +// CountedPlugin is a plugin which is reference counted. +type CountedPlugin interface { + Acquire() + Release() + CompatPlugin +} + // PluginGetter is the interface implemented by Store type PluginGetter interface { Get(name, capability string, mode int) (CompatPlugin, error) diff --git a/components/engine/plugin/v2/plugin.go b/components/engine/plugin/v2/plugin.go index ff8f5ff235..553dfd875c 100644 --- a/components/engine/plugin/v2/plugin.go +++ b/components/engine/plugin/v2/plugin.go @@ -11,6 +11,7 @@ import ( "github.com/docker/docker/api/types" "github.com/docker/docker/oci" + "github.com/docker/docker/pkg/plugingetter" "github.com/docker/docker/pkg/plugins" "github.com/docker/docker/pkg/system" specs "github.com/opencontainers/runtime-spec/specs-go" @@ -294,6 +295,19 @@ func (p *Plugin) AddRefCount(count int) { p.refCount += count } +// Acquire increments the plugin's reference count +// This should be followed up by `Release()` when the plugin is no longer in use. +func (p *Plugin) Acquire() { + p.AddRefCount(plugingetter.ACQUIRE) +} + +// Release decrements the plugin's reference count +// This should only be called when the plugin is no longer in use, e.g. with +// via `Acquire()` or getter.Get("name", "type", plugingetter.ACQUIRE) +func (p *Plugin) Release() { + p.AddRefCount(plugingetter.RELEASE) +} + // InitSpec creates an OCI spec from the plugin's config. func (p *Plugin) InitSpec(s specs.Spec) (*specs.Spec, error) { s.Root = specs.Root{ diff --git a/components/engine/volume/drivers/extpoint.go b/components/engine/volume/drivers/extpoint.go index 78f86948f6..16ac0f3d96 100644 --- a/components/engine/volume/drivers/extpoint.go +++ b/components/engine/volume/drivers/extpoint.go @@ -153,7 +153,7 @@ func CreateDriver(name string) (volume.Driver, error) { if name == "" { name = volume.DefaultDriverName } - return lookup(name, getter.CREATE) + return lookup(name, getter.ACQUIRE) } // RemoveDriver returns a volume driver by its name and decrements RefCount.. @@ -162,7 +162,7 @@ func RemoveDriver(name string) (volume.Driver, error) { if name == "" { name = volume.DefaultDriverName } - return lookup(name, getter.REMOVE) + return lookup(name, getter.RELEASE) } // GetDriverList returns list of volume drivers registered.