feat: re-land graceful scale-down for AlluxioRuntime with e2e test#6061
feat: re-land graceful scale-down for AlluxioRuntime with e2e test#6061jakharmonika364 wants to merge 21 commits into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @jakharmonika364. Thanks for your PR. I'm waiting for a fluid-cloudnative member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new end-to-end test suite for testing the graceful scale-down of AlluxioRuntime, including the test script and associated Kubernetes resource manifests. It also exposes the feature gate flags in the Alluxio controller command. The review feedback highlights a critical missing blank import required to register the feature gates, and suggests improvements to the E2E test script, such as simplifying the wait-loop logging logic and properly scoping variables as local to prevent global namespace pollution.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
Fixed the wait_dataset_bound and wait_job_completed issues. The blank import suggestion isn't needed though - pkg/ddc/alluxio already imports pkg/features directly (replicas.go), and cmd/alluxio/app/alluxio.go imports pkg/ddc/alluxio, so the init() already runs. Verified --feature-gates works via go run ./cmd/alluxio start --help. |
656ceff to
5860b3b
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6061 +/- ##
==========================================
+ Coverage 65.08% 65.32% +0.23%
==========================================
Files 485 487 +2
Lines 33989 34281 +292
==========================================
+ Hits 22122 22393 +271
- Misses 10126 10141 +15
- Partials 1741 1747 +6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Re-lands the AlluxioRuntime graceful worker scale-down feature by adding a new Kind-based GitHub Actions e2e scenario and exposing the --feature-gates flag on the alluxioruntime-controller binary so the gate can be enabled in real deployments.
Changes:
- Add a new
test/gha-e2e/alluxio-scaledown/e2e scenario that scales an AlluxioRuntime from 2→1 and verifies data access before/after. - Expose feature gates on
alluxioruntime-controllerviaDefaultMutableFeatureGate.AddFlag(...). - Wire the new e2e scenario into
.github/scripts/gha-e2e.shso it runs in CI.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/gha-e2e/alluxio-scaledown/test.sh | New e2e driver script that enables the gate, creates runtime, scales down, and re-reads data |
| test/gha-e2e/alluxio-scaledown/read_before_job.yaml | Job used to validate data access before scale-down |
| test/gha-e2e/alluxio-scaledown/read_after_job.yaml | Job used to validate data access after scale-down |
| test/gha-e2e/alluxio-scaledown/dataset.yaml | Dataset + 2-replica AlluxioRuntime manifest for the scale-down scenario |
| cmd/alluxio/app/alluxio.go | Adds --feature-gates flag exposure for the alluxio controller binary |
| .github/scripts/gha-e2e.sh | Runs the new alluxio scale-down e2e scenario in the CI e2e sequence |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
c3028aa to
af5935d
Compare
|
Found why CI was failing: this branch forked from master before #6059's revert merged, so the PR's merge-ref was silently dropping the whole feature (pkg/features, replicas.go logic, etc.) since my commits never touched those files directly. Rebuilt the branch off current master with the revert reverted, then replayed the e2e/flag-fix commits on top - --feature-gates=GracefulWorkerScaleDown=true now actually registers. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| - mountPoint: s3://scaledown/ | ||
| name: scaledown | ||
| options: | ||
| alluxio.underfs.s3.endpoint: "http://scaledown-minio:9000" |
There was a problem hiding this comment.
Leaving this as http - it's an in-cluster only MinIO with throwaway test creds, same as curvine's existing endpoint_url: http://minio:9000 fixture. No real traffic to protect here.
|
@cheyang can you please review this once |
|
Hey @jakharmonika364 — this PR has picked up merge conflicts since the last review round (likely from recent master changes). Could you rebase on top of the latest master and force-push? The code itself looks good and was ready to merge on the last pass, so once the conflicts are resolved we can move forward quickly. git fetch upstream master
git rebase upstream/master
# resolve any conflicts
git push --force-with-lease |
e8c3871 to
d8fa056
Compare
|
Done - rebased on top of upstream/master and resolved the merge conflict in |
|
/copilot review |
|
@gemini-code-assist /review |
There was a problem hiding this comment.
Code Review
This pull request introduces a graceful worker scale-down feature (GracefulWorkerScaleDown) for AlluxioRuntime to decommission workers before they are terminated, allowing the master to migrate cached blocks. It includes decommissioning operations, replica synchronization logic, a new feature gate, and E2E tests. The review feedback highlights three key improvement opportunities: skipping decommissioning for pending worker pods that lack a HostIP to prevent blocking the scale-down process, avoiding redundant decommissioning executions across reconciles by checking if the process is already tracked, and removing a redundant status update call to prevent unnecessary API overhead.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
7d70787 to
19d47c1
Compare
|
/copilot review |
…ort lines Skip pods with a DeletionTimestamp when collecting decommission targets. Kubernetes has no distinct "Terminating" Phase - a pod keeps reporting Running throughout its grace period - so without this check, a pod from an earlier, already-successful drain could be re-targeted: this engine only lowers the StatefulSet's Spec.Replicas once a drain succeeds, and workers.Status.Replicas (used to decide whether to enter the decommission block at all) stays elevated until the terminating pod is actually gone, which can briefly re-trigger this scan. A narrower fix than switching that trigger to Spec.Replicas: the existing comment there documents a deliberate reason to prefer Status.Replicas (avoiding under-counting pods whose spec update already landed), and this change doesn't need to touch it. Also bracket IPv6 host IPs when building the host:port decommission address (unbracketed would produce an invalid pair), and trim each line before matching the "Worker Name" header in the capacity report parser for robustness against incidental leading whitespace. Signed-off-by: Monika Jakhar <jakharmonika364@gmail.com>
|
@cheyang ready for another look |
|
/copilot review |
|
@gemini-code-assist /review |
There was a problem hiding this comment.
Code Review
This pull request introduces a graceful worker scale-down feature for AlluxioRuntime, allowing targeted workers to decommission and migrate their cached blocks to surviving workers before being terminated by the StatefulSet controller. The changes include implementing decommissioning operations, tracking decommissioning states via runtime conditions, and adding comprehensive unit and E2E tests. The review feedback highlights two important improvements: refining the failure detection logic in decommissionOutputIndicatesFailure to avoid false positives (such as matching "failover"), and handling cases where container arguments are null in the E2E test script to prevent jq execution errors.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
…d jq null
"fail" as a bare substring is too broad for a best-effort stdout scan: HA
Alluxio deployments routinely log master failover activity, and a
summary line reporting zero failures ("0 workers failed") still contains
it - either would be misread as an actual failure. Narrow to "failed to",
still specific enough to catch a genuine per-worker failure message
without matching those benign cases. Added a test asserting output like
that no longer trips the check.
Also guard the jq read of the controller deployment's existing args in
test.sh: if the deployment has no args defined at all, jq returns null
for that field, and appending to null later in the script would be a
runtime error rather than treating it as starting from empty.
Signed-off-by: Monika Jakhar <jakharmonika364@gmail.com>
|
@cheyang ready for another look, both suggestions fixed |
|
/copilot review |
|
@gemini-code-assist /review |
There was a problem hiding this comment.
Code Review
This pull request introduces a graceful worker scale-down feature (GracefulWorkerScaleDown) for AlluxioRuntime, which decommissions targeted workers before pod termination so that their cached blocks can migrate to surviving workers. It includes new status conditions, CLI wrappers, unit/E2E tests, and a feature gate. Feedback on the changes highlights a potential issue where scaling down to 0 replicas would unnecessarily trigger the decommissioning logic and cause a 10-minute delay, suggesting that this logic should be skipped when the desired replica count is 0.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Graceful decommission exists to redistribute cached blocks to surviving workers before they're removed. Scaling down to zero replicas leaves no survivors to redistribute to, so it has nothing to accomplish there - only a guaranteed wait for defaultWorkerDecommissionDeadline (10m) before falling back to the same ungraceful scale-down it would have started with anyway. spec.replicas has no enforced minimum (no AlluxioRuntime validating webhook, no kubebuilder Minimum marker on this field) and status.go already treats Replicas()==0 as a legitimate, expected state, so this isn't an unreachable edge case worth ignoring. Gate the whole graceful-scale-down block on runtime.Replicas() > 0 in addition to the existing scale-in check. Signed-off-by: Monika Jakhar <jakharmonika364@gmail.com>
|
ready for another look @cheyang, the suggestion by bot had implemented |
|
/copilot review |
|
@gemini-code-assist /review |
There was a problem hiding this comment.
Code Review
This pull request introduces the GracefulWorkerScaleDown feature gate for AlluxioRuntime, which decommissions targeted workers during a scale-in before their pods are terminated, allowing the Alluxio master to migrate cached blocks to surviving workers. The implementation includes new Alluxio CLI operations for decommissioning and counting active workers, updates to the replica synchronization logic to handle the decommissioning state machine with a 10-minute timeout, and comprehensive unit and end-to-end tests. Additionally, a retry loop was added to the Curvine E2E read job to improve robustness. There are no review comments to address, and I have no further feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
…graceful path Traced by cheyang: the WorkerDecommissioning condition only cleared on a clean drain-to-completion inside the graceful-scale-down block. Any other way of leaving that state - scaling straight to zero (bypassed by the prior commit's guard), scaling back up past the target before a drain finished, or the feature gate being disabled mid-flight - left it stranded at Status=True with its original LastTransitionTime. A later, unrelated scale-down would then read that stale timestamp as its own decommissionStart. Since decommissionStart only resets when !alreadyTracked, and alreadyTracked keys off Status==True, the new attempt would already look like it's run out defaultWorkerDecommissionDeadline on its very first reconcile - forcing scale-down through immediately instead of attempting a graceful drain at all, silently dropping the guarantee for that scale-down. Add an else-if alongside the existing entry gate: whenever the block isn't entered but a decommission attempt is still tracked, clear it. Covers the scale-to-zero case directly and the pre-existing scale-back-up case as the same fix, since both are instances of the same invariant (no tracked condition should outlive an active graceful attempt). Signed-off-by: Monika Jakhar <jakharmonika364@gmail.com>
|
@cheyang all suggestions implemented, ready for another look |
|
/copilot review |
|
@gemini-code-assist /review |
There was a problem hiding this comment.
Code Review
This pull request introduces a graceful worker scale-down feature for AlluxioRuntime on standard Kubernetes StatefulSets, gated by the new Alpha feature gate GracefulWorkerScaleDown. When enabled, workers targeted for removal are decommissioned via the Alluxio master before their pods are terminated, allowing cached blocks to migrate to surviving workers. The implementation includes new API status conditions, decommissioning operations, integration within the replica synchronization loop with a 10-minute timeout deadline, comprehensive unit tests, and a new GHA E2E test suite. Additionally, a minor robustness improvement was made to the Curvine E2E read job. There are no review comments, so I have no feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
…the spec getDecommissionAddresses assumed the worker web port could always be read from runtime.Spec.Worker.Ports (falling back to the Alluxio default). That holds only when the port is pinned. In host network mode - the default, since DefaultNetworkMode is treated as host network - allocatePorts assigns an unpinned worker web port dynamically from the operator's port range and writes it only into the Helm values used to render the worker container spec; it is never written back to runtime.Spec.Worker.Ports. So the common, default configuration would target a port the worker never bound, decommissionWorker would silently no-op against it, the drain would never converge, and after defaultWorkerDecommissionDeadline SyncReplicas would force an ungraceful scale-down - exactly the data loss this feature exists to prevent. Fix by reading the port each pod's own alluxio-worker container actually binds (workerWebPortFromPod), which is correct regardless of network mode or whether the port was pinned or allocated. getWorkerWebPort/ runtime.Spec.Worker.Ports remains only as a defensive fallback for a pod whose spec is missing the port for some other reason. Matching is scoped to the alluxio-worker container specifically, since alluxio-job-worker exposes a differently-numbered port of the same name "web" and would otherwise be picked up by mistake. Reported by cheyang, reproduced via a unit test comparing getWorkerWebPort against the port a default (host-network, unpinned) runtime's worker container actually binds. Signed-off-by: Monika Jakhar <jakharmonika364@gmail.com>
…, not UFS read_after_job.yaml's read succeeded on a cache miss just as well as a cache hit: fixture.txt lives in the minio-backed S3 UFS for the entire test, and Alluxio transparently re-fetches from UFS on a miss. So the read-after check passed regardless of whether graceful decommission actually migrated any cached block off the worker being removed - it would pass identically for an ungraceful scale-down, or a decommission that silently no-ops. Given the revert this PR re-lands (fluid-cloudnative#6059) was specifically about validating data integrity across the drain, this made the check that matters least meaningful. Tear the minio backend down between the two read jobs: read_before still primes the Alluxio cache against a live UFS (via the default CACHE_PROMOTE read type), but by the time read_after runs, UFS is gone and the only way its cat can succeed is if the data is still cached on a surviving worker - either because it was already there, or because decommission migrated it off the worker being removed. This distinguishes graceful from ungraceful scale-down the way the maintainer asked for, without needing to exec the Alluxio CLI or assert on cache-hit metrics from the test script. Suggested by cheyang. Signed-off-by: Monika Jakhar <jakharmonika364@gmail.com>
|
|
Hey @cheyang, all reviews implemented, no CI failure, ready to another look. Thanks! |
|
/copilot review |
|
@gemini-code-assist /review |
There was a problem hiding this comment.
Code Review
This pull request introduces the GracefulWorkerScaleDown feature gate for AlluxioRuntime. When enabled, workers targeted for removal during a scale-in are decommissioned from the Alluxio cluster before their pods are terminated, allowing the master to migrate cached blocks to surviving workers. The implementation includes new operations to decommission workers and count active workers, integration into the replica synchronization logic with a configurable deadline, extensive unit tests, and a new end-to-end test suite. Additionally, a retry loop was added to the Curvine E2E read job to improve robustness. There are no review comments to address, and I have no further feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Re-lands #5805, reverted in #6059 because it had no e2e coverage of the actual scale-down -> drain -> decommission -> data-integrity path.
This PR adds that e2e test under test/gha-e2e/alluxio-scaledown/: binds a 2-replica AlluxioRuntime, reads data, scales down to 1 replica, waits for the worker StatefulSet to actually converge, then reads again.
While wiring this up I found GracefulWorkerScaleDown was registered as a feature gate but never exposed as a flag on the alluxioruntime-controller binary (cmd/alluxio/app/alluxio.go), unlike csi and fluidapp, so it was unreachable in any real deployment. Fixed that too - the e2e test enables it via a deployment patch since there's no Helm value for it yet.
Original feature code is unchanged from #5805.