cmd: add testing tool for volume replication by riya-singhal31 · Pull Request #419 · csi-addons/kubernetes-csi-addons

riya-singhal31 · 2023-07-26T12:05:34Z

Adds the testing tool for replication

[riyasinghal@fedora ceph-csi]$ kubectl exec -c csi-addons csi-rbdplugin-provisioner-66c68cd59b-4gvn8 -n rook-ceph -- csi-addons -operation EnableVolumeReplication -clusterid rook-ceph --secret default/csi-rbd-secret -endpoint unix:///csi/csi-addons.sock -volumeID 0001-0009-rook-ceph-0000000000000002-deda762a-2910-47fa-9dfe-4b492f719ca7
Enable Volume Replication operation successful

[riyasinghal@fedora ceph-csi]$ kubectl exec -c csi-addons csi-rbdplugin-provisioner-66c68cd59b-4gvn8 -n rook-ceph -- csi-addons -operation DisableVolumeReplication -clusterid rook-ceph --secret default/csi-rbd-secret -endpoint unix:///csi/csi-addons.sock -volumeID 0001-0009-rook-ceph-0000000000000002-deda762a-2910-47fa-9dfe-4b492f719ca7
Disable Volume Replication operation successful

nixpanic

Mostly nits, and some considerations that may make things a little nicer.

nixpanic

Thanks!

Pull request has been modified.

Madhu-1

nit

Madhu-1 · 2023-07-31T12:37:17Z

+	if rep.parameters["clusterID"] == "" {
+		return fmt.Errorf("clusterID not set")
+	}
+
+	secrets := strings.Split(c.secret, "/")
+	if len(secrets) != 2 {
+		return fmt.Errorf("secret should be specified in the format `namespace/name`")
+	}
+	rep.secretNamespace = secrets[0]
+	if rep.secretNamespace == "" {
+		return fmt.Errorf("secret namespace is not set")
+	}
+
+	rep.secretName = secrets[1]
+	if rep.secretName == "" {
+		return fmt.Errorf("secret name is not set")
+	}


new errors.New() when formatting is not required

Hi Madhu, just a slight confusion, did you meant to remove this below error

return fmt.Errorf("secret should be specified in the format namespace/name")

No, I meant use errors.New() instead of fmt.Errorf

Thanks Madhu, Updated.

Madhu-1 · 2023-07-31T12:37:36Z

+
+	"github.com/csi-addons/kubernetes-csi-addons/internal/proto"
+	"github.com/csi-addons/kubernetes-csi-addons/internal/sidecar/service"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"


add a new line for package seperation.

Madhu-1 · 2023-07-31T12:39:57Z

+	if pv.Spec.CSI == nil {
+		return fmt.Errorf("pv %q is not a CSI volume", c.persistentVolume)
+	}
+
+	volID := pv.Spec.CSI.VolumeHandle
+
+	rep.volumeID = volID
+	if rep.volumeID == "" {
+		return fmt.Errorf("volume ID is not set")
+	}


Suggested change

if pv.Spec.CSI == nil {

return fmt.Errorf("pv %q is not a CSI volume", c.persistentVolume)

}

volID := pv.Spec.CSI.VolumeHandle

rep.volumeID = volID

if rep.volumeID == "" {

return fmt.Errorf("volume ID is not set")

}

if pv.Spec.CSI == nil {

return fmt.Errorf("pv %q is not a CSI volume", c.persistentVolume)

}

if pv.Spec.CSI.VolumeHandle == ""{

return fmt.Errorf("volume ID is not set")

}

rep.volumeID = pv.Spec.CSI.VolumeHandle

Madhu-1 · 2023-07-31T12:41:35Z

+		return err
+	}
+
+	fmt.Printf("Resync Volume operation successful")


instead of Printf use Println which adds a new line, take care of it in all other places as well

Thanks for the review Madhu, Updated the PR.
PTAL

Pull request has been modified.

Rakshith-R · 2023-08-01T05:09:07Z

@Mergifyio rebase

Signed-off-by: riya-singhal31 <rsinghal@redhat.com>

mergify · 2023-08-01T05:09:16Z

rebase

✅ Branch has been successfully rebased

riya-singhal31 · 2023-08-01T09:34:51Z

Hey @Madhu-1 ,
Addressed all the review comments.
PTAL.

Madhu-1 · 2023-08-01T09:37:39Z

+	_, err := rs.GetVolumeReplicationInfo(context.TODO(), req)
+	if err != nil {
+		return err
+	}
+
+	fmt.Println("GetVolumeReplicationInfo operation successful")


Get should print the response that you got from the API as its meant to display details. i would suggest sending a PR to fix it. for now approving this

Sure, I'll send a PR for same.
Thank you!

mergify Bot requested review from Rakshith-R, matancarmeli7, nixpanic and yati1998 July 26, 2023 12:06

riya-singhal31 force-pushed the rep branch 9 times, most recently from 2b9f991 to a450ce5 Compare July 28, 2023 14:16

nixpanic reviewed Jul 28, 2023

View reviewed changes

Comment thread cmd/csi-addons/README.md Outdated

Comment thread cmd/csi-addons/main.go Outdated

Comment thread cmd/csi-addons/replication.go Outdated

riya-singhal31 force-pushed the rep branch from a450ce5 to 3612b94 Compare July 31, 2023 05:56

riya-singhal31 requested a review from nixpanic July 31, 2023 05:57

riya-singhal31 force-pushed the rep branch 5 times, most recently from c1e212b to dc74779 Compare July 31, 2023 11:10

nixpanic previously approved these changes Jul 31, 2023

View reviewed changes

riya-singhal31 force-pushed the rep branch from dc74779 to e29c55b Compare July 31, 2023 11:47

nixpanic previously approved these changes Jul 31, 2023

View reviewed changes

Madhu-1 reviewed Jul 31, 2023

View reviewed changes

riya-singhal31 force-pushed the rep branch from e29c55b to f3d7d39 Compare July 31, 2023 13:03

riya-singhal31 requested a review from Madhu-1 July 31, 2023 13:05

riya-singhal31 force-pushed the rep branch from f3d7d39 to 36a47c4 Compare July 31, 2023 13:08

riya-singhal31 force-pushed the rep branch 3 times, most recently from 50882d5 to cfd4fa4 Compare July 31, 2023 13:27

riya-singhal31 requested a review from nixpanic July 31, 2023 13:38

nixpanic approved these changes Jul 31, 2023

View reviewed changes

cmd: add testing tool for volume replication

ace0cf1

Signed-off-by: riya-singhal31 <rsinghal@redhat.com>

Rakshith-R force-pushed the rep branch from cfd4fa4 to ace0cf1 Compare August 1, 2023 05:09

Madhu-1 approved these changes Aug 1, 2023

View reviewed changes

mergify Bot merged commit 0fe08b8 into csi-addons:main Aug 1, 2023

riya-singhal31 mentioned this pull request Aug 1, 2023

Add Replication capability to csi-addons admin/test tool #207

Closed

Conversation

riya-singhal31 commented Jul 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nixpanic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nixpanic left a comment

Choose a reason for hiding this comment

Uh oh!

Madhu-1 left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Rakshith-R commented Aug 1, 2023

Uh oh!

mergify Bot commented Aug 1, 2023

✅ Branch has been successfully rebased

Uh oh!

riya-singhal31 commented Aug 1, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

riya-singhal31 commented Jul 26, 2023 •

edited

Loading