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

@@ -1759,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.
@@ -1768,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
@@ -1783,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']})
@@ -2085,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>',
@@ -2093,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 = []
@@ -2154,8 +2167,13 @@ 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
@@ -2169,6 +2187,10 @@ def upgrade(recipes: Tuple[str], execute_hooks: bool, dry_run: bool, redeploy: b
                 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')
@@ -2178,7 +2200,7 @@ def upgrade(recipes: Tuple[str], execute_hooks: bool, dry_run: bool, redeploy: b
                     sleep(redeploy_wait)
                 print(f'deploy {app_domain}')
                 print(abra("app", *upgrade_cmd))
-            except RuntimeError as error:
+            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}")
@@ -2239,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.
     """
@@ -2261,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()

tests/test_upgrade_hooks.py

@@ -12,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
@@ -35,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):
@@ -164,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:
@@ -247,3 +263,126 @@ class TestAFailingApp:
             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