Fix panic in loading plugins

When a plugin is first found, it is loaded into the available plugins
even though it's not activated yet.
If activation fails it is taken out of the list.
While it is in the list, other callers may see it and try to check it's
manifest. If it is not fully activated yet, the manifest will be nil and
cause a panic.

This is especially problematic for drivers that are down and have not
been activated yet.

We could just not load the plugin into the available list until it's
fully active, however that will just cause multiple of the same plugin
to attemp to be loaded.

We could check if the manifest is nil and return early (instead of
panicing on a nil manifest), but this will cause a 2nd caller to receive
a response while the first caller is still waiting, which can be
awkward.

This change uses a condition variable to handle activation (instead of
sync.Once). If the plugin is not activated, callers will all wait until
it is activated and receive a broadcast from the condition variable
signaling that it's ok to proceed, in which case we'll check if their
was an error in activation and proceed accordingly.

Signed-off-by: Brian Goff <cpuguy83@gmail.com>

pkg/plugins/client.go

@@ -130,7 +130,7 @@ func (c *Client) callWithRetry(serviceMethod string, data io.Reader, retry bool)
 				return nil, err
 			}
 			retries++
-			logrus.Warnf("Unable to connect to plugin: %s, retrying in %v", req.URL, timeOff)
+			logrus.Warnf("Unable to connect to plugin: %s:%s, retrying in %v", req.URL.Host, req.URL.Path, timeOff)
 			time.Sleep(timeOff)
 			continue
 		}

pkg/plugins/plugins.go

@@ -65,23 +65,36 @@ type Plugin struct {
 	// Manifest of the plugin (see above)
 	Manifest *Manifest `json:"-"`
 
-	activatErr   error
-	activateOnce sync.Once
+	// error produced by activation
+	activateErr error
+	// specifies if the activation sequence is completed (not if it is sucessful or not)
+	activated bool
+	// wait for activation to finish
+	activateWait *sync.Cond
 }
 
 func newLocalPlugin(name, addr string) *Plugin {
 	return &Plugin{
-		Name:      name,
-		Addr:      addr,
-		TLSConfig: tlsconfig.Options{InsecureSkipVerify: true},
+		Name:         name,
+		Addr:         addr,
+		TLSConfig:    tlsconfig.Options{InsecureSkipVerify: true},
+		activateWait: sync.NewCond(&sync.Mutex{}),
 	}
 }
 
 func (p *Plugin) activate() error {
-	p.activateOnce.Do(func() {
-		p.activatErr = p.activateWithLock()
-	})
-	return p.activatErr
+	p.activateWait.L.Lock()
+	if p.activated {
+		p.activateWait.L.Unlock()
+		return p.activateErr
+	}
+
+	p.activateErr = p.activateWithLock()
+	p.activated = true
+
+	p.activateWait.L.Unlock()
+	p.activateWait.Broadcast()
+	return p.activateErr
 }
 
 func (p *Plugin) activateWithLock() error {
@@ -108,7 +121,19 @@ func (p *Plugin) activateWithLock() error {
 	return nil
 }
 
+func (p *Plugin) waitActive() error {
+	p.activateWait.L.Lock()
+	for !p.activated {
+		p.activateWait.Wait()
+	}
+	p.activateWait.L.Unlock()
+	return p.activateErr
+}
+
 func (p *Plugin) implements(kind string) bool {
+	if err := p.waitActive(); err != nil {
+		return false
+	}
 	for _, driver := range p.Manifest.Implements {
 		if driver == kind {
 			return true
@@ -221,7 +246,7 @@ func GetAll(imp string) ([]*Plugin, error) {
 	var out []*Plugin
 	for pl := range chPl {
 		if pl.err != nil {
-			logrus.Error(err)
+			logrus.Error(pl.err)
 			continue
 		}
 		if pl.pl.implements(imp) {