-
Notifications
You must be signed in to change notification settings - Fork 548
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.
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
rbd: VolumeGroupReplicationContent controller to regenerate the OMAP data #4750
base: devel
Are you sure you want to change the base?
rbd: VolumeGroupReplicationContent controller to regenerate the OMAP data #4750
Conversation
internal/rbd/group/volume_group.go
Outdated
type volumeGroup struct { | ||
// id is a unique value for this volume group in the Ceph cluster, it | ||
// VolumeGroup handles all requests for 'rbd group' operations. | ||
type VolumeGroup struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't export these, they are private intentional. They implement the interface in the internal/rbd/types/*.go
files. You can use rbd.NewManager()
to get a manager struct and use the Manager API to get the VolumeGroup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you are referring to GetVolumeGroupByID
, then It doesn't suite well to use it here. Because in GetVolumeGroup
we have GetPoolName by LocationID
(which is PoolID) and PoolID may be different in secondary cluster.
ceph-csi/internal/rbd/group/volume_group.go
Lines 106 to 109 in 130e8b4
pool, err := util.GetPoolName(mons, creds, csiID.LocationID) | |
if err != nil { | |
return nil, fmt.Errorf("failed to get pool for volume group id %q: %w", id, err) | |
} |
Else we can do it by passing an optional parameter PoolName
for GetVolumeGroup
?
If it is passed as empty, then call GetPoolName
else use the PoolName
passed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may want to add more functions to the manager and other types. Ideally the API that the manager provides stays simple to use.
2a75eec
to
6d757b8
Compare
Initial Test results -
|
0290729
to
628b00b
Compare
This pull request now has conflicts with the target branch. Could you please resolve conflicts and force push the corrected changes? 🙏 |
628b00b
to
3088e07
Compare
The PR description contains the unsupported |
17e3418
to
62e2f4a
Compare
87e64d7
to
01ca0f3
Compare
01ca0f3
to
92de84e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the change, looks much better to me.
internal/controller/volumegroup/volumegroupreplicationcontent.go
Outdated
Show resolved
Hide resolved
d017163
to
7baabbf
Compare
This pull request now has conflicts with the target branch. Could you please resolve conflicts and force push the corrected changes? 🙏 |
internal/controller/volumegroup/volumegroupreplicationcontent.go
Outdated
Show resolved
Hide resolved
internal/controller/volumegroup/volumegroupreplicationcontent.go
Outdated
Show resolved
Hide resolved
internal/controller/volumegroup/volumegroupreplicationcontent.go
Outdated
Show resolved
Hide resolved
internal/controller/volumegroup/volumegroupreplicationcontent.go
Outdated
Show resolved
Hide resolved
4348949
to
c744ae4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a nit, but should get corrected never the less.
This pull request now has conflicts with the target branch. Could you please resolve conflicts and force push the corrected changes? 🙏 |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in two weeks if no further activity occurs. Thank you for your contributions. |
5083243
to
98a23cd
Compare
This commit adds groupUUID param for `ReserveName` to be used for OMAP name reserve instead of auto-generating. This is useful for mirroring and metro-DR ensuring that mirrored resources have consistent OMAP names across mirrored clusters. Signed-off-by: Praveen M <m.praveen@ibm.com>
This commit adds `RegenerateVolumeGroupJournal` to Manager interface. RegenerateVolumeGroupJournal regenerate the omap data for the volume group. This performs the following operations: - extracts clusterID and Mons from the cluster mapping - Retrieves pool and journalPool parameters from the VolumeGroupReplicationClass - Reserves omap data - Add volumeIDs mapping to the reserved volume group omap object - Generate new volume group handle Returns the generated volume group handler. Signed-off-by: Praveen M <m.praveen@ibm.com>
98a23cd
to
8a72e9c
Compare
@nixpanic, I have addressed your requested changes and rebased. |
This commit adds new controller that watches for the VolumeGroupReplicationContent and regenerates the OMAP data if it doesn't exists. Signed-off-by: Praveen M <m.praveen@ibm.com>
8a72e9c
to
0927a48
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Looks like there is a conflict in go.mod
, that needs addressing before this can be merged.
This pull request now has conflicts with the target branch. Could you please resolve conflicts and force push the corrected changes? 🙏 |
Describe what this PR does
This commit adds new controller that watches for the
VolumeGroupReplicationContent and regenerates the OMAP data if
it doesn't exists.
Checklist:
Request
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 unrelatedfailure (please report the failure too!)