feat(upgrade): report the run in three buckets

README.md

@@ -147,6 +147,10 @@ To update all instances:
 
 1. **Update Environment Files**: `alakazam example.com.yml config` to refresh .env files. It's a good practice to keep these files under version control (e.g., in a Git repository at `~/.abra/example.com`) and review changes with `git diff` before proceeding.
 2. **Upgrade Applications**: `alakazam example.com.yml upgrade` to update all applications.
+    - an app that abra refuses and an app that does not become healthy both end the run non-zero,
+      the apps beside them are still upgraded and still get their hooks
+    - a failing upgrade hook is reported but does not end the run, so that the hooks can be fixed
+      one at a time
 
 ### Rotate Secrets
 
@@ -262,6 +266,13 @@ For each app/recipe the following `<app_configurations>` can be used:
     - this is useful for env variables that are used multiple times like `COMPOSE_FILE`
 - **`env`**: Sets values for environment variables.
 - **`*-hooks`**: Specifies `abra.sh` commands or local scripts to run at specific stages.
+    - **`pre-upgrade-hooks`**: commands that should be run before each upgrade, while the old
+      containers are still running
+        - they are a precondition rather than an addition, so unlike every other kind they run
+          without `--execute-hooks`; `--skip-pre-upgrade-hooks` leaves them out
+        - a failing one costs that app its upgrade and nothing else, the run then ends non-zero
+        - the command comes from the recipe version the configuration asks for, so it has to exist
+          there and has to work against the app that is still running the old one
     - **`initial-hooks`**: commands for initialisation
     - **`deploy-hooks`**: commands that should be run after each deployment
     - **`upgrade-hooks`**: commands that should be run after each upgrade

alakazam.py

@@ -61,8 +61,14 @@ GROUP_PATH = None  # path this run was invoked with, either an instance file or
 HIDE_SECRETS = False  # keep secret values out of the output, for runs whose logs are archived
 ABRA_DIR = None  # path to the abra data directory
 FETCH_MAX_AGE = 3600 # seconds; recipe repos are refetched at most once per hour
-ABRA_RETRIES = 6 # number of attempts per abra command that failed to reach the server
+ABRA_RETRIES = 8 # number of attempts per abra command that failed for a reason worth retrying
 ABRA_RETRY_DELAY = 2 # seconds before the first retry, doubled for every further one
+# lower case markers in the abra output for an app whose replaced container has not gone away yet,
+# which leaves abra unable to tell which one a command is meant for
+AMBIGUOUS_CONTAINER_ERRORS = (
+    "ambiguous container list",
+    "expected 1 container but found",
+)
 # lower case markers in the abra output that indicate an unreachable server instead of a failed operation
 CONNECTION_ERRORS = (
     "error during connect",
@@ -79,6 +85,8 @@ CONNECTION_ERRORS = (
 MISSING_SECRET_ERROR = "doesn't exist on server"
 # abra subcommands that take a secret as an argument or return one
 SECRET_COMMANDS = ("insert", "generate")
+# marker in the abra output for an app that is not running, which is not an error to its callers
+NOT_DEPLOYED_ERROR = "is not deployed?"
 # markers in the abra output for a secret run that had nothing left to do, which abra exits 1 on
 NO_SECRETS_GENERATED = "no secrets generated"
 NO_SECRETS_TO_REMOVE = "no secrets to remove"
@@ -704,6 +712,27 @@ def is_connection_error(output: str) -> bool:
     return any(error in output.lower() for error in CONNECTION_ERRORS)
 
 
+def get_retry_reason(output: str) -> Optional[str]:
+    """
+    Names the transient condition an abra output shows, or nothing when the failure is a real one.
+
+    Both conditions leave the server untouched: an unreachable server never ran the command, and an
+    ambiguous container list is decided before the command is handed over. Retrying is therefore
+    safe even for a command that would not survive being run twice.
+
+    Args:
+        output (str): The combined stdout and stderr of the abra command
+
+    Returns:
+        str: What went wrong, phrased for the retry message, or None if it is worth no retry
+    """
+    if is_connection_error(output):
+        return "failed to reach the server"
+    if any(error in output.lower() for error in AMBIGUOUS_CONTAINER_ERRORS):
+        return "found more than one container, the replaced one has not gone away yet"
+    return None
+
+
 def run_streamed(command: List[str]) -> subprocess.CompletedProcess:
     """
     Runs a command and echoes its output line by line while it is still running.
@@ -762,12 +791,13 @@ def abra(*args: str, machine_output: bool = False, ignore_error: bool = False, s
         process = run_streamed(command) if stream else subprocess.run(command, capture_output=True)
         if not process.returncode or attempt == ABRA_RETRIES:
             break
-        if not is_connection_error(process.stdout.decode() + process.stderr.decode()):
+        reason = get_retry_reason(process.stdout.decode() + process.stderr.decode())
+        if not reason:
             break
         # back off exponentially: a server that drops connections during the ssh handshake is
         # usually throttling, and retrying every couple of seconds keeps it throttling
         delay = ABRA_RETRY_DELAY * 2 ** (attempt - 1)
-        logging.warning(f"attempt {attempt}/{ABRA_RETRIES} of '{' '.join(command)}' failed to reach the server, retry in {delay}s")
+        logging.warning(f"attempt {attempt}/{ABRA_RETRIES} of '{' '.join(command)}' {reason}, retry in {delay}s")
         sleep(delay)
     if process.stderr and ignore_error and not quiet:
         logging.warning(process.stderr.decode())
@@ -782,11 +812,30 @@ def abra(*args: str, machine_output: bool = False, ignore_error: bool = False, s
             f'{" ".join(command)} \n STDOUT: \n {process.stdout.decode()} \n STDERR: {process.stderr.decode()}')
     if machine_output:
         if output:= process.stdout.decode():
-            return json.loads(output)
+            return parse_machine_output(output)
         else:
             return {}
     return process.stdout.decode()
 
+
+def parse_machine_output(output: str) -> Any:
+    """
+    Parses the json abra writes with '-m', which some commands spread over one document per line.
+
+    'recipe versions' prints a document per recipe version instead of one for the whole answer, so
+    reading the output as a single document stops after the first line with "Extra data".
+
+    Args:
+        output (str): The stdout of an abra call that asked for machine output
+
+    Returns:
+        Any: The document abra wrote, or the list of them when it wrote more than one
+    """
+    try:
+        return json.loads(output)
+    except json.JSONDecodeError:
+        return [json.loads(line) for line in output.splitlines() if line.strip()]
+
 def sync_repo(path: Path) -> None:
     """
     Fetches a git repository and fast-forwards it, mirroring what abra does when it is online.
@@ -1190,18 +1239,8 @@ def share_secrets(app1_domain: str, app2_domain: str, secrets: Dict[str, str], s
     app1_stored_secrets = {x['name']: str2bool(x['created on server']) for x in app1_stored_secrets}
     app2_stored_secrets = abra("app", "secret", "ls", app2_domain, machine_output=True)
     app2_stored_secrets = {x['name']: str2bool(x['created on server']) for x in app2_stored_secrets}
-    try:
-        app1_container = abra("app" ,"ps", "-m", app1_domain, machine_output=True)
-        app1_container = list(app1_container)
-    except RuntimeError:
-        # App1 is not deployed
-        app1_container = []
-    try:
-        app2_container = abra("app" ,"ps", "-m", app2_domain, machine_output=True)
-        app2_container = list(app2_container)
-    except RuntimeError:
-        # App2 is not deployed
-        app2_container = []
+    app1_container = get_running_containers(app1_domain)
+    app2_container = get_running_containers(app2_domain)
     for app2_secret in secrets:
         app1_secret = secrets[app2_secret]
         try:
@@ -1273,6 +1312,31 @@ def share_secrets(app1_domain: str, app2_domain: str, secrets: Dict[str, str], s
                 )
 
 
+def get_running_containers(domain: str) -> List:
+    """
+    Lists the containers of an app, treating an app that is not deployed as having none.
+
+    Every other failure is raised: reading it as "not running" would turn a server that cannot be
+    reached into a silently skipped secret exchange.
+
+    Args:
+        domain (str): The app domain to read the containers of
+
+    Returns:
+        list: The containers abra reports, empty when the app is not deployed
+
+    Raises:
+        RuntimeError: If abra fails for any other reason
+    """
+    try:
+        return list(abra("app", "ps", "-m", domain, machine_output=True))
+    except RuntimeError as error:
+        if NOT_DEPLOYED_ERROR not in str(error):
+            raise
+        logging.info(f"{domain} is not deployed, it holds no containers to read a secret from")
+        return []
+
+
 def get_secret(domain: str, secret_name: str, containers: List) -> str:
     """
     Retrieves a specified secret from the application specified by its domain using the 'abra' command.
@@ -1287,7 +1351,9 @@ def get_secret(domain: str, secret_name: str, containers: List) -> str:
     """
     for container in containers:
         try:
-            secret = abra("app", "run", domain, container, "cat", f"/var/run/secrets/{secret_name}")
+            # -t is --no-tty: a run that asks for a terminal has none in ci, and a terminal would
+            # translate the line endings of the value on its way out
+            secret = abra("app", "run", "-t", domain, container, "cat", f"/var/run/secrets/{secret_name}")
             return str(secret)
         except RuntimeError:
             continue
@@ -1297,6 +1363,30 @@ def get_secret(domain: str, secret_name: str, containers: List) -> str:
     return get_secret_from_host(domain, secret_name)
 
 
+def run_over_ssh(host: str, command: str, purpose: str) -> str:
+    """
+    Runs a command on a host over ssh and returns its output, refusing to read a failure as empty.
+
+    Args:
+        host (str): The host to run the command on
+        command (str): The shell command to run there
+        purpose (str): What the command is for, used in the error message
+
+    Returns:
+        str: The stdout of the command, unchanged
+
+    Raises:
+        RuntimeError: If ssh or the command itself fails
+    """
+    result = subprocess.run(["ssh", host, command], capture_output=True, text=True)
+    if result.returncode:
+        raise RuntimeError(f"{purpose} on {host} exited with code {result.returncode}: "
+                           f"{result.stderr.strip() or 'no output'}")
+    if error := result.stderr.strip():
+        logging.warning(f"{purpose} on {host} wrote to stderr: {error}")
+    return result.stdout
+
+
 def get_secret_from_host(domain: str, secret_name: str) -> str:
     """
     Read a Docker Swarm secret value by SSHing to the host and reading from the
@@ -1324,8 +1414,7 @@ def get_secret_from_host(domain: str, secret_name: str) -> str:
         f"    '.[]? | .SecretID + \" \" + $cid + \" \" + .SecretName'; "
         f"done | grep ' {stack_name}_{secret_name}_' | head -1"
     )
-    result = subprocess.run(["ssh", host, find_cmd], capture_output=True, text=True)
-    match_line = result.stdout.strip()
+    match_line = run_over_ssh(host, find_cmd, f"the lookup of secret '{secret_name}'").strip()
     if not match_line:
         raise RuntimeError(f"Secret '{secret_name}' not found in stack '{stack_name}' on {host}")
 
@@ -1337,8 +1426,8 @@ def get_secret_from_host(domain: str, secret_name: str) -> str:
         f"cat /var/lib/docker/containers/{container_id}/mounts/secrets/{secret_id} 2>/dev/null "
         f"|| sudo cat /var/lib/docker/containers/{container_id}/mounts/secrets/{secret_id}"
     )
-    read_result = subprocess.run(["ssh", host, read_cmd], capture_output=True, text=True)
-    value = read_result.stdout  # no strip — trailing newline could be part of the secret
+    # no strip — a trailing newline could be part of the secret
+    value = run_over_ssh(host, read_cmd, f"the read of secret '{secret_name}'")
     if not value:
         raise RuntimeError(f"Could not read value for secret '{secret_name}' on {host}")
     return value
@@ -1380,7 +1469,7 @@ def insert_secrets_from_conf(domain: str, app_config: Dict[str, Any]) -> None:
                 insert_secret(domain, secret_name, secret)
 
 
-def run_local_script(tokens: List[str], app_domain: str, server: str, instance_domain: str, dry_run: bool = False, strict: bool = False) -> None:
+def run_local_script(tokens: List[str], app_domain: str, server: str, instance_domain: str, dry_run: bool = False, strict: bool = False) -> Optional[str]:
     """
     Run a local script hook. Relative paths are resolved against ROOT_PATH for execution. Logs an error and returns early if the script is not executable.
 
@@ -1392,6 +1481,9 @@ def run_local_script(tokens: List[str], app_domain: str, server: str, instance_d
         dry_run (bool): If True, prints the command but does not execute it.
         strict (bool): Abort on a non-executable script or a non-zero exit code instead of only logging it.
 
+    Returns:
+        str: Why the script failed, or None when it ran through
+
     Raises:
         click.ClickException: If the script is not executable or exits non-zero and strict is set
     """
@@ -1405,16 +1497,18 @@ def run_local_script(tokens: List[str], app_domain: str, server: str, instance_d
         if strict:
             raise click.ClickException(f"script '{cmd_display}' failed for {app_domain}: {message}")
         logging.error(message)
-        return
+        return message
     if dry_run:
-        return
+        return None
     # the script may print a secret it just created, so its output is captured and dropped
     result = subprocess.run([str(script_path)] + args, env=env, capture_output=HIDE_SECRETS)
     if result.returncode != 0:
         message = f"Script '{cmd_display}' exited with code {result.returncode}"
         if strict:
             raise click.ClickException(f"script '{cmd_display}' failed for {app_domain}: {message}")
-        logging.warning(message)
+        logging.error(message)
+        return message
+    return None
 
 
 def run_secret_hooks(domain: str, app_config: Dict[str, Any], instance_domain: str = "", strict: bool = False) -> None:
@@ -1670,7 +1764,7 @@ def run_readiness_hooks(domain: str, app_config: Dict[str, Any]) -> None:
         run_readiness_hook(domain, hook)
 
 
-def execute_cmds(app_config: Dict[str, Any], commands: Tuple[str] = tuple(), initial: bool = False, deploy: bool = False, upgrade: bool = False, dry_run: bool = False, chaos: bool = False, strict: bool = False, instance_domain: str = "") -> None:
+def execute_cmds(app_config: Dict[str, Any], commands: Tuple[str] = tuple(), pre_upgrade: bool = False, initial: bool = False, deploy: bool = False, upgrade: bool = False, dry_run: bool = False, chaos: bool = False, strict: bool = False, instance_domain: str = "") -> List[str]:
     """
     Execute post-deployment commands for an application based on the provided configuration.
     This can include running scripts or commands inside the application's environment.
@@ -1679,6 +1773,7 @@ def execute_cmds(app_config: Dict[str, Any], commands: Tuple[str] = tuple(), ini
         app_config (dict): A dictionary containing the deployment configuration and commands for an application.
         commands (list): A list of commands that should be executed in the following format:
             '<container_name> <abra.sh command> <optional arguments>'
+        pre_upgrade (bool): execute pre-upgrade-hooks, which run while the old containers still do
         initial (bool): execute initial-hooks
         deploy (bool): execute deploy-hooks
         upgrade (bool): execute upgrade-hooks
@@ -1687,38 +1782,55 @@ def execute_cmds(app_config: Dict[str, Any], commands: Tuple[str] = tuple(), ini
         instance_domain (str): The instance domain, used to set ALAKAZAM_INSTANCE_DOMAIN for script hooks.
 
     Returns:
-        None
+        list: The commands that failed, empty when all of them ran through. Always empty with
+            strict, which ends on the first failure instead of collecting it.
 
     Raises:
         click.ClickException: If a command fails and strict is set
     """
     domain = app_config['app_domain']
     server = app_config['server']
-    all_cmds = []
+    # kept grouped by kind rather than flattened, so that a dry run can say which kind it shows
+    groups = []
+    if pre_upgrade and (pre_upgrade_hooks:= app_config.get('pre-upgrade-hooks')):
+        groups.append(('pre-upgrade-hooks', pre_upgrade_hooks))
     if initial and (initial_hooks:= app_config.get('initial-hooks')):
-        all_cmds = all_cmds + initial_hooks
+        groups.append(('initial-hooks', initial_hooks))
     if deploy and (deploy_hooks:= app_config.get('deploy-hooks')):
-        all_cmds = all_cmds + deploy_hooks
+        groups.append(('deploy-hooks', deploy_hooks))
     if upgrade and (upgrade_hooks:= app_config.get('upgrade-hooks')):
-        all_cmds = all_cmds + upgrade_hooks
+        groups.append(('upgrade-hooks', upgrade_hooks))
     if commands:
-        all_cmds = all_cmds + list(commands)
+        groups.append(('--command', list(commands)))
     chaos_flag = ""
     if chaos:
         chaos_flag = "-C"
-    for cmd in all_cmds:
-        tokens = cmd.split()
-        container = tokens[0]
-        if container == "script" and len(tokens) >= 2 and resolve_path(tokens[1]).exists():
-            run_local_script(tokens, domain, server, instance_domain, dry_run, strict)
-            continue
-        print(f"Run '{cmd}' in {domain}:{container}", flush=True)
+    failed = []
+    for kind, cmds in groups:
+        # a real run has the deployment output between the kinds to tell them apart, a dry run
+        # prints them back to back and needs saying which side of the deployment each one is on
         if dry_run:
-            continue
-        try:
-            abra(*build_app_cmd(domain, cmd, chaos_flag), ignore_error=not strict, stream=True)
-        except RuntimeError as e:
-            raise click.ClickException(f"command '{cmd}' failed for {domain}: {e}")
+            print(f"{kind} for {domain}:")
+        for cmd in cmds:
+            tokens = cmd.split()
+            container = tokens[0]
+            if container == "script" and len(tokens) >= 2 and resolve_path(tokens[1]).exists():
+                if reason := run_local_script(tokens, domain, server, instance_domain, dry_run, strict):
+                    failed.append(cmd)
+                continue
+            print(f"Run '{cmd}' in {domain}:{container}", flush=True)
+            if dry_run:
+                continue
+            try:
+                # the error is raised here even without strict, so that a failure can be reported
+                # rather than only showing up in the streamed output
+                abra(*build_app_cmd(domain, cmd, chaos_flag), stream=True)
+            except RuntimeError as e:
+                if strict:
+                    raise click.ClickException(f"command '{cmd}' failed for {domain}: {e}")
+                logging.error(f"command '{cmd}' failed for {domain}: {e}")
+                failed.append(cmd)
+    return failed
 
 
 @click.group(context_settings={"help_option_names": ['-h', '--help']})
@@ -1993,9 +2105,71 @@ def deploy_apps(instance_apps: Dict[str, List[List]], execute_hooks: bool = Fals
             execute_cmds(app_config, deploy=True, instance_domain=instance)
 
 
+def run_upgrade_hooks(upgraded: List[Tuple[str, Dict[str, Any], str]], unhealthy: List[str]) -> List[Tuple[str, str]]:
+    """
+    Runs the upgrade hooks of every app that came back healthy and collects the ones that failed.
+
+    An app that never became healthy is left out: its hook would reach a container that is not
+    there, and the failure would say nothing beyond what the health check already said. A failing
+    hook is collected rather than raised, so that the apps behind it still get theirs.
+
+    Args:
+        upgraded (list): Tuples of app name, app configuration and instance domain
+        unhealthy (list): App domains whose services did not become healthy
+
+    Returns:
+        list: Tuples of app domain and the hook command that failed
+    """
+    failures = []
+    for app, app_config, instance_domain in upgraded:
+        domain = app_config.get('app_domain')
+        if domain in unhealthy:
+            logging.warning(f"{domain} is not healthy, its upgrade hooks are left out")
+            continue
+        logging.info(f"execute commands for {domain}")
+        failures += [(domain, cmd) for cmd in
+                     execute_cmds(app_config, upgrade=True, instance_domain=instance_domain)]
+    return failures
+
+
+def report_upgrade_run(total: int, failed: List[str], unhealthy: List[str],
+                       hook_failures: List[Tuple[str, str]]) -> None:
+    """
+    Reports what an upgrade run could not finish, and ends it when something needs a person.
+
+    An app that was refused and an app that never became healthy both leave the environment in a
+    state nobody asked for, so they end the run non-zero. A failing hook is reported only: hooks
+    still fail for reasons that are not failures, such as mariadb-upgrade refusing an already
+    upgraded database, and a verdict on them has to wait until those are sorted out.
+
+    Args:
+        total (int): How many apps the run set out to upgrade
+        failed (list): App domains abra refused to upgrade
+        unhealthy (list): App domains whose services did not become healthy
+        hook_failures (list): Tuples of app domain and the hook command that failed
+
+    Raises:
+        click.ClickException: If any app failed to upgrade or never became healthy
+    """
+    if hook_failures:
+        listed = ", ".join(f"{domain} '{cmd}'" for domain, cmd in hook_failures)
+        logging.error(f"{len(hook_failures)} upgrade hook(s) failed, "
+                      f"which does not end this run: {listed}")
+    problems = []
+    if failed:
+        problems.append(f"{len(failed)} of {total} app(s) failed to upgrade: {', '.join(failed)}")
+    if unhealthy:
+        problems.append(f"{len(unhealthy)} of {total} app(s) did not become healthy: "
+                        f"{', '.join(unhealthy)}")
+    if problems:
+        raise click.ClickException("\n".join(problems))
+
+
 @cli.command()
 @click.option('recipes', '-r', '--recipe', multiple=True, metavar='<RecipeName>', help='Filter for selcted recipes, this option can be specified multiple times.')
 @click.option('-e', '--execute-hooks', is_flag=True, help='run post-upgrade commands.')
+@click.option('skip_pre_upgrade_hooks', '--skip-pre-upgrade-hooks', is_flag=True,
+              help='leave out the pre-upgrade-hooks, which run unasked otherwise.')
 @click.option('-d', '--dry-run', is_flag=True, help="don't execute the upgrade process")
 @click.option('-rd', '--redeploy', is_flag=True, help="use undeploy and deploy for the updating process")
 @click.option('--redeploy-wait', type=int, default=20, metavar='<SECONDS>',
@@ -2004,12 +2178,14 @@ def deploy_apps(instance_apps: Dict[str, List[List]], execute_hooks: bool = Fals
 @click.option('--timeout', default=PS_WAIT_TIMEOUT, show_default=True, metavar='<Seconds>', help='how long to wait for all services to become healthy.')
 @click.option('--settle', default=PS_WAIT_SETTLE, show_default=True, metavar='<Seconds>', help='how long to give the replaced services to be taken down before their health is read.')
 @click.option('noninteractive', '-n', '--non-interactive', is_flag=True, help='Run this command non-interactively')
-def upgrade(recipes: Tuple[str], execute_hooks: bool, dry_run: bool, redeploy: bool,
-            converge_checks: bool, timeout: int, settle: int, noninteractive: bool, redeploy_wait: int) -> None:
+def upgrade(recipes: Tuple[str], execute_hooks: bool, skip_pre_upgrade_hooks: bool, dry_run: bool,
+            redeploy: bool, converge_checks: bool, timeout: int, settle: int, noninteractive: bool,
+            redeploy_wait: int) -> None:
     """
     Upgrades specified applications by executing the upgrade commands via the 'abra' command-line interface.
     It checks the current deployment status of the apps and performs upgrades only where necessary, with options to execute additional commands or perform a dry run. It either took the target version from the configuration or it uses the latest available version.
     The apps are upgraded without waiting for each other, then the run blocks until every service of them is healthy, and only then the upgrade hooks are executed.
+    A pre-upgrade hook runs before an app is replaced, while its old containers are still up. It is a precondition rather than an addition, so it needs no --execute-hooks and a failing one costs its app the upgrade.
     """
     deployed_instance_apps = get_apps_by_deployment(recipes, deployed=True)
     upgrade_cmds = []
@@ -2065,37 +2241,64 @@ def upgrade(recipes: Tuple[str], execute_hooks: bool, dry_run: bool, redeploy: b
         print(app)
         print(note)
     if dry_run:
+        # shown instead of run, so that a dry run answers what the hooks would have done. The
+        # pre-upgrade ones are not behind --execute-hooks and are therefore always listed
+        if not skip_pre_upgrade_hooks:
+            for _, app_config, _, instance_domain in upgrade_cmds:
+                execute_cmds(app_config, pre_upgrade=True, dry_run=True,
+                             instance_domain=instance_domain)
         if execute_hooks:
-            # shown instead of run, so that a dry run also answers what the -e would have done
             for _, app_config, _, instance_domain in upgrade_cmds:
                 execute_cmds(app_config, upgrade=True, dry_run=True, instance_domain=instance_domain)
         return
     if noninteractive or input("Do you really want to upgrade these apps? Type YES: ") == "YES":
+        upgraded, failed = [], []
         for app, app_config, upgrade_cmd, instance_domain in upgrade_cmds:
             app_domain = app_config.get('app_domain')
-            # a recipe can bring a new secret along, which abra refuses to deploy without. The app
-            # is running, so nothing else can be missing: it had all of them when it was deployed
-            if missing := get_missing_secrets(app_domain):
-                print(f"{app_domain} is missing {len(missing)} secret(s): {', '.join(sorted(missing))}")
-                create_secrets((app,), instances=(instance_domain,))
-            if redeploy:
-                upgrade_cmd.pop(0)
-                upgrade_cmd.insert(0, 'deploy')
-                print(f'undeploy {app_domain}')
-                print(abra("app", "undeploy", '--no-input', app_domain))
-                print(f'waiting {redeploy_wait}s …')
-                sleep(redeploy_wait)
-            print(f'deploy {app_domain}')
-            print(abra("app", *upgrade_cmd))
-        if not upgrade_cmds:
-            return
-        # the deployments were started without waiting for each other, so the services are still
-        # settling here and a hook would reach a container that is not up again yet
-        wait_for_healthy_apps(tuple({app for app, *_ in upgrade_cmds}), timeout, settle=settle)
-        if execute_hooks:
-            for app, app_config, _, instance_domain in upgrade_cmds:
-                logging.info(f"execute commands for {app_config.get('app_domain')}")
-                execute_cmds(app_config, upgrade=True, instance_domain=instance_domain)
+            try:
+                # a recipe can bring a new secret along, which abra refuses to deploy without. The
+                # app is running, so nothing else can be missing: it had all of them when deployed
+                if missing := get_missing_secrets(app_domain):
+                    print(f"{app_domain} is missing {len(missing)} secret(s): {', '.join(sorted(missing))}")
+                    create_secrets((app,), instances=(instance_domain,))
+                if not skip_pre_upgrade_hooks:
+                    # before the undeploy below, which would leave the hook nothing to act on
+                    execute_cmds(app_config, pre_upgrade=True, strict=True,
+                                 instance_domain=instance_domain)
+                if redeploy:
+                    upgrade_cmd.pop(0)
+                    upgrade_cmd.insert(0, 'deploy')
+                    print(f'undeploy {app_domain}')
+                    print(abra("app", "undeploy", '--no-input', app_domain))
+                    print(f'waiting {redeploy_wait}s …')
+                    sleep(redeploy_wait)
+                print(f'deploy {app_domain}')
+                print(abra("app", *upgrade_cmd))
+            except (RuntimeError, click.ClickException) as error:
+                # the apps behind this one do not depend on it, so they are still upgraded and the
+                # run ends on the failure instead of dropping everything where it stands
+                logging.error(f"upgrade of {app_domain} failed: {error}")
+                failed.append(app_domain)
+                continue
+            upgraded.append((app, app_config, instance_domain))
+        unhealthy, hook_failures = [], []
+        if upgraded:
+            try:
+                # the deployments were started without waiting for each other, so the services are
+                # still settling here and a hook would reach a container that is not up again yet
+                pending = get_unhealthy_apps(tuple({app for app, *_ in upgraded}), timeout,
+                                             settle=settle)
+                unhealthy = sorted({domain for domain, _ in pending})
+                if pending:
+                    logging.error(f"{len(pending)} service(s) still not healthy after {timeout}s:"
+                                  f"\n{format_pending_containers(pending)}")
+            except click.ClickException as error:
+                # with nothing to read the health off, no app can be told apart from another
+                logging.error(error.message)
+                unhealthy = sorted(config.get('app_domain') for _, config, _ in upgraded)
+            if execute_hooks:
+                hook_failures = run_upgrade_hooks(upgraded, unhealthy)
+        report_upgrade_run(len(upgrade_cmds), failed, unhealthy, hook_failures)
 
 
 @cli.command()
@@ -2131,12 +2334,13 @@ def undeploy_apps(instance_apps: Dict[str, List[List]]) -> None:
 @cli.command()
 @click.option('recipes', '-r', '--recipe', multiple=True, metavar='<RecipeName>', help='Filter for selcted recipes, this option can be specified multiple times.')
 @click.option('commands', '-c', '--command', multiple=True, metavar="'<container_name> <abra.sh command> <optional arguments>'", help='commands that should be executed')
+@click.option('pre_upgrade', '-p', '--pre-upgrade', is_flag=True, help='execute pre-upgrade-hooks from config')
 @click.option('-i', '--initial', is_flag=True, help='execute initial-hooks from config')
 @click.option('-d', '--deploy', is_flag=True, help='execute deploy-hooks from config')
 @click.option('-u', '--upgrade', is_flag=True, help='execute upgrade-hooks from config')
 @click.option('-l', '--list-cmds', is_flag=True, help="only show cmds, don't execute them")
 @click.option('-C', '--chaos', is_flag=True, help="execute in chaos mode")
-def cmd(recipes: Tuple[str], commands: Tuple[str], initial: bool, deploy: bool, upgrade: bool, list_cmds: bool = False, chaos: bool = False) -> None:
+def cmd(recipes: Tuple[str], commands: Tuple[str], pre_upgrade: bool, initial: bool, deploy: bool, upgrade: bool, list_cmds: bool = False, chaos: bool = False) -> None:
     """
     Execute commands for all specified applications based on the provided configuration.
     """
@@ -2153,7 +2357,7 @@ def cmd(recipes: Tuple[str], commands: Tuple[str], initial: bool, deploy: bool,
                 print(f"{domain} is not deployed")
                 continue
             logging.info(f'execute commands for {domain}')
-            execute_cmds(app_config, commands=commands, initial=initial, deploy=deploy, upgrade=upgrade, dry_run=list_cmds, chaos=chaos, instance_domain=instance_domain)
+            execute_cmds(app_config, commands=commands, pre_upgrade=pre_upgrade, initial=initial, deploy=deploy, upgrade=upgrade, dry_run=list_cmds, chaos=chaos, instance_domain=instance_domain)
 
 
 @cli.command()
@@ -2412,8 +2616,13 @@ def get_latest_version(app: str) -> str:
     Returns:
         str: The latest version of the application as a string.
     """
-    versions = abra('recipe', 'versions', app, machine_output=True)
-    latest = sorted([(version.parse(v['version']), v['version']) for v in versions])[-1][1]
+    releases = abra('recipe', 'versions', app, machine_output=True)
+    # a release is the list of images of one recipe version, and each of them carries that version
+    images = [image for release in releases
+              for image in (release if isinstance(release, list) else [release])]
+    if not images:
+        raise click.ClickException(f"abra reports no versions for {app}, pin one in the configuration")
+    latest = sorted([(version.parse(i['version']), i['version']) for i in images])[-1][1]
     logging.debug(f'latest version of {app}: {latest}')
     return latest
 
@@ -2640,13 +2849,13 @@ def print_containers(containers: List[Tuple[str, Dict[str, Any]]], verbose: bool
     print(tabulate(rows, 'firstrow', 'rounded_outline'), flush=True)
 
 
-def wait_for_healthy_apps(recipes: Tuple[str], timeout: int, verbose: bool = False, settle: int = 0) -> None:
+def get_unhealthy_apps(recipes: Tuple[str], timeout: int, verbose: bool = False, settle: int = 0) -> List[Tuple[str, Dict[str, Any]]]:
     """
-    Blocks until no service of the selected apps is starting, unknown or unhealthy any more.
+    Waits for the services of the selected apps to settle and reports the ones that never did.
 
     The status is checked after the settle delay and then every PS_WAIT_INTERVAL seconds until the
-    timeout is reached. Having nothing deployed is a failure rather than an early success: waiting
-    for the health of an environment that does not exist would report a broken deployment as green.
+    timeout is reached. Having nothing deployed is a failure rather than an early success: reading
+    the health of an environment that does not exist would report a broken deployment as green.
 
     The delay matters wherever apps are being replaced: such a service keeps reporting its old
     container for a moment, and that one is healthy, it has been running all along. Reading the
@@ -2654,13 +2863,17 @@ def wait_for_healthy_apps(recipes: Tuple[str], timeout: int, verbose: bool = Fal
     built from scratch has nothing to replace and leaves the delay at zero.
 
     Args:
-        recipes (tuple): Recipes to wait for, all of them if empty
+        recipes (tuple): Recipes to read, all of them if empty
         timeout (int): Seconds to wait before giving up
         verbose (bool): Add the chaos, state and image columns to the final table
         settle (int): Seconds to give replaced services to be taken down before the first check
 
+    Returns:
+        list: Tuples of app domain and container for every service that is still not healthy,
+            empty when all of them are
+
     Raises:
-        click.ClickException: If nothing is deployed, or if services are still not healthy when the timeout is reached
+        click.ClickException: If nothing is deployed
     """
     if settle:
         print(f"waiting {settle}s for the replaced services to be taken down", flush=True)
@@ -2675,15 +2888,49 @@ def wait_for_healthy_apps(recipes: Tuple[str], timeout: int, verbose: bool = Fal
         if not pending:
             print_containers(containers, verbose)
             print("All services are healthy")
-            return
+            return []
         if time() >= deadline:
             break
         waiting = ", ".join(f"{domain} {container['service']} ({container['status']})" for domain, container in pending)
         print(f"{len(pending)} service(s) not healthy yet, check again in {PS_WAIT_INTERVAL}s: {waiting}", flush=True)
         sleep(PS_WAIT_INTERVAL)
     print_containers(containers, verbose)
-    report = "\n".join(f"  {domain} {container['service']}: {container['status']}" for domain, container in pending)
-    raise click.ClickException(f"{len(pending)} service(s) still not healthy after {timeout}s:\n{report}")
+    return pending
+
+
+def format_pending_containers(pending: List[Tuple[str, Dict[str, Any]]]) -> str:
+    """
+    Renders the services that never became healthy as one indented line each.
+
+    Args:
+        pending (list): Tuples of app domain and container information
+
+    Returns:
+        str: One line per service, naming its app and its status
+    """
+    return "\n".join(f"  {domain} {container['service']}: {container['status']}"
+                     for domain, container in pending)
+
+
+def wait_for_healthy_apps(recipes: Tuple[str], timeout: int, verbose: bool = False, settle: int = 0) -> None:
+    """
+    Blocks until no service of the selected apps is starting, unknown or unhealthy any more.
+
+    For a caller that has something left to do for the apps that did come back, get_unhealthy_apps()
+    reports them instead of ending the run.
+
+    Args:
+        recipes (tuple): Recipes to wait for, all of them if empty
+        timeout (int): Seconds to wait before giving up
+        verbose (bool): Add the chaos, state and image columns to the final table
+        settle (int): Seconds to give replaced services to be taken down before the first check
+
+    Raises:
+        click.ClickException: If nothing is deployed, or if services are still not healthy when the timeout is reached
+    """
+    if pending := get_unhealthy_apps(recipes, timeout, verbose, settle):
+        raise click.ClickException(f"{len(pending)} service(s) still not healthy after {timeout}s:"
+                                   f"\n{format_pending_containers(pending)}")
 
 
 def get_deployment_traces(recipes: Tuple[str]) -> List[str]:

tests/test_latest_version.py

@@ -0,0 +1,68 @@
+"""Tests for reading the newest recipe version out of what 'abra recipe versions -m' writes."""
+
+import json
+import os
+import sys
+
+import click
+import pytest
+
+sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
+
+import alakazam
+from alakazam import get_latest_version, parse_machine_output
+
+# one document per recipe version, each listing the images of that version, as abra writes it
+GRIST_VERSIONS = (
+    '[{"name":"gristlabs/grist-oss","service":"app","tag":"1.7.19","version":"3.6.4+1.7.19"},'
+    '{"name":"redis","service":"redis","tag":"8.10-alpine","version":"3.6.4+1.7.19"}]\n'
+    '[{"name":"gristlabs/grist-oss","service":"app","tag":"1.7.18","version":"3.6.3+1.7.18"},'
+    '{"name":"redis","service":"redis","tag":"8.10-alpine","version":"3.6.3+1.7.18"}]\n'
+    '[{"name":"gristlabs/grist","service":"app","tag":"1.1.12","version":"1.0.0+1.1.12"},'
+    '{"name":"postgres","service":"db","tag":"13","version":"1.0.0+1.1.12"}]\n'
+)
+
+
+class TestParseMachineOutput:
+    def test_several_documents_are_read_as_a_list(self):
+        """A plain json.loads() stops after the first line with "Extra data"."""
+        assert len(parse_machine_output(GRIST_VERSIONS)) == 3
+
+    def test_a_single_document_stays_one_document(self):
+        assert parse_machine_output('{"a.org": {"apps": []}}') == {"a.org": {"apps": []}}
+
+    def test_a_single_list_stays_one_list(self):
+        assert parse_machine_output('[{"name": "db_password"}]') == [{"name": "db_password"}]
+
+    def test_blank_lines_are_skipped(self):
+        assert parse_machine_output('{"a": 1}\n\n{"b": 2}\n') == [{"a": 1}, {"b": 2}]
+
+    def test_output_that_is_no_json_at_all_still_raises(self):
+        """Swallowing it would turn a broken abra call into an empty but plausible answer."""
+        with pytest.raises(json.JSONDecodeError):
+            parse_machine_output("FATA something went wrong")
+
+
+class TestGetLatestVersion:
+    def install(self, monkeypatch, output):
+        monkeypatch.setattr(alakazam, "abra", lambda *a, **k: parse_machine_output(output))
+
+    def test_the_newest_version_is_taken(self, monkeypatch):
+        self.install(monkeypatch, GRIST_VERSIONS)
+        assert get_latest_version("grist") == "3.6.4+1.7.19"
+
+    def test_the_order_of_the_documents_does_not_matter(self, monkeypatch):
+        self.install(monkeypatch, "\n".join(reversed(GRIST_VERSIONS.strip().splitlines())))
+        assert get_latest_version("grist") == "3.6.4+1.7.19"
+
+    def test_a_single_flat_document_is_read_too(self, monkeypatch):
+        """Which is what abra writes for a recipe that has exactly one version."""
+        self.install(monkeypatch, '[{"name":"redis","tag":"8.10","version":"1.0.0+1.1.12"}]')
+        assert get_latest_version("grist") == "1.0.0+1.1.12"
+
+    def test_a_recipe_without_versions_is_reported(self, monkeypatch):
+        """Reaching for the newest of nothing would end in an IndexError with no hint in it."""
+        monkeypatch.setattr(alakazam, "abra", lambda *a, **k: {})
+        with pytest.raises(click.ClickException) as excinfo:
+            get_latest_version("grist")
+        assert "no versions for grist" in excinfo.value.message

tests/test_retry.py

@@ -1,5 +1,6 @@
 """Tests for which abra failures may be retried."""
 
+import logging
 import os
 import sys
 
@@ -9,7 +10,7 @@ import pytest
 sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
 
 import alakazam
-from alakazam import abra, is_connection_error
+from alakazam import abra, get_retry_reason, is_connection_error
 
 
 class FakeProcess:
@@ -21,19 +22,22 @@ class FakeProcess:
 
 UNREACHABLE = b"FATA error during connect: kex_exchange_identification: Connection closed by remote host"
 REJECTED = b"FATA login.a.org is still deployed"
+AMBIGUOUS = (b"WARN ambiguous container list received, prompting for input\n"
+             b"? which container are you looking for?\nFATA EOF")
+
+
+@pytest.fixture
+def runs(monkeypatch):
+    """Hands out a prepared result per abra call and records what was slept between them."""
+    calls = {"results": [], "slept": []}
+    monkeypatch.setattr(alakazam, "sleep", calls["slept"].append)
+    monkeypatch.setattr(alakazam.subprocess, "run", lambda cmd, capture_output: calls["results"].pop(0))
+    return calls
 
 
 class TestRetryLoop:
     """A server that drops the ssh handshake is usually throttling, so the retries have to spread out."""
 
-    @pytest.fixture
-    def runs(self, monkeypatch):
-        calls = {"results": [], "slept": []}
-        monkeypatch.setattr(alakazam, "sleep", calls["slept"].append)
-        monkeypatch.setattr(alakazam.subprocess, "run",
-                            lambda cmd, capture_output: calls["results"].pop(0))
-        return calls
-
     def test_a_successful_command_does_not_retry(self, runs):
         runs["results"] = [FakeProcess(0)]
         abra("app", "ls")
@@ -83,3 +87,35 @@ class TestConnectionErrors:
     ])
     def test_operational_failures_are_not_retried(self, output):
         assert not is_connection_error(output)
+
+
+class TestAmbiguousContainer:
+    """
+    An app that was just updated keeps its replaced container for a while, and abra then cannot
+    tell which of the two a command is meant for. It asks, and a run without a terminal ends on EOF.
+    """
+
+    @pytest.mark.parametrize("output", [
+        "WARN ambiguous container list received, prompting for input\nFATA EOF",
+        "FATA expected 1 container but found 2: files_app.1.pxo3 (created About a minute ago)",
+    ])
+    def test_it_is_worth_another_attempt(self, output):
+        assert get_retry_reason(output) == ("found more than one container, "
+                                            "the replaced one has not gone away yet")
+
+    def test_an_unreachable_server_keeps_its_own_reason(self):
+        assert get_retry_reason("kex_exchange_identification") == "failed to reach the server"
+
+    def test_a_rejected_operation_is_worth_none(self):
+        assert get_retry_reason("FATA login.a.org is still deployed") is None
+
+    def test_the_command_is_repeated_until_one_container_is_left(self, runs):
+        runs["results"] = [FakeProcess(1, AMBIGUOUS), FakeProcess(1, AMBIGUOUS), FakeProcess(0)]
+        abra("app", "cmd", "-T", "files.a.org", "app", "--", "run_occ", "upgrade")
+        assert runs["slept"] == [alakazam.ABRA_RETRY_DELAY, alakazam.ABRA_RETRY_DELAY * 2]
+
+    def test_the_reason_is_reported(self, runs, caplog):
+        runs["results"] = [FakeProcess(1, AMBIGUOUS), FakeProcess(0)]
+        with caplog.at_level(logging.WARNING):
+            abra("app", "cmd", "-T", "files.a.org", "app", "--", "run_occ", "upgrade")
+        assert "the replaced one has not gone away yet" in caplog.text

tests/test_secret_exchange.py

@@ -0,0 +1,85 @@
+"""Tests for reading a secret out of a running app, and for what happens when that fails."""
+
+import logging
+import os
+import subprocess
+import sys
+
+import pytest
+
+sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
+
+import alakazam
+from alakazam import get_running_containers, get_secret, run_over_ssh
+
+
+class TestGetRunningContainers:
+    def test_the_containers_are_returned(self, monkeypatch):
+        monkeypatch.setattr(alakazam, "abra", lambda *a, **k: [{"service": "app"}])
+        assert get_running_containers("cloud.a.org") == [{"service": "app"}]
+
+    def test_an_app_that_is_not_deployed_has_none(self, monkeypatch):
+        def abra(*args, **kwargs):
+            raise RuntimeError("abra -o app ps -m cloud.a.org\n STDERR: FATA cloud.a.org is not deployed?")
+
+        monkeypatch.setattr(alakazam, "abra", abra)
+        assert get_running_containers("cloud.a.org") == []
+
+    def test_any_other_failure_is_raised(self, monkeypatch):
+        """Reading an unreachable server as "not running" would skip the exchange without a word."""
+        def abra(*args, **kwargs):
+            raise RuntimeError("abra -o app ps -m cloud.a.org\n STDERR: ssh: connect to host")
+
+        monkeypatch.setattr(alakazam, "abra", abra)
+        with pytest.raises(RuntimeError, match="connect to host"):
+            get_running_containers("cloud.a.org")
+
+
+class TestGetSecret:
+    def test_the_run_asks_for_no_terminal(self, monkeypatch):
+        """A ci run has no terminal to give, and one would rewrite the line endings of the value."""
+        calls = []
+        monkeypatch.setattr(alakazam, "abra", lambda *a, **k: calls.append(a) or "s3cret")
+        assert get_secret("cloud.a.org", "db_password", ["app"]) == "s3cret"
+        assert calls[0][:4] == ("app", "run", "-t", "cloud.a.org")
+
+    def test_the_next_container_is_tried(self, monkeypatch):
+        def abra(*args, **kwargs):
+            if args[4] == "db":
+                raise RuntimeError("FATA no such container")
+            return "s3cret"
+
+        monkeypatch.setattr(alakazam, "abra", abra)
+        assert get_secret("cloud.a.org", "db_password", ["db", "app"]) == "s3cret"
+
+
+class TestRunOverSsh:
+    def result(self, returncode=0, stdout="", stderr=""):
+        return subprocess.CompletedProcess(["ssh"], returncode, stdout, stderr)
+
+    def test_the_output_is_returned_unchanged(self, monkeypatch):
+        """A trailing newline can be part of a secret, so nothing is stripped."""
+        monkeypatch.setattr(subprocess, "run", lambda *a, **k: self.result(stdout="s3cret\n"))
+        assert run_over_ssh("a.org", "cat x", "the read") == "s3cret\n"
+
+    def test_a_failing_command_is_raised(self, monkeypatch):
+        """It exited 1 and wrote nothing, which read as an empty secret before."""
+        monkeypatch.setattr(subprocess, "run",
+                            lambda *a, **k: self.result(returncode=1, stderr="Permission denied"))
+        with pytest.raises(RuntimeError) as excinfo:
+            run_over_ssh("a.org", "cat x", "the read")
+        assert "exited with code 1" in str(excinfo.value)
+        assert "Permission denied" in str(excinfo.value)
+
+    def test_a_silent_failure_is_still_raised(self, monkeypatch):
+        monkeypatch.setattr(subprocess, "run", lambda *a, **k: self.result(returncode=255))
+        with pytest.raises(RuntimeError, match="no output"):
+            run_over_ssh("a.org", "cat x", "the read")
+
+    def test_stderr_on_a_successful_command_is_reported(self, monkeypatch, caplog):
+        """ssh writes its own warnings there, and they explain an answer that looks wrong."""
+        monkeypatch.setattr(subprocess, "run",
+                            lambda *a, **k: self.result(stdout="s3cret", stderr="Pseudo-terminal will not be allocated"))
+        with caplog.at_level(logging.WARNING):
+            assert run_over_ssh("a.org", "cat x", "the read") == "s3cret"
+        assert "Pseudo-terminal" in caplog.text

tests/test_upgrade_hooks.py

@@ -1,8 +1,10 @@
 """Tests for what an upgrade run does: deploy everything, wait for all of it, then hook."""
 
+import logging
 import os
 import sys
 
+import click
 import pytest
 from click.testing import CliRunner
 
@@ -10,10 +12,14 @@ sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
 
 import alakazam
 
+# only nextcloud carries a pre-upgrade hook, so that "not configured" is distinguishable from
+# "configured but not run"
 CONFIG = {"a.org": {
     "nextcloud": {"app_domain": "cloud.a.org", "server": "a.org", "version": "2.0.0",
+                  "pre-upgrade-hooks": ["db update_mongo_version"],
                   "upgrade-hooks": ["app occ upgrade"]},
-    "wekan": {"app_domain": "boards.a.org", "server": "a.org", "version": "2.0.0"},
+    "wekan": {"app_domain": "boards.a.org", "server": "a.org", "version": "2.0.0",
+              "upgrade-hooks": ["app ready"]},
 }}
 
 # kept before any fixture replaces it, for the tests that check what a dry run actually prints
@@ -33,9 +39,19 @@ def steps(monkeypatch, versions):
     monkeypatch.setattr(alakazam, "INSTANCE_CONFIGS", CONFIG)
     monkeypatch.setattr(alakazam, "sleep", lambda seconds: steps.append(f"sleep:{seconds}"))
     monkeypatch.setattr(alakazam, "get_missing_secrets", lambda domain: [])
-    # a dry run reaches the same function, so the two have to be told apart here
-    monkeypatch.setattr(alakazam, "execute_cmds", lambda config, dry_run=False, **kw: steps.append(
-        f"{'show' if dry_run else 'hook'}:{config['app_domain']}"))
+    def execute_cmds(config, dry_run=False, **kw):
+        """
+        Labels a hook call by kind, and keeps quiet for an app that configures none of that kind.
+
+        selected() matches on a prefix, so the pre-upgrade label needs one of its own rather than a
+        suffix: "hook:pre:..." would be picked up by every assertion on "hook:".
+        """
+        kind, key = ("pre-hook", "pre-upgrade-hooks") if kw.get("pre_upgrade") else ("hook", "upgrade-hooks")
+        if config.get(key):
+            steps.append(f"{'show:' if dry_run else ''}{kind}:{config['app_domain']}")
+        return []   # the commands that failed, which execute_cmds reports back
+
+    monkeypatch.setattr(alakazam, "execute_cmds", execute_cmds)
 
     # the recipe filter has to be honoured here, it is what the wait narrows itself down with
     def apps_by_deployment(recipes, deployed=True):
@@ -57,6 +73,11 @@ def steps(monkeypatch, versions):
     return steps
 
 
+def unhealthy(*domains):
+    """What get_unhealthy_apps() reports for services that never settled."""
+    return [(domain, {"service": "app", "status": "starting"}) for domain in domains]
+
+
 def selected(steps, prefix):
     return [step for step in steps if step.startswith(prefix)]
 
@@ -162,8 +183,10 @@ class TestDryRun:
 
     def test_a_shown_hook_is_not_executed(self, steps):
         run(["--dry-run", "-e"], input="\n")
-        assert selected(steps, "show:") == ["show:cloud.a.org", "show:boards.a.org"]
+        assert selected(steps, "show:") == ["show:pre-hook:cloud.a.org",
+                                            "show:hook:cloud.a.org", "show:hook:boards.a.org"]
         assert selected(steps, "hook:") == []
+        assert selected(steps, "pre-hook:") == []
 
 
 class TestSettleOption:
@@ -177,3 +200,268 @@ class TestSettleOption:
         run(["-n", "--settle", "0"])
         assert selected(steps, "sleep:") == []
         assert selected(steps, "ps:") == ["ps:cloud.a.org", "ps:boards.a.org"]
+
+
+class TestAFailingApp:
+    """A server that refuses one app must not take the apps behind it down with it."""
+
+    @pytest.fixture
+    def failing(self, monkeypatch, steps):
+        """Lets the upgrade of cloud.a.org fail the way abra reports a rejected deployment."""
+        original = alakazam.abra
+
+        def abra(*args, **kwargs):
+            # the planning phase asks for the release notes with the same command plus -r
+            if args[:2] == ("app", "upgrade") and "-r" not in args and "cloud.a.org" in args:
+                raise RuntimeError("abra -o app upgrade -n cloud.a.org 2.0.0 \n STDERR: FATA "
+                                   "failed to update cloud_a_org_db: context deadline exceeded")
+            return original(*args, **kwargs)
+
+        monkeypatch.setattr(alakazam, "abra", abra)
+        return steps
+
+    def invoke(self, args):
+        return CliRunner().invoke(alakazam.upgrade, ["-n"] + args, standalone_mode=False)
+
+    def test_the_other_apps_are_still_upgraded(self, failing):
+        self.invoke(["-e"])
+        assert "app upgrade -n --no-converge-checks boards.a.org 2.0.0" in failing
+
+    def test_the_run_ends_on_the_failure(self, failing):
+        result = self.invoke(["-e"])
+        assert isinstance(result.exception, click.ClickException)
+        assert "1 of 2 app(s) failed to upgrade: cloud.a.org" in result.exception.message
+
+    def test_the_failing_app_is_neither_waited_for_nor_hooked(self, failing):
+        """Its state is unknown, so a hook against it would run on whatever is left there."""
+        self.invoke(["-e"])
+        assert selected(failing, "ps:") == ["ps:boards.a.org"]
+        assert selected(failing, "hook:") == ["hook:boards.a.org"]
+
+    def test_the_reason_is_reported(self, failing, caplog):
+        with caplog.at_level(logging.ERROR):
+            self.invoke(["-e"])
+        assert "context deadline exceeded" in caplog.text
+
+    def test_every_app_failing_skips_the_wait(self, steps, monkeypatch):
+        def abra(*args, **kwargs):
+            if args[:2] == ("app", "upgrade") and "-r" not in args:
+                raise RuntimeError("FATA failed to update: context deadline exceeded")
+            return ""
+
+        monkeypatch.setattr(alakazam, "abra", abra)
+        result = self.invoke(["-e"])
+        assert isinstance(result.exception, click.ClickException)
+        assert "2 of 2 app(s) failed" in result.exception.message
+        assert selected(steps, "ps:") == []
+
+    def test_an_unhealthy_app_does_not_hide_the_failing_one(self, failing, monkeypatch):
+        """One failure covering up another is how a broken app slips through a green pipeline."""
+        monkeypatch.setattr(alakazam, "get_unhealthy_apps", lambda *a, **k: unhealthy("boards.a.org"))
+        message = self.invoke(["-e"]).exception.message
+        assert "failed to upgrade: cloud.a.org" in message
+        assert "did not become healthy: boards.a.org" in message
+
+
+class TestPreUpgradeHooks:
+    """
+    A pre-upgrade hook prepares an app while its old containers still run, which is the only moment
+    some migrations have: mongo refuses to start on data whose feature compatibility version is two
+    majors behind the binary, so the version has to be raised before the binary is replaced.
+    """
+
+    def test_it_runs_before_that_app_is_deployed(self, steps):
+        run(["-n"])
+        upgrade = next(s for s in steps if "app upgrade" in s and "cloud.a.org" in s)
+        assert steps.index("pre-hook:cloud.a.org") < steps.index(upgrade)
+
+    def test_it_runs_before_the_undeploy_of_a_redeploy(self, steps):
+        """After an undeploy there is nothing left for the hook to read or change."""
+        run(["-n", "--redeploy"])
+        undeploy = next(s for s in steps if "app undeploy" in s and "cloud.a.org" in s)
+        assert steps.index("pre-hook:cloud.a.org") < steps.index(undeploy)
+
+    def test_it_runs_without_execute_hooks(self, steps):
+        """Unlike the other kinds: skipping a precondition does not skip work, it breaks the app."""
+        run(["-n"])
+        assert selected(steps, "pre-hook:") == ["pre-hook:cloud.a.org"]
+        assert selected(steps, "hook:") == []
+
+    def test_an_app_that_configures_none_gets_none(self, steps):
+        run(["-n"])
+        assert "pre-hook:boards.a.org" not in steps
+
+    def test_an_app_already_at_its_version_gets_none(self, steps, versions):
+        versions["nextcloud"] = "2.0.0"
+        run(["-n"])
+        assert selected(steps, "pre-hook:") == []
+
+    def test_the_flag_leaves_them_out(self, steps):
+        run(["-n", "--skip-pre-upgrade-hooks"])
+        assert selected(steps, "pre-hook:") == []
+        assert any("app upgrade" in s for s in steps)
+
+    def test_a_dry_run_shows_them_and_runs_nothing(self, steps):
+        run(["--dry-run"], input="\n")
+        assert selected(steps, "show:") == ["show:pre-hook:cloud.a.org"]
+        assert selected(steps, "pre-hook:") == []
+        assert not any("app upgrade" in s and "-r" not in s for s in steps)
+
+    def test_a_dry_run_honours_the_skip(self, steps):
+        run(["--dry-run", "--skip-pre-upgrade-hooks"], input="\n")
+        assert selected(steps, "show:") == []
+
+
+class TestAFailingPreUpgradeHook:
+    """It is a precondition, so it costs its own app the upgrade and no other app anything."""
+
+    @pytest.fixture
+    def failing(self, monkeypatch, steps):
+        def execute_cmds(config, dry_run=False, **kw):
+            if kw.get("pre_upgrade"):
+                if not config.get("pre-upgrade-hooks"):
+                    return
+                # what execute_cmds(strict=True) raises for a command the recipe does not have
+                raise click.ClickException(
+                    "command 'db update_mongo_version' failed for cloud.a.org: "
+                    "FATA no db function in abra.sh")
+            steps.append(f"hook:{config['app_domain']}")
+            return []
+
+        monkeypatch.setattr(alakazam, "execute_cmds", execute_cmds)
+        return steps
+
+    def invoke(self, args):
+        return CliRunner().invoke(alakazam.upgrade, ["-n"] + args, standalone_mode=False)
+
+    def test_that_app_is_not_upgraded(self, failing):
+        self.invoke([])
+        assert not any("app upgrade" in s and "cloud.a.org" in s for s in failing)
+
+    def test_the_other_app_still_is(self, failing):
+        self.invoke([])
+        assert any("app upgrade" in s and "boards.a.org" in s for s in failing)
+
+    def test_the_run_ends_naming_the_app(self, failing):
+        result = self.invoke([])
+        assert isinstance(result.exception, click.ClickException)
+        assert "1 of 2 app(s) failed to upgrade: cloud.a.org" in result.exception.message
+
+    def test_the_failing_app_is_neither_waited_for_nor_hooked(self, failing):
+        self.invoke(["-e"])
+        assert selected(failing, "ps:") == ["ps:boards.a.org"]
+        assert selected(failing, "hook:") == ["hook:boards.a.org"]
+
+    def test_the_reason_is_reported(self, failing, caplog):
+        with caplog.at_level(logging.ERROR):
+            self.invoke([])
+        assert "no db function in abra.sh" in caplog.text
+
+
+class TestDryRunNamesTheHookKind:
+    """
+    A real run has "deploy <domain>" between the kinds; a dry run prints them back to back, so the
+    kind has to be named or a pre-upgrade hook is indistinguishable from an identical later one.
+    """
+
+    @pytest.fixture
+    def output(self, steps, monkeypatch):
+        monkeypatch.setattr(alakazam, "execute_cmds", REAL_EXECUTE_CMDS)
+        return run(["--dry-run", "-e"], input="\n").output
+
+    def test_both_kinds_are_named(self, output):
+        assert "pre-upgrade-hooks for cloud.a.org:" in output
+        assert "upgrade-hooks for cloud.a.org:" in output
+
+    def test_the_pre_upgrade_kind_comes_first(self, output):
+        assert output.index("pre-upgrade-hooks for cloud.a.org:") < output.index("upgrade-hooks for")
+
+    def test_an_app_without_the_kind_does_not_announce_it(self, output):
+        assert "pre-upgrade-hooks for boards.a.org:" not in output
+
+    def test_a_real_run_names_no_kind(self, steps, monkeypatch):
+        """There the deployment output separates them, and the label would only be noise."""
+        monkeypatch.setattr(alakazam, "execute_cmds", REAL_EXECUTE_CMDS)
+        output = run(["-n", "-e"]).output
+        assert "upgrade-hooks for" not in output
+        assert "Run 'db update_mongo_version' in cloud.a.org:db" in output
+
+
+class TestAnUnhealthyApp:
+    """
+    An app that does not come back must not cost the apps that did their hooks: a run over twenty
+    apps would otherwise leave nineteen of them half migrated because of one.
+    """
+
+    @pytest.fixture
+    def sick(self, monkeypatch, steps):
+        monkeypatch.setattr(alakazam, "get_unhealthy_apps", lambda *a, **k: unhealthy("cloud.a.org"))
+        return steps
+
+    def invoke(self, args):
+        return CliRunner().invoke(alakazam.upgrade, ["-n"] + args, standalone_mode=False)
+
+    def test_the_healthy_app_still_gets_its_hooks(self, sick):
+        self.invoke(["-e"])
+        assert selected(sick, "hook:") == ["hook:boards.a.org"]
+
+    def test_the_unhealthy_app_does_not(self, sick):
+        """Its hook would reach a container that is not there."""
+        self.invoke(["-e"])
+        assert "hook:cloud.a.org" not in sick
+
+    def test_it_is_reported_and_ends_the_run(self, sick):
+        result = self.invoke(["-e"])
+        assert isinstance(result.exception, click.ClickException)
+        assert "1 of 2 app(s) did not become healthy: cloud.a.org" in result.exception.message
+
+    def test_a_health_check_that_reads_nothing_distrusts_every_app(self, steps, monkeypatch):
+        """Without a status for any of them, none of them can be told apart from a broken one."""
+        monkeypatch.setattr(alakazam, "get_unhealthy_apps", lambda *a, **k: (_ for _ in ()).throw(
+            click.ClickException("no apps are deployed, there is nothing to wait for")))
+        result = self.invoke(["-e"])
+        assert selected(steps, "hook:") == []
+        assert "boards.a.org" in result.exception.message
+        assert "cloud.a.org" in result.exception.message
+
+
+class TestAFailingUpgradeHook:
+    """
+    Reported but not a verdict: hooks still fail for reasons that are not failures, so they are
+    collected until those are sorted out and only then become something the run ends on.
+    """
+
+    @pytest.fixture
+    def broken(self, monkeypatch, steps):
+        def execute_cmds(config, dry_run=False, **kw):
+            if kw.get("pre_upgrade"):
+                return []
+            steps.append(f"hook:{config['app_domain']}")
+            return ["app occ upgrade"] if config["app_domain"] == "cloud.a.org" else []
+
+        monkeypatch.setattr(alakazam, "execute_cmds", execute_cmds)
+        return steps
+
+    def invoke(self, args):
+        return CliRunner().invoke(alakazam.upgrade, ["-n"] + args, standalone_mode=False)
+
+    def test_the_run_still_succeeds(self, broken):
+        result = self.invoke(["-e"])
+        assert result.exit_code == 0, result.exception
+
+    def test_it_is_reported(self, broken, caplog):
+        with caplog.at_level(logging.ERROR):
+            self.invoke(["-e"])
+        assert "1 upgrade hook(s) failed" in caplog.text
+        assert "cloud.a.org 'app occ upgrade'" in caplog.text
+
+    def test_the_other_apps_keep_their_hooks(self, broken):
+        self.invoke(["-e"])
+        assert selected(broken, "hook:") == ["hook:cloud.a.org", "hook:boards.a.org"]
+
+    def test_it_is_reported_next_to_a_failure_that_does_end_the_run(self, broken, monkeypatch, caplog):
+        monkeypatch.setattr(alakazam, "get_unhealthy_apps", lambda *a, **k: unhealthy("boards.a.org"))
+        with caplog.at_level(logging.ERROR):
+            result = self.invoke(["-e"])
+        assert "upgrade hook(s) failed" in caplog.text
+        assert "did not become healthy: boards.a.org" in result.exception.message