Support VolumeGroupReplication with VR #605

Madhu-1 · 2024-06-26T10:38:09Z

This PR adds the support for replicating the group with volumegrouplication

controllers/replication.storage/pvc.go

iPraveenParihar

typos

controllers/replication.storage/volumereplication_controller.go

nixpanic

few minor things, and a larger interface improvement request to simplify the code and make maintenance easier.

internal/sidecar/service/volumereplication.go

internal/client/replication-client.go

internal/client/replication-client_test.go

internal/proto/replication.proto

controllers/replication.storage/volumereplication_controller.go

Rakshith-R

Thanks

Madhu-1 · 2024-07-04T11:35:16Z

@iPraveenParihar You have requested changes on this one, can you please review it again.
@nixpanic can you please also review it again, made all the changes

Nikhil-Ladha

lgtm

nixpanic

at least one comment needs careful checking, the others are mostly nits

nixpanic · 2024-07-04T12:00:38Z

cmd/csi-addons/main.go

@@ -58,6 +59,7 @@ func init() {
 	flag.StringVar(&cmd.stagingPath, "stagingpath", stagingPath, "staging path")
 	flag.StringVar(&cmd.operation, "operation", "", "csi-addons operation")
 	flag.StringVar(&cmd.persistentVolume, "persistentvolume", "", "name of the PersistentVolume")
+	flag.StringVar(&cmd.volumeGroupReplicationContent, "volumegroupreplicaitoncontent", "", "name of the VolumeGroupReplicationContent")


typo: volumegroupreplicaitoncontent

nixpanic · 2024-07-04T12:04:03Z

cmd/csi-addons/replication.go

+		}
+		return nil
+	case v.groupID != "":
+		req = &proto.ReplicationSource{


this does not look correct, you don't modify the passed req here.

@nixpanic This is the replication Spec which need to be set nothing else is modified here.

But is it intended to overwrite the req variable this way? The new req is not returned, so the caller of the function will never see it. I expect a warning from golangci about this too.

The original (req.Type) is not updated as is done on line 105.

Sorry, its an overlook from my side, you are correct we should not overwrite the req rather set the .Type as its done in other place.

controllers/replication.storage/replication/replication.go

nixpanic · 2024-07-04T12:09:06Z

internal/client/group-replication-client.go

@@ -0,0 +1,181 @@
+/*
+Copyright 2024 The Kubernetes-CSI-Addons Authors.


the filename is a little weird, it is in the client package, and also ends in -client.go.

nixpanic · 2024-07-04T12:10:55Z

internal/client/volume-replication-client.go

@@ -0,0 +1,181 @@
+/*
+Copyright 2022 The Kubernetes-CSI-Addons Authors.


correct the year?
Also has the unneeded -client in the filename.

removed -client from the filename, this is just a rename of file, left the year to 2022 as it was before, Do we need to change that as well?

internal/sidecar/service/volumereplication.go

Pull request has been modified.

nixpanic

had a quick look, seems reasonable to me. No objections, but I haven't checked the reconciling code in detail

internal/client/group-replication.go

.github/workflows/lint-extras.yaml

iPraveenParihar · 2024-07-09T08:26:45Z

cmd/csi-addons/replication.go

-
-	if pv.Spec.CSI.VolumeHandle == "" {
-		return errors.New("volume ID is not set")
+	if c.persistentVolume != "" {


In case where both PV and VGRC are provided, PV will be considered.
error out before this if both are provided.

Rakshith-R

LGTM

iPraveenParihar

LGTM, will leave it to @nixpanic for final review and approval.
Thanks!

Madhu-1 · 2024-07-10T07:45:04Z

@nixpanic can you please review it again?

update the proto to support the replicaiton of volumegroup. Signed-off-by: Madhu Rajanna <[email protected]>

update the RPC calls to set the replication source for group replication and add support for volumegroup replication Signed-off-by: Madhu Rajanna <[email protected]>

mergify bot requested review from iPraveenParihar, karthik-us, nixpanic, Rakshith-R, riya-singhal31 and yati1998 June 26, 2024 10:38

Madhu-1 force-pushed the support-vgr-in-vr branch from 372a8ff to 52cccd8 Compare June 26, 2024 10:47

Madhu-1 changed the title ~~Support VolueGroupReplication with VR~~ Support VolumeGroupReplication with VR Jun 26, 2024

Madhu-1 force-pushed the support-vgr-in-vr branch from 52cccd8 to 8c226a6 Compare June 26, 2024 10:56

iPraveenParihar reviewed Jun 26, 2024

View reviewed changes

controllers/replication.storage/pvc.go Outdated Show resolved Hide resolved

Madhu-1 force-pushed the support-vgr-in-vr branch from 8c226a6 to 6b09969 Compare June 26, 2024 10:58

Madhu-1 requested a review from iPraveenParihar June 26, 2024 10:59

Madhu-1 force-pushed the support-vgr-in-vr branch from 6b09969 to 445b0cd Compare June 26, 2024 11:00

iPraveenParihar requested changes Jun 26, 2024

View reviewed changes

controllers/replication.storage/volumereplication_controller.go Show resolved Hide resolved

controllers/replication.storage/volumereplication_controller.go Outdated Show resolved Hide resolved

Madhu-1 force-pushed the support-vgr-in-vr branch from 445b0cd to b9d3396 Compare June 26, 2024 11:20

Madhu-1 requested a review from iPraveenParihar June 26, 2024 11:21

Madhu-1 added the DNM Do Not Merge label Jun 27, 2024

Madhu-1 force-pushed the support-vgr-in-vr branch from b9d3396 to b943922 Compare June 27, 2024 09:09

Madhu-1 removed the DNM Do Not Merge label Jun 27, 2024

Madhu-1 force-pushed the support-vgr-in-vr branch from b943922 to c5b2c8c Compare June 27, 2024 10:06

nixpanic reviewed Jun 27, 2024

View reviewed changes

Madhu-1 force-pushed the support-vgr-in-vr branch 4 times, most recently from fca0cff to 310786f Compare July 2, 2024 12:59

Madhu-1 requested a review from nixpanic July 2, 2024 13:11

Rakshith-R reviewed Jul 3, 2024

View reviewed changes

internal/proto/replication.proto Outdated Show resolved Hide resolved

Nikhil-Ladha reviewed Jul 3, 2024

View reviewed changes

controllers/replication.storage/volumereplication_controller.go Show resolved Hide resolved

Madhu-1 force-pushed the support-vgr-in-vr branch from 310786f to efae36c Compare July 3, 2024 10:39

Madhu-1 requested a review from Rakshith-R July 3, 2024 10:41

Madhu-1 requested a review from Nikhil-Ladha July 4, 2024 11:06

Rakshith-R previously approved these changes Jul 4, 2024

View reviewed changes

Nikhil-Ladha reviewed Jul 4, 2024

View reviewed changes

nixpanic requested changes Jul 4, 2024

View reviewed changes

Madhu-1 force-pushed the support-vgr-in-vr branch from 664dfef to a0f564b Compare July 4, 2024 12:46

Madhu-1 requested a review from nixpanic July 4, 2024 12:47

Madhu-1 force-pushed the support-vgr-in-vr branch from a0f564b to 44d4768 Compare July 8, 2024 09:10

nixpanic reviewed Jul 8, 2024

View reviewed changes

internal/client/group-replication.go Show resolved Hide resolved

Madhu-1 force-pushed the support-vgr-in-vr branch from 44d4768 to 4edf583 Compare July 8, 2024 10:45

Madhu-1 requested a review from nixpanic July 8, 2024 10:45

Madhu-1 commented Jul 8, 2024

View reviewed changes

.github/workflows/lint-extras.yaml Outdated Show resolved Hide resolved

Nikhil-Ladha mentioned this pull request Jul 8, 2024

Add volume group replication controller code #610

Open

Madhu-1 force-pushed the support-vgr-in-vr branch from 046493a to 96d75fd Compare July 8, 2024 12:32

iPraveenParihar reviewed Jul 9, 2024

View reviewed changes

Madhu-1 force-pushed the support-vgr-in-vr branch from 96d75fd to bc7ee49 Compare July 9, 2024 08:29

Madhu-1 requested a review from iPraveenParihar July 9, 2024 08:29

Rakshith-R approved these changes Jul 9, 2024

View reviewed changes

iPraveenParihar reviewed Jul 9, 2024

View reviewed changes

nixpanic approved these changes Jul 10, 2024

View reviewed changes

iPraveenParihar approved these changes Jul 10, 2024

View reviewed changes

Madhu-1 added 2 commits July 10, 2024 08:54

replication: proto to support group replication

42c4fdf

update the proto to support the replicaiton of volumegroup. Signed-off-by: Madhu Rajanna <[email protected]>

replication: support for volumegroup replication

560a2e5

update the RPC calls to set the replication source for group replication and add support for volumegroup replication Signed-off-by: Madhu Rajanna <[email protected]>

Madhu-1 force-pushed the support-vgr-in-vr branch from bc7ee49 to 560a2e5 Compare July 10, 2024 08:54

mergify bot merged commit 0f8a18e into csi-addons:main Jul 10, 2024
15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support VolumeGroupReplication with VR #605

Support VolumeGroupReplication with VR #605

Madhu-1 commented Jun 26, 2024

iPraveenParihar left a comment

nixpanic left a comment

Rakshith-R left a comment

Madhu-1 commented Jul 4, 2024

Nikhil-Ladha left a comment

nixpanic left a comment

nixpanic Jul 4, 2024

nixpanic Jul 4, 2024

Madhu-1 Jul 4, 2024

nixpanic Jul 5, 2024

Madhu-1 Jul 8, 2024

nixpanic Jul 4, 2024

nixpanic Jul 4, 2024

Madhu-1 Jul 4, 2024

nixpanic left a comment

iPraveenParihar Jul 9, 2024

Rakshith-R left a comment

iPraveenParihar left a comment

Madhu-1 commented Jul 10, 2024

		@@ -0,0 +1,181 @@
		/*
		Copyright 2024 The Kubernetes-CSI-Addons Authors.

		@@ -0,0 +1,181 @@
		/*
		Copyright 2022 The Kubernetes-CSI-Addons Authors.

Support VolumeGroupReplication with VR #605

Support VolumeGroupReplication with VR #605

Conversation

Madhu-1 commented Jun 26, 2024

iPraveenParihar left a comment

Choose a reason for hiding this comment

nixpanic left a comment

Choose a reason for hiding this comment

Rakshith-R left a comment

Choose a reason for hiding this comment

Madhu-1 commented Jul 4, 2024

Nikhil-Ladha left a comment

Choose a reason for hiding this comment

nixpanic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nixpanic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rakshith-R left a comment

Choose a reason for hiding this comment

iPraveenParihar left a comment

Choose a reason for hiding this comment

Madhu-1 commented Jul 10, 2024