feat(operator,crdt,swim): tenant-budget plumbing via SWIM-gossiped GCounter (grid#40) - #47
Conversation
08e1cf1 to
9427aeb
Compare
|
Pushed a fix commit ( Bugbot — high: spend-only broadcast wipes origin providersConfirmed real. Fix: added Security review — medium: SWIM gossip can forge per-tenant spendConfirmed real. Fix: added Security review — medium: unbounded tenant_spend growth from gossipConfirmed real, no eviction or cap existed. Fix:
Security review — medium/operational: cross-tenant spend visible in
|
|
Filed #48 to track the RBAC/cross-tenant-visibility question on |
|
Ran a self-audit of this PR against the project's own CI matrix (all of Found and fixed in
Re-verified clean after both fixes: Everything else checked out: |
|
Found one more gap while double-checking whether this needed helm-unittest coverage: Fixed in
Re-verified: |
Live cluster validation (Kind, helios08)Validated this PR end-to-end on a real cluster rather than relying solely on 1. Full CI-equivalent Kind lifecycle — ran 2. Targeted
3. Found + fixed a pre-existing, unrelated bug during that last step — No further gaps found from this round of live-cluster testing. |
| kubeconform -v | ||
|
|
||
| - name: Install helm-unittest | ||
| run: helm plugin install https://github.com/helm-unittest/helm-unittest --version "${HELM_UNITTEST_VERSION}" |
There was a problem hiding this comment.
The required Helm validate check currently fails at this install step before any chart tests run. With the workflow-pinned Helm v3.17.3, helm-unittest v1.1.1 cannot load because its plugin manifest contains the unknown platformHooks field. Please pin a Helm/plugin combination that works in CI (and ideally checksum-pin the installed artifact), then confirm this required check is green.
There was a problem hiding this comment.
A minimal candidate is to retain Helm 3 and pin the last pre-platformHooks plugin manifest (then verify the required job):
- HELM_UNITTEST_VERSION: v1.1.1
+ # v0.8.2 uses the Helm 3 `command`/`hooks` plugin manifest.
+ HELM_UNITTEST_VERSION: v0.8.2The v0.8.2 manifest uses the Helm 3-compatible command/hooks fields; v1.1.1 uses platformCommand/platformHooks. The green CI job should remain the deciding proof.
There was a problem hiding this comment.
Fixed in 4a1fb5f — pinned exactly as suggested: HELM_UNITTEST_VERSION: v0.8.2, plus a comment explaining why (v1.x's plugin.yaml uses platformCommand/platformHooks, which the pinned Helm v3.17.3 can't load; v0.8.2 is the last release on the command/hooks schema). Confirmed against both plugin manifests directly before pushing.
Heads up: the Helm workflow run on this push is currently sitting at action_required (the fork-PR approval gate), not failing — would appreciate an approve on the run so the green check shows up.
| .send_modify(|snapshot| snapshot.remove_origin_providers(origin)); | ||
| self.state_tx.send_modify(|snapshot| { | ||
| snapshot.remove_origin_providers(origin); | ||
| snapshot.remove_origin_tenant_spend(origin); |
There was a problem hiding this comment.
This removes historical spend when SWIM evicts an origin, so the supposedly cumulative grow-only counter can decrease during ordinary membership churn. A transient site failure or restart could lower spendRatio and reopen exhausted budget. Provider records are membership-scoped, but cumulative spend should remain until an explicit budget epoch/reset policy removes it; could we retain tenant-spend slots here?
There was a problem hiding this comment.
The minimal cumulative-budget correction is simply to stop coupling spend retention to member liveness:
self.state_tx.send_modify(|snapshot| {
snapshot.remove_origin_providers(origin);
- snapshot.remove_origin_tenant_spend(origin);
});If spend needs expiry, it should be implemented later as an explicit budget epoch/window reset rather than as a side effect of SWIM eviction.
There was a problem hiding this comment.
Fixed in 4a1fb5f exactly per your diff — remove_origin no longer calls remove_origin_tenant_spend; only provider/transport/cert state is cleared on eviction now. Added origin_state_handle_remove_origin_preserves_that_origins_tenant_spend (inverts the old test's assertion) to lock in the new behavior, and expanded the doc comment on remove_origin explaining why cumulative spend must survive membership churn.
Filed grid#52 to track the explicit budget-epoch/window reset design, plus bounding per-tenant site-slot growth now that eviction no longer prunes it — a design spike, not blocking this PR.
| /// Returns [`BudgetPolicyValidationError`] for the first invalid tenant entry | ||
| /// found, in declaration order: a blank `tenantId`, a duplicate `tenantId`, a | ||
| /// negative `capUsd`, or a non-finite `capUsd`. | ||
| pub fn validate_budget_policy(policy: &BudgetPolicyConfig) -> Result<(), BudgetPolicyValidationError> { |
There was a problem hiding this comment.
This validation function currently has no production callers; every reference is in tests. The CRD numeric minimum catches negative caps, but blank and duplicate tenant IDs can still reach reconciliation despite the documented rejection behavior. Please invoke this validation in the reconcile/admission path and surface a clear invalid-policy condition, or enforce the same constraints in the generated schema.
There was a problem hiding this comment.
A small defensive reconcile-time fix would be:
let name = grid_network_name(&network)?;
+
+ if let Some(policy) = network.spec.budget_policy.as_ref() {
+ crate::crd::grid_network::validate_budget_policy(policy).map_err(|error| {
+ OperatorError::InvalidResource(format!("invalid budgetPolicy: {error:?}"))
+ })?;
+ }
info!(name, "reconciling GridNetwork");A proper Display implementation for BudgetPolicyValidationError would make the status/log message cleaner, but the important part is ensuring the production reconcile path actually invokes the validation.
There was a problem hiding this comment.
Wired in 4a1fb5f along the lines of your diff — added a reject_invalid_budget_policy helper (calls validate_budget_policy, maps to OperatorError::InvalidResource) and invoke it as the first line of reconcile(). Also added a thiserror::Error derive + #[error(...)] messages on BudgetPolicyValidationError, so the error renders via {error} (e.g. tenant "tenant-a" appears more than once) instead of a {error:?} Debug dump. Added 6 unit tests for reject_invalid_budget_policy: absent policy, valid policy, blank tenant id, duplicate tenant id, negative cap, non-finite cap.
praxis-bot
left a comment
praxis-bot
left a comment
There was a problem hiding this comment.
Thorough, well-structured PR. The is_metadata_only bug fix (spend-only broadcasts were silently dropped) and the carries_provider_state guard (preventing spend-only broadcasts from wiping existing providers via replace_origin_providers) are well-designed and well-tested. Trust-boundary enforcement in merge_tenant_spend_from_origin (slot stripping + MAX_TRACKED_TENANTS cap) is solid. CRDT property tests (idempotent, commutative, associative) and the real-UDP three-node convergence test give good confidence. The Helm template fix for spec: null when all optional fields are at their falsy defaults is a nice catch. One medium-priority gap noted inline.
| .map(|swim| swim.state_snapshot().tenant_spend) | ||
| .unwrap_or_default(); | ||
| let budget_statuses = | ||
| crate::crd::grid_network::resolve_budget_statuses(network.spec.budget_policy.as_ref(), &tenant_spend); |
There was a problem hiding this comment.
Medium -- validation gap: validate_budget_policy() is defined and thoroughly tested but is not called anywhere in the reconcile path. The CRD schema prevents negative capUsd and requires fields, but it cannot enforce uniqueness on tenants[].tenantId array items. A duplicate tenantId that passes CRD admission would reach tenant_spend_status() and produce duplicate budgetStatus entries with the same tenantId.
Consider calling validate_budget_policy() before resolve_budget_statuses() here and logging a warning (or setting a status condition) for invalid policies. This would also catch any edge cases that slip past CRD schema validation in non-Kubernetes code paths.
There was a problem hiding this comment.
Already resolved on this branch, ahead of this review: reject_invalid_budget_policy() (grid_network.rs:200) wraps validate_budget_policy() and is called as the very first line of reconcile() (line 229), before anything else including resolve_budget_statuses() -- so an invalid budgetPolicy (including a duplicate tenantId, which the CRD schema indeed can't express) now short-circuits reconciliation with OperatorError::InvalidResource rather than reaching status resolution at all. Covered by reject_invalid_budget_policy_rejects_duplicate_tenant_id and 4 sibling unit tests. This landed in commit 062e071 addressing nerdalert's earlier review on this same PR -- should be visible once CI catches up on this rebase.
1b75aba to
4a1fb5f
Compare
…ounter (grid#40)
Adds Grid-side plumbing for MVP 1b tenant-budget tracking: a per-tenant
budget policy on GridNetwork, a tenant-keyed GCounter carried in the
SWIM-gossiped grid state snapshot, and a derived spendRatio exposed on
GridNetwork.status. Enforcement (degrade/reject at a threshold) stays
out of scope here -- that's cross-repo, gateway-side praxis-ai policy
filter work, same precedent as the provider_route gap tracked in
grid#44.
1. CRD field -- GridNetworkSpec gains budget_policy: Option<BudgetPolicyConfig>,
following the sibling scoring_policy field's serde idiom exactly.
validate_budget_policy() rejects negative/NaN/infinite caps and
duplicate or blank tenant_ids; the generated CRD schema additionally
enforces a numeric minimum on capUsd as defense in depth.
2. Tenant-keyed GCounter -- GridStateSnapshot gains
tenant_spend: BTreeMap<String, GCounter>, reusing the crdt crate's
already-proven CRDT laws (commutative/associative/idempotent
merge) rather than inventing new CRDT theory. increment_tenant_spend()
and merge_tenant_spend() round out the API.
3. SWIM broadcast wiring -- StateBroadcastHandler::receive_item merges
the new field alongside the existing capabilities merge. Also fixes
a real bug this surfaced: StateBroadcast::is_metadata_only() only
checked providers/capabilities, so a broadcast carrying *only* a
tenant_spend change (the common case once provider topology has
settled) was silently classified as metadata-only and its merge
was skipped entirely. Caught by a real-UDP two-origin-site gossip
integration test, not a mock.
4. Exposed signal -- GridNetworkStatus gains
budget_status: Vec<TenantBudgetStatus>, populated per-reconcile via
a pure tenant_spend_status()/resolve_budget_statuses() pair,
mirroring the existing overlay_status precedent. Not folded into
render_routing_overlay() or score_backends(): budget is a per-tenant
status concern, a different axis from per-backend routing scores.
Testing (TDD RED-GREEN-REFACTOR, pyramid invariant):
- Unit: 100% line coverage of every pure function above (validate,
spend_ratio, merge, status derivation) via cargo-llvm-cov.
- Integration: CRD schema-generation, receive_item merge-wiring
(including the is_metadata_only regression above), and reconcile
wiring that budget_status is populated from policy + merged CRDT
state.
- Real-network integration (operator::swim_runtime, live-validated on
helios08): three real SwimHandle instances bound to real UDP
sockets. Two sites independently record tenant spend and converge
with each other; a third site then joins late ("partition heals")
and must converge to the true cross-site sum purely through SWIM's
own gossip -- no manual message shuttling. Ties the full pipeline
together end to end by running the same resolve_budget_statuses()
the reconciler calls, against the late joiner's independently
converged view.
Fixes praxis-proxy#40
Signed-off-by: Jordi Gil <jgil@redhat.com>
Bugbot (high): a tenant-spend-only broadcast from an origin was being treated as an authoritative provider-state sync, so `receive_item` ran the destructive `replace_origin_providers` retain-then-replace with an empty provider list and wiped that origin's real provider records at every peer. Added `StateBroadcast::carries_provider_state` and gated the replace call on it, decoupling provider-state sync from tenant spend the same way gateway-address/cert already have independent revision lanes. Security review (medium): `GCounter::merge` blindly took the max of every slot in an incoming counter with no check that the slot belonged to the broadcast's claimed origin, letting a compromised/buggy peer forge another site's recorded spend by embedding extra slots in its own payload. Added `GCounter::retain_origin` and `GridStateSnapshot::merge_tenant_spend_from_origin`, used at the wire-ingest boundary in `receive_item` to accept only the claimed origin's own slot per tenant, mirroring how provider records are already scoped to their claimed origin. The permissive `merge_tenant_spend` is preserved for trusted full-snapshot merges. Security review (medium): tenant_spend had no eviction path and no bound, so a churning or malicious origin could grow the map without limit. Added `GCounter::remove_slot` / `GridStateSnapshot::remove_origin_tenant_spend`, wired into `OriginStateHandle::remove_origin`'s existing eviction sweep, and a `MAX_TRACKED_TENANTS` hard bound (1024, mirroring the existing `max_origins` bound) enforced in `merge_tenant_spend_from_origin`: already-tracked tenants keep accepting updates, but brand-new tenant_ids are refused once at capacity. Full TDD: RED tests added first for all three fixes, then GREEN implementation, then REFACTOR (clippy/fmt/machete clean). 100% line coverage of the new logic confirmed via cargo-llvm-cov; remaining "missed" lines in the touched files are pre-existing gaps (doc comments, struct fields, an unrelated version-mismatch branch) outside this diff. Signed-off-by: Jordi Gil <jgil@redhat.com>
…fields A GA-readiness audit of this PR found the checked-in CRD manifests had not been regenerated after the budgetPolicy/budgetStatus schema change, and the feature was undocumented. 1. deploy/crds/gridnetwork.yaml and charts/grid-operator/crds/gridnetwork.yaml were stale: both were byte-identical to each other but contained zero occurrences of budgetPolicy/budgetStatus, while the Rust CRD struct had 18. The existing budget_policy_appears_in_crd_schema unit test only exercises the in-memory kube-rs/schemars generation and can't catch drift against the exported YAML actually shipped to clusters via Helm or `kubectl apply -f deploy/crds/`. Concretely: any real installation using the previously checked-in manifests would have the Kubernetes API server silently prune spec.budgetPolicy as an unknown field on a structural schema (no preserveUnknownFields on this CRD), so the whole feature would appear to work in every unit/integration test -- none of which go through the K8s API's OpenAPI validation -- while being inert end to end. Regenerated both files via the project's own ./scripts/generate-deployment-crds.sh; diff is purely additive (78 lines, 0 removed) and confined to the new fields. 2. docs/architecture/crds.md had no mention of budgetPolicy/budgetStatus. Sibling GridNetworkSpec fields (scoringPolicy, metricsRefreshInterval) were each documented in the same commit that introduced them; this PR's original commits touched only *.rs files. Added a budgetPolicy example to the GridNetwork sample manifest, added budgetStatus[] to the status-fields list, and a "Tenant budget tracking" section covering what the fields mean, the explicit non-enforcement scope, the per-request-attribution dependency on praxis-ai#130/#104, and a pointer to grid#48 for the RBAC/cross-tenant visibility question already raised on this PR. Re-verified after both changes: cargo clippy/fmt/machete clean, cargo test --workspace passing (79/79 in the touched crates), make coverage-check passing at 85.06% workspace lines (gate is 80%), make doc (rustdoc -D warnings) clean, and a hidden-Unicode scan of every touched file clean (mirrors the org's unicode-safety-check gate). Signed-off-by: Jordi Gil <jgil@redhat.com>
`charts/grid-site` declaratively renders a `GridNetwork` from `values.yaml` (`templates/gridnetwork.yaml`) -- the site-operator-facing deployment path, distinct from `grid-operator` which only installs the controller + CRDs. The two prior GridNetworkSpec field additions (scoringPolicy in c7c8f9e, metricsRefreshInterval in c6e4e6a) both wired their field into this chart's template and values.schema.json; budgetPolicy did not, so anyone deploying via this chart had no way to configure a tenant budget policy at all -- the schema's `additionalProperties: false` would hard-reject even a hand-added `budgetPolicy:` key in a values file. 1. Added a `{{- with .Values.gridNetwork.budgetPolicy }}` block to templates/gridnetwork.yaml, passing the value through verbatim via `toYaml` (same style already used for `gatewayRefs`/`swim`/`tls`, since `tenants[]` is an open-ended list rather than a fixed enum like scoringPolicy). 2. Added a matching `budgetPolicy` object to values.schema.json: a `tenants` array of `{tenantId (non-empty string), capUsd (number, minimum 0)}`, mirroring the CRD schema's own field-level constraints. Also added a helm-unittest suite (new to this chart -- none of the 3 chart `tests/` directories in this repo use the helm-unittest framework today, they're all `helm test` hook Pods for post-install smoke checks instead) scoped to this template's new logic: - budgetPolicy omitted from the rendered spec when unset - budgetPolicy.tenants renders verbatim when set - an explicit empty tenants list renders without erroring Wired `helm unittest charts/grid-site` into the "Helm" CI workflow (.github/workflows/helm.yaml), scoped to this one chart rather than introducing an unittest requirement across all 3 charts, which is out of scope here. Re-verified: `helm unittest charts/grid-site` (3/3 passing), full `./scripts/verify-helm-chart.sh` (162/162 passing, uses real example values files -- grid-site's own values.yaml defaults don't satisfy its schema even before this change, pre-existing and unrelated), `actionlint` and `shellcheck` clean on the touched workflow/scripts, `cargo clippy/fmt/ machete` unaffected (no Rust files touched). Signed-off-by: Jordi Gil <jgil@redhat.com>
…stall Discovered while live-validating budgetPolicy on a Kind cluster (helios08): removing budgetPolicy via `helm upgrade` (leaving every other gridNetwork.* value at its falsy chart default) made spec: render with no children at all, i.e. YAML null. The GridNetwork CRD's structural schema requires spec to be present as an object, so the API server rejected it with "spec: Required value" on both create and update. Confirmed this pre-dates this branch by reproducing the same failure against an unmodified checkout of origin/main's chart via kubectl create --dry-run=server. Filed as grid#49 for traceability. gridId is now rendered unconditionally (defaulting to "", which the CRD already treats as the server-side default) instead of being gated behind `with`, guaranteeing spec always has at least one key. Verified on a live Kind cluster on helios08: - kubectl create --dry-run=server on the minimal-fields render now succeeds (previously: "spec: Required value") - helm upgrade removing budgetPolicy from an existing release now succeeds and status.budgetStatus clears back to empty - full KIND=1 ./scripts/verify-helm-chart.sh (182 checks) still passes Added a helm-unittest regression case asserting spec.gridId is always present when only the required name fields are set. Fixes praxis-proxy#49 Signed-off-by: Jordi Gil <jgil@redhat.com>
- Pin helm-unittest to v0.8.2 in the Helm CI workflow. v1.x's plugin.yaml uses the platformCommand/platformHooks manifest schema, which the pinned Helm v3.17.3 cannot load; v0.8.2 is the last release using the command/hooks schema Helm 3.17 supports. - Stop clearing tenant_spend on SWIM origin eviction (remove_origin). tenant_spend is a cumulative grow-only ledger; wiping it on ordinary membership churn let spendRatio drop on a restart/blip and reopen an already-exhausted budget. Follow-up design for an explicit budget-epoch/window reset (and bounding per-tenant site-slot growth) is tracked in grid#52. - Wire validate_budget_policy into GridNetwork's reconcile path via a new reject_invalid_budget_policy helper, so blank/duplicate tenantIds and non-finite capUsd values (which the CRD schema's numeric minimum cannot express) are rejected before reconciliation proceeds. Added a proper Display impl for BudgetPolicyValidationError via thiserror so the resulting error message is human-readable, not a Debug dump. Fixes review comments on grid#47. Signed-off-by: Jordi Gil <jgil@redhat.com>
4a1fb5f to
062e071
Compare
3 participants
Summary
Fixes #40 — adds Grid-side plumbing for MVP 1b tenant-budget tracking: a per-tenant budget policy on
GridNetwork, a tenant-keyedGCountercarried in the SWIM-gossiped grid state snapshot, and a derivedspendRatioexposed onGridNetwork.status. Enforcement (degrade to a cheaper backend / reject at 100%) is explicitly out of scope here — that's cross-repo, gateway-sidepraxis-aipolicy-filter work, same precedent as theprovider_routegap tracked in #44.What's built
GridNetworkSpecgainsbudget_policy: Option<BudgetPolicyConfig>, following the siblingscoring_policyfield's serde idiom exactly (same#[serde(default, skip_serializing_if = "Option::is_none")]).validate_budget_policy()rejects negative/NaN/infinite caps and duplicate or blanktenant_ids; the generated CRD schema additionally enforces a numeric minimum oncapUsdas defense in depth.GCounter—GridStateSnapshotgainstenant_spend: BTreeMap<String, GCounter>, reusing thecrdtcrate's already-proven CRDT laws (commutative/associative/idempotent merge) rather than inventing new CRDT theory.StateBroadcastHandler::receive_itemmerges the new field alongside the existingcapabilitiesmerge.GridNetworkStatusgainsbudget_status: Vec<TenantBudgetStatus>, populated per-reconcile via a puretenant_spend_status()/resolve_budget_statuses()pair, mirroring the existingoverlay_statusprecedent. Deliberately not folded intorender_routing_overlay()orscore_backends(): budget is a per-tenant status concern, a different axis from per-backend routing scores.A real bug this surfaced
Writing a live, real-UDP integration test (see below) rather than only mocked unit tests caught a genuine defect:
StateBroadcast::is_metadata_only()only checkedproviders/capabilities, so a broadcast carrying only atenant_spendchange (the common case once provider topology has settled and spend keeps accruing) was silently classified as metadata-only, andreceive_itemskipped the merge entirely — meaning tenant spend would never propagate once provider state stopped churning. Fixed by includingtenant_spend.is_empty()in the check, with regression tests at both thecarries_grid_state()andreceive_itemlevels.Testing (TDD RED → GREEN → REFACTOR, pyramid invariant)
cargo-llvm-cov):validate_budget_policy,spend_ratio,cents_to_usd, thetenant_spendCRDT merge (add-only, per-site-max, idempotent, commutative, associative, serde round-trip, old-wire-shape default),tenant_spend_status/resolve_budget_statuses. 35 scenarios acrosscrdt,operator, andswim.budgetPolicyappears in the OpenAPI schema,deny_unknown_fieldsrejects malformed shape),receive_itemmerge-wiring (including theis_metadata_onlyregression above, both truth values), reconcile wiring thatbudget_statusis populated from policy + merged CRDT state.operator::swim_runtime, live-validated on helios08): three realSwimHandleinstances bound to real UDP sockets — no mocked message-passing. Two sites independently record tenant spend and converge with each other; a third site then joins late ("the partition heals") and must converge to the true cross-site sum purely through SWIM's own gossip. Ties the pipeline together end-to-end by running the sameresolve_budget_statuses()the reconciler calls, against the late joiner's independently-converged view.Validation
cargo clippy --workspace --all-targets -- -D warnings— cleancargo +nightly-2026-03-28 fmt --all -- --check— cleancargo machete— cleancargo test --workspace— all passing, including the new real-UDP convergence testExplicit non-goals (AC5)
ai#130/ai#104and doesn't exist yet — the SWIM-level convergence proof above uses realistic cost values (mirroringscoring::BackendConfig::cost_per_1k_input) as a stand-in for the not-yet-existing gateway-side increment call site.