Add partition-weight guard rail for addWagedResource - #219
Open
sjainit wants to merge 4 commits into
Open
Conversation
Block adding a WAGED resource whose per-partition weight exceeds the largest single instance's capacity in any dimension, which would make the partition permanently unplaceable. Existing addWagedResource validation only checks that weight keys are present, never their magnitude, so today such a resource is accepted into ZooKeeper and only fails later at rebalance time. This rule closes that gap by pre-validating the mutation on the REST endpoint. - New PartitionWeightCapacityGuardrailRule computes, per capacity dimension, the maximum capacity advertised by any single instance and fails the mutation when a partition's effective weight exceeds it. - GuardrailContext carries the proposed ResourceConfig so rules can read the to-be-written weights before the object exists in ZK. - ResourceAccessor.addResource wires the rule into the addWagedResource path with force/dryRun, mirroring the existing instance-drop guard rail. - Unit tests for the rule plus an integration test for the endpoint (enforce, dry-run, force bypass, within-capacity happy path). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
sjainit
requested review from
LZD-PratyushBhatt,
arkmish,
laxman-ch,
ngngwr and
thestreak101
as code owners
August 4, 2026 10:50
LZD-PratyushBhatt
requested changes
Aug 14, 2026
The PARTITION_CAPACITY_MAP is operator-supplied and can carry stale or mistyped entries naming partitions the resource does not actually have (e.g. leftovers after lowering NUM_PARTITIONS). WAGED ignores such ghost entries at placement time and ZKHelixAdmin.validateWeightForResourceConfig tolerates them on the write path, so blocking on them made the guard rail stricter than the operation it fronts and produced false positives on valid resources. Thread the proposed IdealState through GuardrailContext and skip any weight-map key that is neither DEFAULT nor a real partition of the resource. Real partition names come from the ideal state's partition set when populated, otherwise from NUM_PARTITIONS via Helix's canonical <resource>_<index> naming (a freshly-proposed WAGED ideal state has no assignment yet, so its partition set is empty at pre-validation time). When no ideal state is supplied the rule falls back to evaluating every key, preserving prior behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…acity, no misblame Fixes three reviewer findings on the partition-weight guard rail: 1. force/dryRun were on the shared addResource dispatch method but only honored in the addWagedResource branch, so dryRun=true on a plain addResource silently performed a real write. Reject both flags with a 400 for any command other than addWagedResource so a "simulation" can never mutate ZK. 3. The rule used getOrDefault(dimension, 0), so when the cluster declared a capacity key the instances did not advertise, a resource weight in that dimension was reported as "exceeds capacity 0" and the resource author was told to lower a weight that cannot go below 0. That missing capacity key is an instance-side misconfiguration already reported by WagedValidationUtil, so skip the dimension instead (mirroring the existing weight == null skip) rather than misblaming the resource. 5. maxInstanceCapacity folded over every InstanceConfig, including non-assignable ones (EVACUATE / SWAP_IN / UNKNOWN). WAGED only places on getAssignableInstanceConfigMap() instances, so counting a decommissioning instance's capacity let the rule certify a resource WAGED can never place. Filter to InstanceConfig.isAssignable() instances, matching the rebalancer. Also documented the fail-closed behavior of the getChildValues(..., true) instance-config read. Adds unit tests for the assignable-only and missing-dimension cases and an integration test asserting force/dryRun are rejected for a non-waged command. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Tier 2 (linkedin#4): Document the cluster-wide blast radius of an unplaceable WAGED resource (CAPACITY_DEFICIT stalls the global rebalance so nothing added afterward gets placed) and stop suggesting force=true, which is the exact action that triggers the deficit. The force capability itself is retained. Tier 3: - linkedin#6: Collect and report every over-capacity partition/dimension in a single verdict (deterministic order: DEFAULT first then natural), so a caller sees all problems at once instead of fixing one and resubmitting. - linkedin#7: Run cheap local structural checks (IdealState/ResourceConfig name match, non-negative weights) before building the guard rail context, so a dry-run reflects them and a structurally invalid request never reaches ZooKeeper. Negative-weight validation is explicit because the endpoint builds ResourceConfig from a raw ZNRecord that bypasses the setter's own check. - linkedin#8: Order the guard rail integration tests after testAddResourceWithWeight via dependsOnMethods. Adds a unit test for multi-violation aggregation/ordering and an integration test asserting structural checks are applied before the guard rail (including the raw-record negative-weight path). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Author
|
@LZD-PratyushBhatt thanks for the thorough review — all eight comments are addressed and each thread has an inline reply with details. Summary: Must-fix
Tier 2 / polish (
Validation: rule unit tests 13/13, |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 a concrete guard rail rule on top of the now-merged guard rail framework (#213):
PartitionWeightCapacityGuardrailRule, wired into theaddWagedResourceREST endpoint. It rejects — before the write reaches ZooKeeper — a WAGED resource whose per-partition weight is larger than any single instance could ever host, which would otherwise be accepted and only surface later as a WAGED rebalance failure.This is the first rule added after the framework landed, so it also serves as a worked example of the framework's extension points (author a rule, add an additive context field, guard another endpoint).
The rule:
PartitionWeightCapacityGuardrailRuleWhat it checks: WAGED places each partition replica on exactly one instance, so a partition can only ever be placed if — for every capacity dimension
d— some instance hascapacity_d >= weight_d. This rule rejectsaddWagedResourcewhen a partition's effective weight in any dimension exceeds the largest single instance's capacity in that dimension, because such a partition is permanently unplaceable no matter how the cluster is arranged.Gap it closes: today
addWagedResourceonly validates that the required weight keys are present (WagedValidationUtil.validateAndGetPartitionCapacitychecks key coverage, never magnitude). So a resource whose weight is larger than any instance can hold is accepted, the write succeeds, and it only surfaces later as a WAGED rebalance failure. This rule compares magnitude against real instance capacity on the write path, so the impossible resource is rejected before it reaches ZooKeeper.It is deliberately a necessary (not sufficient) condition — it compares each dimension independently against the best instance in that dimension, so it never blocks a resource that could plausibly be placed; it only fails the provably-impossible cases. It is a no-op for non-WAGED clusters (no capacity keys), honors cluster-level default instance/partition weights (merged in the same way the rebalancer does), and evaluates the
DEFAULTpartition so resources relying purely on default weights are still checked.How it uses the framework's extension points
GuardrailRule: null-guard → read what it needs from the context (through the read-onlyReadOnlyDataAccessor) → reuse the same weight/capacity primitives WAGED uses → return aValidationResult(feasible, or an infeasible verdict with an actionable message). Mirrors the framework'sLiveInstanceGuardrailRule.GuardrailContext.proposedResourceConfigis added via theBuilder. Existing rules/endpoints are untouched — this is the intended extensibility seam.ResourceAccessor.addResource(theaddWagedResourcecommand) with the sameforce/dryRunpreflight the instance-delete endpoint already uses. No registry, no dispatch — the endpoint just buildsnew GuardrailPipeline(new PartitionWeightCapacityGuardrailRule()).Behavior
PUT /clusters/{cluster}/resources/{resource}?command=addWagedResource(body = map of IdealState + ResourceConfig ZNRecords):400+ JSON verdict, resource not created; within-capacity →200, created.dryRun=true): always200+ verdict, resource never created (validation-probe semantics, consistent with the framework).force=true): proceeds even for an over-weight resource →200, created, with the overridden verdict logged.Tests
TestPartitionWeightCapacityGuardrailRule, 8): null proposed config, null cluster config, no capacity keys (non-WAGED), no instance capacity, weight within capacity, weight exceeding capacity (DEFAULT → unscoped), per-partition override exceeding, and max-across-instances used. Mocked accessor + realClusterConfig/InstanceConfig/ResourceConfig.TestResourceAccessor#testAddWagedResourceWeightGuardrail): full Jersey + embedded ZK, exercising enforce / dryRun / force / within-capacity with status codes, the JSON verdict, and actual resource presence/absence. Saves and restores the cluster + instance capacity configuration intry/finallyso it does not disturb sibling tests.Validated on JDK 11: rule unit tests 8/8 and the full
TestResourceAccessorsuite 18/18 pass.Not in scope
Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com