Add Helix REST guard rails for known rebalance-failure scenarios (CICP-3788) - #204
Draft
arkmish wants to merge 3 commits into
Draft
Add Helix REST guard rails for known rebalance-failure scenarios (CICP-3788)#204arkmish wants to merge 3 commits into
arkmish wants to merge 3 commits into
Conversation
Scoping design for guard rails on known rebalance-failure scenarios in Helix REST: instance removal/capacity shrink, ACM stoppable-check completeness, EVACUATE, and resource onboarding. Recommends a shared RebalanceFeasibilityEvaluator in helix-core that reuses existing 'compute assignment without applying' primitives (the same ones behind ResourceAssignmentOptimizerAccessor). Documents code placement, helix-rest integration, customer usage/migration, testing strategy, and backward compatibility (enforce-by-default + force bypass). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Implements the helix-core foundation from docs/design/002: a ZooKeeper-free invariant checker over a computed (un-applied) rebalancer assignment, plus FeasibilityResult / FeasibilityViolation value types. Checks: - min-active-replica: flags partitions that would drop below minActiveReplicas, using the same active/unhealthy-state definition as InstanceValidationUtil. - WAGED capacity: reuses WagedValidationUtil.validateAndGetInstanceCapacity to flag instances missing required capacity keys; no-ops on non-WAGED clusters. - no-unassigned-partition: flags expected partitions left unassigned. - FeasibilityResult.merge aggregates per-check results for the REST layer. Built test-first; TestRebalanceFeasibilityEvaluator covers all four with 12 cases. REST accessor wiring (PerInstanceAccessor / ResourceAccessor / ACM stoppable) and the ZK-bound assignment computation are follow-ups. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ls (CICP-3788) Implements Step 2 of docs/design/002: pre-flight guard rails on two mutating PerInstanceAccessor endpoints, each enforced by default with a ?force=true bypass. - deleteInstance: derives a post-drop assignment from each resource's current ExternalView and runs the evaluator's min-active-replica check (using the same active/unhealthy-state definition as the controller). Returns 400 when dropping the instance would push a hosted partition below minActiveReplicas. - updateInstanceConfig (update command): simulates the persisted ZNRecord.merge and runs the evaluator's capacity check. Returns 400 when the merged config would be missing a required WAGED capacity key. No-ops on non-WAGED clusters. Both return a uniform 400 with violation details. Adds two integration tests to TestPerInstanceAccessor (block + force-bypass for each); full class (37 tests) green. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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.
Issues
Tracked internally as JIRA CICP-3788 — "Identify guard rails in Helix REST for known failure scenarios". There is no public GitHub issue; this is the first code increment for that work, preceded by a design doc.
Description
What: adds pre-flight "guard rails" to mutating Helix REST endpoints that reject operations which would make the cluster un-rebalanceable, plus the design doc that scopes the effort.
Why: operations like dropping an instance or shrinking instance capacity are accepted unconditionally today and only fail later, on the controller's next WAGED rebalance (min-active-replica / capacity violations), far from the API call that caused them.
How:
RebalanceFeasibilityEvaluator— a ZooKeeper-free invariant checker over a computed (un-applied) assignment, withFeasibilityResult/FeasibilityViolationvalue types. Checks: min-active-replica, WAGED instance capacity, and no-unassigned-partition, plus amergeto aggregate per-check results. Reuses the canonical definitions inWagedValidationUtilandInstanceValidationUtil.PerInstanceAccessor, enforce-by-default with a?force=truebypass and a uniform400response carrying violation details:DELETE /clusters/{c}/instances/{n}— runs the min-active-replica check on a post-drop assignment derived from each resource's current ExternalView.POST /clusters/{c}/instances/{n}/configs(update command) — simulates the persistedZNRecord.mergeand runs the capacity check; no-ops on non-WAGED clusters.This is the first increment; remaining guard rails (add-resource, ACM stoppable, EVACUATE placement-recompute) are follow-ups described in the design doc.
Tests
The following tests are written for this issue:
helix-core:TestRebalanceFeasibilityEvaluator— 12 cases (min-active-replica flagged/satisfied/unset/multi; capacity missing-key/present/no-keys; no-unassigned missing/empty/assigned; merge aggregate/all-feasible). Built test-first (TDD).helix-rest:TestPerInstanceAccessor#testDeleteInstanceMinActiveReplicaGuardRailand#testUpdateInstanceConfigCapacityGuardRail— each asserts the block (400) and the?force=truebypass (200) through the full REST + embedded-ZK stack.Local code review completed
The following is the result of the "mvn test" command on the appropriate module:
Changes that Break Backward Compatibility (Optional)
My PR contains changes that break backward compatibility or previous assumptions for certain methods or API. They include:
DELETE /clusters/{c}/instances/{n}now returns400instead of200when dropping the instance would push a hosted partition belowminActiveReplicas. Callers that must proceed can pass?force=trueto preserve the prior behavior.POST /clusters/{c}/instances/{n}/configs(update) now returns400instead of200when the merged config would be missing a required WAGED capacity key. Same?force=truebypass. Non-WAGED clusters are unaffected (the check no-ops).Both endpoints gained an additive
forcequery param (defaultfalse); existing query strings are unaffected.Documentation (Optional)
Design doc added in-repo at
docs/design/002-helix-rest-guard-rails.md(followsdocs/design/000-TEMPLATE.md).Commits
Code Quality
(helix-style-intellij.xml if IntelliJ IDE is used)
🤖 Generated with GitHub Copilot CLI