You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
[db] add node_k8s.instance_id and the node_k8s_superseded_instances table - #1686
Adds node_k8s.instance_id (nullable TEXT) and the node_k8s_superseded_instances table, in db/postgres/20261106232823_node_k8s_instance_id.{up,down}.sql. Nothing writes or reads them until #1676.
The dead-node check finds a node's instance in EC2 by its private DNS name. EC2 forgets a terminated instance about an hour after it terminates, so an old dead node returns no match, as a wrong region, account or name does, and a same-IP replacement reuses the node id and answers for the new instance. Nothing records which instance a node ran on, or which one it ran on before.
node_k8s_superseded_instances keeps each instance a node_k8s row named before a different one replaced it, with the Kubernetes Node UID the row held then (id, node_id, instance_id, k8s_node_uid, superseded_at). Its rows go with their node_k8s row (ON DELETE CASCADE), indexed on (node_id, superseded_at). The node registration store owns it, declared in the previous pull request.
The test fixtures' execution-record-views schema variant carries the new column through its node_k8s view. That view runs with its caller's privileges, so the credential manager's base grant on node_identities gains the column too.
Validation
At this head (CI's test profile, Postgres fixtures required):
Migration guards against the previous pull request's head: check-migration-versions.sh, check-postgres-migrations-isolated.sh, check-postgres-migrations-squawk.sh (0 issues) and check-migration-isolation-per-commit.sh: pass.
pplx-space-db-migrate privileged group as root: 16 passed, including the 5 execution_record_view_tests.
The crates that read node_k8s (pplx-space-api, pplx-space-cluster-controller-server, pplx-space-internal-api-server, pplx-space-node-registration-store), --all-features: 4297 passed, 0 failed, 0 skipped. No code reads node_k8s with SELECT *.
Severity and risk
Severity: does not apply; new evidence storage, not a fix.
Risk: the migration runs with lock_timeout = '1s' and statement_timeout = '5s'. Adding a nullable column without a default changes the catalog only and rewrites no row; the new table starts empty, and its foreign key briefly locks node_k8s. If a lock is not granted within 1 s, the migration fails and nothing is applied. Code deployed today never names the new column. Roll back with the down migration, after rolling back #1676's binaries: it drops the table and the column, and the instances recorded in them.
Rollout
Deploy this migration before #1676's binaries, which write the column and the table.
Stack
On #1685 (the table's owner and the guard change that lets the fixture travel with this migration), which is on #1673. #1676 builds on this one. Merge in that order.
AI assistance: Claude Code made the change, ran the validation and drafted this description. A human reviews before merge.
Draft PRs are not automatically reviewed by default.
Trigger a manual review
To automatically review draft PRs, update your CodeRabbit configuration:
reviews:
auto_review:
drafts: true
No actionable comments were generated in the recent review.
Walkthrough
The migration adds nullable instance_id to node_k8s and creates an indexed table for superseded instances. The rollback removes the table and column. The schema fixture adds instance_id to the node_k8s compatibility view and the space_credmgr grant.
Possibly related PRs
perplexityai/SPACE#1685: Declares the node_k8s_superseded_instances table in the node registration store manifest.
The migration adds instance tracking, while the compatibility fixture expands the credential manager’s read access in test databases. Confirm that the grant has the intended scope before merging.
Security architecture risk: Low · up to 14e8c
The immediate exposure is limited: this change prepares storage without adding instance-based control decisions, and the additional read privilege is confined to an opt-in test schema. Rollback permanently removes recorded history. Future identity-update behavior remains outside this change, and incomplete security coverage limits assurance.
Retained concerns
No architecture-level concerns identified.
Security review details
Security Blast Radius
inferred — Within the opt-in test variant, space_credmgr can directly select instance_id from node_identities without going through the compatibility view’s Kubernetes-only predicate. The direct-grant pattern predates this PR; instance_id is the newly exposed column. This does not establish production exposure: the variant is test-only, and the production baseline’s node-column grants are unchanged.
Trust Boundaries and Controls
observed — The compatibility view continues to execute with caller privileges, selects only managed_by = 'k8s' rows, and uses WITH CASCADED CHECK OPTION for writes through the view. These view controls do not impose the same row predicate on separately granted direct base-table reads.
Resilience and Maintainability Implications
inferred — The up migration is transactional rather than marked no-transaction. Under the existing migration runner, a failed transactional file rolls back together with its migration-history entry, and eligible lock-timeout failures can be retried without adopting partially applied schema. This contains interruption during the multi-statement schema addition; it does not verify future registration-write atomicity.
Pre-merge checks: 6
Check name
Status
Explanation
Title check
The title "[db] add node_k8s.instance_id and the node_k8s_superseded_instances table" uses the bracketed area prefix the requirements allow, then a summary of what merging does. The summary starts wit…
Description Follows The Pr Template
The description contains all four required items: Summary, Validation, Severity and risk, and an AI assistance line with the tool and its contributions.
Bug Fixes Name A Regression Test
The PR adds database storage and updates a test fixture. It does not fix application behavior under crates/, shim/, or ui/, so the check does not apply.
Weakened Safeguards Are Justified
The diff does not remove or weaken a test, assertion, lint, CI check, or repository guard. It does not change review guidance.
Agent Tooling Runs No New Commands
The diff changes only SQL migrations and a database fixture. It adds no agent tooling commands, server entries, or skill frontmatter.
Public-Safe Content
The diff, title, and description contain no content prohibited by this check.
Full details: Title check
Explanation
The title "[db] add node_k8s.instance_id and the node_k8s_superseded_instances table" uses the bracketed area prefix the requirements allow, then a summary of what merging does. The summary starts with the lowercase word "add" and keeps the code identifiers in their original case. It has no trailing period, ticket ID, or internal link. It matches the main change: a nullable instance_id column on node_k8s and a new node_k8s_superseded_instances table. The title meets the stated format and describes the main change.
Recent review info
Automatic approval policy
Commit: 14e8c52c4af0.
Setting
Effective value
Source
Automatic approvals
Disabled
Override 3
Merge risk
Moderate
Global configuration
Security risk
Not set
Global configuration
Matching overrides (numbered as in the resolved configuration):
Any matching disabled override withholds approval, regardless of risk. Otherwise, the strictest explicitly set limit wins for each score. Limits omitted by all matching overrides inherit the global configuration.
Not set means no limit was configured after inheritance. Use @coderabbitai configuration to inspect the resolved settings and their sources.
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 8 remain after this review.
…able
The dead-node check finds a node's instance in EC2 by its private DNS
name. EC2 forgets a terminated instance about an hour after it
terminates, so an old dead node returns no match, as a wrong region,
account or name does, and a same-IP replacement reuses the node id and
answers for the new instance. Nothing records which instance a node ran
on, or which one it ran on before.
node_k8s gains a nullable instance_id, and node_k8s_superseded_instances
keeps each instance a node_k8s row named before a different one replaced
it, with the Kubernetes Node UID the row held then. Its rows go with
their node_k8s row (ON DELETE CASCADE). The node registration store,
declared as the table's owner in the previous change, writes both in the
next one; until then nothing writes or reads them.
The nullable column has no default, so adding it changes the catalog and
rewrites no row, and the new table starts empty. The migration runs with
a 1 s lock timeout and a 5 s statement timeout. The down migration drops
the table and the column, and the instances recorded in them.
The test fixtures' execution-record-views schema variant carries the new
column through its node_k8s view. That view runs with its caller's
privileges, so the credential manager's base grant on node_identities
gains the column too.
Closing for now: this piece is being rebuilt as part of one combined drain v2 build that drops several of the structures it adds. It will come back as part of a smaller set of PRs once that build passes end to end.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds
node_k8s.instance_id(nullableTEXT) and thenode_k8s_superseded_instancestable, indb/postgres/20261106232823_node_k8s_instance_id.{up,down}.sql. Nothing writes or reads them until #1676.The dead-node check finds a node's instance in EC2 by its private DNS name. EC2 forgets a terminated instance about an hour after it terminates, so an old dead node returns no match, as a wrong region, account or name does, and a same-IP replacement reuses the node id and answers for the new instance. Nothing records which instance a node ran on, or which one it ran on before.
node_k8s_superseded_instanceskeeps each instance anode_k8srow named before a different one replaced it, with the Kubernetes Node UID the row held then (id,node_id,instance_id,k8s_node_uid,superseded_at). Its rows go with theirnode_k8srow (ON DELETE CASCADE), indexed on(node_id, superseded_at). The node registration store owns it, declared in the previous pull request.The test fixtures' execution-record-views schema variant carries the new column through its
node_k8sview. That view runs with its caller's privileges, so the credential manager's base grant onnode_identitiesgains the column too.Validation
At this head (CI's test profile, Postgres fixtures required):
check-migration-versions.sh,check-postgres-migrations-isolated.sh,check-postgres-migrations-squawk.sh(0 issues) andcheck-migration-isolation-per-commit.sh: pass.pplx-space-db-migrateprivileged group as root: 16 passed, including the 5execution_record_view_tests.pplx-space-db-migrate,pplx-space-db-test-fixtures,pplx-space-node-registration-storeunprivileged: 56 passed, 0 failed.node_k8s(pplx-space-api,pplx-space-cluster-controller-server,pplx-space-internal-api-server,pplx-space-node-registration-store),--all-features: 4297 passed, 0 failed, 0 skipped. No code readsnode_k8swithSELECT *.Severity and risk
Severity: does not apply; new evidence storage, not a fix.
Risk: the migration runs with
lock_timeout = '1s'andstatement_timeout = '5s'. Adding a nullable column without a default changes the catalog only and rewrites no row; the new table starts empty, and its foreign key briefly locksnode_k8s. If a lock is not granted within 1 s, the migration fails and nothing is applied. Code deployed today never names the new column. Roll back with the down migration, after rolling back #1676's binaries: it drops the table and the column, and the instances recorded in them.Rollout
Deploy this migration before #1676's binaries, which write the column and the table.
Stack
On #1685 (the table's owner and the guard change that lets the fixture travel with this migration), which is on #1673. #1676 builds on this one. Merge in that order.
AI assistance: Claude Code made the change, ran the validation and drafted this description. A human reviews before merge.
🤖 Generated with Claude Code