Add helm-chart-fluid-1.1.0-alpha.9 - #118
Conversation
![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 the CacheRuntime and CacheRuntimeClass CRDs along with their controller, while removing the deprecated GooseFSRuntime resources. It also adds the AdvancedStatefulSet CRD, updates the JuiceFSRuntime CRD to support volume claim templates, and introduces comprehensive end-to-end tests for the new Curvine cache runtime and multi-mount Jindo configurations. Feedback on these changes highlights a high-severity privilege escalation risk in the controller's RBAC permissions for cluster roles, a regex bug in the Curvine mount script that skips processing mount paths, and a recommendation to simplify the image pull secrets template logic.
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.
| - apiGroups: | ||
| - rbac.authorization.k8s.io | ||
| resources: | ||
| - clusterroles | ||
| - clusterrolebindings | ||
| verbs: | ||
| - create | ||
| - list | ||
| - get | ||
| - delete |
There was a problem hiding this comment.
The controller is granted create, list, get, and delete permissions on clusterroles and clusterrolebindings. This represents a significant security risk (Privilege Escalation), as a compromised controller could create arbitrary ClusterRoles and ClusterRoleBindings to escalate its own privileges or grant cluster-admin access to other identities. Consider if these cluster-scoped RBAC permissions can be avoided, or if they can be restricted to namespaced roles and rolebindings instead.
| local paths=() | ||
| while IFS= read -r line; do | ||
| # 跳过空行、表头行和分隔线 | ||
| if [[ -z "$line" ]] || [[ "$line" == *"|"*"ID"* ]] || [[ "$line" == *"+"*"-"*"+"* ]] || [[ ! "$line" =~ ^\| ]] ; then |
There was a problem hiding this comment.
The regular expression ^\| (with two spaces) will fail to match the table data lines, which typically start with a single space after the pipe (e.g., | 1664140379 |). As a result, [[ ! "$line" =~ ^\| ]] will evaluate to true for all lines, causing the script to skip processing the actual mount paths. Consider using ^\|[[:space:]] or ^\|[[:space:]]+ to robustly match the table rows.
if [[ -z "$line" ]] || [[ "$line" == *"|"*"ID"* ]] || [[ "$line" == *"+"*"-"*"+"* ]] || [[ ! "$line" =~ ^\|[[:space:]] ]] ; then| {{- $secretList := list }} | ||
| {{- range .Values.image.imagePullSecrets }} | ||
| {{- range $name,$v := . }} | ||
| {{- $secretList = append $secretList $v }} | ||
| {{- end }} | ||
| {{- end }} | ||
| value: {{ join "," $secretList | quote }} |
There was a problem hiding this comment.
Instead of using a nested loop to iterate over all keys and values of each imagePullSecret map, you can directly access the .name field. This is more robust and standard, as imagePullSecrets in Kubernetes are strictly defined with a name key.
{{- $secretList := list }}
{{- range .Values.image.imagePullSecrets }}
{{- $secretList = append $secretList .name }}
{{- end }}
value: {{ join "," $secretList | quote }}
3a6096b to
56651a0
Compare
Signed-off-by: cheyang <cheyang@163.com>
57aec8d to
8a355e6
Compare
Signed-off-by: cheyang <cheyang@163.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the CacheRuntime and CacheRuntimeClass CRDs, adds the cacheruntime-controller deployment and RBAC, and integrates the new curvine cache runtime into the end-to-end testing suite. It also removes the deprecated GooseFSRuntime and its associated controller and RBAC configurations. Feedback on the changes highlights several critical issues in the newly added E2E test scripts and RBAC configurations: a subshell execution issue in the mountUfs.sh script that prevents proper error propagation, a fragile regular expression in the same script that could skip valid mount entries, missing delete permissions for daemonsets and advancedstatefulsets in the controller's RBAC rules, and a missing diagnostic script (tools/diagnose-fluid-curvine.sh) referenced in the cleanup routine.
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.
| echo "$mounts_items" | while IFS= read -r item; do | ||
| #echo "第 $index 个 mounts 元素" | ||
| index=$((index + 1)) | ||
|
|
||
| # 提取基础字段 | ||
| mountPoint=$(echo "$item" | sed -nE 's/.*"mountPoint":"([^"]+)".*/\1/p') | ||
| path=$(echo "$item" | sed -nE 's/.*"path":"([^"]+)".*/\1/p') | ||
|
|
||
| mounted=$(/app/curvine/bin/cv mount) | ||
| if echo "$mounted" | grep "$mountPoint" >/dev/null 2>&1; then | ||
| continue; | ||
| fi | ||
|
|
||
| [ -z "$mountPoint" ] && { echo "mountPoint is not set or empty" >&2; exit 1; } | ||
| [ -z "$path" ] && { echo "path is not set or empty" >&2; exit 1; } | ||
|
|
||
| # ==================== 字段提取加p 仅输出匹配结果 ==================== | ||
| # Extract encryptOptions paths from the JSON | ||
| encryptOptions_raw=$(echo "$item" | sed -nE 's/.*"encryptOptions":\{([^}]+)\}.*/\1/p') | ||
|
|
||
| # Extract access and secret file paths from encryptOptions | ||
| access_path="" | ||
| secret_path="" | ||
| if [ -n "$encryptOptions_raw" ]; then | ||
| access_path=$(echo "$encryptOptions_raw" | sed -nE 's/.*"access":"([^"]+)".*/\1/p') | ||
| secret_path=$(echo "$encryptOptions_raw" | sed -nE 's/.*"secret":"([^"]+)".*/\1/p') | ||
| fi | ||
|
|
||
| # Read actual values from secret files if paths are provided | ||
| access="" | ||
| secret="" | ||
| if [ -n "$access_path" ] && [ -f "$access_path" ]; then | ||
| access=$(cat "$access_path") | ||
| fi | ||
| if [ -n "$secret_path" ] && [ -f "$secret_path" ]; then | ||
| secret=$(cat "$secret_path") | ||
| fi | ||
|
|
||
| endpoint=$(echo "$item" | sed -nE 's/.*"endpoint_url":"([^"]+)".*/\1/p') | ||
| region=$(echo "$item" | sed -nE 's/.*"region_name":"([^"]+)".*/\1/p') | ||
| path_style=$(echo "$item" | sed -nE 's/.*"path_style":"([^"]+)".*/\1/p') | ||
|
|
||
| # 打印调试（缺失字段会显示空，正确） | ||
| #echo "access: $access" | ||
| #echo "region: $region" | ||
| #echo "endpoint: $endpoint" | ||
| #echo "path_style: $path_style" | ||
| #echo "secret: $secret" | ||
|
|
||
| # 必填参数校验 | ||
| CV_PARAMS="" | ||
| [ -z "$endpoint" ] && { echo "endpoint option is not set or empty" >&2; exit 1; } | ||
| [ -z "$access" ] && { echo "access option is not set or empty" >&2; exit 1; } | ||
| [ -z "$secret" ] && { echo "secret option is not set or empty" >&2; exit 1; } | ||
|
|
||
| # 拼接必填参数 | ||
| CV_PARAMS="$CV_PARAMS -c s3.endpoint_url=$endpoint" | ||
| CV_PARAMS="$CV_PARAMS -c s3.credentials.access=$access" | ||
| CV_PARAMS="$CV_PARAMS -c s3.credentials.secret=$secret" | ||
|
|
||
| # 可选参数：缺失自动跳过 | ||
| [ -n "$region" ] && CV_PARAMS="$CV_PARAMS -c s3.region_name=$region" | ||
| [ -n "$path_style" ] && CV_PARAMS="$CV_PARAMS -c s3.path_style=$path_style" | ||
|
|
||
| # 最终命令 | ||
| CMD="/app/curvine/bin/cv mount $mountPoint $path --check-path-consist false $CV_PARAMS" | ||
| #echo "执行命令：$CMD" | ||
|
|
||
| eval "$CMD" > /dev/null 2>&1 | ||
| if [ $? -ne 0 ]; then | ||
| echo "mount $mountPoint failed" >&2 | ||
| exit 1 | ||
| fi | ||
| done |
There was a problem hiding this comment.
In Bash, piping a command into a while loop (e.g., echo "$mounts_items" | while ...) causes the loop to run in a subshell. As a result, any exit 1 inside the loop will only exit the subshell, and the main script will continue executing, silently ignoring mount failures.
Additionally, using eval to execute the constructed command string is fragile and can be avoided by using a Bash array to safely handle arguments.
We can fix both issues by using a here-string (<<<) to run the loop in the main shell and using a Bash array for the command arguments.
while IFS= read -r item; do
#echo "第 $index 个 mounts 元素"
index=$((index + 1))
# 提取基础字段
mountPoint=$(echo "$item" | sed -nE 's/.*"mountPoint":"([^"]+)".*/\1/p')
path=$(echo "$item" | sed -nE 's/.*"path":"([^"]+)".*/\1/p')
mounted=$(/app/curvine/bin/cv mount)
if echo "$mounted" | grep "$mountPoint" >/dev/null 2>&1; then
continue;
fi
[ -z "$mountPoint" ] && { echo "mountPoint is not set or empty" >&2; exit 1; }
[ -z "$path" ] && { echo "path is not set or empty" >&2; exit 1; }
# ==================== 字段提取加p 仅输出匹配结果 ====================
# Extract encryptOptions paths from the JSON
encryptOptions_raw=$(echo "$item" | sed -nE 's/.*"encryptOptions":\{([^}]+)\}.*/\1/p')
# Extract access and secret file paths from encryptOptions
access_path=""
secret_path=""
if [ -n "$encryptOptions_raw" ]; then
access_path=$(echo "$encryptOptions_raw" | sed -nE 's/.*"access":"([^"]+)".*/\1/p')
secret_path=$(echo "$encryptOptions_raw" | sed -nE 's/.*"secret":"([^"]+)".*/\1/p')
fi
# Read actual values from secret files if paths are provided
access=""
secret=""
if [ -n "$access_path" ] && [ -f "$access_path" ]; then
access=$(cat "$access_path")
fi
if [ -n "$secret_path" ] && [ -f "$secret_path" ]; then
secret=$(cat "$secret_path")
fi
endpoint=$(echo "$item" | sed -nE 's/.*"endpoint_url":"([^"]+)".*/\1/p')
region=$(echo "$item" | sed -nE 's/.*"region_name":"([^"]+)".*/\1/p')
path_style=$(echo "$item" | sed -nE 's/.*"path_style":"([^"]+)".*/\1/p')
# 必填参数校验
[ -z "$endpoint" ] && { echo "endpoint option is not set or empty" >&2; exit 1; }
[ -z "$access" ] && { echo "access option is not set or empty" >&2; exit 1; }
[ -z "$secret" ] && { echo "secret option is not set or empty" >&2; exit 1; }
# 构造参数数组
CV_PARAMS=()
CV_PARAMS+=("-c" "s3.endpoint_url=$endpoint")
CV_PARAMS+=("-c" "s3.credentials.access=$access")
CV_PARAMS+=("-c" "s3.credentials.secret=$secret")
# 可选参数：缺失自动跳过
[ -n "$region" ] && CV_PARAMS+=("-c" "s3.region_name=$region")
[ -n "$path_style" ] && CV_PARAMS+=("-c" "s3.path_style=$path_style")
# 最终命令
if ! /app/curvine/bin/cv mount "$mountPoint" "$path" --check-path-consist false "${CV_PARAMS[@]}" > /dev/null 2>&1; then
echo "mount $mountPoint failed" >&2
exit 1
fi
done <<< "$mounts_items"| local paths=() | ||
| while IFS= read -r line; do | ||
| # 跳过空行、表头行和分隔线 | ||
| if [[ -z "$line" ]] || [[ "$line" == *"|"*"ID"* ]] || [[ "$line" == *"+"*"-"*"+"* ]] || [[ ! "$line" =~ ^\| ]] ; then |
There was a problem hiding this comment.
The regular expression ^\| requires the line to start with a vertical bar followed by exactly two spaces. However, the output of cv mount typically has only one space after the vertical bar (e.g., | 1664140379 |). This causes the condition [[ ! "$line" =~ ^\| ]] to evaluate to true for valid mount entries, skipping them entirely and resulting in an empty mount list.
We can make this check more robust by simply verifying that the line starts with a vertical bar ^\|.
if [[ -z "$line" ]] || [[ "$line" == *"|"*"ID"* ]] || [[ "$line" == *"+"*"-"*"+"* ]] || [[ ! "$line" =~ ^\| ]] ; then| - apps | ||
| resources: | ||
| - daemonsets | ||
| - daemonsets/status | ||
| verbs: | ||
| - get | ||
| - list | ||
| - watch | ||
| - create | ||
| - update | ||
| - patch | ||
| - apiGroups: | ||
| - workload.fluid.io | ||
| resources: | ||
| - advancedstatefulsets | ||
| verbs: | ||
| - get | ||
| - list | ||
| - watch | ||
| - create | ||
| - update | ||
| - patch | ||
| - apiGroups: |
There was a problem hiding this comment.
The cacheruntime-controller needs to manage the lifecycle of daemonsets and advancedstatefulsets. If the delete verb is missing from these RBAC rules, the controller will not be able to clean up these resources when a CacheRuntime is deleted or scaled down, leading to orphaned resources and permission errors in the controller logs.
- apiGroups:
- apps
resources:
- daemonsets
- daemonsets/status
verbs:
- get
- list
- watch
- create
- update
- patch
- delete
- apiGroups:
- workload.fluid.io
resources:
- advancedstatefulsets
verbs:
- get
- list
- watch
- create
- update
- patch
- delete|
|
||
| function dump_env_and_clean_up() { | ||
| local exit_code=$? | ||
| bash tools/diagnose-fluid-curvine.sh collect --name $dataset_name --namespace default --collect-path ./e2e-tmp/testcase-curvine.tgz |
There was a problem hiding this comment.
The script tools/diagnose-fluid-curvine.sh is referenced here, but it is not included in this pull request or the repository. If this script is missing, the diagnostic collection will fail during cleanup. Please ensure the diagnostic script is added or use an existing generic diagnostic tool if available.
There was a problem hiding this comment.
Pull request overview
Updates the Fluid Helm chart and accompanying GHA E2E assets to align with 1.1.0-alpha.9, including new CacheRuntime capabilities, removal of GooseFS chart resources, and expanded E2E coverage/diagnostics (notably Jindo secret scenarios and a new Curvine E2E flow).
Changes:
- Bump chart/app versions and default image tag to
1.1.0-alpha.9/1.1.0-36f0467, add CacheRuntime controller values/templates/RBAC/CRDs, and adjust existing RBAC/CRDs accordingly. - Remove GooseFS-related chart templates and CRDs; update CSI prune list accordingly.
- Expand/strengthen E2E: add Curvine E2E suite, enhance Jindo E2E with multi-OSS secret projection coverage (including an OSS emulator), and improve failure diagnostics/timeouts.
Reviewed changes
Copilot reviewed 43 out of 43 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| update-fluid-charts.sh | Copies E2E runner script into .github/scripts during chart update. |
| test/gha-e2e/juicefs/test.sh | Adds failure-time diagnostics logging before cleanup. |
| test/gha-e2e/jindo/test.sh | Major Jindo E2E expansion: multi-scenario + multi-OSS support + better diagnostics. |
| test/gha-e2e/jindo/oss-emulator/main.go | Adds a small OSS-like HTTP emulator for CI multi-mount verification. |
| test/gha-e2e/jindo/oss-emulator/Dockerfile | Builds the OSS emulator image for use in E2E. |
| test/gha-e2e/jindo/oss-emulator.yaml | Deploys two OSS emulator instances (A/B) for multi-mount tests. |
| test/gha-e2e/jindo/multi-oss-job.yaml | Adds job to verify multi-mount OSS data visibility from a pod. |
| test/gha-e2e/jindo/multi-oss-dataset.yaml | Adds dataset/runtime + secrets templates for multi-OSS mounts. |
| test/gha-e2e/jindo/minio.yaml | Extends MinIO setup for both single- and multi-OSS test scenarios. |
| test/gha-e2e/jindo/job.yaml | Makes single-mount data check more robust (retry/find). |
| test/gha-e2e/jindo/dataset.yaml | Adds metadata annotations for PV publish method (symlink). |
| test/gha-e2e/curvine/write_job.yaml | Adds Curvine E2E write job to seed data. |
| test/gha-e2e/curvine/test.sh | Introduces Curvine cache runtime E2E end-to-end flow. |
| test/gha-e2e/curvine/ref-dataset.yaml | Adds reference dataset for dataset:// mount scenario. |
| test/gha-e2e/curvine/read_ref_job.yaml | Adds read job for reference dataset validation. |
| test/gha-e2e/curvine/read_job.yaml | Adds read job for primary dataset validation. |
| test/gha-e2e/curvine/mount.yaml | Adds ConfigMap-mounted mount script for Curvine UFS mounting. |
| test/gha-e2e/curvine/minio.yaml | Adds MinIO + secret manifests for Curvine E2E. |
| test/gha-e2e/curvine/minio_create_bucket.yaml | Adds job to create required MinIO bucket for Curvine mounts. |
| test/gha-e2e/curvine/dataset.yaml | Adds Curvine Dataset manifest using encryptOptions/secret refs. |
| test/gha-e2e/curvine/dataload.yaml | Adds DataLoad manifest for Curvine E2E. |
| test/gha-e2e/curvine/cacheruntimeclass.yaml | Adds CacheRuntimeClass definition for Curvine cache runtime. |
| test/gha-e2e/curvine/cacheruntime.yaml | Adds CacheRuntime instance for Curvine E2E. |
| test/gha-e2e/alluxio/test.sh | Improves Alluxio E2E timeout/logging and cleanup behavior. |
| test/gha-e2e/alluxio/dataset.yaml | Changes Alluxio cache path to /tmp/alluxio. |
| charts/fluid/values.yaml | Bumps default image version, adds runtime.cache values, removes GooseFS values, adjusts prune list. |
| charts/fluid/templates/role/webhook/rabc.yaml | Removes GooseFS runtime RBAC entries from webhook role. |
| charts/fluid/templates/role/thin/rbac.yaml | Updates thin controller RBAC (remove GooseFS, add CacheRuntime). |
| charts/fluid/templates/role/goosefs/rbac.yaml | Removes GooseFS controller RBAC template. |
| charts/fluid/templates/role/dataset/rbac.yaml | Updates dataset controller RBAC to include CacheRuntime(+Class) and remove GooseFS. |
| charts/fluid/templates/role/csi/rbac.yaml | Updates CSI RBAC to include CacheRuntime and remove GooseFS. |
| charts/fluid/templates/role/cache/rbac.yaml | Adds CacheRuntime controller RBAC resources. |
| charts/fluid/templates/csi/daemonset.yaml | Cleans up CSI DaemonSet manifest (removes stray imagePullPolicy line). |
| charts/fluid/templates/controller/goosefsruntime_controller.yaml | Removes GooseFS controller deployment template. |
| charts/fluid/templates/controller/cacheruntime_controller.yaml | Adds CacheRuntime controller deployment template. |
| charts/fluid/crds/workload.fluid.io_advancedstatefulsets.yaml | Adds AdvancedStatefulSet CRD used by the chart/runtime. |
| charts/fluid/crds/data.fluid.io_juicefsruntimes.yaml | Extends JuiceFSRuntime CRD schema (e.g., volumeClaimTemplates). |
| charts/fluid/crds/data.fluid.io_goosefsruntimes.yaml | Removes GooseFSRuntime CRD. |
| charts/fluid/crds/data.fluid.io_dataprocesses.yaml | Adds/extends DataProcess fields (policy/schedule). |
| charts/fluid/crds/data.fluid.io_cacheruntimes.yaml | Updates CacheRuntime CRD schema (tieredstore layout changes, etc.). |
| charts/fluid/crds/data.fluid.io_cacheruntimeclasses.yaml | Updates CacheRuntimeClass CRD (scope change + schema updates). |
| charts/fluid/Chart.yaml | Bumps chart version/appVersion to alpha.9 / new commit tag. |
| .github/scripts/gha-e2e.sh | Adds Curvine E2E invocation and minor formatting cleanup. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function wait_job_completed() { | ||
| job_name=$1 | ||
| while true; do | ||
| succeed=$(kubectl get job $job_name -ojsonpath='{@.status.succeeded}') | ||
| failed=$(kubectl get job $job_name -ojsonpath='{@.status.failed}') |
| # 必填参数校验 | ||
| CV_PARAMS="" | ||
| [ -z "$endpoint" ] && { echo "endpoint option is not set or empty" >&2; exit 1; } | ||
| [ -z "$access" ] && { echo "access option is not set or empty" >&2; exit 1; } | ||
| [ -z "$secret" ] && { echo "secret option is not set or empty" >&2; exit 1; } | ||
|
|
||
| # 拼接必填参数 | ||
| CV_PARAMS="$CV_PARAMS -c s3.endpoint_url=$endpoint" | ||
| CV_PARAMS="$CV_PARAMS -c s3.credentials.access=$access" | ||
| CV_PARAMS="$CV_PARAMS -c s3.credentials.secret=$secret" | ||
|
|
||
| # 可选参数：缺失自动跳过 | ||
| [ -n "$region" ] && CV_PARAMS="$CV_PARAMS -c s3.region_name=$region" | ||
| [ -n "$path_style" ] && CV_PARAMS="$CV_PARAMS -c s3.path_style=$path_style" | ||
|
|
||
| # 最终命令 | ||
| CMD="/app/curvine/bin/cv mount $mountPoint $path --check-path-consist false $CV_PARAMS" | ||
| #echo "执行命令：$CMD" | ||
|
|
||
| eval "$CMD" > /dev/null 2>&1 | ||
| if [ $? -ne 0 ]; then | ||
| echo "mount $mountPoint failed" >&2 | ||
| exit 1 | ||
| fi |
| mounted=$(/app/curvine/bin/cv mount) | ||
| if echo "$mounted" | grep "$mountPoint" >/dev/null 2>&1; then | ||
| continue; | ||
| fi |
Signed-off-by: cheyang <cheyang@163.com>
Signed-off-by: cheyang <cheyang@163.com>
Signed-off-by: cheyang <cheyang@163.com>
No description provided.