Repository navigation
api: run the shared lifecycle tests on Kubernetes too - #603
Conversation
39fe181 to
792e2be
Compare
|
Thanks for this, it's a really useful set of twins. The docs on each one make the Direct vs Kubernetes differences easy to follow, and tying the lock waits to Stack headroom. After the |
Review — 6 findingsMedium
Low
Verified against |
the-gigi-pplx
left a comment
There was a problem hiding this comment.
Approved. 6 non-blocking findings in the review comment above.
Tests only. #402 sorted the tests that build a DirectSubstrate and listed the ones whose shared-lifecycle outcome or path differs on Kubernetes (bucket 4). This adds a Kubernetes-backed twin for 21 of the 23 api tests in that bucket and keeps every Direct test. Each twin keeps its Direct twin's setup and assertions, and its doc says whether the Kubernetes outcome is the same and, where it differs, why. Where the Direct name's claim does not hold on Kubernetes (four twins), the twin is renamed and its doc names the Direct test. The twins live in a `kubernetes` submodule beside each Direct test file and use the `kubernetes::tests` substrate fixtures and the scripted apiserver double. Shared fixtures are in `test_support::kubernetes`: an in-memory pod-store apiserver, a live sandbox's active execution row and pod, and the node-api simulator registered as a Kubernetes worker node. That module is the only place these tests construct a `Substrate` (`Substrate::from_kubernetes`). A seeded execution has published its route identity unless a test asks for the legacy, route-less shape; only the tests that hook the route publication do. The fixture's reflectors are seeded once, so pods the apiserver double creates or deletes later reach the substrate only through its uncached reads. The double creates pods already Running and never binds one to a node, and deletes them without a Terminating phase; like a real apiserver, it refuses with a 409 a delete whose `preconditions.uid` names another pod. `kubernetes::tests` gains `substrate_with_store_client_proxy_port_and_pods`, because suspend and observed-only delete need the node's node-api pod and a sandbox pod in the reflectors at once. Finding: the claim of `completed_actions_do_not_wait_for_a_key_share_lock` holds on Direct only. On Kubernetes, start and wake block behind a transaction holding `FOR KEY SHARE` on the sandbox row, which any foreign-key insert into a child table takes. They block on `SELECT lifecycle FROM sandboxes WHERE id = $1 FOR UPDATE`, run by `lock_sandbox_execution_lifecycle_in`; reading the code, that is the execution claim `ensure_pod_exists` takes before materializing the pod. Stop and kill of a sandbox whose pod is gone block too, earlier, on `SELECT id FROM sandboxes WHERE id = $1 FOR UPDATE` (`lock_sandbox_for_execution_mutation_in`), which, reading the code, the pod-tracking cleanup of the missing pod runs before the pod is recreated. Suspend blocks on its commit's `SELECT id ... FOR UPDATE`, as Direct suspend does on its artifact lock. Resume, pause, stop and kill of a sandbox whose pod is cached do not block. In the test's interleaving the holder ends promptly and each waiter then completes. A longer holder is ended by the `space_user` role's `statement_timeout`, which `bootstrap-roles` sets; the api sets no session timeout of its own. In the api, only start and wake are also bounded, by the activation wait budget around `ensure_pod_exists`. Neither the missing-pod stop and kill wait (in pod resolution) nor suspend's commit lock is under an api deadline. The twin `activations_and_suspend_wait_for_a_key_share_lock` pins this with waits tied to the holder's pid through `pg_blocking_pids` and to the waiting statement's `sql!` module, all bounded. The Kubernetes twin of `completed_actions_keep_a_terminal_lifecycle_that_landed_mid_flight` also lands the terminal lifecycle after the node call for start and suspend, which re-read the row and refuse one that landed before them. Start's lands once the node acknowledged its Wake, from a trigger on the legacy route publication. Suspend's lands with the substrate's own `suspended` finalization, from a trigger on that write. Both actions then reach the controller's terminal-state guard, which keeps the lifecycle. Also pinned: an untracked suspend of a `started` or `paused` Kubernetes sandbox writes its row `suspended`/`unplaced`, because Kubernetes suspend claims and finalizes the row whether or not the caller tracked it. Production code is unchanged. Four twins box their action future. Held inline in the test's future, the untracked-actions twin's overflows libtest's default 2 MiB stack when the api crate is built at opt-level 0, as Linux CI builds it; the others are boxed for margin. Not covered here: the two template builder-dispatch tests, because the Kubernetes builder client dials a fixed in-cluster service name, so a deterministic failure needs host DNS state or a production seam; and the cluster controller's `converge_cas_moves_only_restoring_rows`, which has no Kubernetes substrate fixture in that crate yet.
792e2be to
571ddbb
Compare
|
Thanks both. All seven are addressed in 571ddbb (tests and docs only); the PR body's "Review follow-up" section has the details.
|
What merging this does
Tests only; no production code, schema, wire format, metric or configuration change. #402 sorted the tests that build a
DirectSubstrateand listed 23 api tests (bucket 4) whose shared-lifecycle outcome or path differs on Kubernetes. This PR adds a Kubernetes-backed twin for 21 of them and keeps every Direct test.Each twin keeps its Direct twin's setup and assertions. Its doc says whether the Kubernetes outcome is the same and, where it differs, why. Where the Kubernetes outcome differs, the twin asserts it.
Four twins are renamed because the Direct name's claim is false on Kubernetes. Their docs name the Direct test.
Where the twins live. Each sits in a
kubernetessubmodule beside its Direct test file, compiled only with--features kubernetes. The exception islive_cpu_update_preserves_allocation_across_cold_restart, whose Direct copy is undersubstrate/direct/. Its twin went toupdates/postgres_tests/kubernetes.rs, the home of the shared update path it pins.What they're built from. They use the
kubernetes::testssubstrate fixtures and the scripted apiserver double (scripted_kube_client).Shared fixtures are in
test_support::kubernetes:sandbox_k8s_executionsrow, plus the Running pod that the reflector and the apiserver both hold, named as the substrate names generation 1's pod;That module is the only place these tests construct a
Substrate(Substrate::from_kubernetes, since api, cluster-controller: make the Substrate variants private to the substrate module #607 made the variants private).Route shape of a seeded execution. By default it has published its route:
route_activation_idnames the attempt the resident reports, so activations takePreserveExisting, as for executions the current api activated. Only the tests that hook the route publication seed the legacy, route-less shape (RouteIdentity::Legacy): the wake-completion twin, the native-start twin and the start-after-Wake arm ofcompleted_actions_keep….Fixture limitations, stated in their docs. The reflectors are seeded once. Pods the apiserver double later creates or deletes reach the substrate only through its uncached reads, and every pod is at the simulator's address. The double creates pods already Running and, with no scheduler, never binds one created without
spec.nodeNameto a node. It deletes pods at once, with no Terminating phase. Like a real apiserver, it refuses with a 409 a delete whosepreconditions.uidnames another pod; it does not checkpreconditions.resourceVersion.Changes outside the new files:
#[cfg(feature = "kubernetes")] mod kubernetes;line in each of seven test files and intest_support.rs;substrate_with_store_client_proxy_port_and_podsinkubernetes/tests.rs, a multi-pod form of the existing single-pod fixture. The Kubernetes suspend and observed-only delete paths need the node's node-api pod and a sandbox pod in the reflectors at once.public_sandbox_operations/tests.rs, the create-operation setup is extracted intoseed_unpinned_create_operation, so both substrates' create-retry tests share it. The Direct test is otherwise byte-identical.updates/postgres_tests.rsscope note, and api, cluster-controller: sort the tests that use the Direct substrate as a fixture (tests only) #402's placeholder comment abovepublic_sandbox_operations::tests::direct_sandbox_controller.Test count: api 1833 → 1854. That is 21 new tests, with the one
#[ignore]benchmark still skipped.Key-share finding
completed_actions_do_not_wait_for_a_key_share_lockholds on Direct only. Its premise: any foreign-key insert into a child table takesFOR KEY SHAREon the sandbox row, and a completed action should not wait behind it. On Kubernetes, these actions block on a statement whose row lock conflicts withFOR KEY SHARE, while an open transaction holds that lock on the row:sql!module)stoppedrow)SELECT lifecycle FROM sandboxes WHERE id = $1 FOR UPDATE, run bylock_sandbox_execution_lifecycle_in(kubernetes::store::execution_rows)ready_wait_bounds) aroundensure_pod_existsinstart_executionpausedrow)drive_prepared_wakeSELECT id FROM sandboxes WHERE id = $1 FOR UPDATE, run bylock_sandbox_for_execution_mutation_in(kubernetes::store::execution_rows)ensure_execution_ready_for_accesstakes its budgetSELECT id FROM sandboxes WHERE id = $1 FOR UPDATE(kubernetes::store::suspend; the only statement of that shape there, inpersist_or_accept_suspend_commit). This is the counterpart of Direct suspend's artifact lock.suspend_durabilitydeadline covers the node-api resolve, connect and suspend poll, notfinish_suspend_handoff; the suspend also runs in a tracked task that outlives its requestsql!locator records where a statement is defined, not which caller ran it.lock_sandbox_execution_lifecycle_inhas three direct callers:persist_materialized_pod_metadata,claim_started_recoveryandlock_sandbox_for_execution_lifecycle_in. The last is reached from both the execution claim andclaim_active_execution_pod_materialization. That start's and wake's first wait is the execution claimensure_pod_existstakes is an inference from the code.lock_sandbox_for_execution_mutation_inhas many callers; that the missing-pod stop and kill wait in the pod-tracking cleanup of the missing pod (resolve_pod_instance→forget_pod_tracking_if_generation), before their recovering client recreates the pod, is likewise read from the code.unplaced, where Direct suspend leaves the rowon_node. The missing-pod stop and kill recreate the pod (one pod create each) and endstopped/unplaced.lock_timeoutnorstatement_timeouton its own sessions, but it connects asspace_user, whose role-levelstatement_timeoutbootstrap-rolessets (--app-statement-timeout, 10s by default). That ends every wait in the table with an error. The role'slock_timeoutis opt-in (SPACE_DB_APP_LOCK_TIMEOUT). Within the api, only start and wake have a deadline of their own around the wait (last column).The twin
activations_and_suspend_wait_for_a_key_share_lockpins this deterministically: no#[ignore]was needed. Each wait is tied to the holder's backend pid ($holder = ANY(pg_blocking_pids(pid))) and to the waiting statement'ssql!locator. Every wait and join is bounded. Not fixed here.Test → Kubernetes outcome
characterization_tests::wake_refuses_teardown_and_terminal_lifecycles_before_admissioncreatedwith a 429.restoringandsuspendeddefer their charge to the restore-lease claim, which refuses with the same 429 and releases the claim; the row stays as it was. The substrate refuses asuspendinglifecycle with a 503 (suspend finalization in flight). The others reach the substrate's pod create, which this double's apiserver refuses (a 500, as in the next row).…::unreachable_runtime_fails_actions_without_writing_lifecyclefailed to create sandbox pod). If that read also failed, the result would be a 503 with a retry hint, so the 500 belongs to this double, not to every outage. The rest are 503s without a pod create:• start of a
stoppedrow claims a new execution, finds no restore source and abandons the claim;• suspend finds no execution;
• pause and checkpoint use the non-recovering client;
• resume's recovering client does not recover, because
pausedis dormant.…::wake_completion_outcome_maps_to_the_wake_responseensure_pod_existsbefore the node Wake. A row moved after the handler's read is refused there:suspending→ 503, teardown or terminal → 409 (cannot materialize an execution after lifecycle changed …). Direct answers 200 and a CAS-lost 500. A second pass moves the row after the node acknowledged the Wake, from a trigger on the route publication; there the completion-CAS mapping is the same as Direct's.…::completed_actions_keep_a_terminal_lifecycle_that_landed_mid_flightstarted/pausedtosuspending(claim_suspend_execution) with a 503. So start and suspend also land it after the node call. Start's lands once the node acknowledged its Wake, from the route-publication trigger (aRouteIdentity::Legacyexecution). Suspend's lands with the substrate's ownsuspendedfinalization, from a trigger on that write. Both then report success and the controller's guard keeps the lifecycle. After start's Wake the row endson_node: the substrate records the pod's node placement itself (persist_pod_node_placement). Between suspend's claim and its finalization, the substrate's own writes accept only asuspendingrow; no arm lands a lifecycle there.…::native_start_overwrites_a_stopped_written_mid_flight_without_a_retrystoppedis written by a trigger on the legacy route publication (route_activation_id), the Kubernetes counterpart of the Direct route-receipt trigger. The shared, unguarded start write then overwrites it, and the operation succeeds without a retry.…::completed_actions_do_not_wait_for_a_key_share_lock→activations_and_suspend_wait_for_a_key_share_locksuspended/unplaced, not Direct'ssuspended/on_node.…::stop_and_kill_on_a_draining_node_still_reach_the_worker…::untracked_runtime_actions_without_placement_are_unavailable→untracked_runtime_actions_without_a_row_are_not_foundrequire_sandbox), so an id with no row is a 404. Suspend reads the active execution first: a 503. No request reaches the apiserver.…::untracked_actions_report_status_and_leave_the_row→untracked_actions_report_status_and_leave_the_row_unless_suspend_claims_itcreatedrow it is a 503 before reaching the worker, while on astartedorpausedrow it suspends the resident and writes the rowsuspended/unplaced.…::observed_only_delete_removes_the_resident_on_the_observing_node…::redrive_completes_only_a_restoring_rowrestoringrow whose execution has no pod IP, and whose pod the apiserver lacks, staysrestoring.lifecycle::tests::exhausting_the_finalize_retry_budget_stays_retryablelifecycle::tests::a_contended_delete_precheck_stays_retryablelifecycle::tests::resume_uses_resolved_runtime_without_async_observationingress::tests::stored_ingress_access_resumes_paused_execution_before_reporting_readysandbox_k8s_executions.route_activation_id, and the resume goes through the podcreate_postgres_tests::postgres_failed_pooled_create_finalizes_after_claim_cleanup→postgres_failed_pooled_create_defers_cleanup_to_the_reconcilercleanup_failed_pooled_preparemarks the claimed slotcleanup_pendingand returnsDeferredToReconciler, so the row is kept. On Direct the row is deleted.public_sandbox_operations::tests::create_executor_retries_unpinned_transient_substrate_failurecontroller_error_result).public_sandbox_operations::tests::admitted_create_re_execution_after_success_does_not_create_againclaimedslot; different withcleanup_pendingadmitted_pool_cleanup_pendingreads the reserved slot, so acleanup_pendingslot gives workflow progress instead ofSucceeded; on Direct that answer is alwaysfalse. TheSucceededcheck is what catches a re-execution that re-enters create. The 0-executions check stays zero even then: the fixture tags no template releaselatest, so such a create fails resolving the template before it reaches the substrate.updates::transport_tests::untransmitted_update_is_replayed_once_on_a_fresh_clientprobe_runtime_client) instead ofnode_directupdates::transport_tests::interrupted_update_is_observed_not_resentsubstrate::direct::tests::live_cpu_update_preserves_allocation_across_cold_restart→updates::postgres_tests::kubernetes::……-g2), which carriesspace.pplx.net/vcpu: "8". The row's allocation stays 8. The update after the restart reads that-g2pod from the apiserver, which shows it resolved the new pod. The simulator can't show which pod a call reached, because every pod shares its address.Not covered (left for later PRs)
template::tests::build_submission_grants_the_base_before_dispatchandsystem_build_withdraws_its_publication_grant_when_dispatch_fails. The Kubernetes builder client always dials the fixed in-cluster namespace-builder-node-api:9800, with connect retries. A deterministic dispatch failure would need host DNS state or a production seam, and a tests-only PR can't add the seam.cluster-controller::transient_lifecycle_sweeper::lifecycle_writer_tests::converge_cas_moves_only_restoring_rows(5→4). The cluster controller has noKubernetesSubstratefixture yet; api, cluster-controller: sort the tests that use the Direct substrate as a fixture (tests only) #402's comment there is unchanged.AppState::from_pool/SandboxController::from_env(out of api, cluster-controller: sort the tests that use the Direct substrate as a fixture (tests only) #402's scope too).Other characterization notes (pinned, not fixed)
SubstrateError::Failed). If that read failed too, it would berestore_in_flight, a 503 with Retry-After. Native operation execution retries 5xx either way.forget_pod_tracking_if_generation) while refusing with a 503.FOR KEY SHAREin pod resolution, with no api deadline around that wait.statement_timeoutends it.startedorpausedKubernetes sandbox writes its rowsuspended/unplaced.Decisions made unattended
kubernetessubmodules rather than parameterizing each test over both substrates. The setups differ structurally: worker registration throughnode_directversus an execution row plus pod plus apiserver. Parameterizing would interleave the two and hide each one's outcome.test_support::kubernetes, with oneSubstrateconstruction site (Substrate::from_kubernetessince api, cluster-controller: make the Substrate variants private to the substrate module #607).publish_legacy_route_identity_after_ack), before the controller's lifecycle write. This mirrors the Direct tests' route-receipt trigger. Every other test seeds a published route. Suspend has no route publication, so its mid-flight hook is a trigger on the substrate'ssuspending→suspendedwrite, in the same transaction. The finalization holds the row lock until it commits, so this is the write a concurrent writer would make right after it.#[ignore].modlines. They go beside each file'suseblock, or above the Direct fixture helper inupdates/postgres_tests.rs, instead of at EOF, so that open PRs touching the same files still apply.wake_refuses…uses the scripted unavailable apiserver rather than the inert client, so its pod-create refusal is deterministic.Review follow-up
Each comment was re-checked against the code first. Where a premise did not hold, the change follows the code, and the row says what differed.
stopped/unplaced. The cached-pod "no" is scoped in the doc and the table. Premise partly different: the missing-pod stop and kill do not first wait on the materializationSELECT lifecycle … FOR UPDATEthat start waits on. Their first wait isSELECT id FROM sandboxes WHERE id = $1 FOR UPDATE(lock_sandbox_for_execution_mutation_in), which, reading the code, the pod-tracking cleanup of the missing pod runs inresolve_pod_instance, before any recovery. A probe ofpg_stat_activityduring the wait showed that statement; the arm waits on it.completed_actions_keep…: start and suspend never reach the guarded writeRouteIdentity::Legacyseed). Suspend has a test-only seam after its claim: a trigger on its ownsuspending→suspendedfinalization. Both now report success and reach the controller's guard. Dropping the guard from start'smaybe_update_sandbox_statefails the new twin and passes the old one. Exempting asuspendedwrite frommaybe_update_sandbox_lifecycle's guard does the same for the suspend arm. The up-front refusal arms are kept.ensure_pod_exists; the missing-pod stop and kill, nothing in the api; suspend, nothing in the api (itssuspend_durabilitydeadline does not coverfinish_suspend_handoff). Premise partly different: the api sets no session timeout, but it connects asspace_user, whose role-levelstatement_timeoutbootstrap-rolessets by default (--app-statement-timeout, 10s). So by default every one of these waits is ended by that role setting. The role'slock_timeoutis opt-in, as the comment says.a_contended_delete_precheck…retry assertion was the Direct weak formSuspendInFlight; the new twin fails and the old one passes. A teardown returningSubstrateError::Faileddoes not fail either twin, because the controller logs such a teardown failure and still deletes the row (delete).executions == 0is "no longer vacuous"Succeeded(the create resolves no template, aNotFoundfailure). With that assertion relaxed, the executions count stays 0, because the fixture tags no template releaselatestand the create stops before the substrate. The doc and table now say theSucceededcheck is what catches it.pod_store_apiservershortcutspreconditions.uidmismatch with a 409. The Running-on-create, never-scheduled and no-Terminating shortcuts are documented next to the reflector limitation. All 21 twins pass unchanged. Mutation: the substrate sends a wrong precondition UID. Two suspend-path twins now fail (…unless_suspend_claims_it,completed_actions_keep…:pod … changed before suspend retirement); with the old double all 21 passed.live_cpu…passes under it: its retire did not fail on the wrong UID.RUST_MIN_STACKat CI's opt-level 0, in 128 KiB steps: native-start 1.625 MiB (1.875 before); untracked-actions still 1.875 MiB, since its remaining depth is the shared Kubernetes start path's frames below the boxed future; key-share 1.625 MiB; completed-actions 1.5 MiB.Mutation check (one per new or changed assertion)
Each mutation was applied to production code (or, where noted, to the fixture), then the twin was run alone under CI's settings. Each file was restored with
git checkout. The earlier set is unchanged and was run at the previous head (792e2be); it was not re-run after the rebase. The new set ran at this head:lock_sandbox_for_execution_mutation_inFOR UPDATE→FOR NO KEY UPDATEon_node, notunplacedmaybe_update_sandbox_statestarted; old twin passesmaybe_update_sandbox_lifecyclestartedpersist_pod_node_placementskips itssandboxeswriteunplaced, noton_nodemaybe_update_sandbox_lifecycleexempts asuspendedwrite from its guardsuspended; old twin passesdelete_executionreturnsSuspendInFlightdelete_executionreturnsFailedSucceeded; with that relaxed, the executions check still passes (see above)pod_store_apiserverDELETE preconditionpreconditions.uidEarlier set, at 792e2be:
stoppedstoppedcreatedgets 500, not 429charge_restore_claim→Ok(())restoringandsuspendedeach get 503, not 429SuspendInFlightmaps to 409UnavailableUnavailableensure_pod_exists' lifecycle refresh checkstart_execution's recoverable-lifecycle refusalstoppedStoppedFOR UPDATE→FOR NO KEY UPDATEruntime_clientmapsNotFoundto 503created)createdstarted)startedstartedrowpaused)pausedpausedrowNotRoutablefor a routable executionrestoringruntime_clientskips the pod instanceFinalizeSandboxDbDeleteNotFoundadmitted_pool_cleanup_pending→falseSucceeded, notQueuedprobe_runtime_client→NoneRuntimeClient::callreplays ambiguous failures-g2-g2podValidation
Stack overflow in CI's "common / Rust tests" (fixed in an earlier revision). At 39fe181,
characterization_tests::kubernetes::untracked_actions_report_status_and_leave_the_row_unless_suspend_claims_itoverflowed its stack in CI. On Linuxci/scripts/run-rust-checks.shbuildspplx-space-apiandpplx-space-sbx-mgratopt-level=0(TEST_PROFILE_ARGS), not atCargo.toml's test-profileopt-level=1. Boxing therun_actionfuture fixed it. The four twins that drive the shared start or suspend path now box their action future; the measured margins are in the review follow-up above."CI's settings" below means
--config profile.test.package.pplx-space-api.opt-level=0 --config profile.test.package.pplx-space-sbx-mgr.opt-level=0,CARGO_PROFILE_DEV_DEBUG=line-tables-only,CARGO_PROFILE_DEV_BUILD_OVERRIDE_DEBUG=false,CARGO_INCREMENTAL=0,CARGO_TARGET_DIR=target/buildkite-tests,NEXTEST_TEST_THREADS=30andSPACE_CI_REQUIRE_TEST_DEPS=1, on Linux. The nextest setup scripts started Postgres and Redis. WithSPACE_CI_REQUIRE_TEST_DEPS=1, a missing dependency fails its test instead of skipping it.main202ef6b, becausepublic_sandbox_operations/tests.rsno longer merged cleanly after #607; the fixture now usesSubstrate::from_kubernetescargo nextest run --locked -p pplx-space-api --all-features --retries 0, CI's settings, three consecutive runs at this head#[ignore]benchmark) each run; no overflowRUST_MIN_STACKin 128 KiB steps, CI's settingscargo fmt --all -- --checkcargo clippy --workspace --locked --all-targets --all-features -- -D warningsat this headscripts/check_no_inline_rust_test_modules.pycrates/libs/observabilityare unchanged🤖 Generated with Claude Code