From 1070eb7428ad2a17489c2e29e8b9fc9c7b1c1e8c Mon Sep 17 00:00:00 2001 From: Scott Nice Date: Sat, 27 Nov 2021 15:07:19 -0500 Subject: [PATCH 1/3] Fixed issue in plugin.go for bug #106696 Fixed issue in plugin.go where valid plugin events would be skipped if any plugin had an error. This meant that valid plugins would never be installed if another was in an error state as the events fired only once. --- pkg/volume/plugins.go | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/pkg/volume/plugins.go b/pkg/volume/plugins.go index 58ef46940d1..43783a7e86f 100644 --- a/pkg/volume/plugins.go +++ b/pkg/volume/plugins.go @@ -746,11 +746,10 @@ func (pm *VolumePluginMgr) logDeprecation(plugin string) { // If it is, initialize all probed plugins and replace the cache with them. func (pm *VolumePluginMgr) refreshProbedPlugins() { events, err := pm.prober.Probe() - if err != nil { - klog.ErrorS(err, "Error dynamically probing plugins") - return // Use cached plugins upon failure. - } + // because the probe function can return a list of valid plugins + // even when an error is present we still must add the plugins + // or they will be skipped because each event only fires once for _, event := range events { if event.Op == ProbeAddOrUpdate { if err := pm.initProbedPlugin(event.Plugin); err != nil { @@ -767,6 +766,11 @@ func (pm *VolumePluginMgr) refreshProbedPlugins() { "pluginName", event.Plugin.GetPluginName()) } } + + if err != nil { + klog.ErrorS(err, "Error dynamically probing plugins") + return + } } // ListVolumePluginWithLimits returns plugins that have volume limits on nodes From 518351366128e7f13f77013ec9105b71ba061481 Mon Sep 17 00:00:00 2001 From: Scott Nice Date: Thu, 9 Dec 2021 09:18:22 -0500 Subject: [PATCH 2/3] Fix gofmt verify --- pkg/volume/plugins.go | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/pkg/volume/plugins.go b/pkg/volume/plugins.go index 43783a7e86f..3626dc6c905 100644 --- a/pkg/volume/plugins.go +++ b/pkg/volume/plugins.go @@ -747,6 +747,10 @@ func (pm *VolumePluginMgr) logDeprecation(plugin string) { func (pm *VolumePluginMgr) refreshProbedPlugins() { events, err := pm.prober.Probe() + if err != nil { + klog.ErrorS(err, "Error dynamically probing plugins") + } + // because the probe function can return a list of valid plugins // even when an error is present we still must add the plugins // or they will be skipped because each event only fires once @@ -766,11 +770,6 @@ func (pm *VolumePluginMgr) refreshProbedPlugins() { "pluginName", event.Plugin.GetPluginName()) } } - - if err != nil { - klog.ErrorS(err, "Error dynamically probing plugins") - return - } } // ListVolumePluginWithLimits returns plugins that have volume limits on nodes From d9cbe5d3146a1cb52e205533e71e3aafd12ddab3 Mon Sep 17 00:00:00 2001 From: Scott Nice Date: Tue, 14 Dec 2021 10:01:02 -0500 Subject: [PATCH 3/3] Added unit test for flex volume probe and updated DynamicPluginProber Probe() interface description --- pkg/volume/flexvolume/probe_test.go | 43 ++++++++++++++++++++++++----- pkg/volume/plugins.go | 2 +- 2 files changed, 37 insertions(+), 8 deletions(-) diff --git a/pkg/volume/flexvolume/probe_test.go b/pkg/volume/flexvolume/probe_test.go index 8cf7b143cc4..0462a16c322 100644 --- a/pkg/volume/flexvolume/probe_test.go +++ b/pkg/volume/flexvolume/probe_test.go @@ -30,8 +30,9 @@ import ( ) const ( - pluginDir = "/flexvolume" - driverName = "fake-driver" + pluginDir = "/flexvolume" + driverName = "fake-driver" + errorDriverName = "error-driver" ) func assertPathSuffix(t *testing.T, dir1 string, dir2 string) { @@ -185,7 +186,7 @@ func TestEmptyPluginDir(t *testing.T) { pluginDir: pluginDir, watcher: watcher, fs: fs, - factory: fakePluginFactory{error: false}, + factory: fakePluginFactory{}, } prober.Init() @@ -279,7 +280,7 @@ func TestProberError(t *testing.T) { pluginDir: pluginDir, watcher: watcher, fs: fs, - factory: fakePluginFactory{error: true}, + factory: fakePluginFactory{errorDriver: driverName}, } installDriver(driverName, fs) prober.Init() @@ -288,6 +289,34 @@ func TestProberError(t *testing.T) { assert.Error(t, err) } +func TestProberSuccessAndError(t *testing.T) { + + // Arrange + fs := utilfs.NewTempFs() + watcher := newFakeWatcher() + prober := &flexVolumeProber{ + pluginDir: pluginDir, + watcher: watcher, + fs: fs, + factory: fakePluginFactory{errorDriver: errorDriverName}, + } + installDriver(driverName, fs) + prober.Init() + + installDriver(errorDriverName, fs) + driverPath := filepath.Join(pluginDir, errorDriverName) + watcher.TriggerEvent(fsnotify.Create, filepath.Join(driverPath, errorDriverName)) + + // Act + events, err := prober.Probe() + + // Assert + assert.Equal(t, 1, len(events)) + assert.Equal(t, volume.ProbeAddOrUpdate, events[0].Op) + assert.Equal(t, driverName, events[0].PluginName) + assert.Error(t, err) +} + // Installs a mock driver (an empty file) in the mock fs. func installDriver(driverName string, fs utilfs.Filesystem) { driverPath := filepath.Join(pluginDir, driverName) @@ -307,7 +336,7 @@ func initTestEnvironment(t *testing.T) ( pluginDir: pluginDir, watcher: watcher, fs: fs, - factory: fakePluginFactory{error: false}, + factory: fakePluginFactory{}, } driverPath = filepath.Join(pluginDir, driverName) installDriver(driverName, fs) @@ -320,13 +349,13 @@ func initTestEnvironment(t *testing.T) ( // Fake Flexvolume plugin type fakePluginFactory struct { - error bool // Indicates whether an error should be returned. + errorDriver string // the name of the driver in error } var _ PluginFactory = fakePluginFactory{} func (m fakePluginFactory) NewFlexVolumePlugin(_, driverName string, _ exec.Interface) (volume.VolumePlugin, error) { - if m.error { + if driverName == m.errorDriver { return nil, fmt.Errorf("Flexvolume plugin error") } // Dummy Flexvolume plugin. Prober never interacts with the plugin. diff --git a/pkg/volume/plugins.go b/pkg/volume/plugins.go index 3626dc6c905..eedfd7eebcd 100644 --- a/pkg/volume/plugins.go +++ b/pkg/volume/plugins.go @@ -132,7 +132,7 @@ type NodeResizeOptions struct { type DynamicPluginProber interface { Init() error - // If an error occurs, events are undefined. + // aggregates events for successful drivers and errors for failed drivers Probe() (events []ProbeEvent, err error) }