Skip to content

RBD: check for volume group existence #5184

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 3 commits into from
Mar 27, 2025
Merged

RBD: check for volume group existence #5184

merged 3 commits into from
Mar 27, 2025

Conversation

@iPraveenParihar
Copy link
Contributor

@iPraveenParihar iPraveenParihar commented Feb 28, 2025 •
edited
Loading

Describe what this PR does

This PR adds a check for Volume Group existence in generateVolumeGroup call.

Checklist:

  • Commit Message Formatting: Commit titles and messages follow guidelines in the developer guide.
  • Reviewed the developer guide on Submitting a Pull Request
  • Pending release notes updated with breaking and/or notable changes for the next major release.
  • Documentation has been updated, if necessary.
  • Unit tests have been added, if necessary.
  • Integration tests have been added, if necessary.
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!)

@iPraveenParihar iPraveenParihar changed the title Rbd/check for volume group existence RBD: check for volume group existence Feb 28, 2025
nixpanic
nixpanic reviewed Feb 28, 2025
View reviewed changes
nixpanic
nixpanic reviewed Feb 28, 2025
View reviewed changes
@iPraveenParihar iPraveenParihar force-pushed the rbd/check-for-volume-group-existence branch from f05f221 to ca306ac Compare February 28, 2025 10:57
@iPraveenParihar iPraveenParihar marked this pull request as ready for review February 28, 2025 10:58
Rakshith-R
Rakshith-R requested changes Feb 28, 2025
View reviewed changes
nixpanic
nixpanic reviewed Feb 28, 2025
View reviewed changes
nixpanic
nixpanic reviewed Feb 28, 2025
View reviewed changes
nixpanic
nixpanic reviewed Feb 28, 2025
View reviewed changes
nixpanic
nixpanic reviewed Feb 28, 2025
View reviewed changes
nixpanic
nixpanic reviewed Feb 28, 2025
View reviewed changes
@nixpanic nixpanic added the component/rbd Issues related to RBD label Feb 28, 2025
@iPraveenParihar iPraveenParihar force-pushed the rbd/check-for-volume-group-existence branch from 42537fd to bbb8f40 Compare March 3, 2025 08:59
@iPraveenParihar iPraveenParihar marked this pull request as draft March 3, 2025 10:18
@nixpanic
Copy link
Member

nixpanic commented Mar 3, 2025

@iPraveenParihar , the new Exists() function in the interface causes the golangci-lint to fail. As all interfaces have a Destroy() function, you could move that one to a base interface (journalledObject maybe) in a separate commit.

@iPraveenParihar iPraveenParihar force-pushed the rbd/check-for-volume-group-existence branch 3 times, most recently from 9630c7c to 6323c99 Compare March 4, 2025 10:31
@iPraveenParihar
Copy link
Contributor Author

Waiting on @Nikhil-Ladha, for the testing result.

@iPraveenParihar iPraveenParihar force-pushed the rbd/check-for-volume-group-existence branch 3 times, most recently from 8a7f9aa to 813266f Compare March 4, 2025 11:27
@iPraveenParihar
Copy link
Contributor Author

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

@iPraveenParihar iPraveenParihar changed the title RBD: check for volume group existence [WIP] RBD: check for volume group existence Mar 5, 2025
@Nikhil-Ladha
Copy link
Contributor

Waiting on @Nikhil-Ladha, for the testing result.

Tested with different pool ids on both the clusters (and the cluster had other pools too) and, everything looks good.
We can probably optimize the code to avoid dual checks for omap data, but it can be a part of different PR as well.

@iPraveenParihar iPraveenParihar force-pushed the rbd/check-for-volume-group-existence branch from 813266f to 882cefc Compare March 13, 2025 07:43
@iPraveenParihar
Copy link
Contributor Author

Tested with different pool ids on both the clusters (and the cluster had other pools too) and, everything looks good. We can probably optimize the code to avoid dual checks for omap data, but it can be a part of different PR as well.

Thanks @Nikhil-Ladha for the testing! I could think of using the rbd_group_get_id from ceph/ceph but not yet available in go-ceph for now. We could pick it up once we have it in go-ceph.

@iPraveenParihar iPraveenParihar marked this pull request as ready for review March 13, 2025 10:12
@iPraveenParihar iPraveenParihar force-pushed the rbd/check-for-volume-group-existence branch from 882cefc to ce63858 Compare March 13, 2025 10:12
@iPraveenParihar iPraveenParihar changed the title [WIP] RBD: check for volume group existence RBD: check for volume group existence Mar 14, 2025
@iPraveenParihar iPraveenParihar force-pushed the rbd/check-for-volume-group-existence branch 3 times, most recently from 2be7a44 to eedbfb3 Compare March 21, 2025 09:52
Rakshith-R
Rakshith-R previously approved these changes Mar 21, 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, Thanks

nixpanic
nixpanic reviewed Mar 21, 2025
View reviewed changes
internal/csi-addons/rbd/encryptionkeyrotation.go
"errors"

"github.com/ceph/ceph-csi/internal/rbd"
rbderrors "github.com/ceph/ceph-csi/internal/rbd/errors"
Copy link
Member

Choose a reason for hiding this comment

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

maybe it makes sense to move common RBD errors to internal/rbd/types/errors.go? That is what I decided to do with #5221.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO, rbd/errors is more suitable than rbd/types (which should be just for the interface and struct definition?)

Copy link
Member

Choose a reason for hiding this comment

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

That is fine with me too. I just wanted to locate the errors that are documented at the types/interfaces together. I' can move it to internal/rbd/errors too.

internal/rbd/group/util.go
cvg.pool = pool

// Check if the volume group exists in the journal.
_, err = cvg.getVolumeGroupAttributes(ctx)
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be called here? The initialization of the struct does not need to guarantee that the volume group is recorded in the journal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to keep it similar to what we have for generateVolumeFromVolumeID. Here to, we initialise the rbdVol struct and fetch details from journal. Calling outside, made it little complex for readability.

Copy link
Member

Choose a reason for hiding this comment

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

Ok. I just wonder about possible side-effects in case a volumegroup does not exist, or was in the process of getting deleted. I think it should be fine, but maybe you can check that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just wonder about possible side-effects in case a volumegroup does not exist

initCommonVolumeGroup is only been called after a VolumeGroup is created, we are good here.

was in the process of getting deleted

operation locks will prevent this case, but actually I don't see operationa locks on /DeleteVolumeGroup & /CreateVolumeGroup, was that missed or not required at all?

Copy link
Contributor

Choose a reason for hiding this comment

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

initCommonVolumeGroup is only been called after a VolumeGroup is created, we are good here.

Actually, we can. For example, if we try to delete an already deleted (or non-existent) volume. Though, looking at the code it would still fail with the same error i.e, ErrGroupNotFound but after going through some more function calls.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that should be fine.

internal/rbd/group/util.go Outdated

// generateVolumeGroupFromMapping checks the clusterID and poolID mapping and
// generates commonVolumeGroup structure for the mapped clusterID and poolID.
// If the mapping is not found, it returns ErrRBDGroupNotFound.
Copy link
Member

Choose a reason for hiding this comment

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

the comment still contains ErrRBDGroupNotFound -> ErrGroupNotFound

@Rakshith-R Rakshith-R mentioned this pull request Mar 26, 2025
Merged
@iPraveenParihar iPraveenParihar force-pushed the rbd/check-for-volume-group-existence branch from eedbfb3 to 6ba791d Compare March 26, 2025 09:31
@mergify mergify bot dismissed Rakshith-R’s stale review March 26, 2025 09:31

Pull request has been modified.

@nixpanic
Copy link
Member

@Mergifyio rebase

@iPraveenParihar @nixpanic
cleanup: move internal/rbd/errors.go to internal/rbd/errors pacakge
60ea14e 
Signed-off-by: Praveen M <[email protected]>
@iPraveenParihar @nixpanic
rbd: check for volume group existence
727aad5 
Signed-off-by: Praveen M <[email protected]>
@iPraveenParihar @nixpanic
cleanup: move Destroy() method to journalledObject interface
4d7e636 
VolumeGroup interface has more than 10 methods and it causes
golangci-lint to fail. Moving the `Destroy()` method to a base
interface journalledObject.

Signed-off-by: Praveen M <[email protected]>
@mergify
Copy link
Contributor

mergify bot commented Mar 27, 2025

rebase

✅ Branch has been successfully rebased

@nixpanic nixpanic force-pushed the rbd/check-for-volume-group-existence branch from 6ba791d to 4d7e636 Compare March 27, 2025 08:11
@nixpanic
Copy link
Member

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

@nixpanic
Copy link
Member

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

@nixpanic
Copy link
Member

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

@nixpanic
Copy link
Member

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

@nixpanic
Copy link
Member

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

@nixpanic
Copy link
Member

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

@nixpanic
Copy link
Member

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

@nixpanic
Copy link
Member

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

@nixpanic
Copy link
Member

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

@nixpanic
Copy link
Member

/test ci/centos/upgrade-tests

@mergify mergify bot merged commit add4b36 into ceph:devel Mar 27, 2025
26 checks passed
@iPraveenParihar iPraveenParihar added the backport-to-release-v3.13 Label to backport from devel to release-v3.13 branch label Mar 27, 2025
@mergify mergify bot mentioned this pull request Mar 27, 2025
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Nikhil-Ladha Nikhil-Ladha Nikhil-Ladha left review comments

@nixpanic nixpanic nixpanic approved these changes

@Rakshith-R Rakshith-R Rakshith-R approved these changes

@Madhu-1 Madhu-1 Madhu-1 left review comments

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@iPraveenParihar @nixpanic @Nikhil-Ladha @Madhu-1 @Rakshith-R