fix(csi): propagate mount error in recoverBrokenMount#5753
fix(csi): propagate mount error in recoverBrokenMount#5753Sanchit2662 wants to merge 1 commit intofluid-cloudnative:masterfrom
Conversation
Signed-off-by: Sanchit2662 <sanchit2662@gmail.com>
|
|
hey @cheyang @TrafalgarZZZ , please review the fix. |
![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 fixes a variable shadowing bug in the recoverBrokenMount function by switching from a short variable declaration to a direct assignment for the error variable. Feedback suggests further improving the code by removing named return parameters from the function signature to prevent similar shadowing issues and enhance maintainability.
| // Info: Attempting recovery action | ||
| glog.V(3).Infof("FuseRecovery: attempting bind mount, source=%s mountPath=%s options=%v", point.SourcePath, point.MountPath, mountOption) | ||
| if err := r.Mount(point.SourcePath, point.MountPath, "none", mountOption); err != nil { | ||
| if err = r.Mount(point.SourcePath, point.MountPath, "none", mountOption); err != nil { |
There was a problem hiding this comment.
While this change correctly fixes the shadowing issue by using assignment (=) instead of a short variable declaration (:=), the use of named return parameters (like err in this function signature) is generally discouraged in Go unless they provide significant documentation value. In this case, the named return directly contributed to the bug. For better maintainability and to prevent similar shadowing issues in the future, consider removing the named return parameter from the function signature and using explicit return err or return nil statements.
|
[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 @Sanchit2662. 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. |
1 participant
Ⅰ. Describe what this PR does
so i was digging through the fuse recovery code and found something pretty embarrassing honestly. recoverBrokenMount has a named return err but then inside the function does if err := r.Mount(...) with a short variable declaration. that inner err is a completely new variable scoped to the if block, it never touches the named return. so the function literally always returns nil no matter what.
the caller doRecover checks the return value to decide whether to emit FuseRecoverFailed or FuseRecoverSucceed. since it always gets nil back, it always records FuseRecoverSucceed on the dataset. even when the mount actually failed. so operators see green events while pods are sitting on broken mounts getting I/O errors. not great.
fix is just changing := to = on that one line so the error actually propagates.
Ⅱ. 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.
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews