Repository navigation
Ask SubstrateKind row-level questions instead of matching the substrate - #602
Conversation
3930ee1 to
79f5514
Compare
Shared code that decides something from one row's `managed_by` (a sandbox's or a node's) matched `SubstrateKind` variants, restating the Direct-vs-Kubernetes split at each call site. These are questions about the row, whose substrate need not be the running one, so they go on the shared type. The sandbox store and trigger queue also cannot depend on the api crate's `SubstrateCapabilities`, which holds facts about the running substrate. `SubstrateKind` (the alias for `NodeManagedBy` in common-core) gains one `const fn` per question. Each doc states the substrate fact and both variants' answers, and each caller's comment says what its decision does with the answer: - `proves_execution_absence`: sandbox store, `AutoLifecycleExecution::proves_no_execution`. - `wakes_after_failed_control_connect`: api, the wake fallback after a failed connect in `control_runtime_client`. Direct wakes dormant rows on the access path instead. - `listed_in_live_node_inventory`: trigger queue, which targets are dead-lettered when missing from the live node list. `fetch_targets_requires_live_direct_row_for_space_nodes` now also runs with a populated live inventory, which must leave the live Direct worker triggerable. - `restores_through_pods`: cluster controller, the restore-pod GC's row filter. - `has_node_drain_reconciler`: api, `reconciler_reasserts_cordon`. The node-drain store's draining and terminating listings now bind one `DRAIN_RECONCILER_SUBSTRATE` const, and a unit test, which an exhaustive match keeps sweeping every variant, pins that it is the only substrate answering yes. The method's doc and `SubstrateCapabilities::supports_node_drain`'s link to each other. The admin node registry names its substrate once, as `REGISTRY_SUBSTRATE`, used by every registry statement's bind and by `update_node`'s `verify` guard. `node_registry_writes_only_direct_nodes` now also sends a `verify` update to the Kubernetes twin and expects a 404 and an unchanged row. Behavior is unchanged for every input. Kubernetes answers each question the way the old `== Kubernetes` test did, and Space the way the old `== Space` test did. Sites that read `managed_by` as text still parse it first, and a label that does not parse takes the same branch as before. No SQL text, wire format, schema, metric or configuration changes. `docs/rust/space-conventions.md` and the substrate `AGENTS.md` record the convention: a decision about a row's substrate calls a `SubstrateKind` method, a fact about the running substrate reads `capabilities()`, and code never compares the label string. Two exceptions: Rust that repeats a statement's fixed scope compares against the const the statement binds (`REGISTRY_SUBSTRATE`), and mapping a `SubstrateKind` variant to a value (wire labels, route identity, construction) may match it. The `AGENTS.md` bullet follows the ones on `Substrate`'s private variants, `as_kubernetes()` and `capabilities()`. Left as they are: SQL binds of a fixed substrate, route identity (`SandboxRouteIdentity::try_new`'s pod-UID invariant and the inference of a missing `substrate_kind` from a pod UID), wire and label mappings (the console `/me` substrate, internal-api's route-token `substrate_kind`), the default `NodeApiTarget.managed_by` for an observed sandbox with an unknown node, and checks of the running substrate (`failed_active_execution_scope` in the transient lifecycle sweeper), which belong with `SubstrateCapabilities`.
79f5514 to
b3da5e2
Compare
Review — 2 findingsBehavior is unchanged at every converted site. Low
Verified against |
the-gigi-pplx
left a comment
There was a problem hiding this comment.
Approved. 2 non-blocking findings in the review comment above.
…ate comments #611 and #602 each added the same row-decision bullet to docs/rust/space-conventions.md, so it appeared twice; keep the copy with the examples. Correct two comments #602 added: - `wakes_after_failed_control_connect`: Direct's access path wakes a suspended or restoring sandbox (`DirectSubstrate::access_runtime_client`), not every "dormant" sandbox as the Kubernetes bullet defines it. - `REGISTRY_SUBSTRATE`: every registry write binds it and `verify` proceeds only for a live node of it; the registry's reads return nodes of every substrate.
…comments #611 and #602 each added the same row-decision bullet to docs/rust/space-conventions.md, so it appeared twice; keep the copy with the examples. Correct comments #602 added: - `wakes_after_failed_control_connect`: Direct's access path wakes a suspended or restoring sandbox (`DirectSubstrate::access_runtime_client`), not every "dormant" sandbox as the Kubernetes bullet defines it. - `REGISTRY_SUBSTRATE`: every registry write to `nodes`/`node_direct` binds it and `verify` proceeds only for a live node of it; the registry's reads, and the template pull and delete routes, act on nodes of every substrate. - The registry's `substrate_scope` test module header, which said the registry reads only Direct nodes, now describes what it tests: writes and `verify`.
…comments (perplexityai#657) perplexityai#611 and perplexityai#602 each added the same row-decision bullet to docs/rust/space-conventions.md, so it appeared twice; keep the copy with the examples. Correct comments perplexityai#602 added: - `wakes_after_failed_control_connect`: Direct's access path wakes a suspended or restoring sandbox (`DirectSubstrate::access_runtime_client`), not every "dormant" sandbox as the Kubernetes bullet defines it. - `REGISTRY_SUBSTRATE`: every registry write to `nodes`/`node_direct` binds it and `verify` proceeds only for a live node of it; the registry's reads, and the template pull and delete routes, act on nodes of every substrate. - The registry's `substrate_scope` test module header, which said the registry reads only Direct nodes, now describes what it tests: writes and `verify`.
What
Phase 3 of containing the Direct-vs-Kubernetes split. Shared code that decides something from one row's
managed_by(a sandbox'spublic.sandboxes.managed_byor a node'snodes.managed_by) now asks the sharedSubstrateKinda named question instead of matching the variant.SubstrateKind(pub type SubstrateKind = NodeManagedBy;incrates/libs/common/core/src/types/node.rs) gains oneconst fnper question. Each doc states only the substrate fact and both variants' answers. What a caller does with the answer, and with a label that does not parse, is in a comment at the call site.proves_execution_absenceAutoLifecycleExecution::proves_no_executionwakes_after_failed_control_connectspaced.rscontrol_runtime_clientlisted_in_live_node_inventoryfetch_trigger_targetsrestores_through_podsorphaned_restore_pod_gc/store.rs::row_lifecyclehas_node_drain_reconcileradmin/nodes.rs::reconciler_reasserts_cordonWhy methods on the shared type: each question is about the row, whose substrate need not be the running one. The api and cluster-controller sites could reach
SubstrateCapabilities(#368), but that would answer for the running substrate instead. The sandbox store and trigger queue can't depend on the api crate at all. Facts about the running substrate stay inSubstrateCapabilities.Two scopes are now named once:
routes/admin/node_maintenance.rs):const REGISTRY_SUBSTRATE: SubstrateKind = SubstrateKind::Space;is used by every registry statement's bind and byupdate_node'sverifyguard.cluster/controller/server/src/node_drain/store.rs):DRAIN_RECONCILER_SUBSTRATEis bound by the draining and terminating listings. A unit test pins that it is the only substrate whosehas_node_drain_reconciler()is true; an exhaustive match in the test makes a new variant fail to compile until the sweep includes it.has_node_drain_reconcilerandSubstrateCapabilities::supports_node_drainlink to each other.docs/rust/space-conventions.mdandcrates/libs/api/src/controllers/substrate/AGENTS.mdrecord the convention: a decision about a row's substrate calls aSubstrateKindmethod, a fact about the running substrate readscapabilities(), and code never compares the label string. Theimpl SubstrateKinddoc says the same. Two exceptions: Rust that repeats a statement's fixed scope compares against the named const the statement binds (REGISTRY_SUBSTRATE), and mapping aSubstrateKindvariant to a value (wire labels, route identity, construction) may match it. Since #607 madeSubstrate's variants private, theAGENTS.mdbullet follows #607's bullets on them,as_kubernetes()andcapabilities(), and namesSubstrateKindexplicitly.Behavior
No change for any input. Kubernetes answers each question the way the old
== Kubernetestest did, and Space the way the old== Spacetest did. Sites that readmanaged_byas text still parse it first, and an unparseable label takes the same branch as before (parse().is_ok_and(SubstrateKind::method)replacesmatches!(parse(), Ok(SubstrateKind::Kubernetes))). Typed sites still fail at decode, as before. No SQL text, wire format, schema, metric or configuration changes.Site table
Every site outside the api crate's substrate module that matches
SubstrateKind/NodeManagedByvariants or compares the parsed value, in the crates this phase covers. sbx-mgr, net-gateway and node-api-sim are out of scope.sandbox/store/src/lib.rsAutoLifecycleExecution::proves_no_executionproves_execution_absencecluster/trigger-queue/src/lib.rsfetch_trigger_targetspartitionlisted_in_live_node_inventory;fetch_targets_requires_live_direct_row_for_space_nodesnow also runs with a populated live inventory and expects the live Direct worker to stay triggerablecluster/controller/server/src/orphaned_restore_pod_gc/store.rsrow_lifecycleUnrecognizedand skippedrestores_through_podsapi/src/controllers/sandbox/spaced.rscontrol_runtime_clientwakes_after_failed_control_connectapi/src/routes/admin/nodes.rsreconciler_reasserts_cordonhas_node_drain_reconciler, tied to the reconciler'sDRAIN_RECONCILER_SUBSTRATEapi/src/routes/admin/node_maintenance.rsupdate_node(verify)REGISTRY_SUBSTRATE: the same rule the registry's binds enforce, so it is spelled oncecluster/controller/server/src/transient_lifecycle_sweeper/store.rsfailed_active_execution_scopemanaged_by()labelSubstrateCapabilities; no existing field fits (#368 follow-up)transient_lifecycle_sweeper/store.rs(managed_by().parse::<NodeManagedBy>())cluster/controller/server/src/orphaned_sandboxes/quarantine_recovery.rsNodeApiTarget { managed_by: Kubernetes }common/core/src/types/sandbox.rsSandboxRouteIdentity::try_newcommon/core/src/types/sandbox.rseffective_substrate_kind_forcommon/svc/src/space_api_route.rsclaims-vs-localsubstrate_kinddata_plane_sessions/{session_route,service_route,route_token}.rs,route_identity.rs,sandbox_service_ports.rsnode_managed_byinto a route identity'ssubstrate_kindand pass it throughdata_plane_sessions/route_rows.rsmissing_backend_endpoint_messagemanaged_bylib.rs,published_state.rs,status.rs,node_inventory.rs,template_releases_store.rs; trigger queue; cluster-controller stores; apipublic_template_operations.rs,admin/templates.rs,public/sandboxes.rs.bind(SubstrateKind::…)api/src/routes/session_auth.rsConsoleSubstrate::from_managed_bymanaged_byto the console/melabelapi/src/controllers/sandbox/lifecycle.rsdelete_observed_sandboxmanaged_byto Space when building aNodeApiTargetinternal-api has no Rust-side substrate decision left after #383; all of its uses are binds or route-identity plumbing.
Validation
b3da5e2):cargo fmt --all -- --checkcargo clippy --workspace --locked --all-targets --all-features -- -D warningscargo arch, plus main'sarchitecture-baseline.toml/ARCHITECTURE.tomlas base (as CI passes them)kubernetes_accessandcapability_identityratchetspre-commit run --from-ref origin/main --to-ref HEADcargo nextest run --all-features --retries 0-pcommon-core, sandbox-store, cluster-trigger-queue, api, cluster-controller-server,SPACE_CI_REQUIRE_TEST_DEPS=1, ×3#[ignore]benchmarkbenchmark_single_node_occupancycargo fmt --all -- --check,scripts/check-rust-error-policy.shcargo clippy --workspace --locked --all-targets --all-features -- -D warningspre-commit run --from-ref origin/main --to-ref HEADcargo nextest run -p pplx-space-cluster-trigger-queue -p pplx-space-cluster-controller-server, same flags, on the final changecargo nextest runwith-pcommon-core, sandbox-store, cluster-trigger-queue, cluster-controller-server, api, internal-api-server, cluster-reconciler,--all-targets --all-features --run-ignored all --retries 0,SPACE_CI_REQUIRE_TEST_DEPS=1, ×3 on3930ee1(production code unchanged since)c73171f, the same production behavior), fresh clustersmoke-kind-e2e.shpass;smoke-kind-typed.sh allwith CI's env: ECW 79/79, native 24/24, none quarantined. Not rerun, because the review fixes don't change production behavior.Mutations: each flips one answer (the Kubernetes rows on
3930ee1, the Space rows on the final change), and each is caught by a site or scope test:update_nodeverifyguard compares againstKubernetesnode_registry_writes_only_direct_nodes(the Kubernetes twin'sverifyupdate dials a closed port and gets a gateway error instead of 404)DRAIN_RECONCILER_SUBSTRATE = Spacedrain_listings_bind_the_only_substrate_with_a_drain_reconcilerhas_node_drain_reconciler: Kubernetes ⇒ falseonly_draining_and_terminating_promise_an_automatic_cordon_retry,the_two_recovery_hints_disagree_exactly_where_they_should,a_cordon_on_a_draining_node_promises_the_retry_the_reconciler_performswakes_after_failed_control_connect: Kubernetes ⇒ falsecontrol_runtime_client_wakes_only_kubernetes_rows_after_an_unreachable_runtimeproves_execution_absence: Kubernetes ⇒ falseproves_no_execution_only_for_kubernetes_rows; apitimeout_route_distinguishes_dormant_intent_from_unreachable_execution,dormant_timeout_persists_without_waking_and_caches_retrieslisted_in_live_node_inventory: Kubernetes ⇒ falsefanout_resolves_live_nodes_once_and_dead_letters_ghosts_without_resolvingrestores_through_pods: Kubernetes ⇒ falseclassify_sandboxes_reads_lifecycle_and_generationlisted_in_live_node_inventory: Space ⇒ truefetch_targets_requires_live_direct_row_for_space_nodes(the populated-inventory case)proves_execution_absence: Space ⇒ trueproves_no_execution_only_for_kubernetes_rows; apitimeout_route_distinguishes_dormant_intent_from_unreachable_executionwakes_after_failed_control_connect: Space ⇒ truecontrol_runtime_client_wakes_only_kubernetes_rows_after_an_unreachable_runtimerestores_through_pods: Space ⇒ trueclassify_sandboxes_reads_lifecycle_and_generationhas_node_drain_reconciler: Space ⇒ truedrain_listings_bind_the_only_substrate_with_a_drain_reconciler; apionly_draining_and_terminating_promise_an_automatic_cordon_retry🤖 Generated with Claude Code