feat(upgrade): add pre-upgrade-hooks that run before an app is replaced

README.md

@@ -262,6 +262,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
@@ -1670,7 +1759,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 = "") -> None:
     """
     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 +1768,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
@@ -1694,31 +1784,39 @@ def execute_cmds(app_config: Dict[str, Any], commands: Tuple[str] = tuple(), ini
     """
     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)
+    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():
+                run_local_script(tokens, domain, server, instance_domain, dry_run, strict)
+                continue
+            print(f"Run '{cmd}' in {domain}:{container}", flush=True)
+            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}")
 
 
 @click.group(context_settings={"help_option_names": ['-h', '--help']})
@@ -1996,6 +2094,8 @@ def deploy_apps(instance_apps: Dict[str, List[List]], execute_hooks: bool = Fals
 @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 +2104,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 +2167,65 @@ 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))
+        problems = []
+        if failed:
+            problems.append(
+                f"{len(failed)} of {len(upgrade_cmds)} app(s) failed to upgrade: {', '.join(failed)}")
+        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
+                wait_for_healthy_apps(tuple({app for app, *_ in upgraded}), timeout, settle=settle)
+            except click.ClickException as error:
+                # collected rather than raised, so that it cannot hide the apps that failed before
+                problems.append(error.message)
+            else:
+                if execute_hooks:
+                    for app, app_config, instance_domain in upgraded:
+                        logging.info(f"execute commands for {app_config.get('app_domain')}")
+                        execute_cmds(app_config, upgrade=True, instance_domain=instance_domain)
+        if problems:
+            raise click.ClickException("\n".join(problems))
 
 
 @cli.command()
@@ -2131,12 +2261,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 +2284,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 +2543,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
 

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 not config.get(key):
+            return
+        steps.append(f"{'show:' if dry_run else ''}{kind}:{config['app_domain']}")
+
+    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):
@@ -162,8 +178,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 +195,194 @@ 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_a_failing_wait_does_not_hide_the_failing_app(self, failing, monkeypatch):
+        """One failure covering up another is how a broken app slips through a green pipeline."""
+        monkeypatch.setattr(alakazam, "wait_for_healthy_apps", lambda *a, **k: (_ for _ in ()).throw(
+            click.ClickException("1 service(s) still not healthy after 900s")))
+        message = self.invoke(["-e"]).exception.message
+        assert "cloud.a.org" in message
+        assert "still not healthy" in message
+
+    def test_the_hooks_stay_out_when_the_wait_failed(self, failing, monkeypatch):
+        monkeypatch.setattr(alakazam, "wait_for_healthy_apps", lambda *a, **k: (_ for _ in ()).throw(
+            click.ClickException("1 service(s) still not healthy after 900s")))
+        self.invoke(["-e"])
+        assert selected(failing, "hook:") == []
+
+
+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']}")
+
+        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