Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

check path existence and lock in NodeUnpublishVolume #3284

Merged
merged 6 commits into from
Jun 17, 2023

Conversation

wangshli
Copy link
Contributor

Ⅰ. Describe what this PR does

check path existence and lock in NodeUnpublishVolume

Ⅱ. Does this pull request fix one issue?

fixes #3269

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

fluid-e2e-bot bot commented Jun 13, 2023

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
Copy link

codecov bot commented Jun 13, 2023

Codecov Report

Merging #3284 (50970ca) into master (2b554b3) will decrease coverage by 0.04%.
The diff coverage is 0.00%.

❗ Current head 50970ca differs from pull request most recent head 406fbcd. Consider uploading reports for the commit 406fbcd to get more accurate results

@@            Coverage Diff             @@
##           master    #3284      +/-   ##
==========================================
- Coverage   65.49%   65.45%   -0.04%     
==========================================
  Files         398      399       +1     
  Lines       23184    23198      +14     
==========================================
  Hits        15184    15184              
- Misses       6215     6229      +14     
  Partials     1785     1785              
Impacted Files Coverage Δ
pkg/utils/volume_lock.go 0.00% <0.00%> (ø)


// The lock is to avoid race condition
ns.mutex.Lock()
defer ns.mutex.Unlock()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please consider two things:

  1. The lock scope is too wide, we'd better to restrict it to mount path level.
  2. Is the lock only for NodeUnpublishVolume?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have modified: use mount path level lock and also lock for NodePublishVolume

@TrafalgarZZZ
Copy link
Member

/test fluid-e2e

_, err := os.Stat(targetPath)
// No need to unmount non-existing targetPath
if os.IsNotExist(err) {
glog.V(4).Infof("NodeUnpublishVolume: targetPath %s does not exist", targetPath)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest changing logging info to NodeUnpublishVolume: targetPath %s has been cleaned up, so it doesn't need to be unmounted.

Copy link
Collaborator

Choose a reason for hiding this comment

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

And I wonder the reason of setting log level to 4.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just follow other success log such as "glog.V(4).Infof("Succeed in binding %s to %s", mountPath, targetPath)"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And I wonder the reason of setting log level to 4.

The defauld log level is 5. And I thought for csi plugin, the second unmount should not be error so set the log level to 4 to indicate that the second NodeUnplishVolume incurs no error and success. But for devloper, it is should indicate that the kubelet invokes second call and the second call itself is an error. So we use log level 3 here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CleanupMountPoint used by gcp throws a warning when targetpath not exist, do we also use warning?

@cheyang cheyang requested a review from zwwhdls June 14, 2023 06:28
@@ -167,6 +172,23 @@ func (ns *nodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePublis
func (ns *nodeServer) NodeUnpublishVolume(ctx context.Context, req *csi.NodeUnpublishVolumeRequest) (*csi.NodeUnpublishVolumeResponse, error) {
targetPath := req.GetTargetPath()

// check path existence
_, err := os.Stat(targetPath)
Copy link
Member

Choose a reason for hiding this comment

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

Also need to check target is valid or not(0 length).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

@wangshli wangshli requested a review from zwwhdls June 15, 2023 12:28
Copy link
Member

@TrafalgarZZZ TrafalgarZZZ left a comment

Choose a reason for hiding this comment

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

/lgtm

@wangshli Thx for the contribution!

@TrafalgarZZZ
Copy link
Member

/test fluid-e2e

@wangshli
Copy link
Contributor Author

/retest

@fluid-e2e-bot
Copy link

fluid-e2e-bot bot commented Jun 16, 2023

@wangshli: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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.

Signed-off-by: wangshulin <wangshulin@smail.nju.edu.cn>
Signed-off-by: wangshulin <wangshulin@smail.nju.edu.cn>
Signed-off-by: wangshulin <wangshulin@smail.nju.edu.cn>
Signed-off-by: wangshulin <wangshulin@smail.nju.edu.cn>
Signed-off-by: wangshulin <wangshulin@smail.nju.edu.cn>
Signed-off-by: wangshulin <wangshulin@smail.nju.edu.cn>
@TrafalgarZZZ
Copy link
Member

/ok-to-test

@sonarcloud
Copy link

sonarcloud bot commented Jun 16, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@TrafalgarZZZ
Copy link
Member

/test fluid-e2e

1 similar comment
@cheyang
Copy link
Collaborator

cheyang commented Jun 17, 2023

/test fluid-e2e

Copy link
Collaborator

@cheyang cheyang left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@fluid-e2e-bot
Copy link

fluid-e2e-bot bot commented Jun 17, 2023

[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:

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 merged commit 5280197 into fluid-cloudnative:master Jun 17, 2023
8 checks passed
TrafalgarZZZ pushed a commit to TrafalgarZZZ/fluid that referenced this pull request Sep 13, 2023
…ve#3284)

* check path existence and lock in NodeUnpublishVolume

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

* use path level lock and lock for NodePublishVolume

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

* small fix

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

* return codes.aborted when the lock is not acquired

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

* check target path validity

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

* adjust order of check path and acquire lock

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

---------

Signed-off-by: wangshulin <wangshulin@smail.nju.edu.cn>
fluid-e2e-bot bot pushed a commit that referenced this pull request Sep 13, 2023
* check path existence and lock in NodeUnpublishVolume



* use path level lock and lock for NodePublishVolume



* small fix



* return codes.aborted when the lock is not acquired



* check target path validity



* adjust order of check path and acquire lock



---------

Signed-off-by: wangshulin <wangshulin@smail.nju.edu.cn>
Co-authored-by: wangshulin <89928606+wangshli@users.noreply.github.com>
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.

[BUG]Failed on Second NodeUnpublishVolume Call
4 participants