Prevent dataloss due to the concurrent RPC calls (occurrence is very low) #4970
Conversation
|
/test ci/centos/mini-e2e/k8s-1.31 |
cafad3a to
8c455bd
Compare
|
/test ci/centos/mini-e2e/k8s-1.31 |
|
|
||
| return nil, status.Errorf(codes.Aborted, util.TargetPathOperationAlreadyExistsFmt, targetPath) | ||
| } | ||
| defer ns.VolumeLocks.Release(targetPath) | ||
|
|
||
| // Check if that target path exists properly | ||
| notMnt, err := ns.createTargetMountPath(ctx, targetPath, isBlock) |
There was a problem hiding this comment.
If I see it correctly, createTargetMountPath still uses the (possibly unreliable?) IsLikelyNotMountPoint under the hood; should this be adjusted to the same IsMountPoint that the cephfs implementation uses? Or can this remain as-is, as discussed in #4966 (comment)?
There was a problem hiding this comment.
Yes thats correct, it checks the sources, The main problem is with lock and the os.RemoveAll, if os.Remove fails we will get another RPC for unpublish which will trigger umount if its actually a mount. For now lets keep it simple with what we have today.
|
@Mergifyio queue |
✅ The pull request has been merged automaticallyThe pull request has been merged automatically at 00d252e |
We should not be dependent on the CO to ensure that it will serialize the request instead of that we need to have own internal locks to ensure that we dont do concurrent operations for same request. Signed-off-by: Madhu Rajanna <[email protected]>
We should not be dependent on the CO to ensure that it will serialize the request instead of that we need to have own internal locks to ensure that we dont do concurrent operations for same request. Signed-off-by: Madhu Rajanna <[email protected]>
using os.RemoveAll will remove everything in the director after the Umount we should be using os.Remove only to remove the empty directory Signed-off-by: Madhu Rajanna <[email protected]>
using os.RemoveAll will remove everything in the director after the Umount we should be using os.Remove only to remove the empty directory Signed-off-by: Madhu Rajanna <[email protected]>
|
/test ci/centos/k8s-e2e-external-storage/1.30 |
|
/test ci/centos/mini-e2e-helm/k8s-1.30 |
|
/test ci/centos/k8s-e2e-external-storage/1.31 |
|
/test ci/centos/mini-e2e/k8s-1.30 |
|
/test ci/centos/upgrade-tests-cephfs |
|
/test ci/centos/mini-e2e-helm/k8s-1.31 |
|
/test ci/centos/k8s-e2e-external-storage/1.29 |
|
/test ci/centos/mini-e2e/k8s-1.31 |
|
/test ci/centos/upgrade-tests-rbd |
|
/test ci/centos/mini-e2e-helm/k8s-1.29 |
|
/test ci/centos/mini-e2e/k8s-1.29 |
This PR includes series for commits for the following actions
Thanks to @martin-helmich @hensur and @Lucaber for the detailed analysis
fixes: #4966