-
Notifications
You must be signed in to change notification settings - Fork 554
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
[WIP] Implement support for volume group mirroring #4739
base: devel
Are you sure you want to change the base?
Conversation
bd794c1
to
4a5761c
Compare
internal/rbd/group/volume_group.go
Outdated
|
||
// GroupStatus is a wrapper around librbd.MirrorGroupInfo that contains the | ||
// group mirror info. | ||
type GroupStatus 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.
why not just do it like
type GroupStatus *librbd.MirrorGroupInfo
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.
Methods defined on the original type are not available to the new type unless you explicitly defined
if we create a type i need to implement duplication methods of MirrorGroupInfo here which is again not much useful, lets keep it this way which allow us to use methods of librbd struct without any problem
This pull request now has conflicts with the target branch. Could you please resolve conflicts and force push the corrected changes? 🙏 |
3337a23
to
add2d0b
Compare
@nixpanic This is ready for one more round of review :) |
This pull request now has conflicts with the target branch. Could you please resolve conflicts and force push the corrected changes? 🙏 |
add2d0b
to
3808a65
Compare
|
||
// GetMirrorSource returns the source of the mirror for the given volume or group. | ||
GetMirrorSource(ctx context.Context, volumeID string, | ||
rep *replication.ReplicationSource) ([]Volume, Mirror, error) |
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.
it is not clear to me why []Volume
is returned. Should that not be either a single Volume or a 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.
would it make sense to have a Mirror.GetSource()
function so that the API is easier to understand?
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.
Let's put everything related to processing volumes within functions in mirror type ?
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.
would it make sense to have a Mirror.GetSource() function so that the API is easier to understand?
@nixpanic I put it with the Manager because that is the one who is generating the volume and group from an ID, moving this to Mirroring
it is not clear to me why []Volume is returned. Should that not be either a single Volume or a VolumeGroup?
This is required because we need to work on all the volumes if its a group, i am trying to avoid type casting and doing the ListVolumes and generating the volumes again that contains the connections, let me know if you have any suggestions
Let's put everything related to processing volumes within functions in mirror type ?
I mostly added all the mirroring specific things to mirror type but there are again generic one as well that the reason i left it in volume type as it makes more sense.
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.
👍
We can check on volumes and refactor it later on.
Everything LGTM
Only concern now is ceph's API stability and go-ceph pr going in.
internal/rbd/group/volume_group.go
Outdated
@@ -469,3 +474,296 @@ func (vg *volumeGroup) ListVolumes(ctx context.Context) ([]types.Volume, error) | |||
func (vg *volumeGroup) ToMirror() (types.Mirror, error) { | |||
return vg, nil | |||
} | |||
|
|||
func (vg *volumeGroup) EnableMirroring(ctx context.Context, mode librbd.ImageMirrorMode) error { |
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.
I wonder if it is worth to simplify things a little by placing this in a separate file group_mirror.go
?
type volumeGroupMirror *volumeGroup // or make it a struct with extra members?
func (vgm *volumeGroupMirror) EnableMirroring(ctx context.Context, mode librbd.ImageMirrorMode) error {
...
This would keep the VolumeGroup interface smal(ler). I am considering a similar approach for VolumeGroupSnapshot.
internal/rbd/group/volume_group.go
Outdated
return nil, fmt.Errorf("failed to get volume group mirroring info %q: %w", vg, err) | ||
} | ||
|
||
return GroupInfo{MirrorGroupInfo: info}, nil |
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 could also do
type GroupInfo *librbd.MirrorGroupInfo
and cast it here
return GroupInfo(info)
That looks a little simpler to me.
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.
Yes but again this will have problem like #4739 (comment) and adding new field to GroupInfo need more refractoring, IMO this should be okay but the struct name should not be exported(taken care of it)
internal/rbd/group/volume_group.go
Outdated
} | ||
|
||
func (status GlobalMirrorGroupStatus) GetState() string { | ||
return status.GlobalMirrorGroupStatus.Info.State.String() |
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.
could any of the members be nil
?
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.
Not possible as the members are not pointers
internal/rbd/group/volume_group.go
Outdated
|
||
// GroupInfo is a wrapper around librbd.MirrorGroupInfo that contains the | ||
// group mirror info. | ||
type GroupInfo 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.
If you want to use a struct, consider exposing an interface instead of the struct. That encourages a cleaner API towards consumers.
@@ -280,15 +280,12 @@ func (rs *ReplicationServer) EnableVolumeReplication(ctx context.Context, | |||
mgr := rbd.NewManager(rs.csiID, req.GetParameters(), req.GetSecrets()) | |||
defer mgr.Destroy(ctx) | |||
|
|||
rbdVol, err := mgr.GetVolumeByID(ctx, volumeID) | |||
volumes, mirror, err := mgr.GetMirrorSource(ctx, volumeID, req.GetReplicationSource()) |
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.
let's call volumeID
something else like reqID or replicationID.
It can then be either referring to a single volume or group (or later on snapshot).
3808a65
to
20f0600
Compare
d0c541f
to
ea1f176
Compare
The PR description contains the unsupported |
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. |
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. |
ea1f176
to
8082d90
Compare
This adds the required functionality to call the go-ceph API's for the rbd volume group. Signed-off-by: Madhu Rajanna <[email protected]>
8082d90
to
b933aba
Compare
implementing GetMirrorSource in manager to return volume or the volumegroup based on the replication source, if replication source is nil return the volume details for backward compatibility. Signed-off-by: Madhu Rajanna <[email protected]>
This is temporary test only commit and not suitable for review. Signed-off-by: Madhu Rajanna <[email protected]>
95bf128
to
2ac83b6
Compare
This is extra commit will be dropped Signed-off-by: Madhu Rajanna <[email protected]>
2ac83b6
to
2972642
Compare
This PR does the required changes so that the volume group can also implement the Mirror interface and we do need to make many changes in the replication code to work differently for the mirroring of an image or a mirroring of a group.
Note:- The last commit to add use go-ceph is commented out, please don't review it, Will remove that code and make changes to the mirror implementation of the group. except that other commits can be reviewed.
Depends-on: ceph/go-ceph#1010