Merge pull request #44 from varunvarma/fix_plugin_runner_lock

Ensure that lock is actually acquired in plugin runner
yahoo · Apr 8, 2019 · b03099e · b03099e
2 parents d820dc1 + 0a9397a
commit b03099e
Show file tree

Hide file tree

Showing 2 changed files with 45 additions and 17 deletions.
diff --git a/tests/test_runner.py b/tests/test_runner.py
@@ -28,7 +28,7 @@ def _callback_with_exception(*args):
     raise Exception
 
 
-class PanoptesTestPlugin(PanoptesBasePlugin):
+class PanoptesTestPluginNoLock(PanoptesBasePlugin):
     name = None
     signature = None
     data = {}
@@ -40,7 +40,7 @@ def run(self, context):
         pass
 
 
-class PanoptesTestPluginRaiseException:
+class PanoptesTestPluginRaisePluginReleaseException:
     name = None
     version = None
     last_executed = None
@@ -54,7 +54,7 @@ class PanoptesTestPluginRaiseException:
     data = {}
 
     execute_now = True
-    lock = "dummy"
+    lock = MagicMock(locked=True, release=MagicMock(side_effect=Exception))
 
     def run(self, context):
         raise Exception
@@ -82,6 +82,15 @@ class MockPluginLockNone:
     lock = None
 
 
+class MockPluginLockIsNotLocked:
+    name = None
+    signature = None
+    data = {}
+
+    execute_now = True
+    lock = MagicMock(locked=False)
+
+
 class TestPanoptesPluginRunner(unittest.TestCase):
     @staticmethod
     def extract(record):
@@ -132,10 +141,10 @@ def test_logging_methods(self):
                                     PanoptesMetricsGroupSet, _callback)
 
         #  Ensure logging methods run:
-        runner.info(PanoptesTestPlugin(), "Test Info log message")
-        runner.warn(PanoptesTestPlugin(), "Test Warning log message")
-        runner.error(PanoptesTestPlugin(), "Test Error log message", Exception)
-        runner.exception(PanoptesTestPlugin(), "Test Exception log message")
+        runner.info(PanoptesTestPluginNoLock(), "Test Info log message")
+        runner.warn(PanoptesTestPluginNoLock(), "Test Warning log message")
+        runner.error(PanoptesTestPluginNoLock(), "Test Error log message", Exception)
+        runner.exception(PanoptesTestPluginNoLock(), "Test Exception log message")
 
         self._log_capture.check(('panoptes.tests.test_runner', 'INFO', '[None] [{}] Test Info log message'),
                                 ('panoptes.tests.test_runner', 'WARNING', '[None] [{}] Test Warning log message'),
@@ -278,6 +287,21 @@ def test_callback_failure(self):
                                          '[Test Polling Plugin] '
                                          '[None] Results callback function failed'))
 
+    def test_lock_no_lock_object(self):
+        mock_plugin = MagicMock(return_value=PanoptesTestPluginNoLock)
+        mock_get_context = MagicMock(return_value=self._panoptes_context)
+        with patch('yahoo_panoptes.framework.plugins.runner.PanoptesPluginManager.getPluginByName',
+                   mock_plugin):
+            with patch('yahoo_panoptes.framework.plugins.runner.PanoptesPluginRunner._get_context', mock_get_context):
+                runner = self._runner_class("Test Polling Plugin", "polling", PanoptesPollingPlugin, PanoptesPluginInfo,
+                                            None, self._panoptes_context, PanoptesTestKeyValueStore,
+                                            PanoptesTestKeyValueStore, PanoptesTestKeyValueStore, "plugin_logger",
+                                            PanoptesMetricsGroupSet, _callback)
+                runner.execute_plugin()
+
+                self._log_capture.check_present(('panoptes.tests.test_runner', 'ERROR',
+                                                 '[None] [{}] Error in acquiring lock'))
+
     def test_lock_is_none(self):
         mock_get_plugin_by_name = MagicMock(return_value=MockPluginLockNone())
         mock_get_context = MagicMock(return_value=self._panoptes_context)
@@ -294,23 +318,24 @@ def test_lock_is_none(self):
                                                  '[None] [{}] Attempting to get lock for plugin'
                                                  ' "Test Polling Plugin"'))
 
-    def test_lock_error(self):
-        mock_plugin = MagicMock(return_value=PanoptesTestPlugin)
+    def test_lock_is_not_locked(self):
+        mock_get_plugin_by_name = MagicMock(return_value=MockPluginLockIsNotLocked())
         mock_get_context = MagicMock(return_value=self._panoptes_context)
         with patch('yahoo_panoptes.framework.plugins.runner.PanoptesPluginManager.getPluginByName',
-                   mock_plugin):
+                   mock_get_plugin_by_name):
             with patch('yahoo_panoptes.framework.plugins.runner.PanoptesPluginRunner._get_context', mock_get_context):
-                runner = self._runner_class("Test Polling Plugin", "polling", PanoptesPollingPlugin, PanoptesPluginInfo,
-                                            None, self._panoptes_context, PanoptesTestKeyValueStore,
+                runner = self._runner_class("Test Polling Plugin", "polling", PanoptesPollingPlugin,
+                                            PanoptesPluginInfo, None, self._panoptes_context, PanoptesTestKeyValueStore,
                                             PanoptesTestKeyValueStore, PanoptesTestKeyValueStore, "plugin_logger",
                                             PanoptesMetricsGroupSet, _callback)
                 runner.execute_plugin()
 
-                self._log_capture.check_present(('panoptes.tests.test_runner', 'ERROR',
-                                                 '[None] [{}] Error in acquiring lock'))
+                self._log_capture.check_present(('panoptes.tests.test_runner', 'INFO',
+                                                 '[None] [{}] Attempting to get lock for plugin'
+                                                 ' "Test Polling Plugin"'))
 
     def test_plugin_failure(self):
-        mock_plugin = MagicMock(return_value=PanoptesTestPluginRaiseException)
+        mock_plugin = MagicMock(return_value=PanoptesTestPluginRaisePluginReleaseException)
         mock_get_context = MagicMock(return_value=self._panoptes_context)
         with patch('yahoo_panoptes.framework.plugins.runner.PanoptesPluginManager.getPluginByName',
                    mock_plugin):
@@ -456,10 +481,13 @@ def test_callback_failure(self):
 
     # 'pass' is needed for these methods because the only difference in their logging output from
     # TestPanoptesPluginRunner is the presence of the PanoptesResource in some log messages.
+    def test_lock_no_lock_object(self):
+        pass
+
     def test_lock_is_none(self):
         pass
 
-    def test_lock_error(self):
+    def test_lock_is_not_locked(self):
         pass
 
     def test_plugin_failure(self):

diff --git a/yahoo_panoptes/framework/plugins/runner.py b/yahoo_panoptes/framework/plugins/runner.py
@@ -153,7 +153,7 @@ def execute_plugin(self):
             self.exception(plugin, 'Error in acquiring lock')
             return
 
-        if lock is None:
+        if lock is None or not lock.locked:
             return
 
         self.info(plugin, 'Acquired lock')