Check exit codes when spawning processes by default

Also pass --fail to curl so that it sets exit code when download fails

12 files changed, 132 insertions, 127 deletions

diff --git a/bin/nova-import-canonical-imagestore b/bin/nova-import-canonical-imagestore
index e6931d9db..2bc61cf0c 100755
--- a/bin/nova-import-canonical-imagestore
+++ b/bin/nova-import-canonical-imagestore
@@ -56,21 +56,21 @@ def download(img):
     for f in img['files']:
         if f['kind'] == 'kernel':
             dest = os.path.join(tempdir, 'kernel')
-            subprocess.call(['curl', f['url'], '-o', dest])
+            subprocess.call(['curl', '--fail', f['url'], '-o', dest])
             kernel_id = image.Image.add(dest,
                 description='kernel/' + img['title'], kernel=True)
 
     for f in img['files']:
         if f['kind'] == 'ramdisk':
             dest = os.path.join(tempdir, 'ramdisk')
-            subprocess.call(['curl', f['url'], '-o', dest])
+            subprocess.call(['curl', '--fail', f['url'], '-o', dest])
             ramdisk_id = image.Image.add(dest,
                 description='ramdisk/' + img['title'], ramdisk=True)
 
     for f in img['files']:
         if f['kind'] == 'image':
             dest = os.path.join(tempdir, 'image')
-            subprocess.call(['curl', f['url'], '-o', dest])
+            subprocess.call(['curl', '--fail', f['url'], '-o', dest])
             ramdisk_id = image.Image.add(dest,
                 description=img['title'], kernel=kernel_id, ramdisk=ramdisk_id)
 
diff --git a/bin/nova-manage b/bin/nova-manage
index 33141a49e..145294d3d 100755
--- a/bin/nova-manage
+++ b/bin/nova-manage
@@ -56,7 +56,8 @@ class VpnCommands(object):
             vpn = self._vpn_for(project.id)
             if vpn:
                 command = "ping -c1 -w1 %s > /dev/null; echo $?"
-                out, _err = utils.execute(command % vpn['private_dns_name'])
+                out, _err = utils.execute(  command % vpn['private_dns_name'],
+                                            check_exit_code=False)
                 if out.strip() == '0':
                     net = 'up'
                 else:
diff --git a/nova/cloudpipe/bootscript.sh b/nova/cloudpipe/bootscript.sh
index 82ec2012a..30d9ad102 100755
--- a/nova/cloudpipe/bootscript.sh
+++ b/nova/cloudpipe/bootscript.sh
@@ -44,8 +44,8 @@ CSRTEXT=$(python -c "import urllib; print urllib.quote('''$CSRTEXT''')")
 
 # SIGN the csr and save as server.crt
 # CURL fetch to the supervisor, POSTing the CSR text, saving the result as the CRT file
-curl $SUPERVISOR -d "cert=$CSRTEXT" > /etc/openvpn/server.crt
-curl $SUPERVISOR/getca/ > /etc/openvpn/ca.crt
+curl --fail $SUPERVISOR -d "cert=$CSRTEXT" > /etc/openvpn/server.crt
+curl --fail $SUPERVISOR/getca/ > /etc/openvpn/ca.crt
 
 # Customize the server.conf.template
 cd /etc/openvpn
diff --git a/nova/objectstore/image.py b/nova/objectstore/image.py
index fb780a0ec..f3c02a425 100644
--- a/nova/objectstore/image.py
+++ b/nova/objectstore/image.py
@@ -232,13 +232,22 @@ class Image(object):
 
     @staticmethod
     def decrypt_image(encrypted_filename, encrypted_key, encrypted_iv, cloud_private_key, decrypted_filename):
-        key, err = utils.execute('openssl rsautl -decrypt -inkey %s' % cloud_private_key, encrypted_key)
+        key, err = utils.execute(
+                'openssl rsautl -decrypt -inkey %s' % cloud_private_key, 
+                process_input=encrypted_key,
+                check_exit_code=False)
         if err:
             raise exception.Error("Failed to decrypt private key: %s" % err)
-        iv, err = utils.execute('openssl rsautl -decrypt -inkey %s' % cloud_private_key, encrypted_iv)
+        iv, err = utils.execute(
+                'openssl rsautl -decrypt -inkey %s' % cloud_private_key, 
+                process_input=encrypted_iv,
+                check_exit_code=False)
         if err:
             raise exception.Error("Failed to decrypt initialization vector: %s" % err)
-        out, err = utils.execute('openssl enc -d -aes-128-cbc -in %s -K %s -iv %s -out %s' % (encrypted_filename, key, iv, decrypted_filename))
+        _out, err = utils.execute(
+                'openssl enc -d -aes-128-cbc -in %s -K %s -iv %s -out %s'
+                 % (encrypted_filename, key, iv, decrypted_filename),
+                 check_exit_code=False)
         if err:
             raise exception.Error("Failed to decrypt image file %s : %s" % (encrypted_filename, err))
 
diff --git a/nova/process.py b/nova/process.py
index 86f29e2c4..425d9f162 100644
--- a/nova/process.py
+++ b/nova/process.py
@@ -2,6 +2,7 @@
 
 # Copyright 2010 United States Government as represented by the
 # Administrator of the National Aeronautics and Space Administration.
+# Copyright 2010 FathomDB Inc.
 # All Rights Reserved.
 #
 #    Licensed under the Apache License, Version 2.0 (the "License"); you may
@@ -20,17 +21,12 @@
 Process pool, still buggy right now.
 """
 
-import logging
-import multiprocessing
 import StringIO
 
 from twisted.internet import defer
 from twisted.internet import error
-from twisted.internet import process
 from twisted.internet import protocol
 from twisted.internet import reactor
-from twisted.internet import threads
-from twisted.python import failure
 
 from nova import flags
 
@@ -55,111 +51,100 @@ class UnexpectedErrorOutput(IOError):
         IOError.__init__(self, "got stdout: %r\nstderr: %r" % (stdout, stderr))
 
 
-# NOTE(termie): this too
-class _BackRelay(protocol.ProcessProtocol):
+# This is based on _BackRelay from twister.internal.utils, but modified to 
+#  capture both stdout and stderr, without odd stderr handling, and also to 
+#  handle stdin
+class BackRelayWithInput(protocol.ProcessProtocol):
     """
     Trivial protocol for communicating with a process and turning its output
     into the result of a L{Deferred}.
 
     @ivar deferred: A L{Deferred} which will be called back with all of stdout
-        and, if C{errortoo} is true, all of stderr as well (mixed together in
-        one string).  If C{errortoo} is false and any bytes are received over
-        stderr, this will fire with an L{_UnexpectedErrorOutput} instance and
-        the attribute will be set to C{None}.
-
-    @ivar onProcessEnded: If C{errortoo} is false and bytes are received over
-        stderr, this attribute will refer to a L{Deferred} which will be called
-        back when the process ends.  This C{Deferred} is also associated with
-        the L{_UnexpectedErrorOutput} which C{deferred} fires with earlier in
-        this case so that users can determine when the process has actually
-        ended, in addition to knowing when bytes have been received via stderr.
+        and all of stderr as well (as a tuple).  C{terminate_on_stderr} is true
+        and any bytes are received over stderr, this will fire with an
+        L{_UnexpectedErrorOutput} instance and the attribute will be set to 
+        C{None}.
+
+    @ivar onProcessEnded: If C{terminate_on_stderr} is false and bytes are 
+        received over stderr, this attribute will refer to a L{Deferred} which 
+        will be called back when the process ends.  This C{Deferred} is also 
+        associated with the L{_UnexpectedErrorOutput} which C{deferred} fires 
+        with earlier in this case so that users can determine when the process 
+        has actually ended, in addition to knowing when bytes have been received
+        via stderr.
     """
 
-    def __init__(self, deferred, errortoo=0):
+    def __init__(self, deferred, started_deferred=None, 
+                    terminate_on_stderr=False, check_exit_code=True, 
+                    process_input=None):
         self.deferred = deferred
-        self.s = StringIO.StringIO()
-        if errortoo:
-            self.errReceived = self.errReceivedIsGood
-        else:
-            self.errReceived = self.errReceivedIsBad
-
-    def errReceivedIsBad(self, text):
-        if self.deferred is not None:
-            self.onProcessEnded = defer.Deferred()
-            err = UnexpectedErrorOutput(text, self.onProcessEnded)
-            self.deferred.errback(failure.Failure(err))
+        self.stdout = StringIO.StringIO()
+        self.stderr = StringIO.StringIO()
+        self.started_deferred = started_deferred
+        self.terminate_on_stderr = terminate_on_stderr
+        self.check_exit_code = check_exit_code
+        self.process_input = process_input
+        self.on_process_ended = None
+    
+    def errReceived(self, text):
+        self.stderr.write(text)
+        if self.terminate_on_stderr and (self.deferred is not None):
+            self.on_process_ended = defer.Deferred()
+            self.deferred.errback(UnexpectedErrorOutput(
+                                        stdout=self.stdout.getvalue(),
+                                        stderr=self.stderr.getvalue()))
             self.deferred = None
             self.transport.loseConnection()
 
-    def errReceivedIsGood(self, text):
-        self.s.write(text)
-
     def outReceived(self, text):
-        self.s.write(text)
+        self.stdout.write(text)
 
     def processEnded(self, reason):
         if self.deferred is not None:
-            self.deferred.callback(self.s.getvalue())
-        elif self.onProcessEnded is not None:
-            self.onProcessEnded.errback(reason)
-
-
-class BackRelayWithInput(_BackRelay):
-    def __init__(self, deferred, startedDeferred=None, error_ok=0,
-                 input=None):
-        # Twisted doesn't use new-style classes in most places :(
-        _BackRelay.__init__(self, deferred, errortoo=error_ok)
-        self.error_ok = error_ok
-        self.input = input
-        self.stderr = StringIO.StringIO()
-        self.startedDeferred = startedDeferred
-
-    def errReceivedIsBad(self, text):
-        self.stderr.write(text)
-        self.transport.loseConnection()
-
-    def errReceivedIsGood(self, text):
-        self.stderr.write(text)
-
-    def connectionMade(self):
-        if self.startedDeferred:
-            self.startedDeferred.callback(self)
-        if self.input:
-            self.transport.write(self.input)
-        self.transport.closeStdin()
-
-    def processEnded(self, reason):
-        if self.deferred is not None:
-            stdout, stderr = self.s.getvalue(), self.stderr.getvalue()
+            stdout, stderr = self.stdout.getvalue(), self.stderr.getvalue()
             try:
-                # NOTE(termie): current behavior means if error_ok is True
-                #               we won't throw an error even if the process
-                #               exited with a non-0 status, so you can't be
-                #               okay with stderr output and not with bad exit
-                #               codes.
-                if not self.error_ok:
+                if self.check_exit_code:
                     reason.trap(error.ProcessDone)
                 self.deferred.callback((stdout, stderr))
             except:
+                # NOTE(justinsb): This logic is a little suspicious to me...
+                # If the callback throws an exception, then errback will be 
+                # called also. However, this is what the unit tests test for...
                 self.deferred.errback(UnexpectedErrorOutput(stdout, stderr))
+        elif self.on_process_ended is not None:
+            self.on_process_ended.errback(reason)
+
 
+    def connectionMade(self):
+        if self.started_deferred:
+            self.started_deferred.callback(self)
+        if self.process_input:
+            self.transport.write(self.process_input)
+        self.transport.closeStdin()
 
-def getProcessOutput(executable, args=None, env=None, path=None, reactor=None,
-                     error_ok=0, input=None, startedDeferred=None):
-    if reactor is None:
-        from twisted.internet import reactor
+def get_process_output(executable, args=None, env=None, path=None, 
+                       process_reactor=None, check_exit_code=True, 
+                       process_input=None, started_deferred=None,
+                       terminate_on_stderr=False):
+    if process_reactor is None:
+        process_reactor = reactor
     args = args and args or ()
     env = env and env and {}
-    d = defer.Deferred()
-    p = BackRelayWithInput(
-            d, startedDeferred=startedDeferred, error_ok=error_ok, input=input)
+    deferred = defer.Deferred()
+    process_handler = BackRelayWithInput(
+            deferred, 
+            started_deferred=started_deferred, 
+            check_exit_code=check_exit_code, 
+            process_input=process_input,
+            terminate_on_stderr=terminate_on_stderr)
     # NOTE(vish): commands come in as unicode, but self.executes needs
     #             strings or process.spawn raises a deprecation warning
     executable = str(executable)
     if not args is None:
         args = [str(x) for x in args]
-    reactor.spawnProcess(p, executable, (executable,)+tuple(args), env, path)
-    return d
+    process_reactor.spawnProcess(   process_handler, executable, 
+                                    (executable,)+tuple(args), env, path)
+    return deferred
 
 
 class ProcessPool(object):
@@ -185,26 +170,26 @@ class ProcessPool(object):
         return self.execute(executable, args, **kw)
 
     def execute(self, *args, **kw):
-        d = self._pool.acquire()
+        deferred = self._pool.acquire()
 
-        def _associateProcess(proto):
-            d.process = proto.transport
+        def _associate_process(proto):
+            deferred.process = proto.transport
             return proto.transport
 
         started = defer.Deferred()
-        started.addCallback(_associateProcess)
-        kw.setdefault('startedDeferred', started)
+        started.addCallback(_associate_process)
+        kw.setdefault('started_deferred', started)
 
-        d.process = None
-        d.started = started
+        deferred.process = None
+        deferred.started = started
 
-        d.addCallback(lambda _: getProcessOutput(*args, **kw))
-        d.addBoth(self._release)
-        return d
+        deferred.addCallback(lambda _: get_process_output(*args, **kw))
+        deferred.addBoth(self._release)
+        return deferred
 
-    def _release(self, rv=None):
+    def _release(self, retval=None):
         self._pool.release()
-        return rv
+        return retval
 
 
 class SharedPool(object):
diff --git a/nova/tests/network_unittest.py b/nova/tests/network_unittest.py
index 993bfacc2..34b68f1ed 100644
--- a/nova/tests/network_unittest.py
+++ b/nova/tests/network_unittest.py
@@ -166,7 +166,6 @@ class NetworkTestCase(test.TrialTestCase):
             release_ip(mac3, address3, hostname, net.bridge_name)
         net = model.get_project_network(self.projects[0].id, "default")
         self.service.deallocate_fixed_ip(firstaddress)
-        release_ip(mac, firstaddress, hostname, net.bridge_name)
 
     def test_vpn_ip_and_port_looks_valid(self):
         """Ensure the vpn ip and port are reasonable"""
diff --git a/nova/tests/process_unittest.py b/nova/tests/process_unittest.py
index 75187e1fc..25c60c616 100644
--- a/nova/tests/process_unittest.py
+++ b/nova/tests/process_unittest.py
@@ -48,7 +48,7 @@ class ProcessTestCase(test.TrialTestCase):
 
     def test_execute_stderr(self):
         pool = process.ProcessPool(2)
-        d = pool.simple_execute('cat BAD_FILE', error_ok=1)
+        d = pool.simple_execute('cat BAD_FILE', check_exit_code=False)
         def _check(rv):
             self.assertEqual(rv[0], '')
             self.assert_('No such file' in rv[1])
diff --git a/nova/utils.py b/nova/utils.py
index e826f9b71..dc3c626ec 100644
--- a/nova/utils.py
+++ b/nova/utils.py
@@ -56,23 +56,25 @@ def fetchfile(url, target):
 #    c.perform()
 #    c.close()
 #    fp.close()
-    execute("curl %s -o %s" % (url, target))
+    execute("curl --fail %s -o %s" % (url, target))
 
-
-def execute(cmd, input=None, addl_env=None):
+def execute(cmd, process_input=None, addl_env=None, check_exit_code=True):
     env = os.environ.copy()
     if addl_env:
         env.update(addl_env)
     obj = subprocess.Popen(cmd, shell=True, stdin=subprocess.PIPE,
         stdout=subprocess.PIPE, stderr=subprocess.PIPE, env=env)
     result = None
-    if input != None:
-        result = obj.communicate(input)
+    if process_input != None:
+        result = obj.communicate(process_input)
     else:
         result = obj.communicate()
     obj.stdin.close()
     if obj.returncode:
         logging.debug("Result was %s" % (obj.returncode))
+        if check_exit_code and obj.returncode <> 0:
+            raise Exception(    "Unexpected exit code: %s.  result=%s" 
+                                % (obj.returncode, result))
     return result
 
 
@@ -98,9 +100,13 @@ def debug(arg):
     return arg
 
 
-def runthis(prompt, cmd):
+def runthis(prompt, cmd, check_exit_code = True):
     logging.debug("Running %s" % (cmd))
-    logging.debug(prompt % (subprocess.call(cmd.split(" "))))
+    exit_code = subprocess.call(cmd.split(" "))
+    logging.debug(prompt % (exit_code))
+    if check_exit_code and exit_code <> 0:
+        raise Exception(    "Unexpected exit code: %s from cmd: %s" 
+                            % (exit_code, cmd))
 
 
 def generate_uid(topic, size=8):
diff --git a/nova/virt/images.py b/nova/virt/images.py
index a3ca72bdd..a60bcc4c1 100644
--- a/nova/virt/images.py
+++ b/nova/virt/images.py
@@ -60,7 +60,7 @@ def _fetch_s3_image(image, path, user, project):
                                                                      url_path)
     headers['Authorization'] = 'AWS %s:%s' % (access, signature)
 
-    cmd = ['/usr/bin/curl', '--silent', url]
+    cmd = ['/usr/bin/curl', '--fail', '--silent', url]
     for (k,v) in headers.iteritems():
         cmd += ['-H', '%s: %s' % (k,v)]
 
diff --git a/nova/virt/libvirt_conn.py b/nova/virt/libvirt_conn.py
index 2192df126..524646ee5 100644
--- a/nova/virt/libvirt_conn.py
+++ b/nova/virt/libvirt_conn.py
@@ -227,10 +227,10 @@ class LibvirtConnection(object):
         if not os.path.exists(basepath('ramdisk')):
            yield images.fetch(data['ramdisk_id'], basepath('ramdisk'), user, project)
 
-        execute = lambda cmd, input=None: \
+        execute = lambda cmd, process_input=None: \
                   process.simple_execute(cmd=cmd,
-                                         input=input,
-                                         error_ok=1)
+                                         process_input=process_input,
+                                         check_exit_code=True)
 
         key = data['key_data']
         net = None
diff --git a/nova/volume/service.py b/nova/volume/service.py
index 104bafe90..be62f621d 100644
--- a/nova/volume/service.py
+++ b/nova/volume/service.py
@@ -131,8 +131,10 @@ class VolumeService(service.Service):
         if FLAGS.fake_storage:
             return
         # NOTE(vish): these commands sometimes sends output to stderr for warnings
-        yield process.simple_execute("sudo vblade-persist auto all", error_ok=1)
-        yield process.simple_execute("sudo vblade-persist start all", error_ok=1)
+        yield process.simple_execute(   "sudo vblade-persist auto all", 
+                                        terminate_on_stderr=False)
+        yield process.simple_execute(   "sudo vblade-persist start all",
+                                        terminate_on_stderr=False)
 
     @defer.inlineCallbacks
     def _init_volume_group(self):
@@ -247,13 +249,14 @@ class Volume(datastore.BasicModel):
                 "sudo lvcreate -L %s -n %s %s" % (sizestr,
                                                   self['volume_id'],
                                                   FLAGS.volume_group),
-                error_ok=1)
+                terminate_on_stderr=False)
 
     @defer.inlineCallbacks
     def _delete_lv(self):
         yield process.simple_execute(
                 "sudo lvremove -f %s/%s" % (FLAGS.volume_group,
-                                            self['volume_id']), error_ok=1)
+                                            self['volume_id']), 
+                terminate_on_stderr=False)
 
     @property
     def __devices_key(self):
@@ -281,7 +284,8 @@ class Volume(datastore.BasicModel):
                  self['blade_id'],
                  FLAGS.aoe_eth_dev,
                  FLAGS.volume_group,
-                 self['volume_id']), error_ok=1)
+                 self['volume_id']), 
+                 terminate_on_stderr=False)
 
     @defer.inlineCallbacks
     def _remove_export(self):
@@ -294,10 +298,12 @@ class Volume(datastore.BasicModel):
     def _exec_remove_export(self):
         yield process.simple_execute(
                 "sudo vblade-persist stop %s %s" % (self['shelf_id'],
-                                                    self['blade_id']), error_ok=1)
+                                                    self['blade_id']), 
+                terminate_on_stderr=False)
         yield process.simple_execute(
                 "sudo vblade-persist destroy %s %s" % (self['shelf_id'],
-                                                       self['blade_id']), error_ok=1)
+                                                       self['blade_id']), 
+                terminate_on_stderr=False)
 
 
 class FakeVolume(Volume):
diff --git a/tools/install_venv.py b/tools/install_venv.py
index 4e775eb33..1f0fa3cc7 100644
--- a/tools/install_venv.py
+++ b/tools/install_venv.py
@@ -37,7 +37,7 @@ def die(message, *args):
   sys.exit(1)
 
 
-def run_command(cmd, redirect_output=True, error_ok=False):
+def run_command(cmd, redirect_output=True, check_exit_code=True):
   """
   Runs a command in an out-of-process shell, returning the
   output of that command.  Working directory is ROOT.
@@ -49,19 +49,18 @@ def run_command(cmd, redirect_output=True, error_ok=False):
 
   proc = subprocess.Popen(cmd, cwd=ROOT, stdout=stdout)
   output = proc.communicate()[0]
-  if not error_ok and proc.returncode != 0:
+  if check_exit_code and proc.returncode != 0:
     die('Command "%s" failed.\n%s', ' '.join(cmd), output)
   return output
 
 
-HAS_EASY_INSTALL = bool(run_command(['which', 'easy_install']).strip())
-HAS_VIRTUALENV = bool(run_command(['which', 'virtualenv']).strip())
+HAS_EASY_INSTALL = bool(run_command(['which', 'easy_install'], check_exit_code=False).strip())
+HAS_VIRTUALENV = bool(run_command(['which', 'virtualenv'], check_exit_code=False).strip())
 
 
 def check_dependencies():
   """Make sure virtualenv is in the path."""
 
-  print 'Checking for virtualenv...',
   if not HAS_VIRTUALENV:
     print 'not found.'
     # Try installing it via easy_install...