Add scaffoling for volumegroupreplication/replicationclass/replicationcontent #588

Madhu-1 · 2024-06-13T09:10:47Z

Add scaffolding for volumegroupreplication and volumegroupreplicationclass to provide support for replication of volumegroup (internally)

Note: This code is autogenerated we will have a follow-up PR for implementation.

iPraveenParihar · 2024-06-13T09:46:07Z

@Madhu-1 , I see two commits with same commit title and body.
Is that a mistake?

Madhu-1 · 2024-06-13T14:52:17Z

Removed DNM and this is ready for review. PTAL :)

apis/replication.storage/v1alpha1/volumegroupreplicationclass_types.go

Madhu-1 · 2024-06-14T09:39:08Z

@nixpanic for now i updated the Struct members with details. PTAL

nixpanic · 2024-06-14T10:50:14Z

config/samples/replication.storage_v1alpha1_volumegroupreplication.yaml

+    app.kubernetes.io/created-by: kubernetes-csi-addons
+  name: volumegroupreplication-sample
+spec:
+  # TODO(user): Add fields here


add a minimal example here?

@nixpanic added minimal example, most of the examples in this repo doesn't contain anything, we might need to revisit all the yamls in the sample folder

Oh, yes, it would be good to have examples everywhere!

config/samples/replication.storage_v1alpha1_volumegroupreplicationclass.yaml

config/samples/replication.storage_v1alpha1_volumegroupreplicationcontent.yaml

nixpanic · 2024-06-14T11:04:25Z

apis/replication.storage/v1alpha1/volumegroupreplicationcontent_types.go

+	// PVVolumeList is the list of of PV for the group replication
+	// The maximum number of allowed PV in the group is 100.
+	// +optional
+	PVVolumeList []PVVolume `json:"pvVolumeList,omitempty"`


PVVolume is a weird name, this should probably just be PersistentVolumeRefList.

PVVolume looks similar to pvVolumeSnapshotContentList, but that combines PV+VolumeSnapshotContent in each item in the list. PVVolume only contains a single Ref object to a PV.

@Madhu-1 this is still TODO?

nixpanic · 2024-06-14T12:19:10Z

apis/replication.storage/v1alpha1/volumegroupreplicationcontent_types.go

-type PVVolume struct {
-	// PersistentVolumeRef is a reference to the persistent volume resource
-	PersistentVolumeRef corev1.LocalObjectReference `json:"persistentVolumeRef,omitempty"`
+	PersistentVolumeRefList []corev1.LocalObjectReference `json:"persistentVolumeRefList,omitempty"`


ah, no, it is part of a different commit

Please move this change to the other commit, that is the only thing pending that I can spot.

sorry i missed it, pushed it to right commit

nixpanic

Thanks!

Madhu-1 · 2024-06-18T05:20:57Z

@iPraveenParihar @yati1998 PTAL

config/samples/replication.storage_v1alpha1_volumegroupreplication.yaml

adding scaffolding for volumegroupreplicationclass using below commands ``` cp cmd/manager/main.go . kubebuilder create api --group replication.storage \ --version v1alpha1 --kind VolumeGroupReplicationClass --namespaced=false mv main.go cmd/manager/ make generate make manifests ``` Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>

Pull request has been modified.

yati1998 · 2024-06-18T07:57:34Z

config/samples/replication.storage_v1alpha1_volumegroupreplicationcontent.yaml

+    namespace: default
+  volumeGroupReplicationHandle: replicationHandle
+  provisioner: driver.csi.com
+  volumeGroupReplicationClassName: volumegroupreplicationClass


volumeGroupReplicationClassName -> volumeGroupReplicationClass , it should be same as in the above yaml?

it should be the className as defined in the api

adding scaffolding for volumegroupreplication using below commands ``` cp cmd/manager/main.go . kubebuilder create api --group replication.storage \ --version v1alpha1 --kind VolumeGroupReplication mv main.go cmd/manager/ make generate make manifests ``` Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>

adding scaffolding for volumegroupreplicationcontent using below commands ``` cp cmd/manager/main.go . kubebuilder create api --group replication.storage \ --version v1alpha1 --kind VolumeGroupReplicationContent \ --namespaced=false mv main.go cmd/manager/ make generate make manifests ``` Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>

add NotIn to ignore list in codespell as NotIn is auto generated code. Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>

updated sample yamls for reference Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>

yati1998

LGTM

Nikhil-Ladha · 2024-06-18T11:34:37Z

apis/replication.storage/v1alpha1/volumegroupreplicationcontent_types.go

+	// VolumeGroupReplicationClassName is the name of the VolumeGroupReplicationClass from
+	// which this group replication was (or will be) created.
+	// +optional
+	VolumeGroupSnapshotClassName string `json:"volumeGroupReplicationClassName"`


Suggested change

VolumeGroupSnapshotClassName string `json:"volumeGroupReplicationClassName"`

VolumeGroupReplicationClassName string `json:"volumeGroupReplicationClassName"`

Thanks for catching it. #594 here is the fix, feel free to send PR's if the PR's are already merged/closed.

iPraveenParihar · 2024-06-20T07:44:06Z

cmd/manager/main.go

@@ -197,6 +197,20 @@ func main() {
 		os.Exit(1)
 	}

+	if err = (&replicationController.VolumeGroupReplicationReconciler{
+		Client: mgr.GetClient(),
+		Scheme: mgr.GetScheme(),


@Madhu-1, I think we forgot to add the replicationv1alpha1.VolumeGroupReplication CRD scheme?

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

mergify bot requested review from iPraveenParihar, karthik-us, nixpanic, Rakshith-R, riya-singhal31 and yati1998 June 13, 2024 09:11

Madhu-1 force-pushed the add-vrg-api branch from 0db2a2f to 484674c Compare June 13, 2024 09:11

Madhu-1 requested a review from ShyamsundarR June 13, 2024 09:45

Madhu-1 force-pushed the add-vrg-api branch from 484674c to 51030a9 Compare June 13, 2024 09:52

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

nixpanic reviewed Jun 13, 2024

View reviewed changes

apis/replication.storage/v1alpha1/volumegroupreplicationclass_types.go Outdated Show resolved Hide resolved

apis/replication.storage/v1alpha1/volumegroupreplicationclass_types.go Outdated Show resolved Hide resolved

Madhu-1 force-pushed the add-vrg-api branch from 51030a9 to 450a98a Compare June 14, 2024 09:23

Madhu-1 changed the title ~~Add scaffoling for volumegroupreplication and volumegroupreplicationclass~~ Add scaffoling for volumegroupreplication/replicationclass/replicationcontent Jun 14, 2024

Madhu-1 requested a review from nixpanic June 14, 2024 09:38

nixpanic reviewed Jun 14, 2024

View reviewed changes

Madhu-1 force-pushed the add-vrg-api branch from d09a459 to 3f1325e Compare June 14, 2024 11:21

Madhu-1 requested a review from nixpanic June 14, 2024 11:22

nixpanic reviewed Jun 14, 2024

View reviewed changes

Madhu-1 force-pushed the add-vrg-api branch from 3f1325e to ab35fdc Compare June 14, 2024 12:24

Madhu-1 requested a review from nixpanic June 14, 2024 12:24

nixpanic previously approved these changes Jun 14, 2024

View reviewed changes

yati1998 reviewed Jun 18, 2024

View reviewed changes

config/samples/replication.storage_v1alpha1_volumegroupreplication.yaml Outdated Show resolved Hide resolved

Madhu-1 force-pushed the add-vrg-api branch from ab35fdc to 9bda59b Compare June 18, 2024 07:48

Madhu-1 requested a review from nixpanic June 18, 2024 07:49

Madhu-1 requested a review from yati1998 June 18, 2024 07:49

yati1998 reviewed Jun 18, 2024

View reviewed changes

Madhu-1 force-pushed the add-vrg-api branch from 9bda59b to 58bf77e Compare June 18, 2024 08:19

Madhu-1 added 4 commits June 18, 2024 10:22

ci: add NotIn to ignore list in codespell

d962792

add NotIn to ignore list in codespell as NotIn is auto generated code. Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>

replication: update sample yamls

0c9e42b

updated sample yamls for reference Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>

Madhu-1 force-pushed the add-vrg-api branch from 58bf77e to 0c9e42b Compare June 18, 2024 08:22

Madhu-1 requested a review from yati1998 June 18, 2024 08:22

yati1998 approved these changes Jun 18, 2024

View reviewed changes

nixpanic approved these changes Jun 18, 2024

View reviewed changes

mergify bot merged commit c5d138e into csi-addons:main Jun 18, 2024
15 checks passed

nixpanic mentioned this pull request Jun 18, 2024

adding vg api #402

Closed

Nikhil-Ladha reviewed Jun 18, 2024

View reviewed changes

iPraveenParihar reviewed Jun 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add scaffoling for volumegroupreplication/replicationclass/replicationcontent #588

Add scaffoling for volumegroupreplication/replicationclass/replicationcontent #588

Madhu-1 commented Jun 13, 2024

iPraveenParihar commented Jun 13, 2024

Madhu-1 commented Jun 13, 2024

Madhu-1 commented Jun 14, 2024

nixpanic Jun 14, 2024

Madhu-1 Jun 14, 2024

nixpanic Jun 14, 2024

nixpanic Jun 14, 2024

nixpanic Jun 14, 2024

nixpanic Jun 14, 2024

nixpanic Jun 14, 2024

Madhu-1 Jun 14, 2024

nixpanic left a comment

Madhu-1 commented Jun 18, 2024

yati1998 Jun 18, 2024

Madhu-1 Jun 18, 2024

yati1998 left a comment

Nikhil-Ladha Jun 18, 2024

Madhu-1 Jun 18, 2024

iPraveenParihar Jun 20, 2024

	VolumeGroupSnapshotClassName string `json:"volumeGroupReplicationClassName"`
	VolumeGroupReplicationClassName string `json:"volumeGroupReplicationClassName"`

Add scaffoling for volumegroupreplication/replicationclass/replicationcontent #588

Add scaffoling for volumegroupreplication/replicationclass/replicationcontent #588

Conversation

Madhu-1 commented Jun 13, 2024

iPraveenParihar commented Jun 13, 2024

Madhu-1 commented Jun 13, 2024

Madhu-1 commented Jun 14, 2024

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

Madhu-1 commented Jun 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yati1998 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