Skip to content

fix(thin): propagate the transformFuseConfig error in updateFuseConfigOnChange - #6187

Open
cnYui wants to merge 1 commit into
fluid-cloudnative:masterfrom
cnYui:fix/thin-propagate-transform-fuse-config-error
Open

fix(thin): propagate the transformFuseConfig error in updateFuseConfigOnChange#6187
cnYui wants to merge 1 commit into
fluid-cloudnative:masterfrom
cnYui:fix/thin-propagate-transform-fuse-config-error

Conversation

@cnYui

@cnYui cnYui commented Sep 5, 2026

Copy link
Copy Markdown

Ⅰ. Describe what this PR does

ThinEngine.updateFuseConfigOnChange (pkg/ddc/thin/ufs.go) discards the error returned by transformFuseConfig:

updatedThinValue := &ThinValue{}
err = t.transformFuseConfig(runtime, dataset, updatedThinValue)
if err != nil {
	return update, nil     // <- swallows a non-nil err
}

Both sibling error paths in the very same function return the error (kubeclient.GetConfigmapByName at line 98-100, kubeclient.UpdateConfigMap at line 118-120), so this looks like an oversight rather than an intentional choice — git blame shows the line was introduced in the same hunk as its correct neighbours by #3432.

The consequence is a lost error signal. The only caller, ShouldUpdateUFS, reacts to a non-nil error:

update, err := t.updateFuseConfigOnChange(t.runtime, dataset)
if err != nil {
	t.Log.Error(err, "Failed to update fuse config")
	return
}

Because nil is returned, that Log.Error never fires and the transform failure is completely silent. transformFuseConfig is genuinely able to fail here: a pvc:// mount whose PersistentVolumeClaim is missing or not yet Bound makes extractVolumeInfo return an error, which is wrapped as failed to extract volume info from PersistentVolumeClaim "%s" (pkg/ddc/thin/transform_config.go:61-64).

Scope, stated honestly: this is an error-swallowing / observability fix, not a behavioural one. update is still false at that point, so updateFusePod() was skipped before this change and is still skipped after it, and ShouldUpdateUFS returns a nil ufsToUpdate either way. What changes is that the failure is now logged instead of vanishing, and the function no longer holds a latent trap for any future caller that propagates the error.

The fix is one token:

	return update, err

Ⅱ. 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.

Added one table case, "transform fuse config failed", to the existing TestThinEngine_updateFuseConfigOnChange in pkg/ddc/thin/ufs_test.go. It uses a Dataset whose mount is pvc://missing-pvc (no such PVC exists in the fake client), so transformFuseConfig -> extractVolumeInfo -> kubeclient.GetPersistentVolumeClaim fails, and asserts wantUpdate: false, wantErr: true. The three existing cases are untouched and still pass.

This case fails on master and passes with the fix, so it pins the behaviour.

Ⅳ. Describe how to verify it

All commands run locally on master @ 54a41cd with Go 1.25.7.

1. The new test fails without the ufs.go change (test case applied alone):

$ go test ./pkg/ddc/thin/ -run 'TestThinEngine_updateFuseConfigOnChange' -count=1 -gcflags="all=-N -l" -v
=== RUN   TestThinEngine_updateFuseConfigOnChange
    ufs_test.go:571: testcase transform fuse config failed failed due to error <nil>
--- FAIL: TestThinEngine_updateFuseConfigOnChange (0.00s)
FAIL
FAIL	github.com/fluid-cloudnative/fluid/pkg/ddc/thin	0.360s

(error <nil> is exactly the symptom: the transform failed, but the function reported success.)

2. With the fix applied, it passes:

$ go test ./pkg/ddc/thin/ -run 'TestThinEngine_updateFuseConfigOnChange|TestThinEngine_ShouldUpdateUFS' -count=1 -gcflags="all=-N -l" -v
=== RUN   TestThinEngine_ShouldUpdateUFS
=== RUN   TestThinEngine_ShouldUpdateUFS/test
--- PASS: TestThinEngine_ShouldUpdateUFS (0.00s)
    --- PASS: TestThinEngine_ShouldUpdateUFS/test (0.00s)
=== RUN   TestThinEngine_updateFuseConfigOnChange
--- PASS: TestThinEngine_updateFuseConfigOnChange (0.00s)
PASS
ok  	github.com/fluid-cloudnative/fluid/pkg/ddc/thin	0.305s

3. Build, vet and gofmt are clean:

$ go build ./...          # exit 0, no output
$ go vet ./pkg/ddc/thin/...   # exit 0, no output
$ gofmt -l pkg/ddc/thin/      # no output

4. Out-of-band nilerr run (this linter is not enabled in the repo's .golangci.yml, so CI does not flag it today):

$ golangci-lint run --no-config --default=none -E nilerr ./pkg/ddc/thin/...   # before
pkg\ddc\thin\ufs.go:110:3: error is not nil (line 108) but it returns nil (nilerr)
		return update, nil
		^
1 issues:
* nilerr: 1

$ golangci-lint run --no-config --default=none -E nilerr ./pkg/ddc/thin/...   # after
0 issues.

Ⅴ. Special notes for reviews

  • The whole diff is +23 / -1 across two files; the production change is a single token on pkg/ddc/thin/ufs.go:110.
  • Full-package caveat, reported honestly: go test ./pkg/ddc/thin/... -count=1 shows 5 failures on my machine (TestThinEngine_getMountPoint, Test_getMountRoot, and 3 Ginkgo specs in transform_fuse_test.go). I verified these are pre-existing on unmodified master — I stashed my change and got the identical set (120 Passed | 3 Failed in the Ginkgo suite both times). They are Windows path-handling artefacts (getMountRoot() = /thin, want /tmp/thin) from my local environment, unrelated to this patch. This change adds no new failures.
  • ufs_test.go uses gomonkey ApplyFunc, hence the -gcflags="all=-N -l" in the commands above.
  • I have not run the e2e suites locally; happy to follow up on anything CI turns up.

🤖 Generated with Claude Code

…gOnChange

`updateFuseConfigOnChange` returned `(update, nil)` when
`transformFuseConfig` failed, so the error was discarded. Its caller
`ShouldUpdateUFS` only reacts to a non-nil error, which means a fuse
config transform failure (for example an unbound or missing PVC behind a
`pvc://` mount) was completely silent: `t.Log.Error(err, "Failed to
update fuse config")` never fired.

Return `err` instead, matching the two sibling error paths in the very
same function (`kubeclient.GetConfigmapByName` and
`kubeclient.UpdateConfigMap`).

Add a table case to `TestThinEngine_updateFuseConfigOnChange` covering a
Dataset with a `pvc://missing-pvc` mount, so `extractVolumeInfo` fails
and the propagated error is asserted.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: cnYui <xiaobianfuai@gmail.com>
@fluid-e2e-bot

fluid-e2e-bot Bot commented Sep 5, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign ronggu for approval by writing /assign @ronggu in a comment. For more information see:The Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@fluid-e2e-bot

fluid-e2e-bot Bot commented Sep 5, 2026

Copy link
Copy Markdown

Hi @cnYui. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions 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.

@sonarqubecloud

sonarqubecloud Bot commented Sep 5, 2026

Copy link
Copy Markdown

@codecov

codecov Bot commented Sep 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 65.25%. Comparing base (54a41cd) to head (7ac6f29).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6187      +/-   ##
==========================================
+ Coverage   65.24%   65.25%   +0.01%     
==========================================
  Files         486      486              
  Lines       34194    34194              
==========================================
+ Hits        22309    22315       +6     
+ Misses      10135    10131       -4     
+ Partials     1750     1748       -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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant