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.

Sign up for GitHub

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

Jump to bottom

rbd: return group not found error for Get,Delete RPC calls #5001

Merged
merged 1 commit into from
Dec 12, 2024
Merged

rbd: return group not found error for Get,Delete RPC calls #5001

merged 1 commit into from
Dec 12, 2024
+21 −3

Conversation

Nikhil-Ladha
Copy link
Contributor

@Nikhil-Ladha Nikhil-Ladha commented Dec 4, 2024 •
edited
Loading

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 unrelated
    failure (please report the failure too!)

@mergify mergify bot added the component/rbd Issues related to RBD label Dec 4, 2024
@Nikhil-Ladha Nikhil-Ladha force-pushed the issue4999/vg-get-delete branch from 24f2d77 to 7ecbcad Compare December 4, 2024 06:52
iPraveenParihar
iPraveenParihar previously requested changes Dec 4, 2024
View reviewed changes
Copy link
Contributor

@iPraveenParihar iPraveenParihar left a 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.

internal/csi-addons/rbd/volumegroup.go Show resolved Hide resolved
internal/csi-addons/rbd/volumegroup.go Show resolved Hide resolved
internal/rbd/group/volume_group.go Outdated Show resolved Hide resolved
@Nikhil-Ladha Nikhil-Ladha force-pushed the issue4999/vg-get-delete branch from 7ecbcad to 65c8301 Compare December 4, 2024 08:25
@Nikhil-Ladha
Copy link
Contributor Author

Please paste the testing logs.

[nladha@dhcp53-204 ~]$ kubectl exec -it deployments/csi-rbdplugin-provisioner -c csi-addons -n rook-ceph -- csi-addons --endpoint unix:///csi/csi-addons.sock -operation ControllerGetVolumeGroup -volumegroupid 0001-0009-rook-ceph-0000000000000002-71f1dc66-e334-4b9b-9f7c-536f4c3f03fa -secret rook-ceph/rook-csi-rbd-provisioner
E1204 08:42:48.273772      11 volumegroup.go:174] Failed to get volume group: rpc error: code = NotFound desc = could not find volume group "0001-0009-rook-ceph-0000000000000002-71f1dc66-e334-4b9b-9f7c-536f4c3f03fa": failed to get volume group with id "0001-0009-rook-ceph-0000000000000002-71f1dc66-e334-4b9b-9f7c-536f4c3f03fa": RBD group not found
ERROR: failed to execute "ControllerGetVolumeGroup": rpc error: code = NotFound desc = could not find volume group "0001-0009-rook-ceph-0000000000000002-71f1dc66-e334-4b9b-9f7c-536f4c3f03fa": failed to get volume group with id "0001-0009-rook-ceph-0000000000000002-71f1dc66-e334-4b9b-9f7c-536f4c3f03fa": RBD group not found
command terminated with exit code 1
[nladha@dhcp53-204 ~]$ kubectl exec -it deployments/csi-rbdplugin-provisioner -c csi-addons -n rook-ceph -- csi-addons --endpoint unix:///csi/csi-addons.sock -operation DeleteVolumeGroup -volumegroupid 0001-0009-rook-ceph-0000000000000002-71f1dc66-e334-4b9b-9f7c-536f4c3f03fa -secret rook-ceph/rook-csi-rbd-provisioner
Volume Group Deleted.

csi-rbdplugin-provisioner logs

I1204 08:42:48.177599       1 utils.go:266] ID: 24 GRPC call: /volumegroup.Controller/ControllerGetVolumeGroup
I1204 08:42:48.180222       1 utils.go:267] ID: 24 GRPC request: {"secrets":"***stripped***","volume_group_id":"0001-0009-rook-ceph-0000000000000002-71f1dc66-e334-4b9b-9f7c-536f4c3f03fa"}
I1204 08:42:48.260968       1 util.go:102] ID: 24 object for volume group "0001-0009-rook-ceph-0000000000000002-71f1dc66-e334-4b9b-9f7c-536f4c3f03fa" has been initialized
E1204 08:42:48.270666       1 omap.go:212] ID: 24 omap not found (pool="replicapool", namespace="", name="csi.volume.group.71f1dc66-e334-4b9b-9f7c-536f4c3f03fa"): rados: ret=-2, No such file or directory
W1204 08:42:48.271027       1 volumegroupjournal.go:409] ID: 24 unable to read omap values: pool missing: key not found: rados: ret=-2, No such file or directory
E1204 08:42:48.271583       1 volume_group.go:89] ID: 24 volume group with id 0001-0009-rook-ceph-0000000000000002-71f1dc66-e334-4b9b-9f7c-536f4c3f03fa not found
E1204 08:42:48.271747       1 utils.go:271] ID: 24 GRPC error: rpc error: code = NotFound desc = could not find volume group "0001-0009-rook-ceph-0000000000000002-71f1dc66-e334-4b9b-9f7c-536f4c3f03fa": failed to get volume group with id "0001-0009-rook-ceph-0000000000000002-71f1dc66-e334-4b9b-9f7c-536f4c3f03fa": RBD group not found
I1204 08:43:29.919432       1 utils.go:266] ID: 25 GRPC call: /volumegroup.Controller/DeleteVolumeGroup
I1204 08:43:29.920103       1 utils.go:267] ID: 25 GRPC request: {"secrets":"***stripped***","volume_group_id":"0001-0009-rook-ceph-0000000000000002-71f1dc66-e334-4b9b-9f7c-536f4c3f03fa"}
I1204 08:43:29.921395       1 util.go:102] ID: 25 object for volume group "0001-0009-rook-ceph-0000000000000002-71f1dc66-e334-4b9b-9f7c-536f4c3f03fa" has been initialized
E1204 08:43:29.924151       1 omap.go:212] ID: 25 omap not found (pool="replicapool", namespace="", name="csi.volume.group.71f1dc66-e334-4b9b-9f7c-536f4c3f03fa"): rados: ret=-2, No such file or directory
W1204 08:43:29.924554       1 volumegroupjournal.go:409] ID: 25 unable to read omap values: pool missing: key not found: rados: ret=-2, No such file or directory
E1204 08:43:29.924648       1 volume_group.go:89] ID: 25 volume group with id 0001-0009-rook-ceph-0000000000000002-71f1dc66-e334-4b9b-9f7c-536f4c3f03fa not found
I1204 08:43:29.925068       1 volumegroup.go:194] ID: 25 VolumeGroup "0001-0009-rook-ceph-0000000000000002-71f1dc66-e334-4b9b-9f7c-536f4c3f03fa" doesn't exists
I1204 08:43:29.925668       1 utils.go:273] ID: 25 GRPC response: null

@Nikhil-Ladha Nikhil-Ladha force-pushed the issue4999/vg-get-delete branch from 65c8301 to bc593d0 Compare December 10, 2024 09:37
nixpanic
nixpanic requested changes Dec 10, 2024
View reviewed changes
Copy link
Member

@nixpanic nixpanic left a 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.

internal/rbd/group/util.go Outdated Show resolved Hide resolved
@Nikhil-Ladha Nikhil-Ladha force-pushed the issue4999/vg-get-delete branch from bc593d0 to 6c0305c Compare December 11, 2024 04:16
Madhu-1
Madhu-1 approved these changes Dec 11, 2024
View reviewed changes
Copy link
Collaborator

@Madhu-1 Madhu-1 left a comment •
edited
Loading

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

@Nikhil-Ladha
Copy link
Contributor Author

Nikhil-Ladha commented Dec 11, 2024 •
edited
Loading

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.
i.e, The ControllerGetVolumeGroup RPC calls GetVolumeGroupByID [1] function, that internally calls GetVolumeGroup which returns the ErrRBDGroupNotFound error, if the group doesn't exists. And, in the parent ControllerGetVolumeGroup RPC call, we return the NotFound status.code if the err != nil [3].

[1]

ceph-csi/internal/csi-addons/rbd/volumegroup.go

Line 434 in 6c0305c

vg, err := mgr.GetVolumeGroupByID(ctx, req.GetVolumeGroupId())

[2]

ceph-csi/internal/rbd/manager.go

Line 224 in 6c0305c

vg, err := rbd_group.GetVolumeGroup(ctx, id, mgr.csiID, creds, mgr)

[3]

ceph-csi/internal/csi-addons/rbd/volumegroup.go

Line 435 in 6c0305c

if err != nil {

@Madhu-1
Copy link
Collaborator

Madhu-1 commented Dec 11, 2024

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.

i was not able to find the code for it, can you please point me to the code.

@Nikhil-Ladha
Copy link
Contributor Author

@Madhu-1 updated my previous comment with links, please check now.

@nixpanic nixpanic dismissed iPraveenParihar’s stale review December 12, 2024 17:06

test logs have been provided

@nixpanic
Copy link
Member

@Mergifyio queue

@mergify Mergify
Copy link
Contributor

mergify bot commented Dec 12, 2024 •
edited
Loading

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at c7d54ab

@Nikhil-Ladha
rbd: return group not found error for Get,Delete RPC calls
7595457 
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]>
@Nikhil-Ladha Nikhil-Ladha force-pushed the issue4999/vg-get-delete branch from 6c0305c to 7595457 Compare December 12, 2024 19:26
@mergify mergify bot added the ok-to-test Label to trigger E2E tests label Dec 12, 2024
@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.31

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.30

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.31

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e/k8s-1.31

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.30

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/upgrade-tests-cephfs

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.29

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e/k8s-1.30

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.29

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/upgrade-tests-rbd

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e/k8s-1.29

@ceph-csi-bot ceph-csi-bot removed the ok-to-test Label to trigger E2E tests label Dec 12, 2024
@mergify mergify bot merged commit c7d54ab into ceph:devel Dec 12, 2024
37 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Madhu-1 Madhu-1 Madhu-1 approved these changes

@nixpanic nixpanic nixpanic approved these changes

@iPraveenParihar iPraveenParihar Awaiting requested review from iPraveenParihar

Assignees
No one assigned
Labels
component/rbd Issues related to RBD
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ControllerGetVolumeGroup and DeleteVolumeGroup is not idempotent
5 participants
@Nikhil-Ladha @Madhu-1 @nixpanic @ceph-csi-bot @iPraveenParihar