Skip to content

rbd: skip comparing volumes in a group if new group is created #5345

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
Merged
merged 1 commit into from
Jun 2, 2025
Merged

rbd: skip comparing volumes in a group if new group is created #5345

merged 1 commit into from
Jun 2, 2025
+15 −15

Conversation

@Nikhil-Ladha
Copy link
Contributor

While calling CreateVolumeGroup RPC, if a new group is being created we can skip comparing volumes in a group because it is redundant.

Fixes: #5344

@mergify mergify bot added the component/rbd Issues related to RBD label May 30, 2025
@Nikhil-Ladha Nikhil-Ladha requested a review from nixpanic May 30, 2025 06:06
Rakshith-R
Rakshith-R reviewed May 30, 2025
View reviewed changes
iPraveenParihar
iPraveenParihar reviewed May 30, 2025
View reviewed changes
@Rakshith-R
Copy link
Contributor

@Nikhil-Ladha
Shouldn't the solution be to initialize the volumes list in VG to empty list when a VG is created ?

@Nikhil-Ladha
Copy link
Contributor Author

@Nikhil-Ladha Shouldn't the solution be to initialize the volumes list in VG to empty list when a VG is created ?

It's not about initialising the list. If you check the trace mentioned in the lined issue, the error happened here: https://github.com/ceph/ceph-csi/blob/devel/internal/rbd/manager.go#L585 where we are checking using the 0th index directly ([0])

@Rakshith-R
Copy link
Contributor

@Nikhil-Ladha Shouldn't the solution be to initialize the volumes list in VG to empty list when a VG is created ?

It's not about initialising the list. If you check the trace mentioned in the lined issue, the error happened here: https://github.com/ceph/ceph-csi/blob/devel/internal/rbd/manager.go#L585 where we are checking using the 0th index directly ([0])

Then, @iPraveenParihar approach would be preferred #5345 (comment)
than an external check.

@Nikhil-Ladha
rbd: skip comparing volumes in a group if new group is created
66a975a 
While calling CreateVolumeGroup RPC, if a new group is being created
we can skip comparing volumes in a group because it is redundant.

Signed-off-by: Nikhil-Ladha <[email protected]>
@Nikhil-Ladha Nikhil-Ladha force-pushed the issue5344/fix-crash branch from 57a77d9 to 66a975a Compare May 30, 2025 08:34
Rakshith-R
Rakshith-R reviewed May 30, 2025
View reviewed changes
Copy link
Contributor

@Rakshith-R Rakshith-R left a comment

Choose a reason for hiding this comment

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

LGTM

@iPraveenParihar
Copy link
Contributor

@Mergifyio rebase

@mergify
Copy link
Contributor

mergify bot commented Jun 2, 2025

rebase

☑️ Nothing to do, the required conditions are not met

  • any of:
    • #commits > 1 [📌 rebase requirement]
    • #commits-behind > 0 [📌 rebase requirement]
    • -linear-history [📌 rebase requirement]
  • -closed [📌 rebase requirement]
  • -conflict [📌 rebase requirement]
  • queue-position = -1 [📌 rebase requirement]

@iPraveenParihar
Copy link
Contributor

@Mergifyio queue

@mergify
Copy link
Contributor

mergify bot commented Jun 2, 2025 •
edited
Loading

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 2217e5c

@mergify mergify bot added the ok-to-test Label to trigger E2E tests label Jun 2, 2025
@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.31

@ceph-csi-bot
Copy link
Collaborator

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

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/upgrade-tests-rbd

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e-helm/k8s-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.32

@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/mini-e2e/k8s-1.32

@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 Jun 2, 2025
@mergify mergify bot merged commit 2217e5c into ceph:devel Jun 2, 2025
36 of 37 checks passed
@mergify
Copy link
Contributor

mergify bot commented Jun 2, 2025

This pull request has been removed from the queue for the following reason: pull request branch update failed.

The pull request can't be updated.

You should update or rebase your pull request manually. If you do, this pull request will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue, you can requeue the pull request, without updating it, by posting a @mergifyio requeue comment.

@iPraveenParihar
Copy link
Contributor

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

@Nikhil-Ladha
Copy link
Contributor Author

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

Only v3.14

@iPraveenParihar iPraveenParihar added the backport-to-release-v3.14 Label to backport from devel to release-v3.14 branch label Jun 2, 2025
@mergify mergify bot mentioned this pull request Jun 2, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Rakshith-R Rakshith-R Rakshith-R left review comments

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

@iPraveenParihar iPraveenParihar iPraveenParihar approved these changes

@nixpanic nixpanic Awaiting requested review from nixpanic

Assignees

No one assigned

Labels

backport-to-release-v3.14 Label to backport from devel to release-v3.14 branch component/rbd Issues related to RBD

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ceph rbd crashes when volumegroup with 0 volumes is created

5 participants

@Nikhil-Ladha @Rakshith-R @iPraveenParihar @ceph-csi-bot @Madhu-1