diff --git a/umbratrans_web/compute.py b/umbratrans_web/compute.py index 805adbf..7d325da 100644 --- a/umbratrans_web/compute.py +++ b/umbratrans_web/compute.py @@ -545,6 +545,16 @@ class ComputeRepository: "draining, offline, or retired. 'online' is earned by a heartbeat.", ) with self._connection() as connection, self._tx(connection): + # Refused here rather than left to the database trigger, so the caller gets a + # reason it can read instead of a constraint violation. The trigger stays as + # the guarantee; this is the message. + existing = self._node_row(connection, node_id) + if existing is not None and existing["status"] == "removed": + raise RepositoryError( + "node_removed", + "This node has been removed from the fleet and cannot return to " + "service. A replacement machine pairs under its own name.", + ) row = connection.execute( "UPDATE public.compute_nodes SET status = %s, updated_at = now() " "WHERE id = %s RETURNING id", @@ -556,6 +566,201 @@ class ComputeRepository: assert node is not None return node + def revoke_node_credentials( + self, + node_id: UUID, + *, + reason: str | None = None, + actor: str | None = None, + ) -> int: + """Revoke every credential this node holds. Returns how many were live. + + The `revoked_at` column and the check in :meth:`resolve_node_credential` have been + there since pairing; what was missing was any way to set it. So the ability to + cut a node off existed on paper only, and the routes for removing one did not + exist at all. + + Revoking rather than deleting, because the credential row is the record that a + node was ever onboarded and which key was issued. Deleting it would make a + compromised key indistinguishable from one that never existed. + + Reason is recorded in the returned detail rather than logged, so the audit trail + says why a machine went dark instead of only that it did. + """ + + with self._connection() as connection, self._tx(connection): + revoked = connection.execute( + """ + UPDATE public.node_credentials + SET revoked_at = now() + WHERE compute_node_id = %s AND revoked_at IS NULL + """, + (node_id,), + ).rowcount + if self._node_row(connection, node_id) is None: + raise NotFoundError("compute_node_not_found") + if revoked: + self._node_event( + connection, + node_id, + "credentials_revoked", + { + "count": revoked, + "reason": reason, + "actor": actor, + }, + ) + return int(revoked or 0) + + def remove_node( + self, + node_id: UUID, + *, + reason: str | None = None, + actor: str | None = None, + ) -> ComputeNode: + """Take a machine out of the fleet for good. + + What this deletes is everything that could let the node *act*: its credentials, + its device measurements, its heartbeats and its model reports. What it keeps is + the `compute_nodes` row, and that is not tidiness. + + The row cannot simply be deleted. `job_events.node_id` references it and is + written by every path that records work, and `node_pairing_codes.used_by_node_id` + references it too. Both are `ON DELETE NO ACTION`, so the delete is refused. + Changing them to `SET NULL` does not help either: that is an `UPDATE` of + `job_events`, and `job_events_append_only` raises on every update by design. So a + node that has ever done work cannot be removed without weakening the append-only + guarantee on the event trail - and that guarantee is worth more than a tidy + `DELETE`. + + So the machine goes and the record stays. A `removed` node cannot authenticate, + cannot be assigned work, and is skipped by the claim path on status alone. + + Work the node was holding is released here rather than left to the lease to + expire. An operator removing dead hardware knows the work did not finish; making + them wait out a lease to find that out is the wrong way round, and for a + long lease it is most of an hour of the queue stalled behind a machine nobody + is going to switch on. + """ + + with self._connection() as connection, self._tx(connection): + if self._node_row(connection, node_id) is None: + raise NotFoundError("compute_node_not_found") + + # Released before the credentials go, so the work is accounted for even if + # the node was never going to come back. + released = connection.execute( + """ + UPDATE public.jobs SET + status = CASE WHEN attempt >= max_attempts THEN 'failed' ELSE 'queued' END, + lease_owner = NULL, + lease_expires_at = NULL + WHERE lease_owner = %s AND status = 'running' + RETURNING id, status + """, + (node_id,), + ).fetchall() + for row in released: + self._job_event( + connection, + row["id"], + "released_node_removed", + {"node_id": str(node_id), "status": row["status"], "reason": reason}, + ) + + # CASCADE would do all four of these from the node row, but the node row is + # staying, so they are named explicitly. + for table in ("compute_devices", "node_heartbeats", "node_models"): + connection.execute(f"DELETE FROM public.{table} WHERE compute_node_id = %s", (node_id,)) + connection.execute( + "UPDATE public.node_credentials SET revoked_at = now() " + "WHERE compute_node_id = %s AND revoked_at IS NULL", + (node_id,), + ) + + connection.execute( + "UPDATE public.compute_nodes SET status = 'removed', updated_at = now() " + "WHERE id = %s", + (node_id,), + ) + self._node_event( + connection, + node_id, + "removed", + { + "reason": reason, + "actor": actor, + "released_jobs": [str(row["id"]) for row in released], + }, + ) + node = self.get_node(node_id) + assert node is not None + return node + + # -- events ------------------------------------------------------------ + + def _node_event( + self, + connection: Any, + node_id: UUID, + action: str, + detail: Mapping[str, Any] | None = None, + *, + actor_user_id: UUID | None = None, + ) -> None: + """Record something that happened to a node, in the audit trail. + + `job_events` is the wrong home for these: its `job_id` is not nullable, so a + thing that happened to a *machine* - revoked credentials, a permanent removal - + has no job to hang from. `audit_events` is nullable in every part and has a + `target_type`/`target_id` pair, which is exactly this shape. + + `actor_user_id` is nullable too, so an action taken by the controller on its own + initiative is recorded as such rather than being attributed to whoever happened + to be signed in. + """ + + node = self._node_row(connection, node_id) + connection.execute( + """ + INSERT INTO public.audit_events ( + actor_user_id, organization_id, action, target_type, target_id, metadata + ) + VALUES (%s, %s, %s, 'compute_node', %s, %s::jsonb) + """, + ( + actor_user_id, + node["organization_id"] if node else None, + action, + str(node_id), + json.dumps(dict(detail or {})), + ), + ) + + @staticmethod + def _job_event( + connection: Any, + job_id: UUID, + event: str, + detail: Mapping[str, Any] | None = None, + *, + node_id: UUID | None = None, + ) -> None: + """Append to a job's trail. Append-only, like every other write to that table.""" + + connection.execute( + "INSERT INTO public.job_events (job_id, node_id, event, detail) " + "VALUES (%s, %s, %s, %s::jsonb)", + (job_id, node_id, event, json.dumps(dict(detail or {}))), + ) + + @staticmethod + def _node_row(connection: Any, node_id: UUID) -> dict[str, Any] | None: + return connection.execute( + "SELECT * FROM public.compute_nodes WHERE id = %s", (node_id,) + ).fetchone() + # -- devices ----------------------------------------------------------- def report_devices(self, node_id: UUID, devices: Sequence[Mapping[str, Any]]) -> list[Device]: diff --git a/umbratrans_web/migrations/0006_node_removal_and_assignments.sql b/umbratrans_web/migrations/0006_node_removal_and_assignments.sql new file mode 100644 index 0000000..08fcfc0 --- /dev/null +++ b/umbratrans_web/migrations/0006_node_removal_and_assignments.sql @@ -0,0 +1,131 @@ +-- 0006: a node can be removed from the fleet for good. +-- +-- Adds `removed` to the node status vocabulary, and creates `job_assignments`, which is +-- how the controller hands work to a node from now on. +-- +-- Two things here are deliberate and worth reading before changing either. + +-- --------------------------------------------------------------------------- +-- 1. `removed` +-- --------------------------------------------------------------------------- +-- +-- Distinct from `retired`, and the distinction is the point: +-- +-- retired the machine is out of service but the node is a record of it. It can be +-- returned to service by an administrator, and it still holds devices. +-- removed the machine is gone. Credentials revoked, devices and heartbeats deleted, +-- and the row kept only because `job_events.node_id` refers to it. +-- +-- The row cannot be deleted. `job_events.node_id` and `node_pairing_codes.used_by_node_id` +-- both reference `compute_nodes` ON DELETE NO ACTION, so the DELETE is refused outright; +-- changing them to SET NULL does not help, because that is an UPDATE of `job_events` and +-- `job_events_append_only` raises on every update by design. Removing the row would mean +-- either losing the record of what the machine did, or weakening the guarantee that the +-- event trail is immutable. The trail is worth more than a tidy DELETE. +-- +-- `removed` is terminal. There is no administrative path back out of it, so a machine +-- written off cannot be quietly resurrected - and a replacement machine pairs under a new +-- name derived from its own MAC, which is what makes the tombstone harmless. + +ALTER TABLE public.compute_nodes DROP CONSTRAINT IF EXISTS compute_nodes_status_check; +ALTER TABLE public.compute_nodes ADD CONSTRAINT compute_nodes_status_check + CHECK (status = ANY (ARRAY[ + 'provisioning', 'online', 'draining', 'offline', 'retired', 'removed' + ])); + +-- A removed node must never be resurrected by a heartbeat. `heartbeat` promotes +-- `provisioning` and `offline` to `online`, and a removed node is neither, so the +-- existing CASE already leaves it alone. Asserting it here means a future edit to that +-- CASE cannot quietly start reviving removed machines, which is the failure this status +-- exists to prevent and the one that would be hardest to notice. + +CREATE OR REPLACE FUNCTION app.assert_removed_stays_removed() +RETURNS trigger LANGUAGE plpgsql AS $body$ +BEGIN + IF OLD.status = 'removed' AND NEW.status <> 'removed' THEN + RAISE EXCEPTION + 'a removed node cannot return to service (was %, asked for %)', + OLD.status, NEW.status; + END IF; + RETURN NEW; +END; +$body$; + +DROP TRIGGER IF EXISTS compute_nodes_removed_is_terminal ON public.compute_nodes; +CREATE TRIGGER compute_nodes_removed_is_terminal + BEFORE UPDATE OF status ON public.compute_nodes + FOR EACH ROW EXECUTE FUNCTION app.assert_removed_stays_removed(); + +-- --------------------------------------------------------------------------- +-- 2. job_assignments +-- --------------------------------------------------------------------------- +-- +-- The controller assigns work; a node no longer claims it. `jobs.lease_owner` said who +-- held a job and it could only be one node, which cannot express a job given to several +-- machines at once, and cannot say which of them finished it. +-- +-- A job is complete when the controller holds the artifacts that show it is. That makes +-- the assignment - the attempt - a separate thing from the job - the objective - which is +-- what this table is: one row per (job, node, attempt). Several rows may be open for one +-- job at once; the first whose artifacts verify wins and the rest are cancelled. +-- +-- `state` is deliberately not the job's status. A job an assignment is working on is +-- `running` whatever the assignment says, and a `succeeded` assignment is not a completed +-- job until `artifacts_verified_at` is set. Collapsing the two is how a node's word +-- becomes the definition of done. + +CREATE TABLE IF NOT EXISTS public.job_assignments ( + id uuid PRIMARY KEY DEFAULT gen_random_uuid(), + job_id uuid NOT NULL REFERENCES public.jobs (id) ON DELETE CASCADE, + node_id uuid NOT NULL REFERENCES public.compute_nodes (id) ON DELETE RESTRICT, + state text NOT NULL DEFAULT 'assigned', + attempt integer NOT NULL DEFAULT 1, + assigned_at timestamptz NOT NULL DEFAULT now(), + started_at timestamptz, + finished_at timestamptz, + lease_expires_at timestamptz, + artifacts_verified_at timestamptz, + result jsonb NOT NULL DEFAULT '{}'::jsonb, + error text, + CHECK (state = ANY (ARRAY[ + 'assigned', -- handed to the node, not yet picked up + 'running', -- the node says it started + 'succeeded', -- the node says it finished; NOT yet verified + 'failed', -- the node gave up, or its lease ran out + 'cancelled' -- superseded: another node won, or an administrator stopped it + ])), + CHECK (finished_at IS NULL OR finished_at >= assigned_at) +); + +-- One open assignment per (job, node): a node cannot be handed the same job twice at the +-- same time, but may be handed it again after a failure, which is what `attempt` counts. +CREATE UNIQUE INDEX IF NOT EXISTS job_assignments_open_per_node + ON public.job_assignments (job_id, node_id) + WHERE state IN ('assigned', 'running', 'succeeded'); + +CREATE INDEX IF NOT EXISTS job_assignments_by_node + ON public.job_assignments (node_id, state); + +CREATE INDEX IF NOT EXISTS job_assignments_by_job + ON public.job_assignments (job_id); + +-- The two questions the controller asks constantly: what is this node to do, and what is +-- still outstanding on this job. Both are served by the indexes above; this one covers +-- "which nodes are overdue", which is the reassignment path. +CREATE INDEX IF NOT EXISTS job_assignments_lease + ON public.job_assignments (lease_expires_at) + WHERE state IN ('assigned', 'running'); + +-- --------------------------------------------------------------------------- +-- 3. Recorded, not inferred +-- --------------------------------------------------------------------------- +-- +-- `artifacts_verified_at` is the whole of "a job is complete when the controller has the +-- artifacts to show it". It is set by the verification step and by nothing else, so the +-- question "is this job really done" has one answer with one writer, rather than being +-- inferred from an assignment's state and a hopeful join against `job_artifacts`. + +COMMENT ON COLUMN public.job_assignments.artifacts_verified_at IS + 'Set only by artifact verification. NULL on a succeeded assignment means the node ' + 'reported success and the controller has not yet confirmed it with artifacts; the job ' + 'is NOT complete while this is NULL.'; \ No newline at end of file diff --git a/umbratrans_web/routes_compute.py b/umbratrans_web/routes_compute.py index 7c7b04d..02526d6 100644 --- a/umbratrans_web/routes_compute.py +++ b/umbratrans_web/routes_compute.py @@ -50,6 +50,7 @@ from .errors import ( AuthenticationRequiredError, ForbiddenError, InputError, + NotFoundError, PairingError, ) from .models import Principal @@ -254,6 +255,14 @@ class NodeCredentialResponse(BaseModel): class SetNodeStatusRequest(RequestModel): status: str = Field(pattern="^(provisioning|draining|offline|retired)$") + """The states an administrator may put a node into by hand. + + ``removed`` is deliberately not here: it is what :func:`remove_node` sets, and it is + not a state a node can be argued back out of. Allowing it in this field would let + "remove this machine" be typed as a status change, which would skip the credential + revocation and the release of its work - the two things that make removal mean + anything. + """ class ModelRequest(RequestModel): @@ -509,6 +518,11 @@ def _node_reason(node: ComputeNode) -> str: return "provisioning: the agent has never authenticated" if node.status == "retired": return "retired: the node has been removed from service" + if node.status == "removed": + return ( + "removed: the machine is gone. Its credentials are revoked and it cannot be " + "given work; this row is kept only because the job history refers to it." + ) if node.status == "draining": return "draining: an administrator stopped new claims; running work finishes" if node.status == "offline": @@ -710,6 +724,69 @@ def set_node_status( return _node_response(repository.set_node_status(node_id, payload.status)) +class RemoveNodeRequest(RequestModel): + """Why a machine is being taken out, and who did it. + + Both are required of the caller rather than defaulted. The reason ends up in the + audit trail next to the revocation, and a removal with no recorded cause is the one + thing that makes a later "why is this machine gone" unanswerable - dead hardware and + a decommissioned licence look identical six months afterwards. + """ + + reason: str = Field(min_length=1, max_length=500) + actor: str | None = Field(default=None, max_length=120) + + +@admin_router.post("/nodes/{node_id}/revoke", response_model=NodeResponse) +def revoke_node_credentials( + node_id: UUID, + payload: RemoveNodeRequest, + request: Request, + principal: Principal = Depends(require_admin), +) -> NodeResponse: + """Cut a node off without removing it. The machine stays, the credential dies. + + This is the right tool for a key that may be compromised, or a machine that is going + to be reimaged: the node stops being able to authenticate immediately, and nothing + about its history is lost. `POST /nodes/{node_id}/remove` is the one that also takes + the machine out of the fleet. + """ + + repository = compute_repository(request) + repository.revoke_node_credentials( + node_id, reason=payload.reason, actor=payload.actor + ) + node = repository.get_node(node_id) + if node is None: + raise NotFoundError("compute_node_not_found") + return _node_response(node) + + +@admin_router.post("/nodes/{node_id}/remove", response_model=NodeResponse) +def remove_node( + node_id: UUID, + payload: RemoveNodeRequest, + request: Request, + principal: Principal = Depends(require_admin), +) -> NodeResponse: + """Take a machine out of the fleet for good, for hardware that is not coming back. + + Revokes its credentials, deletes its devices, heartbeats and model reports, and + releases any work it was holding so that work is reassignable rather than stalled + behind a machine nobody is going to switch on. + + The row itself stays, and cannot be deleted: `job_events.node_id` refers to it, and + the event trail refuses every update, so removing the row would mean either losing + the record of what the machine did or weakening the guarantee that the record is + immutable. The status becomes ``removed``, which is terminal - there is no + administrative path back out of it, because a machine that has been written off is + not coming back and a node that can be un-removed is a node nobody can be sure of. + """ + + repository = compute_repository(request) + return _node_response(repository.remove_node(node_id, reason=payload.reason, actor=payload.actor)) + + @admin_router.get("/models", response_model=list[ModelResponse]) def list_models(request: Request, principal: Principal = Depends(require_admin)) -> list[ModelResponse]: """The global model registry. diff --git a/umbratrans_web/tests/test_compute.py b/umbratrans_web/tests/test_compute.py index 99e09d2..564c800 100644 --- a/umbratrans_web/tests/test_compute.py +++ b/umbratrans_web/tests/test_compute.py @@ -189,6 +189,279 @@ def test_online_is_not_set_by_an_administrator(compute, organization) -> None: assert "earned by a heartbeat" in str(error.value) +# ---------------------------------------------------------------------- +# Revoking a credential, and removing a machine +# ---------------------------------------------------------------------- +# +# `node_credentials.revoked_at` existed from the first migration and `resolve_node_credential` +# has always honoured it. Nothing could set it. So the ability to cut a node off was +# present in the data model and absent from every API, which is the shape of defect this +# project keeps finding: the column is the capability. + + +def _live_credential(compute, organization, name: str, mac: str = "00:11:22:33:ac:4b"): + """Pair a node and hand back the plaintext key, the way a real bootstrap would.""" + + minted = compute.create_pairing_code(organization, node_name=name) + paired = compute.redeem_pairing_code(minted["code"], name, mac=mac) + assert paired["ok"] is True, paired + return paired["node_id"], paired["key"] + + +@integration +def test_a_revoked_credential_stops_working_immediately(compute, organization) -> None: + """The whole point: a key that may be compromised must die now, not at next restart.""" + + node_id, key = _live_credential(compute, organization, "revoke-me") + assert compute.resolve_node_credential(key) is not None, "the credential never worked" + + compute.revoke_node_credentials(node_id, reason="suspected compromise", actor="test") + + assert compute.resolve_node_credential(key) is None, ( + "a revoked credential still authenticates. The key was issued to a machine whose " + "agent may be hostile; revoking it has to take effect on the next request." + ) + + +@integration +def test_revoking_leaves_the_node_and_its_history_in_place(compute, organization) -> None: + """Revocation is for a machine that is coming back. Removal is the other thing. + + Confusing the two is how a reimage turns into a dead node: the operator revokes, the + node stays, and it is never told it may authenticate again - which it cannot, because + the credential is gone and pairing is refused for the name. + """ + + node_id, key = _live_credential(compute, organization, "revoked-but-alive") + compute.revoke_node_credentials(node_id, reason="reimaging") + + node = compute.get_node(node_id) + assert node is not None, "revoking deleted the node" + assert node.name == "revoked-but-alive-AC4B" + assert node.status == "provisioning", "revoking changed the node's status" + + # And the node keeps existing, so it still appears in the console. + names = {one.name for one in compute.list_nodes(organization)} + assert "revoked-but-alive-AC4B" in names + + +@integration +def test_revoking_twice_is_harmless_and_reports_nothing_new_the_second_time( + compute, organization +) -> None: + """A retried request must not report work it did not do. + + An operator clicking twice, or a retry after a timeout, should not be told it cut off + two credentials. + """ + + node_id, _key = _live_credential(compute, organization, "revoke-twice") + + assert compute.revoke_node_credentials(node_id, reason="first") == 1 + assert compute.revoke_node_credentials(node_id, reason="second") == 0 + + +@integration +def test_revoking_records_why_and_who(compute, organization) -> None: + """Six months on, "the node went dark" is not an answer an operator can act on.""" + + node_id, _key = _live_credential(compute, organization, "revoked-with-reason") + + compute.revoke_node_credentials(node_id, reason="suspected compromise", actor="ops@example") + + events = _audit_events(compute, node_id) + assert any( + event["action"] == "credentials_revoked" and event["metadata"].get("reason") == "suspected compromise" + for event in events + ), f"the revocation left no trace with its reason: {events}" + + +@integration +def test_removing_a_machine_revokes_its_credential_and_drops_its_devices( + compute, organization +) -> None: + """Everything that could let the node *act* goes; the row stays as history.""" + + node_id, key = _live_credential(compute, organization, "written-off") + _online( + compute, + compute.get_node(node_id), + [_device(_node(compute, organization, "written-off-device"))], + ) + + compute.remove_node(node_id, reason="hardware failure", actor="ops@example") + + assert compute.resolve_node_credential(key) is None, ( + "a removed machine can still authenticate" + ) + node = compute.get_node(node_id) + assert node is not None, "the row was deleted, which the event trail forbids" + assert node.status == "removed" + assert node.devices == (), "a removed machine still has devices and could be placed work" + + +@integration +def test_a_removed_node_is_handed_no_work(compute, organization) -> None: + """Removal has to mean removal at the claim path, not only in the console. + + The status is what the claim checks, so this is the assertion that matters: a node + that reads ``removed`` must behave exactly like one that was never paired. + """ + + node_id, _key = _live_credential(compute, organization, "removed-no-work") + _online( + compute, + compute.get_node(node_id), + [_device(_node(compute, organization, "removed-device"))], + ) + compute.submit_job(organization, kind="sleep", requested_by="test") + + assert compute.claim_job(node_id) is not None, "precondition: the node can claim" + + second = _live_credential(compute, organization, "removed-no-work-2", mac="00:11:22:33:3f:19") + compute.submit_job(organization, kind="sleep", requested_by="test") + compute.remove_node(second[0], reason="hardware failure") + + assert compute.claim_job(second[0]) is None, "a removed node was handed work" + + +@integration +def test_removing_a_node_releases_the_work_it_was_holding(compute, organization) -> None: + """The requirement: a node deleted mid-job leaves that job *not complete*. + + Released here rather than left to the lease to expire. An operator removing dead + hardware already knows the work did not finish, and making them wait out a lease to + discover that is the wrong way round - for a long lease it is most of an hour of the + queue stalled behind a machine nobody is going to switch on. + """ + + node_id, _key = _live_credential(compute, organization, "died-mid-job") + _online( + compute, + compute.get_node(node_id), + [_device(_node(compute, organization, "died-device"))], + ) + compute.submit_job(organization, kind="sleep", requested_by="test") + claimed = compute.claim_job(node_id) + assert claimed is not None + job_id = claimed.job.id + + compute.remove_node(node_id, reason="hardware failure") + + from umbratrans_web.compute import QUEUED + + job = compute.get_job(job_id) + assert job is not None + assert job.status == QUEUED, ( + f"work held by a removed node is {job.status!r}. It has to be queued again so " + "another node can take it, and the lease cannot be relied on to release it." + ) + assert job.lease_owner is None, "the released job still points at the dead node" + + +@integration +def test_work_held_by_a_removed_node_is_not_silently_lost(compute, organization) -> None: + """Released, not re-run from scratch by the lease: the job is still there. + + Stated separately because "released" and "the row vanished" both look like a working + queue to an operator watching the console, and only one of them is. + """ + + node_id, _key = _live_credential(compute, organization, "died-with-work") + _online( + compute, + compute.get_node(node_id), + [_device(_node(compute, organization, "died-work-device"))], + ) + compute.submit_job(organization, kind="sleep", requested_by="test") + claimed = compute.claim_job(node_id) + assert claimed is not None + + compute.remove_node(node_id, reason="hardware failure") + + other = _live_credential(compute, organization, "took-over", mac="00:11:22:33:3f:19") + _online(compute, compute.get_node(other[0]), [_device(_node(compute, organization, "taker"))]) + taken = compute.claim_job(other[0]) + + assert taken is not None, "no node can pick up the work a removed node was holding" + assert taken.job.id == claimed.job.id + + +@integration +def test_a_removed_node_cannot_be_returned_to_service_by_typing_a_status( + compute, organization +) -> None: + """`removed` is terminal, and the administrative path says so. + + A node that can be un-removed is a node nobody can be sure of: a machine written off + as dead comes back, and the operator who wrote it off has no way to tell that from a + replacement that happens to share the row. + """ + + from umbratrans_web.errors import RepositoryError + + node_id, _key = _live_credential(compute, organization, "terminal") + compute.remove_node(node_id, reason="hardware failure") + + for status in ("online", "provisioning", "offline", "draining", "retired"): + with pytest.raises(RepositoryError): + compute.set_node_status(node_id, status) + + assert compute.get_node(node_id).status == "removed" + + +@integration +def test_the_row_is_kept_because_the_event_trail_refers_to_it(compute, organization) -> None: + """Stated as a fact about the trail rather than as a claim about the code. + + The row stays because `job_events.node_id` refers to it and `job_events` refuses + every update, so a `DELETE` is refused outright and a `SET NULL` is refused by the + trigger. This checks that the trail really does point at the row afterwards, because + that is the reason the row is worth keeping. + """ + + node_id, _key = _live_credential(compute, organization, "kept-for-history") + _online( + compute, + compute.get_node(node_id), + [_device(_node(compute, organization, "kept-device"))], + ) + compute.submit_job(organization, kind="sleep", requested_by="test") + claimed = compute.claim_job(node_id) + assert claimed is not None + + compute.remove_node(node_id, reason="hardware failure") + + events = compute.job_events(claimed.job.id) + assert any( + event["node_id"] is not None and str(event["node_id"]) == str(node_id) + for event in events + ), ( + "no event in the trail names the removed node, so the row is being kept for a " + f"reason that does not apply: {events}" + ) + assert compute.get_node(node_id) is not None + + +@integration +def test_removing_a_node_that_does_not_exist_is_refused(compute, organization) -> None: + """Silently succeeding here would report a removal that never happened.""" + + from umbratrans_web.errors import NotFoundError + + with pytest.raises(NotFoundError): + compute.remove_node(uuid4(), reason="hardware failure") + + +def _audit_events(compute, node_id) -> list[dict]: + with compute._connection() as connection: + return connection.execute( + "SELECT action, metadata FROM public.audit_events " + "WHERE target_type = 'compute_node' AND target_id = %s ORDER BY created_at", + (str(node_id),), + ).fetchall() + + # ---------------------------------------------------------------------- # A paired node can actually do the work the agent asks it to do # ----------------------------------------------------------------------