Report failures instead of tracebacks and usage in the client tools

A stray trailing comma made the update detail a one element tuple, so a
firmware error printed as a python tuple. A missing status printed the whole
response dict. A failure that named no node was dropped entirely, which is how
a service data request that the server refused came out as silence and a
success exit code, and nodestorage, nodelicense and nodesupport exited zero
even when they had reported an error.

nodeconsole crashed decoding an absent screenshot, and again on the terminal
calls behind a pipe, where a log replay crashed too; refuse the terminal only
modes cleanly and dump the log when there is no terminal to replay into.
nodedefine raised a ValueError on an argument without an equals sign, and
firmware for a category the target does not describe printed usage as though
the question had been malformed.

On the server side the readability check was applied to the path a download is
saved to, so asking for service data or saved licences at a path that does not
exist yet failed claiming the destination was not readable.

confluent_client/bin/nodeconsole

@@ -489,6 +489,10 @@ def direct_console():
     global oldfl
     if console_direct_mode:
         return False
+    if not sys.stdin.isatty():
+        # Nothing to put in raw mode, which is fine for output only usage such
+        # as reporting an error from a screenshot request
+        return False
     console_direct_mode = True
     oldtcattr = termios.tcgetattr(sys.stdin.fileno())
     oldfl = fcntl.fcntl(sys.stdin.fileno(), fcntl.F_GETFL)
@@ -907,7 +911,9 @@ async def do_screenshot():
                         if len(imgdata) < 32: # We were subjected to error
                             errorstr = 'Unable to get screenshot'
                     if errorstr or imgdata:
-                        imgdata = base64.b64decode(imgdata)
+                        # A node may report an error with no image at all, and
+                        # there is nothing to decode in that case
+                        imgdata = base64.b64decode(imgdata) if imgdata else None
                         draw_node(node, imgdata, errorstr, firstnodename, cwidth, cheight)
         urlbynode = {}
         for node in vnconly:
@@ -1086,6 +1092,10 @@ def draw_node(node, imgdata, errorstr, firstnodename, cwidth, cheight):
     sys.stdout.flush()
 
 if options.screenshot or options.video:
+    if not sys.stdout.isatty():
+        sys.stderr.write(
+            'Screenshot and video rendering need a terminal to draw in\n')
+        sys.exit(1)
     streaming = options.video
     try:
         cursor_hide()

confluent_client/bin/nodedefine

@@ -49,6 +49,11 @@ async def main():
     exitcode = 0
     attribs = {'name': noderange}
     for arg in args[1:]:
+        if '=' not in arg:
+            sys.stderr.write(
+                'Attributes must be given as attribute=value, got "{0}"\n'.format(
+                    arg))
+            sys.exit(1)
         key, val = arg.split('=', 1)
         attribs[key] = val
     async for r in session.create('/noderange/', attribs):

confluent_client/bin/nodefirmware

@@ -99,7 +99,7 @@ def get_update_progress(session, url):
     for res in session.read(url):
         status = res.get('phase', 'error')
         percent = res.get('progress', None)
-        detail = res.get('detail', repr(res)),
+        detail = res.get('detail', repr(res))
         if status == 'error':
             text = 'error!'
         else:
@@ -143,6 +143,11 @@ def update_firmware(session, filename):
                 pass
     for res in session.create(resource, upargs):
         if 'created' not in res:
+            if not res.get('databynode', None):
+                # A failure that is not attributed to any node still has to be
+                # reported, or the command looks like it quietly did nothing
+                exitcode |= client.printerror(res)
+                continue
             for nodename in res.get('databynode', ()):
                 output.set_output(nodename, 'error!')
                 noderrs[nodename] = res['databynode'][nodename].get(
@@ -196,7 +201,10 @@ def show_firmware(session):
     if not nodes_matched:
         sys.stderr.write('No matching nodes for noderange "{0}"\n'.format(noderange))
     elif not firmware_shown and not exitcode:
-        argparser.print_help()
+        # Asking about firmware the target does not describe is a legitimate
+        # question with an empty answer, not a mistake in how it was asked
+        sys.stderr.write('No firmware reported for "{0}"\n'.format(
+            ','.join(components)))
 
 
 try:
@@ -204,12 +212,11 @@ try:
     if querystatus:
         for res in session.read(
                 '/noderange/{0}/inventory/firmware/updatestatus'.format(noderange)):
+            exitcode |= client.printerror(res)
             for node in res.get('databynode', {}):
                 currstat = res['databynode'][node].get('status', None)
                 if currstat:
                     print('{}: {}'.format(node, currstat))
-                else:
-                    print(repr(res))
     elif upfile is None:
         show_firmware(session)
     else:

confluent_client/bin/nodelicense

@@ -119,6 +119,7 @@ def show_licenses(session):
     for res in session.read(
             '/noderange/{0}/configuration/management_controller/licenses/'
             'all'.format(noderange)):
+        exitcode |= client.printerror(res)
         for node in res.get('databynode', {}):
             for license in res['databynode'][node].get('License', []):
                 msg = '{0}: {1}'.format(node, license.get('feature',

confluent_client/bin/nodestorage

@@ -244,6 +244,8 @@ def main():
             sys.stdout.write('Aborting\n')
             sys.exit(1)
     handler(noderange, options, args[2:])
+    # The handlers record failures in exitcode, so let a caller see them
+    sys.exit(exitcode)
 
 
 if __name__ == '__main__':

confluent_client/bin/nodesupport

@@ -76,6 +76,11 @@ def download_servicedata(noderange, media, options):
     session.stop_if_noderange_over(noderange, options.maxnodes)
     for res in session.create(resource, upargs):
         if 'created' not in res:
+            if not res.get('databynode', None):
+                # A failure that is not attributed to any node still has to be
+                # reported, or the command looks like it quietly did nothing
+                printerror(res)
+                continue
             for nodename in res.get('databynode', ()):
                 output.set_output(nodename, 'error!')
                 noderrs[nodename] = res['databynode'][nodename].get(
@@ -148,5 +153,9 @@ def main():
         argparser.print_help()
         sys.exit(1)
     handler(noderange, media, options)
+    # The handlers record failures in exitcode, so let a caller see them
+    sys.exit(exitcode)
+
+
 if __name__ == '__main__':
     main()

confluent_client/confluent/logreader.py

@@ -175,6 +175,10 @@ class LogReplay(object):
 
 
 def _replay_to_console(txtfile, binfile):
+    if not sys.stdin.isatty():
+        # Interactive replay needs a terminal to put in raw mode and to take
+        # navigation keys from, so without one just write the log out
+        return dump_to_console(txtfile)
     replay = LogReplay(txtfile, binfile)
     oldtcattr = termios.tcgetattr(sys.stdin.fileno())
     tty.setraw(sys.stdin.fileno())

confluent_server/confluent/messages.py

@@ -584,9 +584,9 @@ def get_input_message(path, operation, inputdata, nodes=None, multinode=False,
     elif '/'.join(path).startswith('media/') and inputdata:
         return InputMedia(path, nodes, inputdata, configmanager)
     elif '/'.join(path).startswith('support/servicedata') and inputdata:
-        return InputMedia(path, nodes, inputdata, configmanager)
+        return InputDownloadTarget(path, nodes, inputdata, configmanager)
     elif '/'.join(path).startswith('configuration/management_controller/save_licenses') and inputdata:
-        return InputMedia(path, nodes, inputdata, configmanager)
+        return InputDownloadTarget(path, nodes, inputdata, configmanager)
     elif '/'.join(path).startswith(
             'configuration/management_controller/licenses') and inputdata:
         return InputLicense(path, nodes, inputdata, configmanager)
@@ -631,6 +631,9 @@ def isurl(value):
 
 class InputFirmwareUpdate(ConfluentMessage):
     urlsupported = False
+    # Whether the named file is something confluent will read from the caller,
+    # or somewhere confluent is being asked to write to
+    isdownload = False
     def __init__(self, path, nodes, inputdata, configmanager):
         self._filename = inputdata.get('filename', inputdata.get('url', inputdata.get('dirname', None)))
         self.bank = inputdata.get('bank', None)
@@ -660,7 +663,8 @@ class InputFirmwareUpdate(ConfluentMessage):
                 if value.startswith('/var/log/confluent'):
                     raise Exception(
                         'File transfer with /var/log/confluent is not supported')
-                if curruser and not value.startswith('/var/lib/confluent/client_assets/'):
+                if (curruser and not self.isdownload
+                        and not value.startswith('/var/lib/confluent/client_assets/')):
                     try:
                         pwent = pwd.getpwnam(curruser)
                         if not checkaccess(curruser, value, pwent):
@@ -700,6 +704,12 @@ class InputFirmwareUpdate(ConfluentMessage):
                 'File transfer with /var/log/confluent is not supported')
         return self.filebynode[node]
 
+class InputDownloadTarget(InputFirmwareUpdate):
+    # Where to save something confluent fetches, so the path is a destination
+    # rather than a file that has to exist and be readable already
+    isdownload = True
+
+
 class InputMedia(InputFirmwareUpdate):
     # Use InputFirmwareUpdate
     pass