diff --git a/client/pluginmanager/drivermanager/instance.go b/client/pluginmanager/drivermanager/instance.go index 5196ff8dd..40f985c4c 100644 --- a/client/pluginmanager/drivermanager/instance.go +++ b/client/pluginmanager/drivermanager/instance.go @@ -186,18 +186,28 @@ func (i *instanceManager) dispense() (plugin drivers.DriverPlugin, err error) { } var pluginInstance loader.PluginInstance + dispenseFn := func() (loader.PluginInstance, error) { + return i.loader.Dispense(i.id.Name, i.id.PluginType, i.pluginConfig, i.logger) + } if reattach, ok := i.fetchReattach(); ok { // Reattach to existing plugin pluginInstance, err = i.loader.Reattach(i.id.Name, i.id.PluginType, reattach) + + // If reattachment fails, get a new plugin instance + if err != nil { + i.logger.Warn("failed to reattach to plugin, starting new instance", "err", err) + pluginInstance, err = dispenseFn() + } } else { // Get an instance of the plugin - pluginInstance, err = i.loader.Dispense(i.id.Name, i.id.PluginType, i.pluginConfig, i.logger) + pluginInstance, err = dispenseFn() } + if err != nil { // Retry as the error just indicates the singleton has exited if err == singleton.SingletonPluginExited { - pluginInstance, err = i.loader.Dispense(i.id.Name, i.id.PluginType, i.pluginConfig, i.logger) + pluginInstance, err = dispenseFn() } // If we still have an error there is a real problem diff --git a/client/pluginmanager/drivermanager/instance_test.go b/client/pluginmanager/drivermanager/instance_test.go new file mode 100644 index 000000000..80af8b641 --- /dev/null +++ b/client/pluginmanager/drivermanager/instance_test.go @@ -0,0 +1,123 @@ +package drivermanager + +import ( + "context" + "fmt" + "testing" + + log "github.com/hashicorp/go-hclog" + "github.com/hashicorp/go-plugin" + "github.com/hashicorp/nomad/helper/pluginutils/loader" + "github.com/hashicorp/nomad/helper/pluginutils/singleton" + "github.com/hashicorp/nomad/helper/testlog" + "github.com/hashicorp/nomad/plugins/base" + dtu "github.com/hashicorp/nomad/plugins/drivers/testutils" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +type mockedCatalog struct { + mock.Mock +} + +func (m *mockedCatalog) Dispense(name, pluginType string, cfg *base.AgentConfig, logger log.Logger) (loader.PluginInstance, error) { + args := m.Called(name, pluginType, cfg, logger) + return loader.MockBasicExternalPlugin(&dtu.MockDriver{}, "0.1.0"), args.Error(0) +} + +func (m *mockedCatalog) Reattach(name, pluginType string, config *plugin.ReattachConfig) (loader.PluginInstance, error) { + args := m.Called(name, pluginType, config) + return loader.MockBasicExternalPlugin(&dtu.MockDriver{}, "0.1.0"), args.Error(0) +} + +func (m *mockedCatalog) Catalog() map[string][]*base.PluginInfoResponse { + m.Called() + return map[string][]*base.PluginInfoResponse{ + base.PluginTypeDriver: {&base.PluginInfoResponse{Name: "mock", Type: base.PluginTypeDriver}}, + } +} + +func (m *mockedCatalog) resetMock() { + m.ExpectedCalls = []*mock.Call{} + m.Calls = []mock.Call{} +} + +func TestInstanceManager_dispense(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + cat := new(mockedCatalog) + cat.Test(t) + var fetchRet bool + i := &instanceManager{ + logger: testlog.HCLogger(t), + ctx: ctx, + cancel: cancel, + loader: cat, + storeReattach: func(*plugin.ReattachConfig) error { return nil }, + fetchReattach: func() (*plugin.ReattachConfig, bool) { return nil, fetchRet }, + pluginConfig: &base.AgentConfig{}, + id: &loader.PluginID{Name: "mock", PluginType: base.PluginTypeDriver}, + updateNodeFromDriver: noopUpdater, + eventHandlerFactory: noopEventHandlerFactory, + firstFingerprintCh: make(chan struct{}), + } + require := require.New(t) + + // First test the happy path, no reattach config is stored, plugin dispenses without error + fetchRet = false + cat.On("Dispense", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil) + plug, err := i.dispense() + require.NoError(err) + cat.AssertNumberOfCalls(t, "Dispense", 1) + cat.AssertNumberOfCalls(t, "Reattach", 0) + + // Dispensing a second time should not dispense a new plugin from the catalog, but reuse the existing + plug2, err := i.dispense() + require.NoError(err) + cat.AssertNumberOfCalls(t, "Dispense", 1) + cat.AssertNumberOfCalls(t, "Reattach", 0) + require.Same(plug, plug2) + + // If the plugin has exited test that the manager attempts to retry dispense + cat.resetMock() + i.plugin = nil + cat.On("Dispense", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(singleton.SingletonPluginExited) + _, err = i.dispense() + require.Error(err) + cat.AssertNumberOfCalls(t, "Dispense", 2) + cat.AssertNumberOfCalls(t, "Reattach", 0) + + // Test that when a reattach config exists it attempts plugin reattachment + // First case is when plugin reattachment is successful + fetchRet = true + cat.resetMock() + i.plugin = nil + cat.On("Dispense", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil) + cat.On("Reattach", mock.Anything, mock.Anything, mock.Anything).Return(nil) + plug, err = i.dispense() + require.NoError(err) + cat.AssertNumberOfCalls(t, "Dispense", 0) + cat.AssertNumberOfCalls(t, "Reattach", 1) + // Dispensing a second time should not dispense a new plugin from the catalog + plug2, err = i.dispense() + require.NoError(err) + cat.AssertNumberOfCalls(t, "Dispense", 0) + cat.AssertNumberOfCalls(t, "Reattach", 1) + require.Same(plug, plug2) + + // Finally test when reattachment fails. A new plugin should be dispensed + cat.resetMock() + i.plugin = nil + cat.On("Dispense", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil) + cat.On("Reattach", mock.Anything, mock.Anything, mock.Anything).Return(fmt.Errorf("failed to dispense")) + plug, err = i.dispense() + require.NoError(err) + cat.AssertNumberOfCalls(t, "Dispense", 1) + cat.AssertNumberOfCalls(t, "Reattach", 1) + // Dispensing a second time should not dispense a new plugin from the catalog + plug2, err = i.dispense() + require.NoError(err) + cat.AssertNumberOfCalls(t, "Dispense", 1) + cat.AssertNumberOfCalls(t, "Reattach", 1) + require.Same(plug, plug2) + +}