test: add an e2e case for a client-less CacheRuntime topology - #6175
test: add an e2e case for a client-less CacheRuntime topology#6175btxu-db wants to merge 4 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 @btxu-db. 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. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6175 +/- ##
==========================================
+ Coverage 65.19% 65.24% +0.05%
==========================================
Files 486 486
Lines 34150 34194 +44
==========================================
+ Hits 22263 22309 +46
+ Misses 10136 10135 -1
+ Partials 1751 1750 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds Mooncake E2E coverage for client-less CacheRuntime topologies.
Changes:
- Adds Mooncake runtime manifests, data-path checks, and cleanup validation.
- Builds a reproducible test image with runtime/reporting scripts.
- Integrates the case into GitHub Actions E2E execution.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
.github/scripts/build-all-images.sh |
Builds and loads the Mooncake image. |
.github/scripts/gha-e2e.sh |
Runs the Mooncake E2E case. |
test/gha-e2e/mooncake/test.sh |
Implements E2E assertions and cleanup. |
test/gha-e2e/mooncake/rw_job.yaml |
Tests Mooncake reads and writes. |
test/gha-e2e/mooncake/bad_mount_pod.yaml |
Tests expected PVC mount failure. |
test/gha-e2e/mooncake/dataset.yaml |
Defines the mountless Dataset. |
test/gha-e2e/mooncake/cacheruntime.yaml |
Configures the runtime instance. |
test/gha-e2e/mooncake/cacheruntimeclass.yaml |
Defines client-less topology. |
test/gha-e2e/mooncake/image/Dockerfile |
Builds the Mooncake test image. |
test/gha-e2e/mooncake/image/custom-entrypoint.sh |
Starts master or worker components. |
test/gha-e2e/mooncake/image/reportSummary.sh |
Reports cache-state metrics. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if ! echo "$failed_mount" | grep -qi "fuse mount point"; then | ||
| syslog "WARNING: FailedMount message does not mention the FUSE mount point; the docs' FAQ wording may need updating" | ||
| fi |
| # Install from wheels only (--only-binary) so no package build or setup script | ||
| # runs at install time, and pin every resolved version explicitly so the image | ||
| # e2e builds stays the same from run to run. | ||
| RUN pip install --no-cache-dir --only-binary=:all: \ | ||
| mooncake-transfer-engine-non-cuda==0.3.12.post1 \ | ||
| nvidia-cuda-runtime-cu12==12.8.90 |
| while true; do | ||
| cached=$(kubectl get dataset ${dataset_name} -ojsonpath='{.status.cacheStates.cached}' 2>/dev/null) | ||
| file_num=$(kubectl get dataset ${dataset_name} -ojsonpath='{.status.cacheStates.fileNum}' 2>/dev/null) | ||
| if [[ -n "$cached" ]] && [[ "$cached" != "0B" ]] && [[ "$file_num" != "0" ]]; then |
|
|
||
| make docker-build-all | ||
| docker build -t "${oss_emulator_img}" test/gha-e2e/jindo/oss-emulator | ||
| docker build -t "${mooncake_img}" test/gha-e2e/mooncake/image |
|
|
||
| RAW=$(curl -s http://localhost:9003/metrics/summary) | ||
|
|
||
| if [ -z "$RAW" ]; then |
There was a problem hiding this comment.
Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich.
6e65b52 to
77a3b32
Compare
Mooncake has no POSIX mount semantics, so its CacheRuntimeClass declares only master and worker. No existing e2e case covers that shape: curvine ships a client component, so the client-less path through the controller is untested. The case pins down the behaviour that path is expected to have: - the controller does not panic and the Dataset reaches Bound with the client component omitted (a regression guard for the nil pointer dereference fixed in fluid-cloudnative#6157); - no client DaemonSet or client pods are created, and status.client.phase stays empty; - the ReportSummary script populates status.cacheStates, and cached reflects data written through the cache system's own client; - the Dataset PVC reaches Bound but cannot be mounted by application pods, which is what the docs' FAQ describes. The image is built in-repo from test/gha-e2e/mooncake/image rather than pulled from an external registry, for the same reason as the jindo oss-emulator: e2e runs on every PR, an external image going away turns the whole pipeline red, and the two scripts Fluid invokes inside the image have to be reviewable. The base image is pinned by digest and every resolved pip version is pinned explicitly, installed from wheels only so no package build script runs at image build time. The CacheRuntimeClass template deliberately declares no container resources. A template that sets them while the CacheRuntime does not currently has them overwritten with an empty value on the first reconcile (fluid-cloudnative#6161), and if the resulting rollout flips the Dataset to Failed it does not recover on its own (fluid-cloudnative#6160). Neither is what this case is meant to cover, so it stays clear of both until fluid-cloudnative#6165 lands. Signed-off-by: btxu-db <btxu-db@outlook.com>
Three of the four findings are fixed here. The negative-case pod declares no memory or ephemeral-storage limits. It is never expected to start, so the values are nominal, but a reviewer's cluster may enforce a policy that rejects a pod without them, and the case would then fail for a reason that has nothing to do with what it tests. The image ran as root. Every listening port is above 1024, the components write nothing to the container filesystem, and the tiered store quota reaches Mooncake as a process memory segment rather than a directory, so nothing needs root. Verified by running mooncake_master from the built image as uid 10001: it binds 50051, 8080 and 9003 and keeps serving. The remaining finding, on clear-text protocols, has no code fix. Mooncake ships no TLS variant for either connection: the master's built-in metadata server (--enable_http_metadata_server) only speaks HTTP, and the transfer engine offers only "tcp" and "rdma". Both connections stay inside the cluster, are addressed by service DNS, and carry cache blocks between components of this runtime only. The entrypoint documents this at the call site, and the finding needs a reviewer to mark it as safe. Signed-off-by: btxu-db <btxu-db@outlook.com>
Assert on the FailedMount message instead of only warning. kubelet reports every mount problem under that one reason, so any transient CSI or node error satisfied the wait loop and the negative case passed without ever exercising the documented behaviour. The pattern relaxes whitespace only: $failed_mount concatenates every matching event and the driver is called fuse.csi.fluid.io, so allowing anything between the two terms would bridge "fuse" in one unrelated message and "mount point" in another. Require fileNum to be non-empty before treating the cache state as reported. An empty string is not equal to "0" in bash, so the condition passed as soon as cached was non-zero and never checked fileNum at all. Install the image's Python dependencies from a hash-locked requirements file. Mooncake declares aiohttp, msgpack and requests without an upper bound, so pinning the two top-level packages left 15 of the 17 installed packages to be resolved afresh on every build; a release of any of them could change or break the image on an unrelated PR. --only-binary is kept and --require-hashes now makes pip fail on anything the lock does not cover. Build the e2e-only helper images behind WITH_E2E_TEST_IMAGES. build-all-images.sh is shared with backward-compatibility-e2e.yml, whose test exercises Alluxio only, so every job of its five-version matrix paid for a ~470MB mooncake build and kind load it never used. oss-emulator is gated with it for the same reason. Signed-off-by: btxu-db <btxu-db@outlook.com>
Switch reportSummary.sh's empty-response guard to [[ ]] as requested in review. custom-entrypoint.sh needs its shebang changed along with the brackets. It ran under #!/bin/sh, where /bin/sh is dash on the Debian-based python:3.12-slim image, and dash has no [[ builtin: it resolves [[ as a command name, fails with "[[: not found" and evaluates the condition as false. Both guards in that script -- the unsupported-action check and the FLUID_RUNTIME_CONFIG_PATH check -- would silently stop rejecting anything while still exiting 0. The image ships bash, so the script now declares it. Signed-off-by: btxu-db <btxu-db@outlook.com>
77a3b32 to
01b19d1
Compare
|
cheyang
left a comment
cheyang
left a comment
There was a problem hiding this comment.
Thanks for adding coverage for the client-less topology. I ran the case end to end on a live cluster (ACK, k8s v1.36) with master controllers and it passes: the Dataset reaches Bound with no panic, the worker comes up with no client component, cacheStates and ufsTotal line up, the rw job round-trips 4 MiB with matching md5, and the negative-case pod gets the expected FUSE FailedMount. I also checked the premise the other way: against a pre-#6157 controller the same manifests panic the cacheruntime-controller and leave the Dataset NotBound, so this case is a genuine regression guard.
One thing I would like fixed before merge: the cleanup assertion in wait_runtime_deleted selects on a label the controller never sets, so it passes vacuously and does not actually test GC (details inline). Two smaller robustness notes, on the panic check and reportSummary.sh, are inline as well; they are non-blocking but cheap to address.
| local counter=0 | ||
| while true; do | ||
| local remaining="" | ||
| remaining=$(kubectl get advancedstatefulset,daemonset,svc -l fluid.io/managed-by=fluid -n default -oname 2>/dev/null) |
There was a problem hiding this comment.
wait_runtime_deleted polls kubectl get advancedstatefulset,daemonset,svc -l fluid.io/managed-by=fluid, but the cacheruntime controller labels the AdvancedStatefulSet and headless Services it creates with cacheruntime.fluid.io/name and cacheruntime.fluid.io/component-name (see getCommonLabelsFromComponent in pkg/ddc/cache/component/component_manager.go), not fluid.io/managed-by=fluid. So this selector matches nothing even while those objects still exist, $remaining is empty on the first pass, and the loop breaks right away. Assertion 6 (AdvancedStatefulSet/Service are garbage collected) then passes whether or not GC actually happened.
I reproduced this on a live cluster: creating an ASTS + Service carrying the labels the controller really sets, then running this exact selector, returns empty while both objects are still there. Could we select on cacheruntime.fluid.io/name=<runtime> instead, or assert on the specific object names, so a GC regression would actually fail here? The curvine case has the same pattern, so it may be worth fixing in both.
| # for Bound and report the root cause directly on failure. | ||
| function check_controller_not_panicked() { | ||
| local logs="" | ||
| logs=$(kubectl logs -n fluid-system -l control-plane=cacheruntime-controller \ |
There was a problem hiding this comment.
This scans only the current container logs. When the client-less topology triggers the #6157 nil-pointer panic, the controller container restarts and the panic trace ends up in the previous container's logs, while the fresh post-restart log is clean. So check_controller_not_panicked can report "No panic found" even though the controller did panic and restart, which is exactly the regression this case exists to catch.
Against a pre-#6157 controller I only saw the panic via kubectl logs --previous (restartCount climbed 4 to 6 to 8) while the current-log scan stayed clean. Adding --previous and tolerating its failure when there is no previous container would close the gap. Non-blocking since the passing path has no panic, but it weakens the guard.
|
|
||
| CACHED_RAW=$(echo "$MEM_LINE" | sed -E 's/Mem Storage: ([^/]+) \/.*/\1/' | xargs) | ||
| CAPACITY_RAW=$(echo "$MEM_LINE" | sed -E 's/.*\/ ([^(]+) \(.*/\1/' | xargs) | ||
| PERCENT_RAW=$(echo "$MEM_LINE" | grep -oE '\([0-9.]+%\)' | tr -d '()%') |
There was a problem hiding this comment.
Under set -euo pipefail this grep exits 1 when there is no percentage group, which aborts the script. That happens in the master-only window before any worker segment registers: /metrics/summary reports Mem Storage: 0 B / 0 B with no (N%), so grep -oE '\([0-9.]+%\)' finds nothing and returns 1.
In the e2e flow the worker is up before this runs, so it does not bite the test, but the script is not safe to run standalone against a master with no workers. Appending || true like the MEM_LINE line above, or defaulting PERCENT_RAW, would make it robust.
Ⅰ. Describe what this PR does
Adds an e2e case covering a client-less CacheRuntime topology — a
CacheRuntimeClasswhosetopologydeclares onlymasterandworker, with noclientcomponent.No existing case covers that shape: the curvine case ships a client component, so the client-less path through
cacheruntime-controlleris currently untested. Mooncake is used as the concrete cache system because it genuinely has no POSIX mount semantics — applications talk to the cache service through its own client library rather than a mount point.test/gha-e2e/mooncake/— the case itself (CacheRuntimeClass/Dataset/CacheRuntimemanifests, a read/write Job, a negative-case pod, andtest.sh).test/gha-e2e/mooncake/image/— the image build context: a Dockerfile on top ofpython:3.12.13-slimplus the two scripts Fluid invokes (custom-entrypoint.sh,reportSummary.sh)..github/scripts/build-all-images.sh— build and kind-load${IMG_REPO}/mooncake:e2e..github/scripts/gha-e2e.sh— run the new case aftercurvine_e2e.Ⅱ. Does this pull request fix one issue?
NONE
Ⅲ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.
This PR is itself an e2e case. It asserts:
panic:/nil pointer dereferencewhile waiting, so a regression reports its root cause instead of just a Bound timeout.cacheruntime.status.client.phasestays empty. (status.clientdoes exist as{"phase":""}andspec.clientis filled in by CRD defaults, so the empty phase is the actual criterion.)status.cacheStatesgets populated,ufsTotal == cacheCapacityfor this UFS-less system, andcached/fileNumreflect data actually written.volumes/volumeMountsanywhere in the pod.FailedMountevent mentioning the missing FUSE mount point. This is the behaviour the docs' FAQ describes, pinned down so it cannot go stale silently.On failure the case dumps controller logs,
describeoutput for the CacheRuntime / Dataset / stuck pod, job logs and namespace events.Ⅳ. Describe how to verify it
against a kind cluster with Fluid deployed and
fluidcloudnative/mooncake:e2eloaded, which is what.github/scripts/build-all-images.shand.github/scripts/gha-e2e.shwire up for CI. The case was run end to end locally on kind.Ⅴ. Special notes for reviews
Guards fix: avoid nil pointer dereference in CacheRuntime configmap builder #6157. That fix is already merged; without it the controller panics on this topology and the case fails at
wait_dataset_bound, which is precisely the regression this case is here to catch.Related to docs: add a Mooncake CacheRuntime sample for client-less cache systems #6163, which documents this same client-less setup and now carries the same build context under
samples/mooncake/docker/. The two copies are kept in sync deliberately: the sample is meant to be read and modified, while this one is pinned for reproducibility (base image by digest, every resolved pip version explicit, wheels only so no package build script runs at image build time). If reviewers would rather have a single copy, I am happy to make one reference the other once both land.The image is built in-repo rather than pulled from an external registry, following
test/gha-e2e/jindo/oss-emulator: e2e runs on every PR, an external image going away turns the pipeline red, and the two scripts Fluid invokes need to be reviewable. The only external dependency is Mooncake's official PyPI package.python:3.12.13-slimis pinned by digest; the non-slim variant would grow the image from ~470MB to ~1.9GB, which e2e then has to kind-load as well.On the SonarCloud findings. Three are fixed in the second commit: the negative-case pod now declares memory and ephemeral-storage limits, and the image runs as an unprivileged user (uid 10001). The latter was verified rather than assumed, by running
mooncake_masterfrom the built image as that user: it binds 50051, 8080 and 9003 and keeps serving. Nothing needs root here since every port is above 1024, the components write nothing to the container filesystem, and the tiered store quota reaches Mooncake as a process memory segment rather than a directory.The fourth, on clear-text protocols, has no code fix available. Mooncake ships no TLS variant for either connection: the master's built-in metadata server (
--enable_http_metadata_server) only speaks HTTP, and the transfer engine offers onlytcpandrdma. Both connections stay inside the cluster, are addressed by service DNS, and carry cache blocks between components of this runtime only.custom-entrypoint.shdocuments this at the call site; the hotspot needs a reviewer to mark it as safe.One worker replica, since the CI kind cluster is single-node and replica scaling is already covered by the curvine case.
test/gha-e2e/mooncake/cacheruntimeclass.yamldeliberately declares noresourceson its containers, and says why in a comment. A template that sets resources while the CacheRuntime does not currently has them overwritten with an empty value on the first reconcile ([BUG]containerresourcesin CacheRuntimeClass are silently dropped #6161), and if the resulting rollout flips the Dataset toFailedit does not recover on its own ([BUG]CacheRuntime: Dataset staysFailedafter the runtime recovers #6160). Neither is what this case is meant to cover, so it stays clear of both. Once fix(cache): preserve CacheRuntimeClass template resources when unset #6165 lands, resources can be declared here and asserted on.