From 721b629f0e6b4b02e7667438d7cf9eb3d1dce015 Mon Sep 17 00:00:00 2001 From: Steve Kriss Date: Mon, 27 Aug 2018 09:41:50 -0700 Subject: [PATCH] terminate plugin clients explicitly instead of using managed ones Signed-off-by: Steve Kriss --- pkg/cmd/server/server.go | 2 +- pkg/controller/backup_controller_test.go | 34 ++++++---------------- pkg/controller/restore_controller_test.go | 2 +- pkg/plugin/client_store.go | 24 ++++++++++++++++ pkg/plugin/manager.go | 35 +++++++++++++---------- 5 files changed, 55 insertions(+), 42 deletions(-) diff --git a/pkg/cmd/server/server.go b/pkg/cmd/server/server.go index eb90d2091..2eebbb936 100644 --- a/pkg/cmd/server/server.go +++ b/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) diff --git a/pkg/controller/backup_controller_test.go b/pkg/controller/backup_controller_test.go index 03df45b8a..22cc83f49 100644 --- a/pkg/controller/backup_controller_test.go +++ b/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 } diff --git a/pkg/controller/restore_controller_test.go b/pkg/controller/restore_controller_test.go index 0cdb0eee4..691d5c274 100644 --- a/pkg/controller/restore_controller_test.go +++ b/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) diff --git a/pkg/plugin/client_store.go b/pkg/plugin/client_store.go index 8656d8ac1..d39f1b90e 100644 --- a/pkg/plugin/client_store.go +++ b/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) +} diff --git a/pkg/plugin/manager.go b/pkg/plugin/manager.go index 2e7b776a9..482bff1ba 100644 --- a/pkg/plugin/manager.go +++ b/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() }