add rpc call for PeerBlockPool #2457

rewantsoni · 2024-02-12T05:08:46Z

Add a new RPC call PeerBlockPool to enable one-way mirroring between the block pools.

The rpc call is initiated from the primary cluster which wants to mirror the pool, the server receives the request and does the following:

Enable RBD Mirror Daemon
Enable Mirroring on the Block Pool in the request
Create the bootstrap secret and set the bootstrap secret ref on the Block Pool in the request.

Depends on:

cephblockpool: allow updation of mirroring field from different components #2480

openshift-ci · 2024-02-12T05:08:51Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

openshift-ci · 2024-02-12T05:08:54Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: rewantsoni
Once this PR has been reviewed and has the lgtm label, please assign jarrpa for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

leelavg

The rpc call is initiated from the primary cluster which wants to mirror the pool, the server receives the request and does the following

a bit incomplete (at-least for me), by above do you mean the server is running in "another" cluster (maybe secondary?) and primary cluster wants to send over the info to secondary cluster?
through out app lifecycle, does primary and secondary change identities and become the opposite? if yes, should we only use server and client terminology for all operations?

Let me know if you don't want review on draft PRs, I need to review this PR again as I'm not totally upto speed w/ RDR.

services/provider/client/client.go

leelavg · 2024-03-04T07:07:16Z

services/provider/server/cephrbdmirror.go

+	}, nil
+}
+
+func (c *cephRBDMirrorManager) Create(ctx context.Context) error {


any reason in not using creatorupdate from controller util?

I was thinking about the case when we run MaintenanceMode (this requires the RBD mirror to be scaled down). If a new PeerBlockPool call is received while MaintenacneMode is in progress it and we use CreateOrUpdate, it will interfere with it.

fair enough, does the owner scale up after the maintenance mode?

Yes, that would be the responsibility of the controller executing the maintenance mode

services/provider/server/cephblockpool.go

leelavg · 2024-03-04T07:11:58Z

services/provider/server/server.go

@@ -690,3 +705,34 @@ func (s *OCSProviderServer) ReportStatus(ctx context.Context, req *pb.ReportStat

 	return &pb.ReportStatusResponse{}, nil
 }
+
+// PeerBlockPool RPC call to send the bootstrap secret for the pool
+func (s *OCSProviderServer) PeerBlockPool(ctx context.Context, req *pb.PeerBlockPoolRequest) (*pb.PeerBlockPoolResponse, error) {


either make this RPC idempotent or ensure different failure for resource already exists requests.

services/provider/server/cephblockpool.go

services/provider/server/server.go

leelavg · 2024-03-04T07:29:28Z

services/provider/server/server.go

+	// enable mirroring on blockPool in the req
+	if err := s.cephBlockPoolManager.EnableBlockPoolMirroring(ctx, req.BlockPoolName); err != nil {
+		if kerrors.IsNotFound(err) {
+			return nil, status.Errorf(codes.NotFound, "Failed to find CephBlockPool resource %s: %v", req.BlockPoolName, err)


I believe the errors are a bit misleading (same for remaining), basically the function asks for enabling mirroring and if you didn't find the source why are we at this line 🤔?

by source I mean, block pool, at the start why can't we return if we didn't find the blockpool? basically we are checking blockpool existence indirectly three times throughtout this function.

updated to return at the beginning of the function if the blockpool doesn't exist

rewantsoni · 2024-03-04T08:41:05Z

a bit incomplete (at-least for me), by above do you mean the server is running in "another" cluster (maybe secondary?) and primary cluster wants to send over the info to secondary cluster?

Yes right, we will have two clusters, one running the server(secondary) and other cluster as a client who sends the request (StorageClusterPeer; primary). The primary cluster sends over the contents of the bootstrap secret (that is pool and token fields) along with the blockPool Name it wants to mirror.

through out app lifecycle, do primary and secondary change identities and become the opposite? if yes, should we only use server and client terminology for all operations?

We are implementing peering in one-way replication, for RDR both primary and secondary clusters will act as both a server and a client, to be more specific,
StorageClusterPeer on the Primary Cluster will be a client for the API Server on the Secondary Cluster and StorageClusterPeer on the Secondary Cluster will be a client for the API Server on Primary Cluster.

Let me know if you don't want review on draft PRs, I need to review this PR again as I'm not totally upto speed w/ RDR.

Yes, I would like reviews on the draft PRs I have tested the PR. I need to complete unit tests and uninstall flow (probably as a part of a different PR).

services/provider/server/cephblockpool.go

iamniting · 2024-03-05T11:25:38Z

/test ocs-operator-bundle-e2e-aws

rchikatw · 2024-03-06T08:02:48Z

services/provider/server/cephblockpool.go

+	namespace string
+}
+
+func newCephBlockPoolManager(cl client.Client, namespace string) (*cephBlockPoolManager, error) {


I believe that this task can be completed without any errors in the return type.

I was following the same function defination that was there for the other two managers.

services/provider/server/cephblockpool.go

leelavg · 2024-03-06T11:05:26Z

services/provider/server/cephblockpool.go

+	blockPoolObj := &rookCephv1.CephBlockPool{}
+	err := c.client.Get(ctx, types.NamespacedName{Name: blockPoolName, Namespace: c.namespace}, blockPoolObj)


maybe we can use utils to construct the type?

Suggested change

blockPoolObj := &rookCephv1.CephBlockPool{}

err := c.client.Get(ctx, types.NamespacedName{Name: blockPoolName, Namespace: c.namespace}, blockPoolObj)

blockPool := &rookCephv1.CephBlockPool{}

blockPool.Name = blockPoolName

blockPool.Namespace = c.namespace

err := c.client.Get(ctx, client.ObjectKeyFromObject(blockPool), blockPool)

similar at remaining places, it just saves us from constructing a struct.

leelavg · 2024-03-06T11:13:44Z

services/provider/server/cephblockpool.go

+		err := ctrl.SetControllerReference(cephBlockPool, bootstrapSecret, c.client.Scheme())
+		if err != nil {
+			return err
+		}
+		return nil


just return ctrl.SetControllerReference....

leelavg · 2024-03-06T11:20:00Z

services/provider/server/server.go

@@ -690,3 +705,34 @@ func (s *OCSProviderServer) ReportStatus(ctx context.Context, req *pb.ReportStat

 	return &pb.ReportStatusResponse{}, nil
 }
+
+// PeerBlockPool RPC call to send the bootstrap secret for the pool
+func (s *OCSProviderServer) PeerBlockPool(ctx context.Context, req *pb.PeerBlockPoolRequest) (*pb.PeerBlockPoolResponse, error) {


is there any chance that two clients call this rpc referring same block pool? if yes, the idea is to just fail one client & make it retry?

Yes two clients can call this rpc referring to the same block pool, in that case it will fail one and make it retry

leelavg · 2024-03-06T11:26:04Z

services/provider/server/cephrbdmirror.go

+	}, nil
+}
+
+func (c *cephRBDMirrorManager) Create(ctx context.Context) error {


fair enough, does the owner scale up after the maintenance mode?

leelavg · 2024-03-06T11:29:35Z

services/provider/server/cephrbdmirror.go

+	err := c.client.Get(ctx, types.NamespacedName{Name: rBDMirrorName, Namespace: c.namespace}, cephRBDMirrorObj)
+	if err == nil {
+		return nil
+	}
+
+	if err != nil && !kerrors.IsNotFound(err) {
+		return err
+	}
+
+	cephRBDMirrorObj.Name = rBDMirrorName
+	cephRBDMirrorObj.Namespace = c.namespace
+	cephRBDMirrorObj.Spec = rookCephv1.RBDMirroringSpec{Count: 1}
+
+	err = c.client.Create(ctx, cephRBDMirrorObj)
+	if err != nil {
+		return err
+	}


maybe always try to create and fail if error is other than exists? or do if !kerrors.IsNotFound(err) { create }

leelavg · 2024-03-06T11:45:48Z

I would like reviews on the draft PRs

ack, I usually open all for reviews but if untested I put a /hold flag, just saying

can resolved my previous addressed comments, thanks.

leelavg · 2024-04-08T03:46:26Z

@rewantsoni this PR was marked as a dependent on which a comment was made until which invalidates the changes in this PR, let me know if this still need a review, thanks.

leelavg · 2024-04-29T06:57:08Z

services/provider/server/cephblockpool.go

+		if kerrors.IsNotFound(err) {
+			return nil, fmt.Errorf("CephBlockPool resource %q not found: %v", blockPoolName, err)
+		}
+		return nil, fmt.Errorf("failed to get CephBlockPool resource with name %q: %v", blockPoolName, err)


I don't see the result is different if the resource is not found here, as such don't make any differentiation as the returned error already contains the reason.

leelavg · 2024-04-29T07:00:39Z

services/provider/server/cephrbdmirror.go

+	cephRBDMirror := &rookCephv1.CephRBDMirror{}
+	cephRBDMirror.Name = rBDMirrorName
+	cephRBDMirror.Namespace = c.namespace
+	err := c.client.Get(ctx, client.ObjectKeyFromObject(cephRBDMirror), cephRBDMirror)


any reason in making two calls rather than a single create?

We need one rbdMirror for the cluster, hence I am checking if it already exists, and create it only if it doesn't

leelavg · 2024-04-29T07:12:52Z

services/provider/server/server.go

+	// enable mirroring on blockPool in the req
+	if err := s.cephBlockPoolManager.EnableBlockPoolMirroring(ctx, cephBlockPool); err != nil {
+		return nil, status.Errorf(codes.Internal, "Failed to enable mirroring for CephBlockPool resource %s: %v", req.Pool, err)
+	}
+
+	// create and set secret ref on the blockPool
+	if err := s.cephBlockPoolManager.SetBootstrapSecretRef(


It's guaranteed that the call to SetBootstrap will fail first time since the resource version changes due to update in EnableBlock func. I believe we need to get fresh copy every time we want to update I guess.

I will check it again

Signed-off-by: Rewant Soni <resoni@redhat.com>

rewantsoni · 2024-05-14T08:15:21Z

/retest-required

Signed-off-by: Rewant Soni <resoni@redhat.com>

openshift-ci · 2024-05-14T10:45:36Z

@rewantsoni: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/ocs-operator-bundle-e2e-aws	`d520db2`	link	true	`/test ocs-operator-bundle-e2e-aws`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 12, 2024

rewantsoni force-pushed the server-client branch from a73e09d to f721b9b Compare February 12, 2024 05:08

rewantsoni force-pushed the server-client branch 5 times, most recently from 6cb2451 to 201eadd Compare February 19, 2024 13:57

rewantsoni force-pushed the server-client branch 7 times, most recently from c9c775f to a6c7525 Compare February 28, 2024 08:44

rewantsoni changed the title ~~Server client~~ add rpc call for PeerBlockPool Feb 28, 2024

rewantsoni force-pushed the server-client branch from a6c7525 to 605e15e Compare February 29, 2024 08:39

rewantsoni mentioned this pull request Mar 4, 2024

Storageclusterpeer #2466

Open

leelavg reviewed Mar 4, 2024

View reviewed changes

rewantsoni marked this pull request as ready for review March 4, 2024 10:16

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 4, 2024

rewantsoni force-pushed the server-client branch from 605e15e to 42c6b87 Compare March 4, 2024 11:04

rewantsoni commented Mar 4, 2024

View reviewed changes

services/provider/server/cephblockpool.go Outdated Show resolved Hide resolved

rewantsoni force-pushed the server-client branch 5 times, most recently from 5b03275 to dd90597 Compare March 4, 2024 13:48

rchikatw reviewed Mar 6, 2024

View reviewed changes

leelavg reviewed Mar 6, 2024

View reviewed changes

rewantsoni force-pushed the server-client branch 5 times, most recently from d861d4a to 08ac1ed Compare March 11, 2024 11:34

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 14, 2024

rewantsoni force-pushed the server-client branch from 08ac1ed to 48323a3 Compare March 18, 2024 10:47

openshift-merge-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 18, 2024

rewantsoni force-pushed the server-client branch from 48323a3 to 675c086 Compare March 26, 2024 05:42

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 26, 2024

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 3, 2024

rewantsoni force-pushed the server-client branch from 675c086 to d06eb77 Compare April 17, 2024 09:11

openshift-merge-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 17, 2024

leelavg reviewed Apr 29, 2024

View reviewed changes

rewantsoni added 2 commits April 29, 2024 13:56

add rpc call for peerBlockPool

df9b868

Signed-off-by: Rewant Soni <resoni@redhat.com>

add client implementation for the peerblockpool rpc call

7a480fb

Signed-off-by: Rewant Soni <resoni@redhat.com>

rewantsoni force-pushed the server-client branch from d06eb77 to c681a2b Compare April 29, 2024 08:32

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 29, 2024

rewantsoni added 2 commits May 14, 2024 14:49

add server implementation for peerblockpool rpc call

ace6209

Signed-off-by: Rewant Soni <resoni@redhat.com>

add unit-tests for peerblockpool

d520db2

Signed-off-by: Rewant Soni <resoni@redhat.com>

rewantsoni force-pushed the server-client branch from cdf297c to d520db2 Compare May 14, 2024 09:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add rpc call for PeerBlockPool #2457

add rpc call for PeerBlockPool #2457

rewantsoni commented Feb 12, 2024 •

edited

openshift-ci bot commented Feb 12, 2024

openshift-ci bot commented Feb 12, 2024

leelavg left a comment

leelavg Mar 4, 2024

rewantsoni Mar 4, 2024

leelavg Mar 6, 2024

rewantsoni Mar 6, 2024

leelavg Mar 4, 2024

leelavg Mar 4, 2024

rewantsoni Mar 4, 2024

rewantsoni commented Mar 4, 2024 •

edited

iamniting commented Mar 5, 2024

rchikatw Mar 6, 2024

rewantsoni Mar 6, 2024

leelavg Mar 6, 2024

leelavg Mar 6, 2024

leelavg Mar 6, 2024

rewantsoni Mar 6, 2024 •

edited

leelavg Mar 6, 2024

leelavg Mar 6, 2024

leelavg commented Mar 6, 2024

leelavg commented Apr 8, 2024

leelavg Apr 29, 2024

leelavg Apr 29, 2024

rewantsoni Apr 29, 2024

leelavg Apr 29, 2024

rewantsoni Apr 29, 2024

rewantsoni commented May 14, 2024

openshift-ci bot commented May 14, 2024

		blockPoolObj := &rookCephv1.CephBlockPool{}
		err := c.client.Get(ctx, types.NamespacedName{Name: blockPoolName, Namespace: c.namespace}, blockPoolObj)

-	blockPoolObj := &rookCephv1.CephBlockPool{}
-	err := c.client.Get(ctx, types.NamespacedName{Name: blockPoolName, Namespace: c.namespace}, blockPoolObj)
+	blockPool := &rookCephv1.CephBlockPool{}
+        blockPool.Name = blockPoolName
+        blockPool.Namespace = c.namespace
+	err := c.client.Get(ctx, client.ObjectKeyFromObject(blockPool), blockPool)

add rpc call for PeerBlockPool #2457

Are you sure you want to change the base?

add rpc call for PeerBlockPool #2457

Conversation

rewantsoni commented Feb 12, 2024 • edited

openshift-ci bot commented Feb 12, 2024

openshift-ci bot commented Feb 12, 2024

leelavg 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

rewantsoni commented Mar 4, 2024 • edited

iamniting commented Mar 5, 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

rewantsoni Mar 6, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leelavg commented Mar 6, 2024

leelavg commented Apr 8, 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

rewantsoni commented May 14, 2024

openshift-ci bot commented May 14, 2024

rewantsoni commented Feb 12, 2024 •

edited

rewantsoni commented Mar 4, 2024 •

edited

rewantsoni Mar 6, 2024 •

edited