[frenemy17]

Ⅰ. Describe what this PR does

Follow-up to #6187 and fixes #6203.

When updateFuseConfigOnChange was updated to propagate the error from transformFuseConfig (#6187), the caller ShouldUpdateUFS began unconditionally logging all errors at Error level:

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

When a Dataset specifies a pvc:// mount whose PVC is not yet bound (ClaimPending or empty VolumeName), extractVolumeInfo returns an error. This is a normal, transient self-resolving state during provisioning, but ShouldUpdateUFS emitted an Error log roughly once per sync period (default 5s) until the PVC bound, causing sustained log noise and triggering alerts wired to controller error rates.

Following approach 2 outlined in #6203:

1. Introduce a typed sentinel error var ErrPVCNotBound = errors.New("persistent volume claim not bounded yet") in pkg/ddc/thin.
2. Wrap ErrPVCNotBound in extractVolumeInfo when checking len(pvc.Spec.VolumeName) == 0 || pvc.Status.Phase != corev1.ClaimBound.
3. In ShouldUpdateUFS, check if errors.Is(err, ErrPVCNotBound):
   • If true, demote to t.Log.V(1).Info("PVC is not bounded yet, will update fuse config once bounded", "error", err).
   • Otherwise, preserve t.Log.Error(err, "Failed to update fuse config") for all unexpected errors.

Ⅱ. Does this pull request fix one issue?

fixes #6203

Ⅲ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.

• pkg/ddc/thin/transform_config_test.go:
  • Added Ginkgo spec in ThinEngine extractVolumeInfo verifying ErrPVCNotBound is returned when a PVC is pending.
  • Added errors.Is(err, ErrPVCNotBound) assertion to the existing pending PVC test in ThinEngine transformFuseConfig.
• pkg/ddc/thin/ufs_ginkgo_test.go:
  • Added spec demotes log to V(1) and returns nil when a pvc mount is not yet bound verifying zero Error logs and the V(1) info message.
  • Added spec logs an error and returns nil when updateFuseConfigOnChange fails with an unexpected error verifying unexpected errors still log at Error level.
• pkg/ddc/thin/ufs_test.go:
  • Added table case "transform fuse config failed due to pending pvc" to TestThinEngine_updateFuseConfigOnChange asserting wantErrIs: ErrPVCNotBound.

Ⅳ. Describe how to verify it

go test -v -gcflags="all=-N -l" ./pkg/ddc/thin/...
go vet ./pkg/ddc/thin/...
gofmt -l pkg/ddc/thin/

Ⅴ. Special notes for reviews

None.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 65.78%. Comparing base (a01c038) to head (68b6a5d).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6233      +/-   ##
==========================================
+ Coverage   65.75%   65.78%   +0.02%     
==========================================
  Files         486      486              
  Lines       34459    34470      +11     
==========================================
+ Hits        22660    22675      +15     
+ Misses      10044    10041       -3     
+ Partials     1755     1754       -1     

☔ 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]

🟡 Changes recommended

Lost PVC bindings are misclassified as transient, suppressing error-level visibility of storage failures, and the new test does not verify verbosity level 1 for pending-PVC logs.

2 open issues

• Preserve error-level visibility for ClaimLost PVCs · New
• Assert pending-PVC logs are emitted at verbosity level 1 · New

What changed in this PR

This PR follows up on #6187 to reduce ThinRuntime log noise while PVC mounts await binding, addressing #6203 without changing error propagation.

Changes:

• Adds ErrPVCNotBound and wraps it when a PVC is not bound.
• Logs matching errors at V(1), retaining Error-level logging for other errors.
• Adds tests for sentinel propagation and pending-PVC versus unexpected-error logging.

File	Description
pkg/​ddc/​thin/​ufs.go	Selects log level using the PVC-not-bound sentinel.
pkg/​ddc/​thin/​ufs_test.go	Tests sentinel propagation through config updates.
pkg/​ddc/​thin/​ufs_ginkgo_test.go	Adds log capture and logging behavior tests.
pkg/​ddc/​thin/​transform_config.go	Defines and wraps the PVC-not-bound sentinel.
pkg/​ddc/​thin/​transform_config_test.go	Checks sentinel identity during volume extraction and config transformation.

_{Review effort: Balanced}

[cheyang]

The premise behind this PR checks out end to end: on the base branch, a pending pvc:// mount makes ShouldUpdateUFS log Failed to update fuse config at Error level every sync period (~5s), and on this head the same harnesses record zero Error logs and one V(1) notice. Two independent verification harnesses agree — fake-client unit tests on both, plus envtest against a real API server (k8s 1.34.1) — and the errors.Is(err, ErrPVCNotBound) chain holds through the full pkg/errors Wrapf, unexpected errors keep Error-level visibility, and the sync loop's nil-guard is untouched. The whole pkg/ddc/thin suite is green at head and every CI check passes (23/23; only the tide merge bot is pending). No blocking findings — this is a small, well-scoped, well-tested change.

Next round

[MUST] None — merge is not gated on anything from this review.
[SHOULD] Decide the ClaimLost disposition: the not-yet-bound demotion also swallows ClaimLost (a lost binding is a storage failure, not transient provisioning) into V(1) on the sync path. Either narrow the wrap predicate to ClaimPending || VolumeName == "", or record that demoting it is intended.
[SHOULD] Make the new demotion spec level-aware: the capture sink discards the verbosity level, so a V(1)→V(0) regression would keep the test green — record the level and assert it is >= 1.
[SHOULD] Deduplicate the wrapped message text (... not bounded yet: persistent volume claim not bounded yet); it also shows up in setup-path reconcile errors.
[Filed] None.

The one place the two reviews diverged was framing, not substance: whether the missing-PVC Error cadence belongs to #6203's scope (it is pre-existing and unchanged by this PR, so it stays a maintainer question rather than a fix request). Copilot's advisory review flagged the same two items the debate confirmed (ClaimLost over-demotion, level-blind test sink) and raised nothing the debaters could not corroborate. Harnesses: verify/thin-pvc-notbound-claude and verify/pr6233-pvcnotbound-log-codex on the reviewer fork, re-runnable via each branch's re-verify.sh.

Non-blocking observations (5) — not part of this round's ask; fix now, follow up, or file an issue as you see fit

• pkg/ddc/thin/transform_config.go:165 [minor] — ClaimLost PVCs are folded into the transient not-yet-bound demotion
  F2 (consensus after cross-critique; also flagged by Copilot as C4507914753): the wrap at transform_config.go:165-166 fires for any non-Bound phase — len(pvc.Spec.VolumeName) == 0 || pvc.Status.Phase != corev1.ClaimBound — which includes ClaimLost (a bound PV lost underneath the claim; VolumeName still set). A lost binding is a storage failure, not the self-resolving provisioning state issue #6203 describes, yet on the periodic sync path it is now demoted to V(1) and invisible at default verbosity. Proven PR-introduced by two independent base/head canaries: on base a01c038 a ClaimLost PVC produces one Error record ('Failed to update fuse config'); on head 952e37e it produces none (V(1) notice only). Impact is bounded — the update is skipped either way, and the setup path (transform_fuse.go:125) still surfaces the error as a reconcile failure — hence minor rather than major. Suggested fix: wrap the sentinel only when pvc.Status.Phase == corev1.ClaimPending || len(pvc.Spec.VolumeName) == 0 so ClaimLost keeps Error-level visibility, or record explicitly that demoting it is intended.
• pkg/ddc/thin/ufs.go:71 [minor] — pre-existing, not introduced by this PR — PVC-not-found still logs at Error with the same ~5s cadence (pre-existing)
  F4 (question, pre-existing — facts confirmed by both reviewers): a pvc:// mount whose PVC does not exist at all still logs 'Failed to update fuse config' at Error level once per sync period (~5s) — the same alert-noise shape as issue #6203, one state earlier. Issue #6203 scopes the noise to a PVC that exists but is not yet Bound, so demoting apierrors.IsNotFound here would be a scope expansion rather than an in-scope fix; but in dynamic-mount flows (#3432) the dataset can briefly precede its PVC, and operators wired to controller error rates still see a sustained Error stream. Maintainer call: demote IsNotFound too, or document the Error level as intended.
• pkg/ddc/thin/ufs_ginkgo_test.go:412 [minor] — New demotion test cannot catch a V(1)→V(0) regression
  F3 (consensus after cross-critique; also flagged by Copilot as C4507914788): the new spec 'demotes log to V(1) and returns nil when a pvc mount is not yet bound' cannot detect a V(1)→V(0) regression. testCaptureLogSink (ufs_ginkgo_test.go:411-414) returns true from Enabled for every level and discards the level parameter in Info, so the assertion only checks message containment in infoMsgs — replacing t.Log.V(1).Info with plain t.Log.Info would keep the spec green while restoring per-sync spam at default verbosity, the exact failure mode this PR exists to remove. The head code is correct today (level-recording sinks from both reviewers record the notice at level 1), so this is a test-hardening gap, not a code bug. Fix: record the level in the sink and assert it is >= 1 (see verifyPR6233LogSink in the verification harness for a working example).
• pkg/ddc/thin/transform_config.go:166 [nit] — Sentinel message duplicates the wrapper text
  F1 (consensus — independently found by both reviewers): the sentinel ErrPVCNotBound = errors.New("persistent volume claim not bounded yet") is wrapped by fmt.Errorf("persistent volume claim %s not bounded yet: %w", pvcName, ErrPVCNotBound), so the rendered chain reads 'failed to extract volume info from PersistentVolumeClaim "my-pvc": persistent volume claim my-pvc not bounded yet: persistent volume claim not bounded yet'. Cosmetic on the demoted V(1) sync path, but the same text surfaces on the setup path (transform_fuse.go:125 → reconcile error/events) where operators read it. Either shorten the sentinel (e.g. errors.New("pvc not bound")) or drop the duplicated phrase from the wrapper; the existing substring-style tests keep passing either way.
• pkg/ddc/thin/transform_fuse.go:250 [nit] — pre-existing, not introduced by this PR — Pre-existing gofmt drift in transform_fuse.go (file untouched by this PR)
  F5 (pre-existing; confirmed by both reviewers): gofmt -l pkg/ddc/thin/ flags transform_fuse.go (misindented createVolumeAndMount around line 250). Reproduces on both base a01c038 and head 952e37e; the PR diff does not touch the file (it touches only transform_config.go, ufs.go and their tests), and nothing in CI gates on it (all checks SUCCESS at head). Worth a one-off gofmt -w cleanup commit at some point so a gofmt -l pkg/ddc/thin/ verify recipe stays clean.

Verified review delta

• F2: open (initial-pr)
• F4: open (preexisting)
• F3: open (initial-pr)
• F1: open (initial-pr)
• F5: open (preexisting)

Code: clear. CI: success; attribution: baseline. Required CI checks still apply.

[frenemy17]

@cheyang Thanks for the review — addressed all three [SHOULD] items:

• Exclude ClaimLost / terminal phases from ErrPVCNotBound: Updated extractVolumeInfo in pkg/ddc/thin/transform_config.go so corev1.ClaimLost (and unexpected non-ClaimPending/non-ClaimBound phases) return a plain error without wrapping ErrPVCNotBound, preserving Error-level log visibility in ShouldUpdateUFS. Added DescribeTable coverage in transform_config_test.go and a ClaimLost spec in ufs_ginkgo_test.go.
• Deduplicate sentinel error message: Shortened ErrPVCNotBound to errors.New("pvc not bound") so the wrapped error reads persistent volume claim <name> not bounded yet: pvc not bound.
• Level-aware log sink assertion: Updated testCaptureLogSink in ufs_ginkgo_test.go to record verbosity level via capturedInfoLog and assert level: 1 for the pending-PVC message.

Re-verified with go vet ./pkg/ddc/thin/..., go test -gcflags="all=-N -l" ./pkg/ddc/thin/... (130/130 Thin suite specs passing), and gofmt -l.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[cheyang]

Re-checked the delta since the 11-08 review (952e37e -> 68b6a5d); all three SHOULDs are in. ClaimLost and any other phase that is neither Pending nor Bound now return a plain error and keep Error-level visibility on the sync path, while only Pending or an unset volumeName map to ErrPVCNotBound. The demotion spec records the logr level and asserts V(1), and the wrapped message no longer repeats itself. CI is green on this head, and pkg/ddc/thin passes locally on top of current master (together with #6234). Thanks @frenemy17!