Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 51 additions & 0 deletions ARCHITECTURE.toml
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,57 @@ mode = "advisory"
[rules.pattern_ratchet.direct_tables.stale_entries] # entries with fewer matches, none, or gone
mode = "advisory"

# Shared code reaches Kubernetes-only state through `Substrate::as_kubernetes()`,
# binding and using the payload. A presence test on it, in any form, only asks
# which substrate is running, which belongs in a `SubstrateCapabilities` field
# (crates/libs/api/src/controllers/substrate/AGENTS.md). A line matcher cannot
# tell the two apart, so every call outside the substrate module counts, and a
# new caller needs a reviewed baseline entry.
[rules.pattern_ratchet.kubernetes_access]
patterns = ['\bas_kubernetes\s*\(']
allow = ["crates/libs/api/src/controllers/substrate/**"]

[rules.pattern_ratchet.kubernetes_access.no_crossing]
mode = "required"

[rules.pattern_ratchet.kubernetes_access.no_new_entries]
mode = "advisory"

[rules.pattern_ratchet.kubernetes_access.no_raised_caps]
mode = "advisory"

[rules.pattern_ratchet.kubernetes_access.listed_no_growth]
mode = "required"

[rules.pattern_ratchet.kubernetes_access.stale_entries]
mode = "required"

# `SubstrateCapabilities` derives `PartialEq` and its per-substrate consts are
# public, so comparing `capabilities()` with one of them is another way to ask
# which substrate is running. Only the two substrates' `capabilities()` name
# them in production code; tests are outside the inventory.
[rules.pattern_ratchet.capability_identity]
patterns = ['\bSubstrateCapabilities\s*::\s*(?:DIRECT|KUBERNETES)\b']
allow = [
"crates/libs/api/src/controllers/substrate/direct.rs",
"crates/libs/api/src/controllers/substrate/kubernetes/adapter/substrate_adapter.rs",
]

[rules.pattern_ratchet.capability_identity.no_crossing]
mode = "required"

[rules.pattern_ratchet.capability_identity.no_new_entries]
mode = "advisory"

[rules.pattern_ratchet.capability_identity.no_raised_caps]
mode = "advisory"

[rules.pattern_ratchet.capability_identity.listed_no_growth]
mode = "required"

[rules.pattern_ratchet.capability_identity.stale_entries]
mode = "required"

# Rootfs copies must preserve NoCoW and sparse extents through common-io's
# reflink helper. A line matcher cannot infer a path variable's contents, so
# conservatively fence all new plain filesystem copies and cp launches.
Expand Down
4 changes: 4 additions & 0 deletions architecture-baseline.toml
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,10 @@
"crates/libs/internal/api/server/src/lib.rs" = 1
"crates/libs/internal/api/server/src/published_state.rs" = 4
"crates/libs/sandbox/store/src/lib.rs" = 2

[pattern_ratchet.kubernetes_access]
# Spawns the Kubernetes-only reconcilers from the bound `KubernetesSubstrate`.
"crates/libs/cluster/controller/server/src/reconcilers.rs" = 1

Check warning on line 133 in architecture-baseline.toml

View workflow job for this annotation

GitHub Actions / rust_static / Rust clippy

[archcheck] reported by reviewdog 🐶 crates/libs/cluster/controller/server/src/reconcilers.rs is in [pattern_ratchet.kubernetes_access] in architecture-baseline.toml but not in the base branch's copy; files outside the paths [rules.pattern_ratchet.kubernetes_access] in ARCHITECTURE.toml allows can't be added (a renamed file counts as added unless --renames pairs it with an old path the base branch's copy lists). Move that code behind an allowed path instead. Raw Output: message:"crates/libs/cluster/controller/server/src/reconcilers.rs is in [pattern_ratchet.kubernetes_access] in architecture-baseline.toml but not in the base branch's copy; files outside the paths [rules.pattern_ratchet.kubernetes_access] in ARCHITECTURE.toml allows can't be added (a renamed file counts as added unless --renames pairs it with an old path the base branch's copy lists). Move that code behind an allowed path instead." location:{path:"architecture-baseline.toml" range:{start:{line:133}}} severity:WARNING source:{name:"archcheck"} code:{value:"pattern_ratchet.kubernetes_access.no_new_entries"}
[pattern_ratchet.rootfs_copy]
# Local nspawn and privileged builder copies are deferred; builders stay CoW.
"crates/apps/local/node-mgr/svc/src/snapshot.rs" = 2
Expand Down
6 changes: 3 additions & 3 deletions crates/libs/api/AGENTS.md
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
# `pplx-space-api`

Before changing the substrate, or shared code that branches on the substrate
(`Substrate::Direct`, `managed_by()`) or names a Direct-only table, read
`src/controllers/substrate/AGENTS.md`.
Before changing the substrate, or shared code that depends on the substrate
(`capabilities()`, `as_kubernetes()`, `managed_by()`) or names a Direct-only
table, read `src/controllers/substrate/AGENTS.md`.

## Changes

Expand Down
4 changes: 2 additions & 2 deletions crates/libs/api/src/controllers/sandbox/ingress/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -285,9 +285,9 @@ async fn stored_ingress_access_resumes_paused_execution_before_reporting_ready()
.await?;
let backend = StoredAccess {
pool: db.pool.clone(),
controller: SandboxController::new(Substrate::Direct(Box::new(
controller: SandboxController::new(Substrate::from_direct(
crate::controllers::substrate::direct::DirectSubstrate::new(db.pool.clone()),
))),
)),
};
for (wake_enabled, pause_after_observation, worker_allows_access) in [
(true, false, true),
Expand Down
27 changes: 13 additions & 14 deletions crates/libs/api/src/controllers/sandbox/lifecycle/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -213,9 +213,9 @@ async fn rejected_activation_emits_no_orphaned_requested_event() -> TestResult {
// Direct is only a placeholder capacity source: it reports zero eligible nodes,
// as a synced, empty Kubernetes node cache would.
let make_substrate = || {
crate::controllers::substrate::Substrate::Direct(Box::new(
crate::controllers::substrate::Substrate::from_direct(
crate::controllers::substrate::direct::DirectSubstrate::new(db.pool.clone()),
))
)
};
let admission = crate::cluster_admission::ClusterSandboxAdmission::with_config(
make_substrate(),
Expand Down Expand Up @@ -287,9 +287,9 @@ async fn direct_start_restore_spends_deferred_admission_at_entry() -> TestResult
.await?
.expect("sandbox row exists");
let make_substrate = || {
crate::controllers::substrate::Substrate::Direct(Box::new(
crate::controllers::substrate::Substrate::from_direct(
crate::controllers::substrate::direct::DirectSubstrate::new(db.pool.clone()),
))
)
};
let admission = crate::cluster_admission::ClusterSandboxAdmission::with_config(
make_substrate(),
Expand Down Expand Up @@ -540,9 +540,9 @@ async fn invalid_activation_options_reject_before_admission_and_requested_event(
// Direct is only a placeholder capacity source: it reports zero eligible nodes,
// as a synced, empty Kubernetes node cache would.
let make_substrate = || {
crate::controllers::substrate::Substrate::Direct(Box::new(
crate::controllers::substrate::Substrate::from_direct(
crate::controllers::substrate::direct::DirectSubstrate::new(db.pool.clone()),
))
)
};
let admission = crate::cluster_admission::ClusterSandboxAdmission::with_config(
make_substrate(),
Expand Down Expand Up @@ -748,9 +748,9 @@ async fn exhausting_the_finalize_retry_budget_stays_retryable() -> TestResult {
.await?
.expect("sandbox row exists");
let make_substrate = || {
crate::controllers::substrate::Substrate::Direct(Box::new(
crate::controllers::substrate::Substrate::from_direct(
crate::controllers::substrate::direct::DirectSubstrate::new(db.pool.clone()),
))
)
};
let admission = crate::cluster_admission::ClusterSandboxAdmission::with_config(
make_substrate(),
Expand Down Expand Up @@ -838,10 +838,9 @@ async fn a_contended_delete_precheck_stays_retryable() -> TestResult {
.execute(&mut *blocker)
.await?;

let controller =
SandboxController::new(crate::controllers::substrate::Substrate::Direct(Box::new(
crate::controllers::substrate::direct::DirectSubstrate::new(bounded.clone()),
)));
let controller = SandboxController::new(crate::controllers::substrate::Substrate::from_direct(
crate::controllers::substrate::direct::DirectSubstrate::new(bounded.clone()),
));
let error = controller
.delete_control_plane(&bounded, sandbox_id, Some(&sandbox))
.await
Expand Down Expand Up @@ -921,9 +920,9 @@ async fn resume_uses_resolved_runtime_without_async_observation() -> TestResult
record.execution_generation = generation;
worker.state().insert_new(record).await?;
let controller =
SandboxController::new(crate::controllers::substrate::Substrate::Direct(Box::new(
SandboxController::new(crate::controllers::substrate::Substrate::from_direct(
crate::controllers::substrate::direct::DirectSubstrate::new(db.pool.clone()),
)));
));
let mut client = controller
.substrate
.runtime_client(id, crate::controllers::substrate::RecoveryCharge::Exempt)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,14 +60,14 @@ async fn wake_completion_cas_outcome_for_every_prior_lifecycle() -> TestResult {
// `wake_completion_outcome_maps_to_the_wake_response`; the Kubernetes
// side, like the Restore arm, is observable only in the race.
assert_eq!(
Substrate::Direct(Box::new(DirectSubstrate::new(pool.clone()))).capabilities(),
Substrate::from_direct(DirectSubstrate::new(pool.clone())).capabilities(),
SubstrateCapabilities::DIRECT
);
#[cfg(feature = "kubernetes")]
assert_eq!(
Substrate::Kubernetes(Box::new(
Substrate::from_kubernetes(
crate::controllers::substrate::kubernetes::tests::substrate_with_store(pool.clone()),
))
)
.capabilities(),
SubstrateCapabilities::KUBERNETES
);
Expand Down Expand Up @@ -208,10 +208,8 @@ async fn wake_completion_redrive_on_kubernetes_hydrates_before_completing() -> T
kube_status(StatusCode::NOT_FOUND, "NotFound")
}
});
let substrate = Substrate::Kubernetes(Box::new(substrate_with_store_and_client(
pool.clone(),
client,
)));
let substrate =
Substrate::from_kubernetes(substrate_with_store_and_client(pool.clone(), client));

// A zero budget makes exactly one attempt: read, routability check,
// fenced CAS.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -496,9 +496,9 @@ fn test_federated_owner(upstream_org_id: &str) -> TestResult<FederatedOwner> {

// Direct is only a placeholder: no test using this depends on substrate behavior.
fn direct_sandbox_controller(pool: &PgPool) -> SandboxController {
SandboxController::new(crate::controllers::substrate::Substrate::Direct(Box::new(
SandboxController::new(crate::controllers::substrate::Substrate::from_direct(
crate::controllers::substrate::direct::DirectSubstrate::new(pool.clone()),
)))
))
}

async fn install_failing_federated_grant_triggers(pool: &PgPool) -> TestResult {
Expand Down
4 changes: 2 additions & 2 deletions crates/libs/api/src/controllers/sandbox/spaced/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -241,9 +241,9 @@ async fn reconnect_node_api_is_blocked_by_access_drain_gate() -> TestResult {
let node_api = start_fake_reconnect_node_api().await?;
let sandbox = insert_direct_sandbox_on_node(&db.pool, node_api.addr, Some("draining")).await?;
// Direct is only a placeholder here: this test never reaches the substrate.
let substrate = Substrate::Direct(Box::new(
let substrate = Substrate::from_direct(
crate::controllers::substrate::direct::DirectSubstrate::new(db.pool.clone()),
));
);
let node_api_addr = node_api.addr.to_string();
let endpoint = crate::controllers::substrate::node_api_endpoint(
&node_api_addr,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,9 +72,9 @@ fn metadata_request(metadata: Option<SandboxMetadata>) -> UpdateSandboxRequest {

// Direct is only a placeholder: the tests using this never reach the substrate.
fn direct_sandbox_controller(pool: &PgPool) -> SandboxController {
SandboxController::new(crate::controllers::substrate::Substrate::Direct(Box::new(
SandboxController::new(crate::controllers::substrate::Substrate::from_direct(
crate::controllers::substrate::direct::DirectSubstrate::new(pool.clone()),
)))
))
}

/// The whole path: request in, row written, and the value visible through
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -259,9 +259,9 @@ impl Fixture {
.execute(&db.pool)
.await?;
let controller =
SandboxController::new(crate::controllers::substrate::Substrate::Direct(Box::new(
SandboxController::new(crate::controllers::substrate::Substrate::from_direct(
crate::controllers::substrate::direct::DirectSubstrate::new(db.pool.clone()),
)));
));
Ok(Self {
db,
controller,
Expand Down
57 changes: 40 additions & 17 deletions crates/libs/api/src/controllers/substrate/AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -57,30 +57,52 @@ For new work:
arm give both substrates the default and skip the Kubernetes override. Only
a hook shared code calls on every path may default to "nothing to do", as
`retire_fence_mismatch` does, and it still needs its arm.
- Where shared code needs a static fact about the substrate, add a
`SubstrateCapabilities` field, set it in both `SubstrateCapabilities::DIRECT`
and `::KUBERNETES`, and read it through `capabilities()`; where the
substrate must act, add a required method (above). Don't match
`Substrate::Direct` or branch behavior on `managed_by()`. Matching
`Substrate::Kubernetes(k)` to reach Kubernetes-only state is fine.
- `Substrate`'s variants are private to this module (`Inner` in
`mod.rs`), so shared code cannot match them; it builds one with
`Substrate::from_direct` or `Substrate::from_kubernetes`. Where shared code
needs a static fact about the substrate, add a `SubstrateCapabilities`
field with a doc comment stating what it means, set it in both
`SubstrateCapabilities::DIRECT` and `::KUBERNETES`, and read it through
`capabilities()`; where the substrate must act, add a required method
(above). Don't widen the variants' visibility.
- Code that needs Kubernetes-only state (its client, caches, or a reconciler
typed on `KubernetesSubstrate`) uses `as_kubernetes()` and binds and uses
the payload. A presence test on it (`Some(_)`, `if let Some(_)`,
`.is_some()`, `.is_none()` and the like) only asks which substrate is
running: add a capability instead, as the cluster controller's leader
election reads `contends_for_leader_lease`. Comparing `capabilities()` with
`SubstrateCapabilities::DIRECT` or `::KUBERNETES` asks the same and is
equally off limits. archcheck enforces two line counts over production code
(tests are outside them). `pattern_ratchet.kubernetes_access` counts every
`as_kubernetes(` call outside this module, in any form, against its
`architecture-baseline.toml` entry, so a new caller needs a reviewed
baseline entry. `pattern_ratchet.capability_identity` rejects any mention
of the two consts outside `direct.rs` and
`kubernetes/adapter/substrate_adapter.rs`.
- `managed_by()` names the running substrate for SQL row scope and wire
labels. Never parse it to decide behavior. The only parse is
`failed_active_execution_scope` in `transient_lifecycle_sweeper/store.rs`,
which turns the running label into the fixed SQL scope of the sweeper's
failed-active-execution cleanup; don't add another.
- Scoping a query to the rows a substrate owns is required, not debt. In new
SQL, bind the substrate value instead of a `'k8s'`/`'space'` literal:
`managed_by = $n` with `.bind(substrate.managed_by())` for the running
substrate's rows, as the transient-lifecycle sweeper does, or a fixed
`SubstrateKind::Kubernetes`/`::Space` where code targets one substrate's
rows. When a fixed-scope statement may run under another substrate, return
early in Rust instead of also ANDing the running substrate's value, as the
sweeper's failed-active-execution cleanup does. Existing literals, such as
the shared `routable_execution_or_non_k8s` fragment, are debt. Never choose
a runtime endpoint from a row's label.
early in Rust instead of also ANDing the running substrate's value: read a
capability, or reuse `failed_active_execution_scope` where its scope fits,
as the sweeper's failed-active-execution cleanup does. Existing literals,
such as the shared `routable_execution_or_non_k8s` fragment, are debt.
Never choose a runtime endpoint from a row's label.
- `node_direct`, `sandbox_direct_route_identities` and
`sandbox_direct_route_provenance` belong to Direct. Outside tests, write
them only from `direct.rs` and the Direct node registry; read them through
an existing query that already joins them rather than adding a join.
archcheck's `pattern_ratchet.direct_tables` rule enforces this (advisory
today).
- Other existing substrate matches, Direct-table joins in shared code and
`Unsupported` trait defaults (`list_pool_configs`, `upsert_pool_config`,
- Existing Direct-table joins in shared code and `Unsupported` trait
defaults (`list_pool_configs`, `upsert_pool_config`,
`disable_pool_config`) are debt, not precedent.

## Tests
Expand All @@ -93,11 +115,12 @@ restore, and pooled manifests in one place.
Keep narrow tests only for behavior that is hard to express clearly in the broad
YAML contract test.

Use `Substrate::Direct` only to test Direct behavior (tests under `direct/`) or
behavior that genuinely doesn't depend on the substrate. Test shared lifecycle
behavior Kubernetes deployments depend on against a `Substrate::Kubernetes`
built with the `kubernetes::tests` helpers (`substrate_with_store` and its
variants), as `routes/public/sandboxes/admission_test_fixture.rs` does.
Use `Substrate::from_direct` only to test Direct behavior (tests under
`direct/`) or behavior that genuinely doesn't depend on the substrate. Test
shared lifecycle behavior Kubernetes deployments depend on against a
`Substrate::from_kubernetes` built with the `kubernetes::tests` helpers
(`substrate_with_store` and its variants), as
`routes/public/sandboxes/admission_test_fixture.rs` does.
Adapter tests inside `kubernetes/` use `scripted_test_doubles.rs`, as
`adapter_characterization_tests/` does. Both compile only with
`--features kubernetes`: CI's `--workspace --all-features` run includes them,
Expand Down
4 changes: 2 additions & 2 deletions crates/libs/api/src/controllers/substrate/direct/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2751,7 +2751,7 @@ async fn direct_create_fixture() -> TestResult<Option<DirectCreateFixture>> {
.fetch_one(&postgres.pool)
.await?;
let controller = crate::controllers::sandbox::SandboxController::new(
super::super::Substrate::Direct(Box::new(DirectSubstrate::new(postgres.pool.clone()))),
super::super::Substrate::from_direct(DirectSubstrate::new(postgres.pool.clone())),
);
Ok(Some(DirectCreateFixture {
postgres,
Expand Down Expand Up @@ -2953,7 +2953,7 @@ async fn live_cpu_update_preserves_allocation_across_cold_restart() -> TestResul
.execute(&fixture.pool)
.await?;
let controller = crate::controllers::sandbox::SandboxController::new(
super::super::Substrate::Direct(Box::new(DirectSubstrate::new(fixture.pool.clone()))),
super::super::Substrate::from_direct(DirectSubstrate::new(fixture.pool.clone())),
);
for quota in [2, 8] {
let sandbox = crate::controllers::sandbox::fetch_sandbox(&fixture.pool, sandbox_id)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -345,9 +345,9 @@ async fn routability_check_answers_with_the_hydration_verdict(
HydrationPod::Forbidden => kube_status(StatusCode::FORBIDDEN, "Forbidden"),
HydrationPod::PriorGeneration => unreachable!("no case scripts a predecessor's pod"),
});
let substrate = Substrate::Kubernetes(Box::new(substrate_with_stale_cached_pod(
let substrate = Substrate::from_kubernetes(substrate_with_stale_cached_pod(
&db.pool, client, sandbox_id,
)));
));

let check = substrate
.ensure_routable_execution(sandbox_id, Duration::from_secs(5))
Expand Down
Loading
Loading