rbd: refactor mirroring to work with volume and volumegroup #4720

Madhu-1 · 2024-07-19T08:44:44Z

refactor mirroring to work with volume and volumegroup

Madhu-1 · 2024-07-19T08:45:29Z

@nixpanic can you please check this PR for early review, i still need to do testing, once everything is good i will move this to Ready state.

scripts/golangci.yml.in

internal/csi-addons/rbd/replication.go

internal/rbd/mirror.go

internal/rbd/types/mirror.go

internal/rbd/types/volume.go

internal/rbd/types/mirror.go

nixpanic · 2024-07-19T09:00:39Z

internal/rbd/types/mirror.go

+	Resync() error
+
+	// GetMirroringInfo returns the mirroring information of the resource
+	GetMirroringInfo() (interface{}, error)


It is really ugly to return an untyped interface. Can that be improved?

i tried but it gets more difficult for implementation where i need to have different duplicate checks for both volume and group, that's the reason to settle on this after lot of other considerations

Maybe add a MirrorInfo interface too?

Am not sure adding MirrorInfo interface helps but the problem is that the volumemirror and groupmirror in go-ceph return different structs, if both are returning the same i could have solved this or else I need to own a local struct and fill the required values from both, i can do this if you prefer (this will have lot of duplication logic and maintenance work)

Maybe, or have a MirrorInfo interface with only some basic function, or completely empty? At least the type can then describe that it is actually one of the go-ceph types.

nixpanic · 2024-07-19T18:27:35Z

internal/rbd/types/volume.go

+	"google.golang.org/protobuf/types/known/timestamppb"
 )

+//nolint:interfacebloat // more than 10 methods are needed for the interface


In #4719 I added the journalledObject interface. Such an approach also prevents the need for skipping the interfacebloat linter.

nixpanic · 2024-07-22T08:01:17Z

internal/csi-addons/rbd/replication.go

-			rbdVol.Destroy(ctx)
-		}
-	}()
+	mgr := rbd.NewManager(req.GetParameters(), req.GetSecrets())


note that the CSI-instance id (drivername) needs to be passed to the manager in an upcoming change.

Madhu-1 · 2024-07-22T12:28:10Z

@nixpanic I have tested this code and works as expected, let me know if you want to make any changes you are referring to the manager as part of this PR.

nixpanic

Splitting the commit would be nice.
At least add the new types in a separate commit, and use them in subsequent ones.

The single commit is +488 -300 lines of code, which is rather a lot.

internal/rbd/mirror.go

internal/rbd/types/mirror.go

nixpanic · 2024-07-26T07:29:48Z

internal/rbd/types/mirror.go

+	// IsPrimary returns true if the resource is primary
+	IsPrimary() bool
+	// GetState returns the state of the resource
+	GetState() string


consider adding a MirrorState type to replace the string value

This needs more work, will take it in next update when implementing this for group

internal/rbd/types/volume.go

internal/rbd/types/mirror.go

nixpanic · 2024-07-26T08:43:10Z

internal/rbd/types/volume.go

 	RemoveFromGroup(ctx context.Context, vg VolumeGroup) error
+
+	// GetPoolName returns the name of the pool where the volume is stored.
+	GetPoolName() string


Maybe Volume should inherit the journalledObject too?

ceph-csi/internal/rbd/types/group.go

Lines 26 to 38 in aa88b4c

type journalledObject interface {

// GetID returns the CSI-Addons VolumeGroupId of the VolumeGroup.

GetID(ctx context.Context) (string, error)

// GetName returns the name in the backend storage for the VolumeGroup.

GetName(ctx context.Context) (string, error)

// GetPool returns the name of the pool that holds the VolumeGroup.

GetPool(ctx context.Context) (string, error)

// GetClusterID returns the ID of the cluster of the VolumeGroup.

GetClusterID(ctx context.Context) (string, error)

}

This needs a little more refactoring as journalledObject is specific to volumeGroup now, will address this in follow-up when implementing for group support

It is only the comments that refer to VolumeGroup. The functions could be for Volume as well.

nixpanic · 2024-07-26T08:46:39Z

internal/csi-addons/rbd/replication.go


 		return nil, err
 	}
+	mirror = rbdVol


Would it make sense to have rbdVol.ToMirror() that validates everything required for the new Mirror type?

@nixpanic can you explain a bit more on this one?

updated the code, PTAL and see if this matches your expectation

nixpanic · 2024-07-26T08:50:29Z

internal/rbd/types/volume.go

 //nolint:interfacebloat // more than 10 methods are needed for the interface
 type Volume interface {
+	// Mirror interface implementation for Volume
+	Mirror


because you use mirror = rbdVol, I would expect that the Mirror interface is kept seperate. I do not see a need to have it part of the Volume interface. The rbdVolume struct can still implement both interfaces.

nixpanic · 2024-07-26T09:23:40Z

internal/rbd/group.go

 	return librbd.GroupImageRemove(ioctx, name, rv.ioctx, rv.RbdImageName)
 }
+
+func (rv *rbdVolume) ToMirror() types.Mirror {


Thanks! Would it make sense to have some checking if mirror attributes are valid?

Not for now, do you have any specific checks in your mind?

Not really. Mostly just to have the function match func (*rbdVolume) ToCSI(ctx) (csi.Volume, error)

the signature is matching now, it returns nil for now but can be updated later as required.

nixpanic · 2024-07-26T09:44:21Z

@Mergifyio rebase

Adding mirror interface for rbd image and group which can be used to manage mirroring for both. Signed-off-by: Madhu Rajanna <[email protected]>

Add helper methods for the rbd image mirroring. Signed-off-by: Madhu Rajanna <[email protected]>

Refractoring code to use mirror interface. Signed-off-by: Madhu Rajanna <[email protected]>

mergify · 2024-07-26T09:44:29Z

rebase

✅ Branch has been successfully rebased

ceph-csi-bot · 2024-07-26T09:44:47Z

/test ci/centos/upgrade-tests-cephfs

ceph-csi-bot · 2024-07-26T09:44:48Z

/test ci/centos/upgrade-tests-rbd

ceph-csi-bot · 2024-07-26T09:44:48Z

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

ceph-csi-bot · 2024-07-26T09:44:48Z

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

ceph-csi-bot · 2024-07-26T09:44:48Z

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

ceph-csi-bot · 2024-07-26T09:44:49Z

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

ceph-csi-bot · 2024-07-26T09:44:49Z

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

ceph-csi-bot · 2024-07-26T09:44:49Z

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

ceph-csi-bot · 2024-07-26T09:44:49Z

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

ceph-csi-bot · 2024-07-26T09:44:50Z

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

ceph-csi-bot · 2024-07-26T09:44:50Z

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

ceph-csi-bot · 2024-07-26T09:44:54Z

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

ceph-csi-bot · 2024-07-26T09:44:55Z

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

ceph-csi-bot · 2024-07-26T09:44:56Z

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

iPraveenParihar

nit

iPraveenParihar · 2024-07-26T09:52:27Z

internal/rbd/types/mirror.go

+	AddSnapshotScheduling(interval admin.Interval, startTime admin.StartTime) error
+}
+
+// MirrorImage is the interface for managing mirroring on an RBD image or group of images.


Suggested change

// MirrorImage is the interface for managing mirroring on an RBD image or group of images.

// MirrorInfo is the interface for managing mirroring on an RBD image or group of images.

Will address this in follow up. Thanks @Madhu-1 for catching it.

iPraveenParihar · 2024-08-02T08:01:05Z

internal/rbd/driver/driver.go

 		r.cas.RegisterService(fcs)

-		rcs := casrbd.NewReplicationServer(NewControllerServer(r.cd))
+		rcs := casrbd.NewReplicationServer(rbd.CSIInstanceID, NewControllerServer(r.cd))


@Madhu-1 , shouldn't this be

Suggested change

rcs := casrbd.NewReplicationServer(rbd.CSIInstanceID, NewControllerServer(r.cd))

rcs := casrbd.NewReplicationServer(conf.InstanceID, NewControllerServer(r.cd))

?

yes, I think so. Can you send a PR for that?

conf.InstanceID will not work if its not set, we need to use default in that case we are not updating conf.InstanceID with rbd.CSIInstanceID, that's the reason to use rbd.CSIInstanceID as its gets updated always or it contains the default

@Madhu-1, we are doing that for InitJournals -

ceph-csi/internal/rbd/globals.go

Lines 93 to 100 in db6f54f

func InitJournals(instance string) {

// Use passed in instance ID, if provided for omap suffix naming

if instance != "" {

CSIInstanceID = instance

}

volJournal = journal.NewCSIVolumeJournal(CSIInstanceID)

snapJournal = journal.NewCSISnapshotJournal(CSIInstanceID)

And conf.InstanceID is passed to InitJournals

if the conf.InsatceID is empty its not updated with the default one that we have in the code right rbd.InitJournals(conf.InstanceID)? with this one

ceph-csi/internal/rbd/globals.go

Line 28 in db6f54f

CSIInstanceID = "default"

Apologies, I was mistaken it was other way around.
Thanks @Madhu-1.

Then I guess for VolumeGroupServer we need to use rbd.CSIInstanceID

ceph-csi/internal/rbd/driver/driver.go

Line 223 in db6f54f

vgcs := casrbd.NewVolumeGroupServer(conf.InstanceID)

@nixpanic ?

rbd.CSIInstanceID seems to be a global value, it is configurable on the commandline, so I think using the configured value is cleaner (all config options should have default values anyway?).

In internal/rbd/driver/driver.go there is rbd.InitJournals(conf.InstanceID), so in the end it is all the same.

conf.InstanceID default value is empty and rbd.CSIInstanceID is replaced with conf.InstanceID when it is configured in command line.
So using conf.InstanceID when its not configured will lead to creating journal directory csi.groups. ?

yeah, I noticed that too. The global rbd.CSIInstanceID could be set to something earlier on, in case the ordering of (seemingly unrelated) initialization of the journals is done later. This is a little fragile, and using global variables is quite ugly in any case. I'm preparing a PR for improving it.

Madhu-1 requested a review from nixpanic July 19, 2024 08:44

mergify bot added the component/rbd Issues related to RBD label Jul 19, 2024

nixpanic reviewed Jul 19, 2024

View reviewed changes

Madhu-1 force-pushed the mirror-vol-grp branch 3 times, most recently from 1899b64 to 7dbb200 Compare July 19, 2024 14:17

Madhu-1 requested a review from nixpanic July 19, 2024 14:18

Madhu-1 force-pushed the mirror-vol-grp branch from 7dbb200 to 0c44d86 Compare July 19, 2024 14:20

nixpanic reviewed Jul 19, 2024

View reviewed changes

nixpanic reviewed Jul 22, 2024

View reviewed changes

Madhu-1 force-pushed the mirror-vol-grp branch from 0c44d86 to 13f9e05 Compare July 22, 2024 12:27

Madhu-1 marked this pull request as ready for review July 22, 2024 12:27

Madhu-1 requested review from a team and nixpanic July 22, 2024 12:28

Madhu-1 force-pushed the mirror-vol-grp branch from 13f9e05 to 6b9ac05 Compare July 26, 2024 06:54

nixpanic reviewed Jul 26, 2024

View reviewed changes

Madhu-1 force-pushed the mirror-vol-grp branch from 6b9ac05 to 55e783d Compare July 26, 2024 08:01

Madhu-1 requested a review from nixpanic July 26, 2024 08:05

Madhu-1 force-pushed the mirror-vol-grp branch from 55e783d to 416352a Compare July 26, 2024 08:09

nixpanic reviewed Jul 26, 2024

View reviewed changes

Madhu-1 force-pushed the mirror-vol-grp branch from 416352a to 430b47d Compare July 26, 2024 09:09

Madhu-1 requested a review from nixpanic July 26, 2024 09:11

nixpanic reviewed Jul 26, 2024

View reviewed changes

Madhu-1 requested a review from nixpanic July 26, 2024 09:25

Madhu-1 force-pushed the mirror-vol-grp branch from 430b47d to 92eacfe Compare July 26, 2024 09:29

nixpanic approved these changes Jul 26, 2024

View reviewed changes

Madhu-1 requested a review from a team July 26, 2024 09:34

Madhu-1 added 3 commits July 26, 2024 09:44

rbd: add mirror interface

7d9a166

Adding mirror interface for rbd image and group which can be used to manage mirroring for both. Signed-off-by: Madhu Rajanna <[email protected]>

rbd: add mirroring helper for image

509d11f

Add helper methods for the rbd image mirroring. Signed-off-by: Madhu Rajanna <[email protected]>

rbd: refractor to use mirror interface

d19e47d

Refractoring code to use mirror interface. Signed-off-by: Madhu Rajanna <[email protected]>

nixpanic force-pushed the mirror-vol-grp branch from 92eacfe to d19e47d Compare July 26, 2024 09:44

nixpanic added the ok-to-test Label to trigger E2E tests label Jul 26, 2024

ceph-csi-bot removed the ok-to-test Label to trigger E2E tests label Jul 26, 2024

Madhu-1 requested review from Rakshith-R and iPraveenParihar July 26, 2024 09:48

Rakshith-R approved these changes Jul 26, 2024

View reviewed changes

iPraveenParihar approved these changes Jul 26, 2024

View reviewed changes

mergify bot merged commit b185bfd into ceph:devel Jul 26, 2024

iPraveenParihar reviewed Aug 2, 2024

View reviewed changes

	type journalledObject interface {
	// GetID returns the CSI-Addons VolumeGroupId of the VolumeGroup.
	GetID(ctx context.Context) (string, error)

	// GetName returns the name in the backend storage for the VolumeGroup.
	GetName(ctx context.Context) (string, error)

	// GetPool returns the name of the pool that holds the VolumeGroup.
	GetPool(ctx context.Context) (string, error)

	// GetClusterID returns the ID of the cluster of the VolumeGroup.
	GetClusterID(ctx context.Context) (string, error)
	}

	// MirrorImage is the interface for managing mirroring on an RBD image or group of images.
	// MirrorInfo is the interface for managing mirroring on an RBD image or group of images.

	rcs := casrbd.NewReplicationServer(rbd.CSIInstanceID, NewControllerServer(r.cd))
	rcs := casrbd.NewReplicationServer(conf.InstanceID, NewControllerServer(r.cd))

	func InitJournals(instance string) {
	// Use passed in instance ID, if provided for omap suffix naming
	if instance != "" {
	CSIInstanceID = instance
	}

	volJournal = journal.NewCSIVolumeJournal(CSIInstanceID)
	snapJournal = journal.NewCSISnapshotJournal(CSIInstanceID)

Uh oh!

rbd: refactor mirroring to work with volume and volumegroup #4720

rbd: refactor mirroring to work with volume and volumegroup #4720

Uh oh!

Conversation

Madhu-1 commented Jul 19, 2024

Uh oh!

Madhu-1 commented Jul 19, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Madhu-1 Jul 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Madhu-1 commented Jul 22, 2024

Uh oh!

nixpanic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nixpanic Jul 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nixpanic Jul 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nixpanic commented Jul 26, 2024

Uh oh!

mergify bot commented Jul 26, 2024

✅ Branch has been successfully rebased

Madhu-1 Jul 19, 2024 •

edited

Loading

nixpanic Jul 26, 2024 •

edited

Loading

nixpanic Jul 26, 2024 •

edited

Loading