cxmanage: Add more informative output

The first here is to make cxmanage indicate, up front, what it is
doing -- mainly, what hosts are being operated on.

Second goal is to make the indicator more meaningful while the
command is in progress. We can easily print the number of nodes,
successes, and errors encountered so far.

6 files changed, 137 insertions, 95 deletions

diff --git a/cxmanage/controller.py b/cxmanage/controller.py
index e099bcb..33d036f 100644
--- a/cxmanage/controller.py
+++ b/cxmanage/controller.py
@@ -239,32 +239,36 @@ class Controller:
 
     def power(self, mode):
         """ Send the given power command to all targets """
-        results, errors = self._run_command("power", mode)
 
-        # Print successful addresses
-        if self.verbosity >= 1 and len(results) > 0:
-            print ("Power %s command executed successfully on the following hosts:"
-                    % mode)
+        if self.verbosity >= 1:
+            print "Sending power %s command to these hosts:" % mode
             for target in self.targets:
-                if target.address in results:
-                    print target.address
+                print target.address
             print
 
+        results, errors = self._run_command("power", mode)
+
+        if self.verbosity >= 1 and len(errors) == 0:
+            print "Command completed successfully.\n"
         self._print_errors(errors)
 
         return len(errors) > 0
 
     def power_status(self):
         """ Retrieve power status from all targets in group """
+
         results, errors = self._run_command("power_status")
 
         # Print results
         if len(results) > 0:
-            print "Chassis power status"
+            print "Power status"
             for target in self.targets:
                 if target.address in results:
-                    print "%s: %s" % (target.address.ljust(16),
-                            results[target.address])
+                    if results[target.address]:
+                        result = "on"
+                    else:
+                        result = "off"
+                    print "%s: %s" % (target.address.ljust(16), result)
             print
 
         # Print errors
@@ -274,17 +278,17 @@ class Controller:
 
     def power_policy(self, mode):
         """ Set the power policy for all targets """
-        results, errors = self._run_command("power_policy", mode)
 
-        # Print successful addresses
-        if self.verbosity >= 1 and len(results) > 0:
-            print ("Power policy set to \"%s\" for the following hosts:"
-                    % mode)
+        if self.verbosity >= 1:
+            print "Setting power policy on these hosts:"
             for target in self.targets:
-                if target.address in results:
-                    print target.address
+                print target.address
             print
 
+        results, errors = self._run_command("power_policy", mode)
+
+        if self.verbosity >= 1 and len(errors) == 0:
+            print "Command completed successfully.\n"
         self._print_errors(errors)
 
         return len(errors) > 0
@@ -309,49 +313,36 @@ class Controller:
 
     def mc_reset(self):
         """ Send an MC reset command to all targets """
-        results, errors = self._run_command("mc_reset")
 
-        # Print successful addresses
-        if self.verbosity >= 1 and len(results) > 0:
-            print "MC reset successfully on the following hosts:"
+        if self.verbosity >= 1:
+            print "Sending MC reset to these hosts:"
             for target in self.targets:
-                if target.address in results:
-                    print target.address
+                print target.address
             print
 
+        results, errors = self._run_command("mc_reset")
+
+        if self.verbosity >= 1 and len(errors) == 0:
+            print "Command completed successfully.\n"
         self._print_errors(errors)
 
         return len(errors) > 0
 
-    def update_firmware(self, slot_arg="INACTIVE", skip_reset=False):
+    def update_firmware(self, slot_arg="INACTIVE"):
         """ Send firmware update commands to all targets in group. """
-        # Start a progress indicator
-        indicator = Indicator("Updating")
-        if self.verbosity == 1:
-            indicator.start()
+
+        if self.verbosity >= 1:
+            print "Updating firmware on these hosts:"
+            for target in self.targets:
+                print target.address
+            print
 
         # Get results and errors
         results, errors = self._run_command("update_firmware",
                 self.work_dir, self.tftp, self.images, slot_arg)
 
-        # Reset MC upon completion
-        if not skip_reset:
-            for image in self.images:
-                if image.type == "SOC_ELF":
-                    self._run_command("mc_reset")
-                    break
-
-        # Signal indicator to stop
-        indicator.stop()
-
-        # Print successful addresses
-        if self.verbosity >= 1 and len(results) > 0:
-            print "Firmware updated successfully on the following hosts:"
-            for target in self.targets:
-                if target.address in results:
-                    print target.address
-            print
-
+        if self.verbosity >= 1 and len(errors) == 0:
+            print "Command completed successfully.\n"
         self._print_errors(errors)
 
         return len(errors) > 0
@@ -395,7 +386,7 @@ class Controller:
                             print "%s: %s" % (address.ljust(16), reading)
 
                 # Print average
-                if len(self.targets) > 1 and average != None:
+                if len(sensors[sensor_name]) > 1 and average != None:
                     average /= len(self.targets)
                     print "%s: %.2f %s" % ("Average".ljust(16),
                             average, suffix)
@@ -438,7 +429,8 @@ class Controller:
                     macaddrs = results[target.address]
                     for i in range(len(macaddrs)):
                         print "Node %i, Port %i: %s" % macaddrs[i]
-                    print
+                    if target != self.targets[-1] or len(errors) > 0:
+                        print
 
         self._print_errors(errors)
 
@@ -446,34 +438,36 @@ class Controller:
 
     def config_reset(self):
         """ Send config reset command to all targets """
-        results, errors = self._run_command("config_reset",
-            self.work_dir, self.tftp)
 
-        # Print successful addresses
-        if self.verbosity >= 1 and len(results) > 0:
-            print "Configuration reset successfully on the following hosts:"
+        if self.verbosity >= 1:
+            print "Resetting configuration on these hosts:"
             for target in self.targets:
-                if target.address in results:
-                    print target.address
+                print target.address
             print
 
+        results, errors = self._run_command("config_reset",
+            self.work_dir, self.tftp)
+
+        if self.verbosity >= 1 and len(errors) == 0:
+            print "Command completed successfully.\n"
         self._print_errors(errors)
 
         return len(errors) > 0
 
     def config_boot(self, boot_args):
         """ Send config boot command to all targets """
-        results, errors = self._run_command("config_boot",
-                self.work_dir, self.tftp, boot_args)
 
-        # Print successful addresses
-        if self.verbosity >= 1 and len(results) > 0:
-            print "Boot order changed successfully on the following hosts:"
+        if self.verbosity >= 1:
+            print "Setting boot order on these hosts:"
             for target in self.targets:
-                if target.address in results:
-                    print target.address
+                print target.address
             print
 
+        results, errors = self._run_command("config_boot",
+                self.work_dir, self.tftp, boot_args)
+
+        if self.verbosity >= 1 and len(errors) == 0:
+            print "Command completed successfully.\n"
         self._print_errors(errors)
 
         return len(errors) > 0
@@ -509,12 +503,17 @@ class Controller:
     def _run_command(self, name, *args):
         """ Run a target command with multiple threads
 
-        Returns a mapping of addresses to a (results, errors) tuple """
+        Returns (results, errors) which map addresses to their results """
 
         threads = [ControllerCommandThread(target, name, args)
                 for target in self.targets]
         running_threads = set()
 
+        # Start indicator
+        indicator = Indicator(len(self.targets))
+        if self.verbosity == 1:
+            indicator.start()
+
         for thread in threads:
             # Wait while we have too many running threads
             while len(running_threads) >= self.max_threads:
@@ -522,31 +521,48 @@ class Controller:
                 for running_thread in running_threads:
                     if not running_thread.is_alive():
                         running_threads.remove(running_thread)
+                        if running_thread.error == None:
+                            indicator.add_success()
+                        else:
+                            indicator.add_error()
                         break
 
-            # Start the thread
+            # Start the new thread
             thread.start()
             running_threads.add(thread)
 
         # Join with any remaining threads
-        for thread in running_threads:
-            thread.join()
+        while len(running_threads) > 0:
+            time.sleep(0.001)
+            for running_thread in running_threads:
+                if not running_thread.is_alive():
+                    running_threads.remove(running_thread)
+                    if running_thread.error == None:
+                        indicator.add_success()
+                    else:
+                        indicator.add_error()
+                    break
 
         # Get results and errors
         results = {}
         errors = {}
         for thread in threads:
-            if thread.error != None:
-                errors[thread.target.address] = thread.error
-            else:
+            if thread.error == None:
                 results[thread.target.address] = thread.result
+            else:
+                errors[thread.target.address] = thread.error
+
+        # Stop indicator
+        if self.verbosity == 1:
+            indicator.stop()
+            print "\n"
 
         return results, errors
 
     def _print_errors(self, errors):
         """ Print errors if they occured """
         if len(errors) > 0:
-            print "The following errors occured"
+            print "Command failed on these hosts"
             for target in self.targets:
                 if target.address in errors:
                     print "%s: %s" % (target.address.ljust(16),
@@ -554,8 +570,12 @@ class Controller:
             print
 
 class ControllerCommandThread(threading.Thread):
+    """ Thread for executing a command on a target """
+
     def __init__(self, target, name, args):
         threading.Thread.__init__(self)
+        self.daemon = True
+
         self.target = target
         self.function = getattr(target, name)
         self.args = args
diff --git a/cxmanage/indicator.py b/cxmanage/indicator.py
index fa5dbd1..63a7046 100644
--- a/cxmanage/indicator.py
+++ b/cxmanage/indicator.py
@@ -37,39 +37,61 @@ class Indicator(threading.Thread):
 
     Updating... """
 
-    def __init__(self, message):
+    def __init__(self, total):
         threading.Thread.__init__(self)
         self.lock = threading.Lock()
-        self.message = message
+        self.daemon = True
         self.running = False
 
+        self.total = total
+        self.successes = 0
+        self.errors = 0
+
     def run(self):
         self.lock.acquire()
 
-        sys.stdout.write(self.message)
+        message = "\r%i successes  |  %i errors  |  %i nodes left  |  %s"
+
+        # Print initial message
+        nodes_left = self.total - self.successes - self.errors
+        dots = ""
+        sys.stdout.write(message % (self.successes,
+                self.errors, nodes_left, dots.ljust(3)))
         sys.stdout.flush()
 
         self.running = True
-        deadline = time.time()
         while self.running:
-            current_time = time.time()
-            if current_time >= deadline:
-                deadline = current_time + 1
-                sys.stdout.write(".")
-                sys.stdout.flush()
-
             self.lock.release()
-            time.sleep(0.1)
+            time.sleep(0.25)
             self.lock.acquire()
 
+            nodes_left = self.total - self.successes - self.errors
+            if len(dots) >= 3:
+                dots = ""
+            else:
+                dots += "."
+
+            sys.stdout.write(message % (self.successes,
+                self.errors, nodes_left, dots.ljust(3)))
+            sys.stdout.flush()
+
         self.lock.release()
 
     def stop(self):
         self.lock.acquire()
+        was_running = self.running
+        self.running = False
+        self.lock.release()
 
-        if self.running:
-            sys.stdout.write("\n")
-            sys.stdout.flush()
-            self.running = False
+        if was_running:
+            self.join()
+
+    def add_success(self):
+        self.lock.acquire()
+        self.successes += 1
+        self.lock.release()
 
+    def add_error(self):
+        self.lock.acquire()
+        self.errors += 1
         self.lock.release()
diff --git a/cxmanage/target.py b/cxmanage/target.py
index b7e2520..06feb6a 100644
--- a/cxmanage/target.py
+++ b/cxmanage/target.py
@@ -171,10 +171,7 @@ class Target:
     def power_status(self):
         """ Return power status reported by IPMI """
         try:
-            if self.bmc.get_chassis_status().power_on:
-                return "on"
-            else:
-                return "off"
+            return self.bmc.get_chassis_status().power_on
         except IpmiError:
             raise CxmanageError("Failed to retrieve power status")
 
@@ -192,13 +189,15 @@ class Target:
         try:
             return self.bmc.sdr_list()
         except IpmiError:
-            raise CxmanageError("Failed to retrieve sensor value")
+            raise CxmanageError("Failed to retrieve sensor list")
 
     def get_firmware_info(self):
         """ Get firmware info from the target """
         try:
             fwinfo = [x for x in self.bmc.get_firmware_info()
                     if hasattr(x, "slot")]
+            if len(fwinfo) == 0:
+                raise CxmanageError("Failed to retrieve firmware info")
 
             # Flag CDB as "in use" based on socman info
             for a in range(1, len(fwinfo)):
diff --git a/cxmanage_test/controller_test.py b/cxmanage_test/controller_test.py
index 8d8ccf5..bb40cdc 100644
--- a/cxmanage_test/controller_test.py
+++ b/cxmanage_test/controller_test.py
@@ -271,7 +271,7 @@ class ControllerTargetTest(unittest.TestCase):
         self.controller.add_image("factory.cdb", "CDB")
 
         # Perform firmware update
-        self.assertFalse(self.controller.update_firmware(skip_reset=True))
+        self.assertFalse(self.controller.update_firmware())
 
         for target in self.controller.targets:
             # Check updated types
@@ -302,7 +302,7 @@ class DummyTarget:
 
     def power_status(self):
         self.executed.append("power_status")
-        return "off"
+        return False
 
     def power_policy(self, mode):
         self.executed.append(("power_policy", mode))
diff --git a/cxmanage_test/target_test.py b/cxmanage_test/target_test.py
index 8ff4c6b..7dc88b8 100644
--- a/cxmanage_test/target_test.py
+++ b/cxmanage_test/target_test.py
@@ -95,7 +95,7 @@ class TargetTest(unittest.TestCase):
             executed = target.bmc.executed
             self.assertEqual(len(executed), 1)
             self.assertEqual(executed[0], "get_chassis_status")
-            self.assertEqual(result, "off")
+            self.assertEqual(result, False)
 
     def test_power_policy(self):
         """ Test power_policy command """
diff --git a/scripts/cxmanage b/scripts/cxmanage
index 18b8a5b..ef17b45 100755
--- a/scripts/cxmanage
+++ b/scripts/cxmanage
@@ -315,7 +315,7 @@ def fwupdate_command(controller, args):
     if not args.all_nodes:
         sys.stdout.write("WARNING: Updating firmware without --all-nodes is dangerous.")
         if args.force_yes:
-            sys.stdout.write("\n")
+            sys.stdout.write("\n\n")
             sys.stdout.flush()
         else:
             sys.stdout.write(" Continue? (y/n) ")
@@ -323,6 +323,7 @@ def fwupdate_command(controller, args):
             while True:
                 command = raw_input().rstrip().lstrip().lower()
                 if command in ["y", "yes"]:
+                    print
                     break
                 elif command in ["n", "no"]:
                     return 1
@@ -338,7 +339,7 @@ def fwupdate_command(controller, args):
             simg, args.version, args.daddr, args.skip_crc32)
 
     # Do firmware update
-    if controller.update_firmware(args.slot, True):
+    if controller.update_firmware(args.slot):
         return 1
     return 0