terminate plugin clients explicitly instead of using managed ones

Signed-off-by: Steve Kriss <steve@heptio.com>

pkg/cmd/server/server.go

@@ -205,7 +205,7 @@ func newServer(namespace, baseName, pluginDir, metricsAddr string, logger *logru
 }
 
 func (s *server) run() error {
-	defer s.pluginManager.CleanupClients()
+	defer s.pluginManager.CloseAllClients()
 
 	signals.CancelOnShutdown(s.cancelFunc, s.logger)
 

pkg/controller/backup_controller_test.go

@@ -204,7 +204,7 @@ func TestProcessBackup(t *testing.T) {
 				cloudBackups.On("UploadBackup", "bucket", backup.Name, mock.Anything, mock.Anything, mock.Anything).Return(nil)
 
 				pluginManager.On("GetBackupItemActions", backup.Name).Return(nil, nil)
-				pluginManager.On("CloseBackupItemActions", backup.Name).Return(nil)
+				pluginManager.On("CloseBackupItemActions", backup.Name).Return()
 			}
 
 			// this is necessary so the Patch() call returns the appropriate object
@@ -321,17 +321,9 @@ type MockManager struct {
 }
 
 // CloseBackupItemActions provides a mock function with given fields: backupName
-func (_m *MockManager) CloseBackupItemActions(backupName string) error {
-	ret := _m.Called(backupName)
-
-	var r0 error
-	if rf, ok := ret.Get(0).(func(string) error); ok {
-		r0 = rf(backupName)
-	} else {
-		r0 = ret.Error(0)
-	}
-
-	return r0
+func (_m *MockManager) CloseBackupItemActions(backupName string) {
+	_ = _m.Called(backupName)
+	return
 }
 
 // GetBackupItemActions provides a mock function with given fields: backupName, logger, level
@@ -358,17 +350,9 @@ func (_m *MockManager) GetBackupItemActions(backupName string) ([]backup.ItemAct
 }
 
 // CloseRestoreItemActions provides a mock function with given fields: restoreName
-func (_m *MockManager) CloseRestoreItemActions(restoreName string) error {
-	ret := _m.Called(restoreName)
-
-	var r0 error
-	if rf, ok := ret.Get(0).(func(string) error); ok {
-		r0 = rf(restoreName)
-	} else {
-		r0 = ret.Error(0)
-	}
-
-	return r0
+func (_m *MockManager) CloseRestoreItemActions(restoreName string) {
+	_ = _m.Called(restoreName)
+	return
 }
 
 // GetRestoreItemActions provides a mock function with given fields: restoreName, logger, level
@@ -440,8 +424,8 @@ func (_m *MockManager) GetObjectStore(name string) (cloudprovider.ObjectStore, e
 	return r0, r1
 }
 
-// CleanupClients provides a mock function
-func (_m *MockManager) CleanupClients() {
+// CloseAllClients provides a mock function
+func (_m *MockManager) CloseAllClients() {
 	_ = _m.Called()
 	return
 }

pkg/controller/restore_controller_test.go

@@ -377,7 +377,7 @@ func TestProcessRestore(t *testing.T) {
 
 			if test.restore != nil {
 				pluginManager.On("GetRestoreItemActions", test.restore.Name).Return(nil, nil)
-				pluginManager.On("CloseRestoreItemActions", test.restore.Name).Return(nil)
+				pluginManager.On("CloseRestoreItemActions", test.restore.Name).Return()
 			}
 
 			err = c.processRestore(key)

pkg/plugin/client_store.go

@@ -71,6 +71,22 @@ func (s *clientStore) list(kind PluginKind, scope string) ([]*plugin.Client, err
 	return nil, errors.New("clients not found")
 }
 
+// listAll returns all plugin clients for all kinds/scopes, or a
+// zero-valued slice if there are none.
+func (s *clientStore) listAll() []*plugin.Client {
+	s.lock.RLock()
+	defer s.lock.RUnlock()
+
+	var clients []*plugin.Client
+	for _, pluginsByName := range s.clients {
+		for name := range pluginsByName {
+			clients = append(clients, pluginsByName[name])
+		}
+	}
+
+	return clients
+}
+
 // add stores a plugin client for the given kind/name/scope.
 func (s *clientStore) add(client *plugin.Client, kind PluginKind, name, scope string) {
 	s.lock.Lock()
@@ -103,3 +119,11 @@ func (s *clientStore) deleteAll(kind PluginKind, scope string) {
 
 	delete(s.clients, clientKey{kind, scope})
 }
+
+// clear removes all clients for all kinds/scopes from the store.
+func (s *clientStore) clear() {
+	s.lock.Lock()
+	defer s.lock.Unlock()
+
+	s.clients = make(map[clientKey]map[string]*plugin.Client)
+}

pkg/plugin/manager.go

@@ -44,7 +44,6 @@ func baseConfig() *plugin.ClientConfig {
 	return &plugin.ClientConfig{
 		HandshakeConfig:  Handshake,
 		AllowedProtocols: []plugin.Protocol{plugin.ProtocolGRPC},
-		Managed:          true,
 	}
 }
 
@@ -112,7 +111,7 @@ type Manager interface {
 
 	// CloseBackupItemActions terminates the plugin sub-processes that
 	// are hosting BackupItemAction plugins for the given backup name.
-	CloseBackupItemActions(backupName string) error
+	CloseBackupItemActions(backupName string)
 
 	// GetRestoreItemActions returns all restore.ItemAction plugins.
 	// These plugin instances should ONLY be used for a single restore
@@ -123,10 +122,10 @@ type Manager interface {
 
 	// CloseRestoreItemActions terminates the plugin sub-processes that
 	// are hosting RestoreItemAction plugins for the given restore name.
-	CloseRestoreItemActions(restoreName string) error
+	CloseRestoreItemActions(restoreName string)
 
-	// CleanupClients kills all plugin subprocesses.
-	CleanupClients()
+	// CloseAllClients terminates all plugin subprocesses.
+	CloseAllClients()
 }
 
 type manager struct {
@@ -349,8 +348,8 @@ func (m *manager) GetBackupItemActions(backupName string) ([]backup.ItemAction,
 
 // CloseBackupItemActions terminates the plugin sub-processes that
 // are hosting BackupItemAction plugins for the given backup name.
-func (m *manager) CloseBackupItemActions(backupName string) error {
-	return closeAll(m.clientStore, PluginKindBackupItemAction, backupName)
+func (m *manager) CloseBackupItemActions(backupName string) {
+	closeAll(m.clientStore, PluginKindBackupItemAction, backupName)
 }
 
 func (m *manager) GetRestoreItemActions(restoreName string) ([]restore.ItemAction, error) {
@@ -398,14 +397,18 @@ func (m *manager) GetRestoreItemActions(restoreName string) ([]restore.ItemActio
 
 // CloseRestoreItemActions terminates the plugin sub-processes that
 // are hosting RestoreItemAction plugins for the given restore name.
-func (m *manager) CloseRestoreItemActions(restoreName string) error {
-	return closeAll(m.clientStore, PluginKindRestoreItemAction, restoreName)
+func (m *manager) CloseRestoreItemActions(restoreName string) {
+	closeAll(m.clientStore, PluginKindRestoreItemAction, restoreName)
 }
 
-func closeAll(store *clientStore, kind PluginKind, scope string) error {
+func closeAll(store *clientStore, kind PluginKind, scope string) {
 	clients, err := store.list(kind, scope)
 	if err != nil {
-		return err
+		// store.list(...) only returns an error if no clients are
+		// found for the specified kind and scope. We don't need
+		// to treat this as an error when trying to close all clients,
+		// because this means there are no clients to close.
+		return
 	}
 
 	for _, client := range clients {
@@ -413,10 +416,12 @@ func closeAll(store *clientStore, kind PluginKind, scope string) error {
 	}
 
 	store.deleteAll(kind, scope)
-
-	return nil
 }
 
-func (m *manager) CleanupClients() {
-	plugin.CleanupClients()
+func (m *manager) CloseAllClients() {
+	for _, client := range m.clientStore.listAll() {
+		client.Kill()
+	}
+
+	m.clientStore.clear()
 }