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

util: return correct status code for VolumeGroupSnapshot #5024

Merged
merged 1 commit into from
Dec 19, 2024
Merged

util: return correct status code for VolumeGroupSnapshot #5024

merged 1 commit into from
Dec 19, 2024

Conversation

Nikhil-Ladha
Copy link
Contributor

Fix status codes that are returned for Get/Delete RPC calls for VolumeGroup/VolumeGroupSnapshot.

@Nikhil-Ladha
Copy link
Contributor Author

@Madhu-1 updated the ControllerGetVolumeGroup as well, to differentiate the case of internal errors vs actual not found errors, please take a look now.

@Nikhil-Ladha Nikhil-Ladha force-pushed the fix-vgr-snapshot-rpc branch 3 times, most recently from bf9292a to f7ed505 Compare December 16, 2024 09:05
Madhu-1
Madhu-1 requested changes Dec 16, 2024
View reviewed changes
internal/cephfs/store/volumegroup.go Outdated Show resolved Hide resolved
internal/csi-addons/rbd/volumegroup.go Outdated
@@ -433,9 +433,19 @@ func (vs *VolumeGroupServer) ControllerGetVolumeGroup(
// resolve the volume group
vg, err := mgr.GetVolumeGroupByID(ctx, req.GetVolumeGroupId())
if err != nil {
if errors.Is(err, group.ErrRBDGroupNotFound) {
log.DebugLog(ctx, "VolumeGroup %q doesn't exists", req.GetVolumeGroupId())
Copy link
Collaborator

Choose a reason for hiding this comment

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

log it as error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are logging it as error in previous functions, hence just added it as a DebugLog instead so that only if required these should be logged. Do you think we should log it anyway?

Copy link
Collaborator

Choose a reason for hiding this comment

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

it should be fine we should log it as error here as well.

internal/rbd/group/group_snapshot.go Outdated
@@ -69,6 +71,12 @@ func GetVolumeGroupSnapshot(

attrs, err := vgs.getVolumeGroupAttributes(ctx)
if err != nil {
if errors.Is(err, librbd.ErrNotFound) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

why we can get this error condition met? i don't think getVolumeGroupAttributes directly return librbd error can you please check on this one again?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are adding the librbd.ErrNotFound error to ErrRBDGroupNotFound error here:

ceph-csi/internal/rbd/group/volume_group.go

Line 37 in c7d54ab

ErrRBDGroupNotFound = fmt.Errorf("%w: RBD group not found", librbd.ErrNotFound)
, therefore, this check would work (I have tested as well)

Copy link
Collaborator

Choose a reason for hiding this comment

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

lets compare this with ErrRBDGroupNotFound instead of librbd.ErrNotFound as getVolumeGroupAttributes is returning ErrRBDGroupNotFound ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, missed that change.
Updated it now.

internal/rbd/group_controllerserver.go Outdated
@@ -229,10 +238,20 @@ func (cs *ControllerServer) GetVolumeGroupSnapshot(

groupSnapshot, err := mgr.GetVolumeGroupSnapshotByID(ctx, groupSnapshotID)
if err != nil {
if errors.Is(err, group.ErrRBDGroupNotFound) {
log.DebugLog(ctx, "VolumeGroupSnapshot %q doesn't exists", groupSnapshotID)
Copy link
Collaborator

Choose a reason for hiding this comment

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

log as error

@Nikhil-Ladha Nikhil-Ladha force-pushed the fix-vgr-snapshot-rpc branch 3 times, most recently from 21b0954 to 87acd2b Compare December 16, 2024 10:45
@iPraveenParihar
Copy link
Contributor

@Nikhil-Ladha, do we need to back port this to release-v3.13?

@Nikhil-Ladha
Copy link
Contributor Author

Yeah, we should backport this along with the previous PR.

@Nikhil-Ladha
Copy link
Contributor Author

@Mergifyio backport release-v3.13

@mergify Mergify
Copy link
Contributor

mergify bot commented Dec 19, 2024

backport release-v3.13

❌ Command disallowed due to command restrictions in the Mergify configuration.

  • sender-permission >= write

@Nikhil-Ladha
Copy link
Contributor Author

I don't seem to have permissions to do so :/
Can you please trigger the command for this PR as well #5001 ?

@iPraveenParihar
Copy link
Contributor

@Mergifyio queue

@mergify Mergify
Copy link
Contributor

mergify bot commented Dec 19, 2024 •
edited
Loading

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 18a62ec

@Nikhil-Ladha
util: return correct status code for VolumeGroupSnapshot
851e4d1 
Fix status codes that are returned for Get/Delete RPC calls
for VolumeGroup/VolumeGroupSnapshot.

Signed-off-by: Nikhil-Ladha <[email protected]>
@mergify mergify bot added the ok-to-test Label to trigger E2E tests label Dec 19, 2024
@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-helm/k8s-1.29

@ceph-csi-bot
Copy link
Collaborator

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

@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/upgrade-tests-cephfs

@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-helm/k8s-1.30

@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.31

@ceph-csi-bot
Copy link
Collaborator

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

@ceph-csi-bot ceph-csi-bot removed the ok-to-test Label to trigger E2E tests label Dec 19, 2024
@iPraveenParihar
Copy link
Contributor

I don't seem to have permissions to do so :/
Can you please trigger the command for this PR as well #5001 ?

Sure, lets merge this first then will backport both in order.

@mergify mergify bot merged commit 18a62ec into ceph:devel Dec 19, 2024
37 checks passed
@iPraveenParihar iPraveenParihar added the backport-to-release-v3.13 Label to backport from devel to release-v3.13 branch label Dec 20, 2024
@mergify mergify bot mentioned this pull request Dec 20, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iPraveenParihar iPraveenParihar iPraveenParihar approved these changes

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

Assignees
No one assigned
Labels
backport-to-release-v3.13 Label to backport from devel to release-v3.13 branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Nikhil-Ladha @iPraveenParihar @ceph-csi-bot @Madhu-1