Merge ~asbalderson/juju-wait:machine-wait into juju-wait:master

[Alexander Balderson]

Start checking if machines are down during deploys

There are cases when machines never come up or enter a bad state
during juju deploys. This checks if those machines are in bad
states, and exits.

This should bubble up errors and failures much faster.

[Jason Hobbs (jason-hobbs)]

review: Needs Fixing

[Jason Hobbs (jason-hobbs)]

lgtm, thanks!

review: Approve

[Stuart Bishop (stub)]

The argument order in the wait function needs to be adjusted, so it is backwards compatible with callsites using positional parameters (of which there are probably none, as the only user of this as a library I'm aware of is using keyword parameters, but might as well avoid the possibility).

It would be better if the code used datetime instances for timestamps rather than floats, to match what is already being done elsewhere in the code. I'm not going to require this to be fixed though.

review: Needs Fixing

[Alexander Balderson (asbalderson)]

> The argument order in the wait function needs to be adjusted, so it is
> backwards compatible with callsites using positional parameters (of which
> there are probably none, as the only user of this as a library I'm aware of is
> using keyword parameters, but might as well avoid the possibility).

Good call on this; I had overlooked that option.

> It would be better if the code used datetime instances for timestamps rather
> than floats, to match what is already being done elsewhere in the code. I'm
> not going to require this to be fixed though.

This is good feedback, The adjustments have been made to help keep things consistent.

[Stuart Bishop (stub)]

Looks good, except for what I think is some debugging code that leaked in on line 90 of the diff.

[Alexander Balderson (asbalderson)]

> Looks good, except for what I think is some debugging code that leaked in on
> line 90 of the diff.

Whoops; good catch!

diff --git a/juju_wait/__init__.py b/juju_wait/__init__.py
index ffd0417..02449ee 100755
--- a/juju_wait/__init__.py
+++ b/juju_wait/__init__.py
@@ -47,6 +47,17 @@ class EnvironmentAction(argparse.Action):
         os.environ['JUJU_MODEL'] = values[0]
 
 
+class MachineState(object):
+    def __init__(self, state, state_time):
+        self.state = state
+        self.time = state_time
+
+    def __eq__(self, other):
+        if self.state == other.state and self.time == other.time:
+            return True
+        return False
+
+
 def parse_ts(ts):
     '''Parse the Juju provided timestamp, which must be UTC.'''
     return datetime.strptime(ts, '%d %b %Y %H:%M:%SZ')
@@ -192,6 +203,11 @@ WORKLOAD_OK_STATES = ['active', 'unknown']
 # values, consider that fatal and raise.
 WORKLOAD_ERROR_STATES = ['error']
 
+# If a machine is in one of these states it may be broken.
+MACHINE_ERROR_STATES = ['down', 'error', 'stopped']
+
+MACHINE_PENDING_STATES = ['pending']
+
 StatusFields = namedtuple('StatusFields', ['timestamp',
                                            'status_type',
                                            'state',
@@ -229,6 +245,14 @@ def wait_cmd(args=sys.argv[1:]):
                         action='append', default=list(), type=str,
                         help='Exclude the application from "active" status'
                         ' checks.  Can be used multiple times.')
+    parser.add_argument('--machine-exclude', dest='machine_exclude', action='append',
+                        default=list(), type=str,
+                        help='exclude these machines from checking for downed states.'
+                        ' Can be used multiple times.')
+    parser.add_argument('--machine-error-timeout', dest='machine_error_timeout', type=int, action='store', default=300,
+                        help='Time for a machine to be in a bad state before exiting (seconds).')
+    parser.add_argument('--machine-pending-timeout', dest='machine_pending_timeout', type=int, action='store', default=3600,
+                        help='Time for a machine to be in a pending state before exiting (seconds).')
     parser.add_argument('--version', default=False, action='store_true')
     args = parser.parse_args(args)
 
@@ -269,7 +293,10 @@ def wait_cmd(args=sys.argv[1:]):
             args.wait_for_workload,
             max_wait,
             args.retry_errors,
-            args.exclude
+            args.exclude,
+            args.machine_error_timeout,
+            args.machine_pending_timeout,
+            args.machine_exclude
             )
         return 0
     except JujuWaitException as x:
@@ -309,7 +336,8 @@ def get_error_repeat_count(uname):
     return len(status_lines)
 
 
-def wait(log=None, wait_for_workload=False, max_wait=None, retry_errors=0, exclude=None):
+def wait(log=None, wait_for_workload=False, max_wait=None, retry_errors=0, exclude=None,
+         machine_error_timeout=300, machine_pending_timeout=3600, machine_exclude=None):
     # Note that wait_for_workload is only useful as a workaround for
     # broken setups. It is impossible for a charm to report that it has
     # completed setup, because a charm has no information about units
@@ -325,6 +353,10 @@ def wait(log=None, wait_for_workload=False, max_wait=None, retry_errors=0, exclu
         exclude = []
     exclude = set(exclude)
 
+    if machine_exclude is None:
+        machine_exclude = []
+    machine_exclude = set(machine_exclude)
+
     # pre-juju 1.24, we can only detect idleless by looking for changes
     # in the logs.
     prev_logs = {}
@@ -333,12 +365,15 @@ def wait(log=None, wait_for_workload=False, max_wait=None, retry_errors=0, exclu
     ready_since = None
     logging_reset = False
 
+    machine_status = {}
+
     while True:
         status = get_status()
         ready = True
 
         # If defined, fail if max_wait is exceeded
-        epoch_elapsed = time.time() - epoch_started
+        loop_starttime = time.time()
+        epoch_elapsed = loop_starttime - epoch_started
         if max_wait and epoch_elapsed > max_wait:
             ready = False
             logging.error('Not ready in {}s (max_wait)'.format(max_wait))
@@ -501,6 +536,54 @@ def wait(log=None, wait_for_workload=False, max_wait=None, retry_errors=0, exclu
                                   ''.format(uname, logs[uname].strip()))
                     ready = False
 
+        for machine in status.get('machines', {}).keys():
+            machine_data = status['machines'][machine]
+            machine_state = MachineState(
+                machine_data['juju-status']['current'],
+                parse_ts(
+                    machine_data['juju-status']['since']
+                )
+            )
+
+            if not machine_status.get(machine):
+                machine_status[machine] = machine_state
+            
+            if machine_status[machine] != machine_state:
+                machine_status[machine] = machine_state
+
+            for container in machine_data.get('containers', {}).keys():
+                container_data = machine_data['containers'][container]
+                container_state = MachineState(
+                    container_data['juju-status']['current'],
+                    parse_ts(
+                        container_data['juju-status']['since']
+                    )
+                )
+
+                if not machine_status.get(container):
+                    machine_status[container] = container_state
+
+                if machine_status[container] != container_state:
+                    machine_status[container] = container_state
+
+        for machine, state in machine_status.items():
+            if machine in machine_exclude:
+                logging.debug('Ignoring machine {} because it is excluded'.format(machine))
+                continue
+            if state.state in MACHINE_ERROR_STATES:
+                ready = False
+                logging.debug('Machine {} is in {}'.format(machine, state.state))
+                if loop_starttime - state.time.timestamp() > machine_error_timeout:
+                    minutes = round((loop_starttime - state.time.timestamp())/60)
+                    logging.error('Machine {} is in {} for {} minutes.'.format(machine, state.state, minutes))
+                    raise JujuWaitException(1)
+            if state.state in MACHINE_PENDING_STATES:
+                ready = False
+                if loop_starttime - state.time.timestamp() > machine_pending_timeout:
+                    minutes = round((loop_starttime - state.time.timestamp())/60)
+                    logging.error('Machine {} has been pending for {} minutes'.format(machine, minutes))
+                    raise JujuWaitException(1)
+
         if ready:
             # We are never ready until this check has been running until
             # IDLE_CONFIRMATION time has passed. This ensures that if we