Merge lp:~javier.collado/utah/bug1101189-2 into lp:utah

[Javier Collado]

This branch is a follow up on the changes from:
lp:~javier.collado/utah/bug1095669

The previous branch updated `_cobble` method to gather cobbler logs. However, a
cobbler command was using `subprocess.check_output` to do that and the logs for
that command weren't captured.

In this branch, `_cobble` and `_runargs` methods signature is updated
to have both the return code and the output (stdout and stderr combined)
so that `_cobble` can be used for the command above.

Note that it's assumed that `cobbler system find` doesn't print anything to
stderr. This is true in my tests, but I'd like someone else to confirm that to
be 100% sure.

[Max Brustkern (nuclearbob)]

If line 21 is changing to use _cobble, you should omit the 'sudo' 'cobbler' or else you'll have that twice. I don't see any stderr from cobbler system find.

While we're doing this, would it make any sense to separate stdout and stderr the way SSHMachine.run currently does? We could also do that later, and I think updating SSHMachine to return a dict, like you did with these methods, is a good idea.

I like the rest of it. I think your method of improving these is better than what we've got in other places right now.

[lp:~javier.collado/utah/bug1101189-2]

809. By Javier Collado on 2013-01-28

Fixed cobbler command

[Javier Collado (javier.collado)]

Thanks for your feedback. I've fixed the command in which 'sudo cobbler' was
duplicated.

With regard to the stdout/stderr separation, yes, I think it would make sense
and that I can take care of that in a separate merge request.

[Max Brustkern (nuclearbob)]

Sounds good. We can come back to that later, I just wanted to make sure it was written down somewhere so we didn't forget, since I think it's a good idea.

review: Approve

=== modified file 'utah/provisioning/baremetal/bamboofeeder.py'
--- utah/provisioning/baremetal/bamboofeeder.py	2012-12-12 18:56:13 +0000
+++ utah/provisioning/baremetal/bamboofeeder.py	2013-01-28 18:34:39 +0000
@@ -297,5 +297,5 @@
         """
         super(BambooFeederMachine, self)._depcheck()
         cmd = ['which', 'mkimage']
-        if self._runargs(cmd) != 0:
+        if self._runargs(cmd)['returncode'] != 0:
             raise UTAHBMProvisioningException('u-boot-tools not installed')

=== modified file 'utah/provisioning/baremetal/cobbler.py'
--- utah/provisioning/baremetal/cobbler.py	2013-01-18 18:29:31 +0000
+++ utah/provisioning/baremetal/cobbler.py	2013-01-28 18:34:39 +0000
@@ -83,8 +83,7 @@
         """
         # TODO: consider reworking _cobble to provide this
         # (only if we'll be using cobbler for a while longer)
-        machines = subprocess.check_output(['sudo', 'cobbler', 'system',
-                                            'find']).splitlines()
+        machines = self._cobble(['system', 'find'])['output'].splitlines()
         if self.name not in machines:
             raise UTAHBMProvisioningException('No machine named ' + self.name
                                               + ' exists in cobbler')
@@ -218,17 +217,19 @@
                               '/var/log/cobbler/cobbler.log'],
                              stdout=subprocess.PIPE)
         try:
-            retcode = self._runargs(['sudo', 'cobbler'] + cmd)
+            command_results = \
+                self._runargs(['sudo', 'cobbler'] + cmd)['returncode']
+            retcode = command_results['returncode']
             if retcode != 0:
                 error_msg = 'Cobbler command failed: ' + ' '.join(cmd)
                 if failok:
                     self.logger.debug(error_msg)
-                    return retcode
+                    return command_results
                 else:
                     self.logger.error(error_msg)
                     raise UTAHBMProvisioningException(error_msg)
             else:
-                return retcode
+                return command_results
         finally:
             # tail process will wait forever because of the -f/--follow option,
             # so it has to be terminated in order to read from stdout.
@@ -243,7 +244,7 @@
         """
         self.logger.debug('Starting system')
         retcode = self._cobble(['system', 'poweron', '--name=' + self.name],
-                               failok=True)
+                               failok=True)['returncode']
         if retcode != 0:
             self.logger.info('Cobbler power command failed; falling back to '
                              'manual command')
@@ -257,7 +258,7 @@
         """
         self.logger.debug('Stopping system')
         retcode = self._cobble(['system', 'poweroff', '--name=' + self.name],
-                               failok=True)
+                               failok=True)['returncode']
         if retcode != 0:
             self.logger.info('Cobbler power command failed; falling back to '
                              'manual command')
@@ -281,7 +282,7 @@
         super(CobblerMachine, self)._depcheck()
         if self.installtype == 'desktop':
             cmd = config.nfscommand + ['status']
-            if self._runargs(cmd) != 0:
+            if self._runargs(cmd)['returncode'] != 0:
                 raise UTAHBMProvisioningException('NFS needed for desktop'
                                                   ' install')
             if not os.path.isfile(config.nfsconfigfile):

=== modified file 'utah/provisioning/provisioning.py'
--- utah/provisioning/provisioning.py	2013-01-22 17:24:12 +0000
+++ utah/provisioning/provisioning.py	2013-01-28 18:34:39 +0000
@@ -307,7 +307,8 @@
                          .format(timeout=timeout))
         time.sleep(timeout)
         self.logger.info('Checking network connectivity (ping)')
-        returncode = self._runargs(['ping', '-c1', '-w5', self.name])
+        returncode = \
+            self._runargs(['ping', '-c1', '-w5', self.name])['returncode']
         if returncode != 0:
             err = 'Ping returned {0}'.format(returncode)
             raise UTAHProvisioningException(err, retry=True)
@@ -542,16 +543,20 @@
         self.logger.debug('Output follows:')
         p = subprocess.Popen(arglist,
                              stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
+        output = []
         while p.poll() is None:
             line = p.stdout.readline().strip()
             self.logger.debug(line)
+            output.append(line)
+        output = '\n'.join(output)
 
         returncode = p.returncode
         log_level = logging.DEBUG if returncode == 0 else logging.WARNING
         log_message = 'Return code: {}'.format(returncode)
         self.logger.log(log_level, log_message)
 
-        return returncode
+        return {'returncode': returncode,
+                'output': output}
 
     def _depcheck(self):
         """

=== modified file 'utah/provisioning/vm/vm.py'
--- utah/provisioning/vm/vm.py	2013-01-22 14:13:17 +0000
+++ utah/provisioning/vm/vm.py	2013-01-28 18:34:39 +0000
@@ -257,7 +257,7 @@
                 cmd = ['qemu-img', 'create', '-f', 'qcow2', diskfile, disksize]
                 self.logger.debug('Creating ' + disksize + ' disk using:')
                 self.logger.debug(' '.join(cmd))
-                if self._runargs(cmd) != 0:
+                if self._runargs(cmd)['returncode'] != 0:
                     raise UTAHVMProvisioningException(
                         'Could not create disk image at ' + diskfile)
             disk = {'bus': self.diskbus,