diff --git a/src/tasks/deploy_tasks.py b/src/tasks/deploy_tasks.py index ac6ffd9..69ece78 100644 --- a/src/tasks/deploy_tasks.py +++ b/src/tasks/deploy_tasks.py @@ -212,6 +212,7 @@ def _provision_one_stack_assignment( total_stacks: int, cancel_check: Callable[[], bool] | None = None, preserved_user_json: dict | None = None, + on_stack_created: Callable[[str], None] | None = None, ) -> tuple[Optional[str], Optional[DeploymentInstance]]: """Run the create-stack → Ansible → persist-credentials cycle for one stack assignment. @@ -248,6 +249,14 @@ def _provision_one_stack_assignment( preserved_user_json: When set, skip credential GENERATION and reuse this user_json verbatim. Used by ``redeploy_instance`` with ``preserve_credentials=True`` so students keep their logins. + on_stack_created: Optional callback invoked with the new Heat + ``stack_id`` IMMEDIATELY after ``create_stack`` returns and + BEFORE the long-running Ansible phase starts. Callers use this + to persist the new stack id onto the parent deployment so a + worker crash mid-Ansible can't leave the Heat stack orphaned. + Failures inside the callback are logged and swallowed — + persistence is best-effort here; the caller's later final + commit is the source of truth. Returns: Tuple ``(stack_id, instance)``: @@ -313,6 +322,25 @@ def _provision_one_stack_assignment( ) stack_id: str = stack_result["stack_id"] + # Persist the new stack id onto the parent deployment IMMEDIATELY, + # before the long-running Ansible phase. Without this, a worker crash + # / OOM / pod restart mid-Ansible would leave the Heat stack alive in + # OpenStack but invisible to ``delete_deployment`` (which only walks + # ``deployment.openstack_stack_id``), creating an orphan stack with + # no DB pointer to clean it up. Failures here are best-effort: the + # caller still does a final commit on the way out which is the source + # of truth, and ``delete_deployment`` has a defensive fallback that + # also walks ``DeploymentInstance.openstack_server_id``. + if on_stack_created is not None: + try: + on_stack_created(stack_id) + except Exception as persist_err: + logger.warning( + f"on_stack_created callback failed for stack {stack_index} " + f"({stack_id}): {persist_err}", + exc_info=True, + ) + log_service.log( deployment_id=deployment_id, event_type=DeploymentLogEventType.STACK_CREATE, @@ -726,6 +754,23 @@ def _ansible_factory(*, floating_ip: str, cancel_check): stack_name = _build_stack_name(deployment, idx) cancel_check = lambda: is_cancel_requested(db, deployment_id) # noqa: E731 + def _persist_stack_id_now(new_id: str) -> None: + """Append ``new_id`` to ``created_stack_ids`` and flush + onto the deployment row immediately — invoked by the + helper right after ``heat_service.create_stack`` returns, + before Ansible. Closes the orphan-stack window that + would otherwise span the (multi-minute) Ansible phase. + """ + created_stack_ids.append(new_id) + try: + deployment.openstack_stack_id = json.dumps(created_stack_ids) + db.commit() + except Exception as persist_err: + db.rollback() + logger.warning( + f"Failed to incrementally persist stack id {new_id}: {persist_err}" + ) + stack_id, _instance = _provision_one_stack_assignment( db=db, deployment=deployment, @@ -740,16 +785,15 @@ def _ansible_factory(*, floating_ip: str, cancel_check): stack_index=idx, total_stacks=len(stack_assignments_raw), cancel_check=cancel_check, + on_stack_created=_persist_stack_id_now, ) - if stack_id: + # The callback above already appended + persisted ``stack_id``. + # Guard against an edge case where the helper returns a stack + # id WITHOUT having called the callback (e.g. future refactor + # that produces a stack id by another route): make sure we + # don't leave the id out of the snapshot. + if stack_id and stack_id not in created_stack_ids: created_stack_ids.append(stack_id) - - # Persist the new stack ID incrementally so a parallel cancel - # (DELETE request → delete_deployment task) can find every - # Heat stack we've created so far. Without this, the cleanup - # would miss stacks created in later loop iterations because - # `openstack_stack_id` is otherwise only flushed at the very - # end of the task. try: deployment.openstack_stack_id = json.dumps(created_stack_ids) db.commit() @@ -1003,6 +1047,79 @@ def _gc_orphan_student_memberships(db, user_ids: set[str]) -> None: ) +def _collect_stack_ids_for_cleanup(db, deployment) -> list[str]: + """Return every Heat stack id this deployment owns, drawing from BOTH + the deployment row and its DeploymentInstance rows. + + ``deployment.openstack_stack_id`` is a JSON array maintained by + ``deploy_stack`` / ``redeploy_instance`` as they go. The + ``on_stack_created`` callback persists each new id incrementally so + the array stays in sync with OpenStack, but a worker crash between + ``heat_service.create_stack`` and that commit would leave a fresh + Heat stack alive with no entry in the array. + + To make ``delete_deployment`` resilient to that exact failure mode, + we additionally walk ``DeploymentInstance.openstack_server_id`` + (which stores the same Heat stack id and is committed inside the + transaction that persists the instance row). The union of both + sources, deduplicated while preserving insertion order, is what we + actually pass to Heat for deletion. + + Args: + db: SQLAlchemy session bound to this task. + deployment: The Deployment row being torn down. Must be live in + ``db`` so the lazy-loaded ``instances`` relationship resolves. + + Returns: + Ordered list of unique Heat stack ids to delete. Empty when the + deployment never produced any stack (rare, but happens on early + failures). + """ + seen: set[str] = set() + out: list[str] = [] + + raw = deployment.openstack_stack_id + if raw: + try: + parsed = json.loads(raw) + if isinstance(parsed, list): + ids_from_deployment: list[str] = [str(s) for s in parsed if s] + else: + ids_from_deployment = [str(parsed)] + except (json.JSONDecodeError, TypeError): + # Treat as a single legacy id. + ids_from_deployment = [str(raw)] + for sid in ids_from_deployment: + if sid and sid not in seen: + seen.add(sid) + out.append(sid) + + # Fallback / defense-in-depth: any DeploymentInstance whose Heat stack + # id never made it into the deployment-level array (e.g. crash mid- + # Ansible on first redeploy of a wedged VM) still gets cleaned up. + try: + instances = db.query(DeploymentInstance).filter( + DeploymentInstance.deployment_id == deployment.id + ).all() + except Exception as e: + logger.warning( + f"Could not query DeploymentInstance rows for stack-id fallback: {e}" + ) + instances = [] + + for inst in instances: + sid = inst.openstack_server_id + if sid and sid not in seen: + seen.add(sid) + out.append(sid) + logger.info( + f"Stack id {sid} recovered from DeploymentInstance {inst.id} " + "(was missing from deployment.openstack_stack_id)" + ) + + return out + + @celery_app.task(bind=True) def delete_deployment(self, deployment_id: str) -> dict: """Delete a deployment's OpenStack resources and remove DB record. @@ -1066,9 +1183,20 @@ def delete_deployment(self, deployment_id: str) -> dict: if deployment is None: return {"status": "already_gone", "deployment_id": deployment_id} + # Build the full set of stack ids to tear down. Primary source is + # ``deployment.openstack_stack_id`` (JSON array), but we also walk + # ``DeploymentInstance.openstack_server_id`` as a defensive fallback: + # if a previous redeploy / deploy_stack ever crashed between + # ``heat_service.create_stack`` and the final commit that flushes + # the id onto the deployment row, the stack would otherwise be + # invisible to the cleanup here even though its DeploymentInstance + # row points at it. Union of both sources guarantees no orphan + # stacks survive a clean delete. + stack_ids = _collect_stack_ids_for_cleanup(db, deployment) + # If there is an associated Heat stack, attempt to delete it any_stack_delete_failed = False - if deployment.openstack_stack_id: + if stack_ids: try: # The OpenStack project is now persisted on the deployment row # itself (FK), so deletion always targets the project the @@ -1084,15 +1212,6 @@ def delete_deployment(self, deployment_id: str) -> dict: else: heat_service = HeatStackService(openstack_project) - # Parse stack IDs (can be single ID or JSON array) - try: - stack_ids = json.loads(deployment.openstack_stack_id) - if not isinstance(stack_ids, list): - stack_ids = [stack_ids] - except (json.JSONDecodeError, TypeError): - # Fallback: treat as single stack ID - stack_ids = [deployment.openstack_stack_id] - logger.info(f"Deleting {len(stack_ids)} Heat stack(s)") deleted_count = 0 surviving_stack_ids: list[str] = [] @@ -1914,6 +2033,29 @@ def _ansible_factory(*, floating_ip: str, cancel_check): stack_name = _build_redeploy_stack_name(deployment, stack_idx, instance_id) try: + def _persist_new_stack_id_now(new_id: str) -> None: + """Append the newly-created Heat stack id onto the parent + deployment's id list AS SOON AS Heat returns it — before + the multi-minute Ansible phase. Closes the orphan window: + without this, a worker crash mid-Ansible would leave the + fresh stack in OpenStack with no reference in + ``deployment.openstack_stack_id`` for ``delete_deployment`` + to find. ``delete_deployment`` has a defensive fallback + via ``DeploymentInstance.openstack_server_id`` as well, + but persisting eagerly here keeps that fallback as a + belt-and-braces measure rather than the only safety net. + """ + remaining_stack_ids.append(new_id) + try: + deployment.openstack_stack_id = json.dumps(remaining_stack_ids) + db.commit() + except Exception as persist_err: + db.rollback() + logger.warning( + f"Failed to incrementally persist redeployed stack id " + f"{new_id}: {persist_err}" + ) + new_stack_id, new_instance = _provision_one_stack_assignment( db=db, deployment=deployment, @@ -1929,6 +2071,7 @@ def _ansible_factory(*, floating_ip: str, cancel_check): total_stacks=1, cancel_check=None, preserved_user_json=preserved_user_json, + on_stack_created=_persist_new_stack_id_now, ) except Exception as e: logger.exception(f"Redeploy of instance {instance_id} failed: {e}") @@ -1943,9 +2086,11 @@ def _ansible_factory(*, floating_ip: str, cancel_check): # the Heat service (CREATE_FAILED / timeout) or by the credential # persist step (which keeps the stack id alive so the user can # retry). Add it back to the deployment so a future delete cleans - # up rather than silently leaking the stack. + # up rather than silently leaking the stack. The ``on_stack_created`` + # callback may have ALREADY appended it for the CREATE_COMPLETE- + # then-Ansible-failed path, so dedupe before persisting. orphan = getattr(e, "stack_id", None) - if orphan: + if orphan and orphan not in remaining_stack_ids: remaining_stack_ids.append(orphan) try: deployment.openstack_stack_id = json.dumps(remaining_stack_ids) @@ -1970,8 +2115,13 @@ def _ansible_factory(*, floating_ip: str, cancel_check): "error": str(e), } - # Persist the new stack id back onto the parent deployment. - if new_stack_id: + # Persist the new stack id back onto the parent deployment. The + # ``on_stack_created`` callback above already appended it right + # after Heat returned, so this block is a no-op in the normal + # flow. It stays as a safety net for the edge case where the + # callback was skipped or failed: we re-flush the in-memory + # ``remaining_stack_ids`` list so the on-disk JSON matches. + if new_stack_id and new_stack_id not in remaining_stack_ids: remaining_stack_ids.append(new_stack_id) try: deployment.openstack_stack_id = json.dumps(remaining_stack_ids) diff --git a/tests/unit/test_orphan_stack_safeguards.py b/tests/unit/test_orphan_stack_safeguards.py new file mode 100644 index 0000000..36d0439 --- /dev/null +++ b/tests/unit/test_orphan_stack_safeguards.py @@ -0,0 +1,303 @@ +"""Tests for the orphan-stack-id safeguards. + +Two related guarantees, tested independently: + +1. ``_provision_one_stack_assignment`` invokes the ``on_stack_created`` + callback IMMEDIATELY after ``heat_service.create_stack`` returns — + before the long-running Ansible phase. Without that callback, a + worker crash mid-Ansible would leave the Heat stack alive in + OpenStack with no entry in ``deployment.openstack_stack_id`` for + ``delete_deployment`` to find. + +2. ``_collect_stack_ids_for_cleanup`` walks BOTH ``deployment.openstack_stack_id`` + AND ``DeploymentInstance.openstack_server_id`` and returns the union + (deduplicated, insertion order preserved). This is the defensive + fallback for any orphan that slipped past safeguard #1 — a redeploy + from a previous deploy build that didn't have it. + +These behaviors are policy, not implementation detail — both have to +hold for the orphan-stack guarantee to be real. +""" +import json +from types import SimpleNamespace +from unittest.mock import MagicMock, patch + +from src.tasks import deploy_tasks + + +# --------------------------------------------------------------------------- +# on_stack_created — fires immediately after create_stack +# --------------------------------------------------------------------------- + + +def _make_template_context(): + """Minimal context the helper needs. ``split_parameters`` returns + empty Heat + Ansible dicts so we don't have to thread real params.""" + ctx = MagicMock() + ctx.split_parameters.return_value = ({}, {}) + ctx.heat_template = "heat: {}" + ctx.files_dict = {} + ctx.credentials_spec = {"per_group": [], "teacher": []} + ctx.playbooks = [] # disables the Ansible branch + ctx.scripts = {} + ctx.template_files = {} + return ctx + + +def _make_deployment_row(): + return SimpleNamespace( + id="dep-1", + name="dep", + course_id="course-1", + template_version_id="tv-1", + ) + + +def _make_stack_assignment_data(): + return { + "stack_index": 1, + "groups": [{ + "group_name": "G1", "group_index": 1, + "students": [], "course_group_id": "grp-1", + }], + } + + +def _teacher_info(): + return { + "id": "kc-teacher", "username": "prof", + "email": "p@x.de", "first_name": "Prof", "last_name": "X", + } + + +def test_on_stack_created_fires_before_credential_persistence(): + """The callback must run AS SOON AS Heat returns a stack id — before + the credential-persistence call that creates the DeploymentInstance + row, and before Ansible. We assert the order via a shared list.""" + events: list[str] = [] + + heat = MagicMock() + def _create_stack(**_kwargs): + events.append("heat.create_stack") + return {"stack_id": "stack-NEW", "floating_ip": "1.2.3.4", "outputs": {}} + heat.create_stack.side_effect = _create_stack + + fake_instance = MagicMock(id="inst-NEW") + persist_mock = MagicMock(return_value=fake_instance) + def _persist(**_kwargs): + events.append("credentials.persist") + return fake_instance + persist_mock.side_effect = _persist + + def _on_stack_created(sid: str) -> None: + events.append(f"callback({sid})") + + db = MagicMock() + + with ( + patch.object( + deploy_tasks.DeploymentCredentialService, + "persist_credentials_for_stack", + persist_mock, + ), + patch.object( + deploy_tasks.CredentialGeneratorService, + "generate", + return_value={"deployment_groups": [], "teacher": {}}, + ), + patch.object(deploy_tasks, "get_settings", return_value=SimpleNamespace( + ansible_ssh_private_key="", ansible_ssh_key_name="kp", + )), + ): + stack_id, instance = deploy_tasks._provision_one_stack_assignment( + db=db, + deployment=_make_deployment_row(), + stack_assignment_data=_make_stack_assignment_data(), + template_context=_make_template_context(), + heat_service=heat, + ansible_service_factory=lambda **kw: MagicMock(), + log_service=MagicMock(), + all_parameters={}, + teacher_info=_teacher_info(), + stack_name="dep-s1-abcd", + stack_index=1, + total_stacks=1, + on_stack_created=_on_stack_created, + ) + + assert stack_id == "stack-NEW" + assert instance is fake_instance + # The callback ran AFTER Heat but BEFORE credentials.persist (which + # is itself before Ansible — Ansible is gated out by empty playbooks). + # If a future refactor moves credential persistence above the callback + # this assertion catches it: the callback must close the orphan + # window first. + assert events.index("callback(stack-NEW)") < events.index("credentials.persist") + assert events.index("heat.create_stack") < events.index("callback(stack-NEW)") + + +def test_on_stack_created_failure_is_swallowed_not_fatal(): + """A failing callback must not abort the provisioning — Ansible / + credential persistence are the actual contract, the callback is a + best-effort safety net. The helper logs and continues.""" + heat = MagicMock() + heat.create_stack.return_value = {"stack_id": "stack-NEW", "floating_ip": "1.2.3.4", "outputs": {}} + + fake_instance = MagicMock(id="inst-NEW") + with ( + patch.object( + deploy_tasks.DeploymentCredentialService, + "persist_credentials_for_stack", + return_value=fake_instance, + ), + patch.object( + deploy_tasks.CredentialGeneratorService, + "generate", + return_value={"deployment_groups": [], "teacher": {}}, + ), + patch.object(deploy_tasks, "get_settings", return_value=SimpleNamespace( + ansible_ssh_private_key="", ansible_ssh_key_name="kp", + )), + ): + stack_id, instance = deploy_tasks._provision_one_stack_assignment( + db=MagicMock(), + deployment=_make_deployment_row(), + stack_assignment_data=_make_stack_assignment_data(), + template_context=_make_template_context(), + heat_service=heat, + ansible_service_factory=lambda **kw: MagicMock(), + log_service=MagicMock(), + all_parameters={}, + teacher_info=_teacher_info(), + stack_name="dep-s1-abcd", + stack_index=1, + total_stacks=1, + on_stack_created=lambda _sid: (_ for _ in ()).throw(RuntimeError("db down")), + ) + + # Provisioning still succeeded. + assert stack_id == "stack-NEW" + assert instance is fake_instance + + +def test_on_stack_created_omitted_is_allowed(): + """The callback is optional — when None, the helper just doesn't + invoke it. Used by code paths that don't need eager persistence + (or by tests that don't care).""" + heat = MagicMock() + heat.create_stack.return_value = {"stack_id": "stack-X", "floating_ip": "", "outputs": {}} + + fake_instance = MagicMock(id="inst-X") + with ( + patch.object( + deploy_tasks.DeploymentCredentialService, + "persist_credentials_for_stack", + return_value=fake_instance, + ), + patch.object( + deploy_tasks.CredentialGeneratorService, + "generate", + return_value={"deployment_groups": [], "teacher": {}}, + ), + patch.object(deploy_tasks, "get_settings", return_value=SimpleNamespace( + ansible_ssh_private_key="", ansible_ssh_key_name="kp", + )), + ): + stack_id, _ = deploy_tasks._provision_one_stack_assignment( + db=MagicMock(), + deployment=_make_deployment_row(), + stack_assignment_data=_make_stack_assignment_data(), + template_context=_make_template_context(), + heat_service=heat, + ansible_service_factory=lambda **kw: MagicMock(), + log_service=MagicMock(), + all_parameters={}, + teacher_info=_teacher_info(), + stack_name="dep-s1-abcd", + stack_index=1, + total_stacks=1, + # on_stack_created omitted on purpose + ) + assert stack_id == "stack-X" + + +# --------------------------------------------------------------------------- +# _collect_stack_ids_for_cleanup — union with DeploymentInstance fallback +# --------------------------------------------------------------------------- + + +def _make_db_with_instances(*instances): + """Build a MagicMock db whose ``query(DeploymentInstance).filter(...).all()`` + returns the given instance stubs.""" + db = MagicMock() + chain = MagicMock() + chain.filter.return_value.all.return_value = list(instances) + db.query.return_value = chain + return db + + +def test_collect_stack_ids_unions_deployment_and_instance_sources(): + """Stack ids from the deployment row come first; instance rows + contribute any additional ids that aren't already in the array. + The exact dedup-while-preserving-order is what makes the fallback + safe to combine with the primary source.""" + dep = SimpleNamespace( + id="dep-1", + openstack_stack_id=json.dumps(["A", "B"]), + ) + instances = [ + SimpleNamespace(id="inst-1", openstack_server_id="B"), # dup + SimpleNamespace(id="inst-2", openstack_server_id="C"), # new! + SimpleNamespace(id="inst-3", openstack_server_id=None), # ignored + ] + db = _make_db_with_instances(*instances) + + out = deploy_tasks._collect_stack_ids_for_cleanup(db, dep) + assert out == ["A", "B", "C"] + + +def test_collect_stack_ids_handles_null_openstack_stack_id(): + """Deployment with no array but with one instance row whose + openstack_server_id IS set — that single id must still survive + (this is exactly the orphan scenario the fallback exists for).""" + dep = SimpleNamespace(id="dep-1", openstack_stack_id=None) + instances = [SimpleNamespace(id="inst-1", openstack_server_id="orphan-X")] + db = _make_db_with_instances(*instances) + + assert deploy_tasks._collect_stack_ids_for_cleanup(db, dep) == ["orphan-X"] + + +def test_collect_stack_ids_handles_empty_json_array(): + """The dangerous-but-real shape: redeploy left an empty list after + deleting the OLD stack but before persisting the NEW one. Instance + row still has the new id.""" + dep = SimpleNamespace(id="dep-1", openstack_stack_id="[]") + instances = [SimpleNamespace(id="inst-1", openstack_server_id="new-stack-id")] + db = _make_db_with_instances(*instances) + + assert deploy_tasks._collect_stack_ids_for_cleanup(db, dep) == ["new-stack-id"] + + +def test_collect_stack_ids_handles_legacy_single_string(): + """Older rows persisted a bare stack id, not a JSON array. Treat as + a single id, don't error out.""" + dep = SimpleNamespace(id="dep-1", openstack_stack_id="legacy-id") + db = _make_db_with_instances() + + assert deploy_tasks._collect_stack_ids_for_cleanup(db, dep) == ["legacy-id"] + + +def test_collect_stack_ids_returns_empty_when_no_sources(): + dep = SimpleNamespace(id="dep-1", openstack_stack_id=None) + db = _make_db_with_instances() + assert deploy_tasks._collect_stack_ids_for_cleanup(db, dep) == [] + + +def test_collect_stack_ids_swallows_instance_query_errors(): + """A broken instances relationship must not crash the cleanup — the + primary source (deployment.openstack_stack_id) still gets returned.""" + dep = SimpleNamespace(id="dep-1", openstack_stack_id=json.dumps(["A"])) + db = MagicMock() + db.query.side_effect = RuntimeError("db down") + + assert deploy_tasks._collect_stack_ids_for_cleanup(db, dep) == ["A"]