-
Notifications
You must be signed in to change notification settings - Fork 554
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
rbd: return group not found error for Get,Delete RPC calls #5001
Conversation
24f2d77
to
7ecbcad
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please paste the testing logs.
7ecbcad
to
65c8301
Compare
csi-rbdplugin-provisioner logs
|
65c8301
to
bc593d0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The approach looks good to me. Just a small mistake that needs correcting.
bc593d0
to
6c0305c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A followup PR is required to update the DeleteVolumeGroupSnapshot and GetvolumeGroupsnapshot RPC to make use of ErrRBDGroupNotFound to return success and not found and even in ControllerGetVolumeGroup RPC as well
ControllerGetVolumeGroup is taken care off in this PR itself, only the GroupSnapshot RPCs need to be updated. [1]
[2] ceph-csi/internal/rbd/manager.go Line 224 in 6c0305c
[3]
|
i was not able to find the code for it, can you please point me to the code. |
@Madhu-1 updated my previous comment with links, please check now. |
test logs have been provided
@Mergifyio queue |
✅ The pull request has been merged automaticallyThe pull request has been merged automatically at c7d54ab |
We should return NotFound status if the group doesn't exists for ControllerGetVolumeGroup RPC call. And, an empty/OK response for DeleteVolumeGroup if the group doesn't exists Signed-off-by: Nikhil-Ladha <[email protected]>
6c0305c
to
7595457
Compare
/test ci/centos/k8s-e2e-external-storage/1.31 |
/test ci/centos/k8s-e2e-external-storage/1.30 |
/test ci/centos/mini-e2e-helm/k8s-1.31 |
/test ci/centos/mini-e2e/k8s-1.31 |
/test ci/centos/mini-e2e-helm/k8s-1.30 |
/test ci/centos/upgrade-tests-cephfs |
/test ci/centos/k8s-e2e-external-storage/1.29 |
/test ci/centos/mini-e2e/k8s-1.30 |
/test ci/centos/mini-e2e-helm/k8s-1.29 |
/test ci/centos/upgrade-tests-rbd |
/test ci/centos/mini-e2e/k8s-1.29 |
@Mergifyio backport release-v3.13 |
❌ Command disallowed due to command restrictions in the Mergify configuration.
|
We should return
NotFound
GRPC status if the group doesn't exists for ControllerGetVolumeGroup RPC call.And, an empty/OK response for DeleteVolumeGroup if the group doesn't exists
Fixes: #4999
Show available bot commands
These commands are normally not required, but in case of issues, leave any of
the following bot commands in an otherwise empty comment in this PR:
/retest ci/centos/<job-name>
: retest the<job-name>
after unrelatedfailure (please report the failure too!)