Bugfix: ignore not connected error in NodeUnpublishVolume #3445

wangshli · 2023-09-08T11:52:26Z

Ⅰ. Describe what this PR does

ignore not connected error at the beginning of NodeUnpublishVolume

Ⅱ. Does this pull request fix one issue?

fixes #3444

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

Signed-off-by: wangshulin <wangshulin@smail.nju.edu.cn>

fluid-e2e-bot · 2023-09-08T11:52:42Z

Hi @wangshli. 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.

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.

codecov · 2023-09-08T12:12:19Z

Codecov Report

Merging #3445 (beaa625) into master (321dc08) will increase coverage by 0.00%.
Report is 2 commits behind head on master.
The diff coverage is 100.00%.

❗ Current head beaa625 differs from pull request most recent head b73fce8. Consider uploading reports for the commit b73fce8 to get more accurate results

@@           Coverage Diff           @@
##           master    #3445   +/-   ##
=======================================
  Coverage   64.28%   64.28%           
=======================================
  Files         442      442           
  Lines       26414    26420    +6     
=======================================
+ Hits        16980    16984    +4     
- Misses       7429     7430    +1     
- Partials     2005     2006    +1

Files Changed	Coverage Δ
pkg/ddc/jindocache/transform.go	`66.07% <100.00%> (+0.13%)`	⬆️
pkg/ddc/jindofsx/transform.go	`64.86% <100.00%> (+0.14%)`	⬆️

... and 1 file with indirect coverage changes

cheyang · 2023-09-09T07:09:44Z

pkg/csi/plugins/nodeserver.go

@@ -193,7 +193,8 @@ func (ns *nodeServer) NodeUnpublishVolume(ctx context.Context, req *csi.NodeUnpu
 		glog.V(3).Infof("NodeUnpublishVolume: targetPath %s has been cleaned up, so it doesn't need to be unmounted", targetPath)


Could you reduce nesting levels to decrease code complexity? The nesting levels can be reduced by utilizing the short-circuit mechanism of the if statement. For example, check os.IsNotExist(err) first and then check errors.Is(err, syscall.ENOTCONN). Such as:

if os.IsNotExist(err) { .... } else if err != nil { if !errors.Is(err, syscall.ENOTCONN) { ... } }

Signed-off-by: wangshulin <wangshulin@smail.nju.edu.cn>

cheyang · 2023-09-10T03:26:58Z

pkg/csi/plugins/nodeserver.go

+	} else if err != nil {
+		// if error is "transport endpoint is not connected", ingore and do umount
+		if !errors.Is(err, syscall.ENOTCONN) {
+			return nil, errors.Wrapf(err, "NodeUnpublishVolume: stat targetPath %s error %v", targetPath, err)


Please also provide the log info for syscall.ENOTCONN.

The error will be logged in the code below:

fluid/pkg/csi/plugins/nodeserver.go

Lines 204 to 206 in 52499cc

notMount, err := mounter.IsLikelyNotMountPoint(targetPath)

if err != nil {

glog.V(3).Infoln(err)

TrafalgarZZZ · 2023-09-11T06:37:09Z

pkg/csi/plugins/nodeserver.go

-	}
-	if err != nil {
-		return nil, errors.Wrapf(err, "NodeUnpublishVolume: stat targetPath %s error %v", targetPath, err)
+	} else if err != nil {


How about:

Suggested change

} else if err != nil {

}

if err != nil && !errors.Is(err, syscall.ENOTCONN) {

return ...

}

Signed-off-by: wangshulin <wangshulin@smail.nju.edu.cn>

sonarcloud · 2023-09-11T08:53:32Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

cheyang

/lgtm
/approve

fluid-e2e-bot · 2023-09-11T09:15:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cheyang

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [cheyang]

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

…dnative#3445) * ignore not connected error in NodeUnpublishVolume Signed-off-by: wangshulin <wangshulin@smail.nju.edu.cn> * fix check nil error Signed-off-by: wangshulin <wangshulin@smail.nju.edu.cn> * simplify error judgment Signed-off-by: wangshulin <wangshulin@smail.nju.edu.cn> --------- Signed-off-by: wangshulin <wangshulin@smail.nju.edu.cn>

…and NodeUnpublishVolume (#3448) (#3453) * Bugfix: ignore not connected error in NodeUnpublishVolume (#3445) * ignore not connected error in NodeUnpublishVolume Signed-off-by: wangshulin <wangshulin@smail.nju.edu.cn> * fix check nil error Signed-off-by: wangshulin <wangshulin@smail.nju.edu.cn> * simplify error judgment Signed-off-by: wangshulin <wangshulin@smail.nju.edu.cn> --------- Signed-off-by: wangshulin <wangshulin@smail.nju.edu.cn> * bugfix: fix csi plugin concurrency issue on FuseRecovery and NodeUnpublishVolume (#3448) * Add comments for NodeUnpublishVolume Signed-off-by: trafalgarzzz <trafalgarz@outlook.com> * Refactor NodeUnpublishVolume code Signed-off-by: trafalgarzzz <trafalgarz@outlook.com> * FuseRecovery uses volume locks to avoid race conditions Signed-off-by: trafalgarzzz <trafalgarz@outlook.com> * Refactor node server with codes.Internal error code Signed-off-by: trafalgarzzz <trafalgarz@outlook.com> * Rename CSI Config to RunningContext Signed-off-by: trafalgarzzz <trafalgarz@outlook.com> * Fix github actions checks Signed-off-by: trafalgarzzz <trafalgarz@outlook.com> * Fix lock release Signed-off-by: trafalgarzzz <trafalgarz@outlook.com> * Refactor recover logic Signed-off-by: trafalgarzzz <trafalgarz@outlook.com> --------- Signed-off-by: trafalgarzzz <trafalgarz@outlook.com> --------- Signed-off-by: wangshulin <wangshulin@smail.nju.edu.cn> Signed-off-by: trafalgarzzz <trafalgarz@outlook.com> Co-authored-by: wangshulin <89928606+wangshli@users.noreply.github.com>

ignore not connected error in NodeUnpublishVolume

de40e26

Signed-off-by: wangshulin <wangshulin@smail.nju.edu.cn>

fluid-e2e-bot bot added the needs-ok-to-test label Sep 8, 2023

cheyang requested changes Sep 9, 2023

View reviewed changes

fix check nil error

8b655b9

Signed-off-by: wangshulin <wangshulin@smail.nju.edu.cn>

cheyang reviewed Sep 10, 2023

View reviewed changes

TrafalgarZZZ reviewed Sep 11, 2023

View reviewed changes

simplify error judgment

b73fce8

Signed-off-by: wangshulin <wangshulin@smail.nju.edu.cn>

cheyang self-requested a review September 11, 2023 09:01

cheyang approved these changes Sep 11, 2023

View reviewed changes

fluid-e2e-bot bot assigned cheyang Sep 11, 2023

fluid-e2e-bot bot added the lgtm label Sep 11, 2023

fluid-e2e-bot bot added the approved label Sep 11, 2023

fluid-e2e-bot bot merged commit 3e8ba24 into fluid-cloudnative:master Sep 11, 2023
9 checks passed

TrafalgarZZZ mentioned this pull request Sep 13, 2023

Cherry-pick to 0.9: fix csi plugin concurrency issue on FuseRecovery and NodeUnpublishVolume (#3448) #3453

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix: ignore not connected error in NodeUnpublishVolume #3445

Bugfix: ignore not connected error in NodeUnpublishVolume #3445

wangshli commented Sep 8, 2023

fluid-e2e-bot bot commented Sep 8, 2023

codecov bot commented Sep 8, 2023 •

edited

Loading

cheyang Sep 9, 2023 •

edited

Loading

wangshli Sep 9, 2023

cheyang Sep 10, 2023

wangshli Sep 10, 2023

TrafalgarZZZ Sep 11, 2023

wangshli Sep 11, 2023

sonarcloud bot commented Sep 11, 2023

cheyang left a comment

fluid-e2e-bot bot commented Sep 11, 2023

		@@ -193,7 +193,8 @@ func (ns nodeServer) NodeUnpublishVolume(ctx context.Context, req csi.NodeUnpu
		glog.V(3).Infof("NodeUnpublishVolume: targetPath %s has been cleaned up, so it doesn't need to be unmounted", targetPath)

	notMount, err := mounter.IsLikelyNotMountPoint(targetPath)
	if err != nil {
	glog.V(3).Infoln(err)

-	} else if err != nil {
+	}
+	if err != nil && !errors.Is(err, syscall.ENOTCONN) {
+	    return ...
+	}

Bugfix: ignore not connected error in NodeUnpublishVolume #3445

Bugfix: ignore not connected error in NodeUnpublishVolume #3445

Conversation

wangshli commented Sep 8, 2023

Ⅰ. Describe what this PR does

Ⅱ. Does this pull request fix one issue?

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

fluid-e2e-bot bot commented Sep 8, 2023

codecov bot commented Sep 8, 2023 • edited Loading

Codecov Report

cheyang Sep 9, 2023 • edited Loading

Choose a reason for hiding this comment

wangshli Sep 9, 2023

Choose a reason for hiding this comment

cheyang Sep 10, 2023

Choose a reason for hiding this comment

wangshli Sep 10, 2023

Choose a reason for hiding this comment

TrafalgarZZZ Sep 11, 2023

Choose a reason for hiding this comment

wangshli Sep 11, 2023

Choose a reason for hiding this comment

sonarcloud bot commented Sep 11, 2023

cheyang left a comment

Choose a reason for hiding this comment

fluid-e2e-bot bot commented Sep 11, 2023

codecov bot commented Sep 8, 2023 •

edited

Loading

cheyang Sep 9, 2023 •

edited

Loading