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 13 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. |
jakharmonika364
force-pushed
the
feat-alluxio-scaledown-e2e
branch
from
656ceff to
5860b3b
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6061 +/- ##
=========================================
Coverage ? 64.97%
=========================================
Files ? 487
Lines ? 34216
Branches ? 0
=========================================
Hits ? 22233
Misses ? 10246
Partials ? 1737 ☔ 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.
jakharmonika364
force-pushed
the
feat-alluxio-scaledown-e2e
branch
from
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 |
jakharmonika364
force-pushed
the
feat-alluxio-scaledown-e2e
branch
from
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.
jakharmonika364
force-pushed
the
feat-alluxio-scaledown-e2e
branch
from
7d70787 to
19d47c1
Compare
|
/copilot review |
|
Requesting changes. The graceful scale-down flow is well structured and the test coverage is solid, but two things block merge as-is. The bigger one is cross-cutting: the drain path depends on The second is that the decommission command's result is trusted too readily — stdout/stderr are dropped and success is taken from a nil error alone, so partial failures reported in the output (or an unsupported subcommand) can be read as "drain launched." Please inspect the command output/exit code and handle partial failures. Details inline. |
cheyang
left a comment
cheyang
left a comment
There was a problem hiding this comment.
Requesting changes. Two blocking items, both detailed in inline comments on pkg/ddc/alluxio/operations/decommission.go:
-
Hard dependency on Alluxio >= 2.9. The drain path calls
alluxio fsadmin decommissionWorker, which doesn't exist on older masters. TheGracefulWorkerScaleDowngate doesn't know the master's Alluxio version, so enabling it against a pre-2.9 image turns every scale-in into a stuck reconcile that only clears at the 10m forced-proceed deadline. Please version-gate or runtime-detect the capability and degrade gracefully on older versions. -
The decommission command result is assumed successful. stdout/stderr are discarded and success is taken from a nil error alone, so partial failures reported in the command output (or an unsupported subcommand) can be read as "drain launched." Please inspect the command output/exit code and handle partial failures; the same care applies to the fixed-layout parsing in CountActiveWorkers/parseActiveWorkerCount.
Happy to re-review once these are addressed.
…mission output Addresses cheyang's review on fluid-cloudnative#6061 (two blocking items) plus a Copilot suggestion on the feature-gates patch in test.sh. 1. Old Alluxio masters (< 2.9, where "fsadmin decommissionWorker" does not exist) previously caused SyncReplicas to stall forever, not just until the 10m deadline as it looked at first glance: returning drainErr immediately bypassed the deadline-tracking logic entirely, since decommissionStart is only ever persisted inside the "not drained" branch. A drain error is now routed through the same deadline-bounded path as "not yet drained", so it's retried and logged at Error level (visible immediately, not masked as the routine errWorkersNotYetDrained) and, past the deadline, the engine degrades to an ungraceful scale-down instead of hanging - the same fallback the codebase already accepts for a stuck/unhealthy drain. 2. DecommissionWorkers now captures stdout/stderr (previously discarded) and does a best-effort scan of stdout for failure indicators, since decommissionWorker can report a per-worker problem while still exiting 0. stderr is deliberately not scanned - Alluxio's CLI is a JVM process that can print benign warnings to stderr on a successful run, matching how every other command wrapper in this package already only inspects stderr once err != nil. The real verification remains CountActiveWorkers on subsequent reconciles; a false positive here just costs one extra bounded retry. 3. parseActiveWorkerCount now returns an error instead of silently returning 0 when the "Worker Name" header is missing from the capacity report. A report this caller can't recognize must not be misread as "every worker has drained", since that's exactly the signal the engine acts on to let a scale-down proceed. 4. test.sh's enable_graceful_scale_down now merges into an existing --feature-gates arg via jq instead of always appending a new one, which would silently override it (last-flag-wins string flag) and disable whatever gates Helm values had already set. Signed-off-by: Monika Jakhar <jakharmonika364@gmail.com>
|
@cheyang I addressed both in 0707c4a - replied inline with details on each. Also merged the Copilot suggestion on the
|
|
/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 the Alluxio runtime, allowing targeted workers to be decommissioned and their cached blocks migrated before pod termination. It adds decommissioning CLI wrappers, integrates them into the replica synchronization logic with a 10-minute deadline fallback, and includes comprehensive unit and E2E tests. The review feedback highlights a critical issue where transient failures during the initial decommission command could prevent subsequent retries because the decommissioning state is immediately marked as tracked. It is recommended to record the success or failure of the decommission command in the runtime condition's reason to ensure robust retries.
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-review at f97dbd5 — both blocking items from the last round look addressed. Alluxio version dependency (blocker A): Exec result handling (blocker B): Secondary changes on top of the fixes look sensible: One prior Copilot note ("the e2e doesn't assert decommissioning actually ran") is already answered in the source: the added comment above CI is green across the matrix (kind-e2e + backward-compat on v1.22.17 / v1.24.17 / v1.28.15 / v1.30.13 / v1.33.2, plus CodeQL, staticcheck, lint, unittest, Sonar, DCO). No new blocking issues from my side — from a code-review standpoint this is ready. Remaining gate is on the maintainer to remove |
…dvancedStatefulSet (fluid-cloudnative#4193) (fluid-cloudnative#5805)" (fluid-cloudnative#6059) This reverts commit 333e13a. Signed-off-by: Monika Jakhar <jakharmonika364@gmail.com>
Adds a real-cluster scenario exercising the scale-down -> drain -> decommission -> data-integrity path: bind a 2-replica AlluxioRuntime, read data, scale down to 1 replica, wait for the worker StatefulSet to actually converge (proving the drain doesn't stall), then read again. Also wires --feature-gates into the alluxioruntime-controller binary (cmd/alluxio/app/alluxio.go), mirroring cmd/csi and cmd/fluidapp - without it GracefulWorkerScaleDown was registered but unreachable on a real deployment, so the e2e test enables it via a deployment patch. Signed-off-by: Monika Jakhar <jakharmonika364@gmail.com>
Simplify wait_dataset_bound's elapsed-time tracking to a single counter (matches wait_worker_replicas), and declare succeed/job_failed as local in wait_job_completed to avoid polluting global scope. Signed-off-by: Monika Jakhar <jakharmonika364@gmail.com>
- check kubectl patch exit codes instead of letting failures surface as confusing downstream timeouts - log the WorkerDecommissioning condition while polling so a stuck drain is debuggable from CI output - replace the external apache.org mirror mount with an in-cluster MinIO bucket seeded with one known object, and assert its exact content before/after scale-down instead of just checking the directory is non-empty - the previous assertion would pass even if decommission silently dropped cached data instead of migrating it Signed-off-by: Monika Jakhar <jakharmonika364@gmail.com>
Signed-off-by: Monika Jakhar <jakharmonika364@gmail.com>
Alluxio mounts s3://scaledown/ at /scaledown inside its namespace (named after the Dataset mount's name), not at the FUSE root. The existing alluxio test never noticed this because its assertion just checks "ls /data" is non-empty, which passes either way. Use the correct /data/scaledown/fixture.txt path so the content check actually exercises the fixture. Signed-off-by: Monika Jakhar <jakharmonika364@gmail.com>
…ommission logic - test.sh: wait for the scaledown-minio deployment rollout before creating the bucket-create job; without it the job could exhaust its retries before minio finished starting, causing a flaky e2e failure on some Kubernetes versions. - replicas.go: skip (instead of aborting the whole batch) a scale-down target pod that hasn't been assigned a host IP yet, so sibling workers that are ready still get decommissioned this reconcile. - replicas.go: avoid re-issuing the (exec-heavy) decommissionWorker call on every reconcile once a decommission attempt is already tracked; only poll the active worker count until the drain completes. Signed-off-by: Monika Jakhar <jakharmonika364@gmail.com>
Without a readinessProbe, "kubectl rollout status" marks the minio deployment ready as soon as the container starts, not once minio is actually listening - so the earlier rollout-status wait didn't fully close the race with the bucket-create job. Add an httpGet probe against minio's own /minio/health/ready endpoint so rollout status genuinely waits for the server to be reachable. Also dump the bucket-create job's pod logs to the test output before panicking on failure, since previously a failure gave no signal at all about what mc actually hit. Signed-off-by: Monika Jakhar <jakharmonika364@gmail.com>
… wait alluxio fsadmin decommissionWorker addresses workers by their web port (default 30000), not the RPC port (29999) used elsewhere - it monitors the worker's workload as exposed on the web server. Rename defaultWorkerRPCPort/getWorkerRPCPort (unused for anything else) to their web-port equivalents and fix the address construction in drainScalingDownWorkers accordingly. Also pass --wait 0s to decommissionWorker: it defaults to 5m, which would otherwise block the exec call - and therefore the engine's reconcile loop - waiting for the worker to idle. The engine already polls CountActiveWorkers across reconciles, so the command only needs to initiate the decommission and return immediately. Signed-off-by: Monika Jakhar <jakharmonika364@gmail.com>
…diagnostics A pod gets a HostIP as soon as it's scheduled, well before its containers actually start (Pending/ContainerCreating), so HostIP alone doesn't mean the Alluxio worker process is up and listening on the web port. Also require pod.Status.Phase == Running before targeting a pod for decommission, consistent with the existing skip-and-continue behavior for not-yet-ready pods. Updated the test fixtures accordingly. Also harden the bucket-create job diagnostics in test.sh: addressing a job by "job/$name" makes kubectl logs resolve the newest pod and can itself hang waiting for that pod to reach Running (as seen in a recent CI run, where it timed out and gave no signal). List the job's pods by label selector, describe each, and fetch logs by pod name with --request-timeout so a stuck pod can't block the diagnostics anymore. Signed-off-by: Monika Jakhar <jakharmonika364@gmail.com>
…mission output Addresses cheyang's review on fluid-cloudnative#6061 (two blocking items) plus a Copilot suggestion on the feature-gates patch in test.sh. 1. Old Alluxio masters (< 2.9, where "fsadmin decommissionWorker" does not exist) previously caused SyncReplicas to stall forever, not just until the 10m deadline as it looked at first glance: returning drainErr immediately bypassed the deadline-tracking logic entirely, since decommissionStart is only ever persisted inside the "not drained" branch. A drain error is now routed through the same deadline-bounded path as "not yet drained", so it's retried and logged at Error level (visible immediately, not masked as the routine errWorkersNotYetDrained) and, past the deadline, the engine degrades to an ungraceful scale-down instead of hanging - the same fallback the codebase already accepts for a stuck/unhealthy drain. 2. DecommissionWorkers now captures stdout/stderr (previously discarded) and does a best-effort scan of stdout for failure indicators, since decommissionWorker can report a per-worker problem while still exiting 0. stderr is deliberately not scanned - Alluxio's CLI is a JVM process that can print benign warnings to stderr on a successful run, matching how every other command wrapper in this package already only inspects stderr once err != nil. The real verification remains CountActiveWorkers on subsequent reconciles; a false positive here just costs one extra bounded retry. 3. parseActiveWorkerCount now returns an error instead of silently returning 0 when the "Worker Name" header is missing from the capacity report. A report this caller can't recognize must not be misread as "every worker has drained", since that's exactly the signal the engine acts on to let a scale-down proceed. 4. test.sh's enable_graceful_scale_down now merges into an existing --feature-gates arg via jq instead of always appending a new one, which would silently override it (last-flag-wins string flag) and disable whatever gates Helm values had already set. Signed-off-by: Monika Jakhar <jakharmonika364@gmail.com>
kind-e2e-test (v1.22.17) failed with the bucket-create job exhausting its retries; the new per-pod diagnostics from a prior commit did fire this time and showed the mc container actually ran and crashed repeatedly (Reason: Error, 4 restarts, kubelet BackOff) rather than being stuck Pending - ruling out a minio-readiness race, which rolled out in ~3s. The container's actual logs were still unavailable though: the pod was already Terminating by the time we looked, most likely because the Job controller kills the active pod right after backoffLimit is exceeded, racing our own kubectl logs call. Retry with --previous, which targets the last fully-exited container instance instead of the one mid-teardown, so it isn't lost to that race. Also pin minio/minio and minio/mc to explicit RELEASE tags instead of :latest. MinIO stopped publishing to Docker Hub and archived minio/minio in early 2026, so :latest is frozen rather than a moving target today - but pinning removes the dependency on however Docker Hub ends up serving :latest for an archived repo going forward, and rules image drift out as a variable in this flake. Signed-off-by: Monika Jakhar <jakharmonika364@gmail.com>
…ipping it drainScalingDownWorkers previously skipped re-issuing DecommissionWorkers whenever a decommission attempt was merely tracked (RuntimeWorkerDecommissioning condition present), regardless of whether that attempt actually reached the Alluxio master. A transient failure (network glitch, master pod restarting) would still record the condition on the first reconcile, so every subsequent reconcile skipped retrying it entirely and just polled a worker count that could never drop - the scale-down would sit doing nothing until the 10m deadline forced an ungraceful proceed, even though a simple retry next reconcile would likely have succeeded. Add RuntimeWorkerDecommissionFailedReason and set it on the condition whenever the attempt that produced it failed, so drainScalingDownWorkers can tell "attempted and failed" apart from "attempted and reached the master" via decommissionSucceeded(), and only skip re-issuing the command in the latter case. decommissionStart continues to anchor the condition's LastTransitionTime across these Reason transitions, so the deadline clock is unaffected by how many times the Reason flips between failed and reached. Signed-off-by: Monika Jakhar <jakharmonika364@gmail.com>
jakharmonika364
force-pushed
the
feat-alluxio-scaledown-e2e
branch
from
0b2cb59 to
91896b6
Compare
|
|
@cheyang — rebased onto latest master in 91896b6 to pick up your review feedback plus commit 676f47a. All 5 kind-e2e-test jobs are showing red, but the alluxioruntime graceful scale-down e2e test this PR actually touches passed cleanly in all 5:
The actual failure in every job is a later, unrelated test case in the same suite: "curvine cache runtime basic e2e" times out with Is this a known issue on your end, or should I open a separate issue for it? Happy to re-run the jobs if you think it's just a flake, but wanted to flag it rather than assume. |
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.