Builders distinguish between failure and exception

It would be great if builders distinguished between a job failure
(invalid args, config, etc) and an exception (our code is broken). To do
this, we need to make our own exceptions and use them.

Change-Id: I31abb6fc2379ccac73b2045673eba453ac4a67a0

nodepool/builder.py

@@ -27,6 +27,7 @@ import shlex
 from stats import statsd
 
 from config import loadConfig
+import exceptions
 
 MINS = 60
 HOURS = 60 * MINS
@@ -65,7 +66,9 @@ class DibImageFile(object):
         my_path = os.path.join(images_dir, self.image_id)
         if with_extension:
             if self.extension is None:
-                raise ValueError('Cannot specify image extension of None')
+                raise exceptions.BuilderError(
+                    'Cannot specify image extension of None'
+                )
             my_path += '.' + self.extension
         return my_path
 
@@ -91,7 +94,7 @@ class NodePoolBuilder(object):
 
         with self._start_lock:
             if self._running:
-                raise RuntimeError('Cannot start, already running.')
+                raise exceptions.BuilderError('Cannot start, already running.')
             self._running = True
 
             self._gearman_worker = self._initializeGearmanWorker(
@@ -188,12 +191,14 @@ class NodePoolBuilder(object):
                 args = json.loads(job.arguments)
                 image_id = args['image-id']
                 if '/' in image_id:
-                    raise RuntimeError('Invalid image-id specified')
+                    raise exceptions.BuilderInvalidCommandError(
+                        'Invalid image-id specified'
+                    )
 
                 image_name = job.name.split(':', 1)[1]
                 try:
                     self._buildImage(image_name, image_id)
-                except RuntimeError:
+                except exceptions.BuilderError:
                     self.log.exception('Error building image')
                     job.sendWorkFail()
                 else:
@@ -204,10 +209,17 @@ class NodePoolBuilder(object):
                 args = json.loads(job.arguments)
                 image_name = args['image-name']
                 image_id = job.name.split(':')[1]
-                external_id = self._uploadImage(image_id,
+                try:
-                                                args['provider'],
+                    external_id = self._uploadImage(image_id,
-                                                image_name)
+                                                    args['provider'],
-                job.sendWorkComplete(json.dumps({'external-id': external_id}))
+                                                    image_name)
+                except exceptions.BuilderError:
+                    self.log.exception('Error uploading image')
+                    job.sendWorkFail()
+                else:
+                    job.sendWorkComplete(
+                        json.dumps({'external-id': external_id})
+                    )
             elif self._canHandleImageidJob(job, 'image-delete'):
                 image_id = job.name.split(':')[1]
                 self._deleteImage(image_id)
@@ -245,12 +257,14 @@ class NodePoolBuilder(object):
                                                  image_id)
         image_files = filter(lambda x: x.extension == image_type, image_files)
         if len(image_files) == 0:
-            self.log.error("Unable to find image file for id %s to upload",
+            raise exceptions.BuilderInvalidCommandError(
-                           image_id)
+                "Unable to find image file for id %s to upload" % image_id
-            return
+            )
         if len(image_files) > 1:
-            self.log.error("Found more than one image for id %s. This should "
+            raise exceptions.BuilderError(
-                           "never happen.", image_id)
+                "Found more than one image for id %s. This should never "
+                "happen.", image_id
+            )
 
         image_file = image_files[0]
         filename = image_file.to_path(self._config.imagesdir,
@@ -267,9 +281,9 @@ class NodePoolBuilder(object):
         try:
             provider_image = provider.images[image_name]
         except KeyError:
-            self.log.error("Could not find matching provider image for %s",
+            raise exceptions.BuilderInvalidCommandError(
-                           image_name)
+                "Could not find matching provider image for %s", image_name
-            return
+            )
         image_meta = provider_image.meta
         external_id = manager.uploadImage(ext_image_name, filename,
                                           image_file.extension, 'bare',
@@ -334,8 +348,9 @@ class NodePoolBuilder(object):
                 stderr=subprocess.STDOUT,
                 env=env)
         except OSError as e:
-            raise Exception("Failed to exec '%s'. Error: '%s'" %
+            raise exceptions.BuilderError(
-                            (cmd, e.strerror))
+                "Failed to exec '%s'. Error: '%s'" % (cmd, e.strerror)
+            )
 
         while True:
             ln = p.stdout.readline()
@@ -346,7 +361,9 @@ class NodePoolBuilder(object):
         p.wait()
         ret = p.returncode
         if ret:
-            raise Exception("DIB failed creating %s" % (filename,))
+            raise exceptions.DibFailedError(
+                "DIB failed creating %s" % filename
+            )
 
     def _getDiskimageByName(self, name):
         for image in self._config.diskimages.values():
@@ -357,8 +374,9 @@ class NodePoolBuilder(object):
     def _buildImage(self, image_name, image_id):
         diskimage = self._getDiskimageByName(image_name)
         if diskimage is None:
-            raise RuntimeError('Could not find matching image in config for '
+            raise exceptions.BuilderInvalidCommandError(
-                               '%s', image_name)
+                'Could not find matching image in config for %s', image_name
+            )
 
         start_time = time.time()
         image_file = DibImageFile(image_id)

nodepool/exceptions.py

@@ -0,0 +1,25 @@
+#!/usr/bin/env python
+#
+# Licensed under the Apache License, Version 2.0 (the "License"); you may
+# not use this file except in compliance with the License. You may obtain
+# a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+# License for the specific language governing permissions and limitations
+# under the License.
+
+
+class BuilderError(RuntimeError):
+    pass
+
+
+class BuilderInvalidCommandError(BuilderError):
+    pass
+
+
+class DibFailedError(BuilderError):
+    pass

nodepool/tests/test_builder.py

@@ -17,7 +17,7 @@ import os
 
 import fixtures
 
-from nodepool import builder, tests
+from nodepool import builder, exceptions, tests
 
 
 class TestNodepoolBuilderDibImage(tests.BaseTestCase):
@@ -82,4 +82,4 @@ class TestNodepoolBuilderDibImage(tests.BaseTestCase):
                          '/imagedir/myid1234')
 
         image = builder.DibImageFile('myid1234')
-        self.assertRaises(ValueError, image.to_path, '/imagedir/')
+        self.assertRaises(exceptions.BuilderError, image.to_path, '/imagedir/')