Skip to content

fix(juicefs): read metaurl from SharedEncryptOptions when deciding the edition - #6148

Open
cheyang wants to merge 1 commit into
fluid-cloudnative:masterfrom
cheyang:fix/juicefs-edition-from-shared-encrypt-options
Open

fix(juicefs): read metaurl from SharedEncryptOptions when deciding the edition#6148
cheyang wants to merge 1 commit into
fluid-cloudnative:masterfrom
cheyang:fix/juicefs-edition-from-shared-encrypt-options

Conversation

@cheyang

@cheyang cheyang commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

genEdition takes the dataset-wide encrypt options as its third parameter, but transform passes the mount's own options:

// pkg/ddc/juicefs/transform.go
j.genEdition(dataset.Spec.Mounts[0], value, dataset.Spec.Mounts[0].EncryptOptions)

Mounts[0].EncryptOptions is therefore scanned twice and Dataset.Spec.SharedEncryptOptions is never looked at. A Dataset that declares metaurl only in SharedEncryptOptions is classified as the enterprise edition.

genValue does read SharedEncryptOptions, so such a Dataset ends up with Source = "${METAURL}" while Edition = enterprise. Two things follow from that mismatch:

1. The file system is never formatted. genFormatCmd takes the enterprise branch, where an empty TokenSecret returns early:

// ee
if value.Configs.TokenSecret == "" {
    // skip juicefs auth
    return
}

so Configs.FormatCmd stays empty. parseJuiceFSImage also selects the enterprise image for what is a community deployment.

2. Teardown fails and keeps retrying. getUUID returns early for the enterprise edition with uuid = Source, so the literal string ${METAURL} is joined into the cache path. cleanupCache then asks to delete <cacheDir>/${METAURL}/raw/chunks, which cmdguard rejects for containing $, so Shutdown returns an error and increments retryShutdown.

The change

Pass dataset.Spec.SharedEncryptOptions, which is what the parameter is named after:

-j.genEdition(dataset.Spec.Mounts[0], value, dataset.Spec.Mounts[0].EncryptOptions)
+j.genEdition(dataset.Spec.Mounts[0], value, dataset.Spec.SharedEncryptOptions)

genEdition itself is unchanged — it already scans both lists correctly.

Compatibility

This changes how a Dataset with metaurl only in SharedEncryptOptions is classified, so I checked whether anyone could be relying on the current behaviour. They cannot: as described above such a deployment gets the enterprise image, an empty FormatCmd, and a teardown that always fails. It never worked, so the change is a fix rather than a break. Datasets that declare metaurl on the mount are unaffected.

Testing

The existing genEdition specs call the function directly with hand-built arguments, so they cover its logic but not how transform invokes it — which is exactly where the defect was. Added a table that goes through transform and asserts the edition for metaurl declared on the mount, in SharedEncryptOptions, in both, and in neither.

To confirm the new coverage actually pins the call site, I reverted the one-line change and re-ran: only the metaurl in SharedEncryptOptions case fails (173 passed / 1 failed), the other three still pass.

Verified on Linux with the CI command (go list ./... | grep -v controller | grep -v e2etest | xargs go test -gcflags="all=-N -l"), comparing the run with and without the fix:

failing packages only WITH fix (regressions)   -> none
failing tests only WITH fix (regressions)      -> none
common failures (pre-existing both sides)      -> TestWebhook

pkg/ddc/juicefs is green at 174/174 specs; go build ./... and go vet are clean. The single pre-existing TestWebhook failure reproduces identically without this change.

…e edition

genEdition takes the dataset-wide encrypt options as its third argument, but
transform passed the mount's own options instead:

    j.genEdition(dataset.Spec.Mounts[0], value, dataset.Spec.Mounts[0].EncryptOptions)

so Mounts[0].EncryptOptions was scanned twice and Dataset.Spec.SharedEncryptOptions
was never looked at. A dataset that declares metaurl only in SharedEncryptOptions
was therefore classified as the enterprise edition.

genValue does read SharedEncryptOptions, so such a dataset ended up with
Source = "${METAURL}" while Edition = enterprise. That mismatch has two visible
effects:

  - transformFuse takes the enterprise branch of genFormatCmd, where an empty
    TokenSecret makes it return early, so Configs.FormatCmd stays empty and the
    file system is never formatted. parseJuiceFSImage also picks the enterprise
    image for what is a community deployment.
  - On teardown, getUUID returns early for the enterprise edition with
    uuid = Source, so the literal string "${METAURL}" is joined into the cache
    path. cleanupCache then asks to remove
    "<cacheDir>/${METAURL}/raw/chunks", which cmdguard rejects for containing
    `$`, so Shutdown fails and keeps retrying.

Pass dataset.Spec.SharedEncryptOptions, which is what the parameter is named
after. genEdition itself is unchanged: it already scans both lists correctly.

The existing genEdition specs call the function directly with hand-built
arguments, so they cover its logic but not how transform invokes it, which is
where the defect was. Add a table that goes through transform and asserts the
edition for metaurl declared on the mount, in SharedEncryptOptions, in both and
in neither. Reverting the one-line change fails only the SharedEncryptOptions
case, so the new coverage pins the call site rather than the helper.

Signed-off-by: cheyang <cheyang.cy@alibaba-inc.com>
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

@sonarqubecloud

sonarqubecloud Bot commented Aug 4, 2026

Copy link
Copy Markdown

@codecov

codecov Bot commented Aug 4, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 65.15%. Comparing base (7c00ed2) to head (c763734).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6148      +/-   ##
==========================================
+ Coverage   65.13%   65.15%   +0.02%     
==========================================
  Files         485      485              
  Lines       34039    34039              
==========================================
+ Hits        22171    22179       +8     
+ Misses      10127    10121       -6     
+ Partials     1741     1739       -2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes JuiceFS edition detection by ensuring transform passes dataset-wide SharedEncryptOptions into genEdition, so metaurl declared at the dataset level correctly selects the community edition (preventing downstream formatting/teardown issues caused by an enterprise/community mismatch).

Changes:

  • Update transform to pass dataset.Spec.SharedEncryptOptions to genEdition (instead of the mount’s EncryptOptions).
  • Add table-driven coverage that exercises transform end-to-end for metaurl placement (mount vs shared vs both vs neither).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
pkg/ddc/juicefs/transform.go Fixes the genEdition call site to use SharedEncryptOptions, aligning edition detection with genValue behavior.
pkg/ddc/juicefs/transform_test.go Adds integration-style table cases through transform to prevent regressions at the call site.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@cheyang
cheyang requested a review from zwwhdls August 10, 2026 06:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants