rbd: use pre-existing volume group if content matches #5221
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nixpanic
force-pushed
the
rbd/re-use-existing-volume-group
branch
from
65987ab to
12f7dce
Compare
|
Manually testing by creating a VolumeGroup with the Then creating a VolumeGroupSnapshot with the same volumes fails: The last line from the Ceph-CSI logs suggest that the volumes are attempted to be added to a wrong group: |
nixpanic
force-pushed
the
rbd/re-use-existing-volume-group
branch
2 times, most recently
from
27e9df7 to
6c7e7ed
Compare
|
The current state seems to work well. Only the Ceph main branch supports the |
nixpanic
force-pushed
the
rbd/re-use-existing-volume-group
branch
from
6c7e7ed to
0f6445e
Compare
Rakshith-R
reviewed
Mar 21, 2025
Rakshith-R
reviewed
Mar 21, 2025
6 tasks
Rakshith-R
reviewed
Mar 21, 2025
nixpanic
force-pushed
the
rbd/re-use-existing-volume-group
branch
2 times, most recently
from
1925c37 to
ebf62d7
Compare
See #5232. |
internal/rbd/group.go
Outdated
| return librbd.GroupImageRemove(ioctx, id, rv.ioctx, rv.RbdImageName) | ||
| } | ||
|
|
||
| // GetVolumeGroupID returns the name of the VolumeGroup where this rbdVolume |
There was a problem hiding this comment.
Suggested change
| // GetVolumeGroupID returns the name of the VolumeGroup where this rbdVolume | |
| // GetVolumeGroupID returns the ID of the VolumeGroup where this rbdVolume |
internal/rbd/group.go
Outdated
| return fmt.Errorf("could not get id for volume group %q: %w", vg, err) | ||
| } | ||
|
|
||
| return librbd.GroupImageRemove(ioctx, id, rv.ioctx, rv.RbdImageName) |
There was a problem hiding this comment.
GroupImageRemove accepts the group name and not id, so shouldn't we continue to fetch the name and pass it to the API? Or, am I missing something here?
There was a problem hiding this comment.
The naming has been a bit confusing to me as well. The GetID() function returns the generated unique name of the rbd-group (something like csi-group-<UUID>), the GetName() function returns the name that was requested by the caller with the CreateVolumeGroup procedure (stored in the journal and pointing to the generated name/id of the real RBD-group).
Throughout Ceph-CSI there are many id fields, and the naming has not always been consistent. This commit corrects an earlier mistake that I found when manually creating a volume-group with a name my-group and trying to use that to create a VolumeGroupSnapshot of. ID/name was incorrect, with this change it looks way cleaner.
There was a problem hiding this comment.
I'll add a better description in internal/rbd/types.journalledObject for these functions.
There was a problem hiding this comment.
It seems the confusion is on my part. The GetID() function returns the CSI-handle formatted unique ID, it can be parsed and used to obtain the details from the journal, which then point to the actual object in the backend (RBD-image or RBD-group name). Updated the documentation for internal/rbd/types.journalledObject again.
Madhu-1
reviewed
Mar 24, 2025
Comment on lines
101
to
108
| if vol == nil { | ||
| continue | ||
| } |
There was a problem hiding this comment.
@nixpanic we dont get to this state isnt it?
There was a problem hiding this comment.
Not normally, but we can. The commit that adds this part has the following commit message:
When an incorrect volumeID is passed while creating a VolumeGroup, the
.Destroy()function causes a panic. With this extra check to skip
nilvolumes, the panic does not happen anymore.
There was a problem hiding this comment.
This can probably also be solved by creating an empty volumes slice, and appending each volume after it has been resolved. If that is something you prefer, I can take that approach.
There was a problem hiding this comment.
that makes sense, we could avoid this check
There was a problem hiding this comment.
Updated, please check again.
| return nil, err | ||
| // reuse the existing group, it contains the volumes already | ||
| // check for pre-existing volume group name | ||
| vgHandle, err := volumes[0].GetVolumeGroupID(ctx, mgr) |
There was a problem hiding this comment.
length of volumes can be 0?
There was a problem hiding this comment.
Hmm, yes, I think that might be possible. I'll add a check for that.
nixpanic
force-pushed
the
rbd/re-use-existing-volume-group
branch
from
ebf62d7 to
24a843e
Compare
nixpanic
force-pushed
the
rbd/re-use-existing-volume-group
branch
from
24a843e to
a80af79
Compare
Rakshith-R
requested changes
Mar 25, 2025
nixpanic
force-pushed
the
rbd/re-use-existing-volume-group
branch
from
a80af79 to
3068c73
Compare
|
LGTM, |
nixpanic
force-pushed
the
rbd/re-use-existing-volume-group
branch
from
3068c73 to
fb4e754
Compare
|
This is now based on #5184, making sure both PRs can be merged together nicely. |
|
@Mergifyio rebase |
When an incorrect volumeID is passed while creating a VolumeGroup, the `.Destroy()` function caused a panic. By appending each volume to the volumes slice, the slice won't contain any `nil` volumes anymore. Signed-off-by: Niels de Vos <[email protected]>
The `prefix` is passed to several functions, but it can easily be obtained with a small helper function. This makes calling the functions a little simpler. Signed-off-by: Niels de Vos <[email protected]>
The Manager.MakeVolumeGroupID() function can be used to build a CSI VolumeGroupID from the backend (pool and name of the RBD-group). This will be used when checking if an RBD-image belongs to a group already. It is also possible to resolve the VolumeGroup by passing the VolumeGroupID to the existing Manager.GetVolumeGroupByID() function. Signed-off-by: Niels de Vos <[email protected]>
CompareVolumesInGroup() verifies that all the volumes are part of the given VolumeGroup. It does so by obtaining the VolumeGroupID for each volume with GetVolumeGroupByID(). The helper VolumesInSameGroup() verifies that all volumes belong to the same (or no) VolumeGroup. It can be called by CSI(-Addons) procedures before acting on a VolumeGroup. Signed-off-by: Niels de Vos <[email protected]>
When a VolumeGroup has been created through the CSI-Addons API, the VolumeGroupSnapshot CSI API will now use the existing VolumeGroup. There are checks in place to validate that the Volumes in the VolumeGroup match the Volumes in the VolumeGroupSnapshot request. Signed-off-by: Niels de Vos <[email protected]>
The `GetID()` and `GetName()` functions can be confusing, as names and ID's are not always distinctive enough. The name is used to reference an object that exists in a pool. The ID the CSI-handle formatted and can be used to locate the entry for the object in the journal. Signed-off-by: Niels de Vos <[email protected]>
✅ Branch has been successfully rebased |
nixpanic
force-pushed
the
rbd/re-use-existing-volume-group
branch
from
fb4e754 to
a771326
Compare
|
/test ci/centos/mini-e2e-helm/k8s-1.32 |
|
/test ci/centos/mini-e2e-helm/k8s-1.31 |
|
/test ci/centos/mini-e2e-helm/k8s-1.30 |
|
/test ci/centos/mini-e2e/k8s-1.32 |
|
/test ci/centos/mini-e2e/k8s-1.31 |
|
/test ci/centos/mini-e2e/k8s-1.30 |
|
/test ci/centos/k8s-e2e-external-storage/1.32 |
|
/test ci/centos/k8s-e2e-external-storage/1.31 |
|
/test ci/centos/k8s-e2e-external-storage/1.30 |
|
/test ci/centos/upgrade-tests |
|
/retest ci/centos/mini-e2e/k8s-1.31 |
Failed while deploying minikube cluster logs |
Madhu-1
approved these changes
Mar 27, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
CSI-Addons Volume Groups makes it possible to create a long-living volume group. This is incompatible with the VolumeGroupSnapshot feature, as that creates a temporary VolumeGroup before creating a snapshot of it. When a Volume is already part of a different Volume Group, things will fail.
In order to make both features compatible with each other, the following things are required:
Depends-on: #5184