cleanup: migration of volrep to csi-addons #3608

riya-singhal31 · 2023-01-12T10:01:41Z

This commit moves the volrep logic(replication controller server) from internal/rbd to internal/csi-addons/rbd.

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

riya-singhal31 · 2023-01-12T10:03:48Z

Addresses task item 1 for - #3314

yati1998 · 2023-02-01T08:59:12Z

@riya-singhal31 please update the PR description, removing the default text.

Madhu-1 · 2023-02-01T09:03:41Z

internal/csi-addons/rbd/replicationcontrollerserver.go

@@ -26,6 +26,7 @@ import (
 	"strings"
 	"time"

+	rbdutil "github.com/ceph/ceph-csi/internal/rbd"


Instead of rbdutil call it as corerbd?

Madhu-1 · 2023-02-01T09:04:50Z

internal/csi-addons/rbd/replicationcontrollerserver.go

@@ -840,29 +794,6 @@ func getLastSyncTime(description string) (*timestamppb.Timestamp, error) {
 	return lastSyncTime, nil
 }

-func resyncVolume(localStatus librbd.SiteMirrorImageStatus, rbdVol *rbdVolume, force bool) error {


This is an internal function no need to move to internal/rbd

Madhu-1 · 2023-02-01T09:05:05Z

internal/csi-addons/rbd/replicationcontrollerserver.go

 	default:
 		return nil, status.Errorf(codes.InvalidArgument, "image is in %s Mode", mirroringInfo.State)
 	}

 	return &replication.DisableVolumeReplicationResponse{}, nil
 }

-func disableVolumeReplication(rbdVol *rbdVolume,


This is an internal function no need to move to internal/rbd

Madhu-1 · 2023-02-01T09:05:23Z

internal/csi-addons/rbd/replicationcontrollerserver.go

-
-// resyncRequired returns true if local image is in split-brain state and image
-// needs resync.
-func resyncRequired(localStatus librbd.SiteMirrorImageStatus) bool {


This is an internal function no need to move to internal/rbd

Madhu-1 · 2023-02-01T09:05:27Z

internal/csi-addons/rbd/replicationcontrollerserver.go

-}
-
-// repairResyncedImageID updates the existing image ID with new one.
-func repairResyncedImageID(ctx context.Context, rv *rbdVolume, ready bool) error {


This is an internal function no need to move to internal/rbd

Thanks Madhu, reason for doing this was I'll have to export this rbdVolume parameter then, so to prevent this variable from exporting moved it.
So should we go with exporting rbdVolume? @Madhu-1

So instead of exporting the variable, I exported the function.

Madhu-1 · 2023-02-01T09:06:28Z

internal/rbd/driver/driver.go

@@ -24,6 +24,7 @@ import (
 	casrbd "github.com/ceph/ceph-csi/internal/csi-addons/rbd"
 	csiaddons "github.com/ceph/ceph-csi/internal/csi-addons/server"
 	csicommon "github.com/ceph/ceph-csi/internal/csi-common"
+


No need for a new line here as both are internal packages

Madhu-1 · 2023-02-01T09:06:48Z

internal/rbd/replication.go

@@ -0,0 +1,133 @@
+/*
+Copyright 2021 The Ceph-CSI Authors.


nixpanic

a little more cleanup would be nice, this is already a great start!

nixpanic · 2023-02-01T09:06:33Z

internal/csi-addons/rbd/replicationcontrollerserver.go

@@ -325,61 +326,14 @@ func (rs *ReplicationServer) DisableVolumeReplication(ctx context.Context,
 	case librbd.MirrorImageDisabling:
 		return nil, status.Errorf(codes.Aborted, "%s is in disabling state", volumeID)
 	case librbd.MirrorImageEnabled:
-		return disableVolumeReplication(rbdVol, mirroringInfo, force)
+		return rbdutil.DisableVolumeReplication(rbdVol, mirroringInfo, force)


rbdutil functions should not return a GRPC response. Only the packages under internal/csi-addons/ should use the CSI-Addons GRPC structures/functions.

Maybe DisableVolumeReplication() can return an error instead, which can then be used to construct a DisableVolumeReplicationResponse reply here?

nixpanic · 2023-02-01T09:09:00Z

internal/csi-addons/rbd/replicationcontrollerserver.go

@@ -671,7 +625,7 @@ func (rs *ReplicationServer) ResyncVolume(ctx context.Context,
 		ready = checkRemoteSiteStatus(ctx, mirrorStatus)
 	}

-	err = resyncVolume(localStatus, rbdVol, req.Force)
+	err = rbdutil.ResyncVol(localStatus, rbdVol, req.Force)


maybe make this a function of rbdVol so that you can call is like

err = rbdVol.ResyncVolume(localStatus, req.Force)

nixpanic · 2023-02-01T09:10:14Z

internal/csi-addons/rbd/replicationcontrollerserver.go

@@ -683,7 +637,7 @@ func (rs *ReplicationServer) ResyncVolume(ctx context.Context,
 		return nil, status.Error(codes.Internal, err.Error())
 	}

-	err = repairResyncedImageID(ctx, rbdVol, ready)
+	err = rbdutil.RepairResyncedImageID(ctx, rbdVol, ready)


Same here, it would be cleaner to have RepairResyncedImageID a function of rbdVol so you can do

err = rbdVol.RepairResyncedImageID(ctx, ready)

nixpanic · 2023-02-01T09:17:43Z

internal/rbd/replication.go

+	"google.golang.org/grpc/status"
+)
+
+func ResyncVol(localStatus librbd.SiteMirrorImageStatus, rbdVol *rbdVolume, force bool) error {


this is a great opportunity to cleanup even more. These functions should all be part of the rbdVolume object, so make them look like

func (ri *rbdVolume) ResyncVol(localStatus librbd.SiteMirrorImageStatus, force bool) error {

These changes can be a 2nd commit in the same PR if you like.

This is still open, could you let me know what you want to do with it?

Thanks Niels for the review. Yes, addressing all these changes as a part of 2nd commit. So they are not present in this commit.

nixpanic · 2023-02-01T09:19:11Z

internal/rbd/replication.go

+	"strings"
+
+	librbd "github.com/ceph/go-ceph/rbd"
+	"github.com/csi-addons/spec/lib/go/replication"


csi-addons and grpc packages should not be imported. Where used, return an error on failure instead.

This is still something that needs to be addressed. The cleanup is important so that internal/rbd does not use CSI-Addons functions/structs anymore.

I can take this in a separate PR, @nixpanic, will that be fine?

nixpanic · 2023-02-02T08:51:05Z

internal/csi-addons/rbd/replicationcontrollerserver.go

@@ -26,6 +26,7 @@ import (
 	"strings"
 	"time"

+	corerbd "github.com/ceph/ceph-csi/internal/rbd"


just a nit, the filename could be just replication.go as all the files in the directory are CSI-Addons controllers.

nixpanic · 2023-02-02T08:52:52Z

internal/rbd/replication.go

+	"google.golang.org/grpc/status"
+)
+
+func ResyncVol(localStatus librbd.SiteMirrorImageStatus, rbdVol *rbdVolume, force bool) error {


This is still open, could you let me know what you want to do with it?

nixpanic · 2023-02-02T08:54:33Z

internal/rbd/replication.go

+	"strings"
+
+	librbd "github.com/ceph/go-ceph/rbd"
+	"github.com/csi-addons/spec/lib/go/replication"


This is still something that needs to be addressed. The cleanup is important so that internal/rbd does not use CSI-Addons functions/structs anymore.

Madhu-1 · 2023-02-03T07:33:30Z

@riya-singhal31 I Hope you have done e2e testing for volumeReplication as we dont have CI testing for this one.

github-actions · 2023-03-05T21:01:28Z

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.

riya-singhal31 · 2023-03-17T20:23:54Z

Up to avoid stale.

riya-singhal31 · 2023-04-12T13:13:20Z

@riya-singhal31 I Hope you have done e2e testing for volumeReplication as we dont have CI testing for this one.

Yes, that is done.

Madhu-1

Please fix the CI problem. The build itself is not passing. Please make sure it's tested once you make the required changes to make CI green

Madhu-1

LGTM, https://github.com/ceph/ceph-csi/pull/3608/files#r1092953730, still waiting for the response

riya-singhal31 · 2023-04-20T07:48:01Z

LGTM, https://github.com/ceph/ceph-csi/pull/3608/files#r1092953730, still waiting for the response

cc @nixpanic

nixpanic · 2023-04-20T18:39:47Z

@Mergifyio rebase

mergify · 2023-04-20T18:40:01Z

rebase

✅ Branch has been successfully rebased

yati1998

LGTM

nixpanic · 2023-04-21T10:56:49Z

@Mergifyio rebase

This commit moves the volrep logic from internal/rbd to internal/csi-addons/rbd. Signed-off-by: riya-singhal31 <rsinghal@redhat.com>

mergify · 2023-04-21T10:56:57Z

rebase

✅ Branch has been successfully rebased

github-actions · 2023-04-21T10:57:17Z

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

github-actions · 2023-04-21T10:57:18Z

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

github-actions · 2023-04-21T10:57:19Z

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

github-actions · 2023-04-21T10:57:20Z

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

github-actions · 2023-04-21T10:57:21Z

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

github-actions · 2023-04-21T10:57:22Z

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

github-actions · 2023-04-21T10:57:23Z

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

github-actions · 2023-04-21T10:57:24Z

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

github-actions · 2023-04-21T10:57:25Z

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

github-actions · 2023-04-21T10:57:25Z

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

github-actions · 2023-04-21T10:57:26Z

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

github-actions · 2023-04-21T10:57:27Z

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

github-actions · 2023-04-21T10:57:29Z

/test ci/centos/upgrade-tests-cephfs

github-actions · 2023-04-21T10:57:29Z

/test ci/centos/upgrade-tests-rbd

mergify bot added the cleanup label Jan 12, 2023

riya-singhal31 marked this pull request as draft January 12, 2023 10:13

riya-singhal31 changed the title ~~cleanup: migration of volrep to csi-addons~~ [WIP]cleanup: migration of volrep to csi-addons Jan 12, 2023

nixpanic self-requested a review January 18, 2023 09:07

riya-singhal31 changed the title ~~[WIP]cleanup: migration of volrep to csi-addons~~ cleanup: migration of volrep to csi-addons Feb 1, 2023

riya-singhal31 marked this pull request as ready for review February 1, 2023 08:50

nixpanic requested a review from a team February 1, 2023 09:01

Madhu-1 reviewed Feb 1, 2023

View reviewed changes

nixpanic requested changes Feb 1, 2023

View reviewed changes

nixpanic requested changes Feb 2, 2023

View reviewed changes

github-actions bot added the stale label Mar 5, 2023

github-actions bot removed the stale label Mar 17, 2023

riya-singhal31 requested review from nixpanic and Madhu-1 April 12, 2023 13:14

Madhu-1 requested changes Apr 14, 2023

View reviewed changes

Madhu-1 approved these changes Apr 18, 2023

View reviewed changes

riya-singhal31 mentioned this pull request Apr 20, 2023

rbd: complete migration of volrep to csi-addons #3314

Closed

5 tasks

yati1998 approved these changes Apr 21, 2023

View reviewed changes

nixpanic approved these changes Apr 21, 2023

View reviewed changes

cleanup: migration of volrep to csi-addons

50adbe3

This commit moves the volrep logic from internal/rbd to internal/csi-addons/rbd. Signed-off-by: riya-singhal31 <rsinghal@redhat.com>

nixpanic added the ok-to-test Label to trigger E2E tests label Apr 21, 2023

github-actions bot removed the ok-to-test Label to trigger E2E tests label Apr 21, 2023

nixpanic mentioned this pull request Apr 21, 2023

ci: update mergify rules for kubernetes 1.27 #3744

Merged

mergify bot merged commit 304194a into ceph:devel Apr 21, 2023

cleanup: migration of volrep to csi-addons #3608

cleanup: migration of volrep to csi-addons #3608

Conversation

riya-singhal31 commented Jan 12, 2023 • edited Loading

riya-singhal31 commented Jan 12, 2023

yati1998 commented Feb 1, 2023

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

riya-singhal31 Feb 1, 2023 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Madhu-1 commented Feb 3, 2023

github-actions bot commented Mar 5, 2023

riya-singhal31 commented Mar 17, 2023

riya-singhal31 commented Apr 12, 2023

Madhu-1 left a comment

Choose a reason for hiding this comment

Madhu-1 left a comment

Choose a reason for hiding this comment

riya-singhal31 commented Apr 20, 2023

nixpanic commented Apr 20, 2023

mergify bot commented Apr 20, 2023

✅ Branch has been successfully rebased

yati1998 left a comment

Choose a reason for hiding this comment

nixpanic commented Apr 21, 2023

mergify bot commented Apr 21, 2023

✅ Branch has been successfully rebased

github-actions bot commented Apr 21, 2023

github-actions bot commented Apr 21, 2023

github-actions bot commented Apr 21, 2023

github-actions bot commented Apr 21, 2023

github-actions bot commented Apr 21, 2023

github-actions bot commented Apr 21, 2023

github-actions bot commented Apr 21, 2023

github-actions bot commented Apr 21, 2023

github-actions bot commented Apr 21, 2023

github-actions bot commented Apr 21, 2023

github-actions bot commented Apr 21, 2023

github-actions bot commented Apr 21, 2023

github-actions bot commented Apr 21, 2023

github-actions bot commented Apr 21, 2023

riya-singhal31 commented Jan 12, 2023 •

edited

Loading

riya-singhal31 Feb 1, 2023 •

edited

Loading