From 44b64c317c1e3f9ac67bfbd1c85d25d8abbdd677 Mon Sep 17 00:00:00 2001
From: Erik Olof Gunnar Andersson <eandersson@blizzard.com>
Date: Sat, 28 Jul 2018 20:37:09 -0700
Subject: [PATCH] Change Bad Action from a traceback into a warning

* Added new exception BadAction.
* Cleaned up processing code.

Partial-Bug: #1768618
Change-Id: I1255b87209a94ab25519aaa83648b7abfd49f067
---
 designate/exceptions.py                       |  4 +++
 .../tests/test_workers/test_processing.py     | 27 ++++++++++++++-----
 .../tests/test_workers/test_zone_tasks.py     |  3 ++-
 designate/worker/processing.py                | 19 ++++++++-----
 designate/worker/tasks/zone.py                |  2 +-
 5 files changed, 40 insertions(+), 15 deletions(-)

diff --git a/designate/exceptions.py b/designate/exceptions.py
index 7462c1416..fdcd75b5f 100644
--- a/designate/exceptions.py
+++ b/designate/exceptions.py
@@ -127,6 +127,10 @@ class InvalidObject(Base):
     expected = True
 
 
+class BadAction(Base):
+    error_type = 'bad_action'
+
+
 class BadRequest(Base):
     error_code = 400
     error_type = 'bad_request'
diff --git a/designate/tests/test_workers/test_processing.py b/designate/tests/test_workers/test_processing.py
index 1918bf9fe..4f55cf4c8 100644
--- a/designate/tests/test_workers/test_processing.py
+++ b/designate/tests/test_workers/test_processing.py
@@ -13,12 +13,17 @@
 # WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
 # License for the specific language governing permissions and limitations
 # under the License.mport threading
-from unittest import TestCase
-
+from designate import exceptions
+from designate.tests import TestCase
+from designate.tests import fixtures
 from designate.worker import processing
 
 
 class TestProcessingExecutor(TestCase):
+    def setUp(self):
+        super(TestProcessingExecutor, self).setUp()
+        self.stdlog = fixtures.StandardLogging()
+        self.useFixture(self.stdlog)
 
     def test_execute_multiple_tasks(self):
         def t1():
@@ -31,16 +36,24 @@ class TestProcessingExecutor(TestCase):
         exe = processing.Executor()
 
         results = exe.run(tasks)
-        assert results == [1, 2, 1, 2, 1]
+        self.assertEqual(results, [1, 2, 1, 2, 1])
 
     def test_execute_single_task(self):
         def t1():
             return 1
 
-        def t2():
-            return 2
-
         exe = processing.Executor()
 
         results = exe.run(t1)
-        assert results == [1]
+        self.assertEqual(results[0], 1)
+
+    def test_execute_bad_task(self):
+        def failed_task():
+            raise exceptions.BadAction('Not Great')
+
+        exe = processing.Executor()
+
+        results = exe.run(failed_task)
+        self.assertEqual(results[0], None)
+
+        self.assertIn('Not Great', self.stdlog.logger.output)
diff --git a/designate/tests/test_workers/test_zone_tasks.py b/designate/tests/test_workers/test_zone_tasks.py
index 83eaa7d72..6116e3023 100644
--- a/designate/tests/test_workers/test_zone_tasks.py
+++ b/designate/tests/test_workers/test_zone_tasks.py
@@ -98,7 +98,8 @@ class TestZoneActor(TestCase):
         )
 
     def test_invalid_action(self):
-        with testtools.ExpectedException(Exception, "Bad Action"):
+        with testtools.ExpectedException(exceptions.BadAction,
+                                         'Unexpected action: BAD'):
             self.actor._validate_action('BAD')
 
     def test_threshold_from_config(self):
diff --git a/designate/worker/processing.py b/designate/worker/processing.py
index 1bef590df..cf43383a8 100644
--- a/designate/worker/processing.py
+++ b/designate/worker/processing.py
@@ -19,6 +19,8 @@ from concurrent import futures
 from oslo_log import log as logging
 from oslo_config import cfg
 
+from designate import exceptions
+
 LOG = logging.getLogger(__name__)
 CONF = cfg.CONF
 
@@ -39,14 +41,19 @@ class Executor(object):
     executor that can map multiple tasks across a configurable number of
     threads
     """
+
     def __init__(self, executor=None):
         self._executor = executor or default_executor()
 
     @staticmethod
     def do(task):
-        return task()
+        try:
+            return task()
+        except exceptions.BadAction as e:
+            LOG.warning(e)
 
-    def task_name(self, task):
+    @staticmethod
+    def task_name(task):
         if hasattr(task, 'task_name'):
             return str(task.task_name)
         if hasattr(task, 'func_name'):
@@ -63,17 +70,17 @@ class Executor(object):
 
         If a single task is pass
         """
-        self.start_time = time.time()
+        start_time = time.time()
 
         if callable(tasks):
             tasks = [tasks]
         results = [r for r in self._executor.map(self.do, tasks)]
 
-        self.end_time = time.time()
-        self.task_time = self.end_time - self.start_time
+        end_time = time.time()
+        task_time = end_time - start_time
 
         task_names = [self.task_name(t) for t in tasks]
         LOG.debug("Finished Tasks %(tasks)s in %(time)fs",
-                {'tasks': task_names, 'time': self.task_time})
+                  {'tasks': task_names, 'time': task_time})
 
         return results
diff --git a/designate/worker/tasks/zone.py b/designate/worker/tasks/zone.py
index 5b40a8a94..71eb7decc 100644
--- a/designate/worker/tasks/zone.py
+++ b/designate/worker/tasks/zone.py
@@ -162,7 +162,7 @@ class ZoneActor(base.Task, ThresholdMixin):
 
     def _validate_action(self, action):
         if action not in ['CREATE', 'UPDATE', 'DELETE']:
-            raise Exception('Bad Action')
+            raise exceptions.BadAction('Unexpected action: %s' % action)
 
     def _execute(self):
         results = self.executor.run([