Add RPC calls for peering storageCluster #2671

rewantsoni · 2024-06-20T08:14:29Z

Add the RPC calls for peer remote odf cluster, these include

PeerStorageCluster

openshift-ci · 2024-06-20T08:14:33Z

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

rchikatw · 2024-07-08T08:18:35Z

PR LGTM

leelavg · 2024-07-16T06:10:18Z

services/provider/proto/provider.proto

+message OffboardMirroringPeerResponse{}
+
+// AcknowledgeMirrorPeerOnboardingRequest holds the information required to acknowledge the onboarding
+message AcknowledgeMirrorPeerOnboardingRequest{


I believe AcknowledgeOnboardingMirrorPeerRequest would be inline w/ other requests? wdyt?

I was thinking of changing it from MirroringPeer (as this name coincides with MirrorPeer CR from MCO) to whatever we decide to name the CR we decide in #2677

updated the name for the RPC calls

leelavg · 2024-07-16T06:11:43Z

services/provider/proto/provider.proto

+// OnboardMirroringPeerRequest holds the required information to validate and onboard Storage Provider cluster
+message OnboardMirroringPeerRequest{
+    // onboardingTicket authenticates the secondary provider cluster
+    string onboardingTicket = 1;


is this going to be the same ticket that we use today?

Yes, this will be the same ticket but with a role for mirroring, changes for which are in #2669

rewantsoni · 2024-07-30T09:08:31Z

/retest-required

rewantsoni · 2024-09-27T06:38:56Z

/retest-required

nb-ohad · 2024-09-29T12:52:14Z

services/provider/api/client/client.go

+func (cc *OCSProviderClient) AcknowledgeOnboardingStorageClusterPeer(ctx context.Context, storageClusterPeerUID string) (*pb.AcknowledgeOnboardingStorageClusterPeerResponse, error) {
+	if cc.Client == nil || cc.clientConn == nil {
+		return nil, fmt.Errorf("provider client is closed")
+	}
+
+	req := &pb.AcknowledgeOnboardingStorageClusterPeerRequest{
+		StorageClusterPeerUID: storageClusterPeerUID,
+	}
+
+	apiCtx, cancel := context.WithTimeout(ctx, cc.timeout)
+	defer cancel()
+
+	return cc.Client.AcknowledgeOnboardingStorageClusterPeer(apiCtx, req)
+}


Why do we need this? Why can't we use the same ack function that we have for client requests?

nb-ohad · 2024-09-29T12:53:15Z

services/provider/api/client/client.go

+	return cc.Client.AcknowledgeOnboardingStorageClusterPeer(apiCtx, req)
+}
+
+func (cc *OCSProviderClient) GetMirroringInfo(ctx context.Context, storageClusterPeerUID string, blockPoolNames []string) (*pb.MirroringInfoResponse, error) {


Shouldn't this be GetPeerInfo, onboarding peer does not have, conceptually, anything to do with mirroring. We might use it in the future to pass other type of information between 2 or more clusters.

nb-ohad · 2024-09-29T12:53:52Z

services/provider/api/client/client.go

+
+	req := &pb.MirroringInfoRequest{
+		StorageClusterPeerUID: storageClusterPeerUID,
+		BlockPoolNames:        blockPoolNames,


This need to be nested under a BlockPoolMirroring optional section

nb-ohad · 2024-09-29T12:54:34Z

services/provider/api/client/client.go

+	req := &pb.AcknowledgeOnboardingStorageClusterPeerRequest{
+		StorageClusterPeerUID: storageClusterPeerUID,
+	}


Is block pool mirroring intent provided via a separate call? if so I was not able to find it

nb-ohad · 2024-09-29T12:56:15Z

services/provider/server/server.go

+	return &pb.AcknowledgeOnboardingStorageClusterPeerResponse{}, nil
+}
+
+func (s *OCSProviderServer) GetMirroringInfo(_ context.Context, _ *pb.MirroringInfoRequest) (*pb.MirroringInfoResponse, error) {


Same logic as last comment

Suggested change

func (s *OCSProviderServer) GetMirroringInfo(_ context.Context, _ *pb.MirroringInfoRequest) (*pb.MirroringInfoResponse, error) {

func (s *OCSProviderServer) GetPeerInfo(_ context.Context, _ *pb.MirroringInfoRequest) (*pb.MirroringInfoResponse, error) {

nb-ohad · 2024-09-29T12:57:07Z

services/provider/server/server.go

+func (s *OCSProviderServer) AcknowledgeOnboardingStorageClusterPeer(_ context.Context, _ *pb.AcknowledgeOnboardingStorageClusterPeerRequest) (*pb.AcknowledgeOnboardingStorageClusterPeerResponse, error) {
+	return &pb.AcknowledgeOnboardingStorageClusterPeerResponse{}, nil
+}


As explained in a previous comment, I don't see any value in having a separate ack process dedicated for cluster peering

openshift-ci · 2024-10-08T13:25:51Z

[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 ask for approval from nb-ohad. 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

Signed-off-by: Rewant Soni <[email protected]>

ran make gen-protobuf to generate the code Signed-off-by: Rewant Soni <[email protected]>

Signed-off-by: Rewant Soni <[email protected]>

rewantsoni · 2024-10-09T17:00:47Z

/retest-required

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 Jun 20, 2024

rewantsoni marked this pull request as ready for review June 24, 2024 08:18

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 Jun 24, 2024

This was referenced Jun 25, 2024

Implement the server side rpc calls for peering #2675

Open

storageClusterPeer: add logic for storageclusterpeer controller #2678

Open

leelavg reviewed Jul 16, 2024

View reviewed changes

rewantsoni mentioned this pull request Jul 16, 2024

update provider-api-server to be deployed in internal mode #2690

Closed

1 task

leelavg reviewed Jul 16, 2024

View reviewed changes

rewantsoni force-pushed the rpc branch 3 times, most recently from d77bc77 to 2aa07b6 Compare July 24, 2024 06:38

This was referenced Jul 29, 2024

add rpc call for PeerBlockPool #2457

Closed

add rpc call for RevokeBlockPoolPeering #2493

Closed

Update provider-api-server to be deployed in internal and provider mode #2718

Merged

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

rewantsoni force-pushed the rpc branch from 2aa07b6 to 13be847 Compare August 13, 2024 05:42

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

rewantsoni force-pushed the rpc branch 2 times, most recently from 0840f28 to 6ed794c Compare August 28, 2024 03:59

rewantsoni force-pushed the rpc branch 2 times, most recently from d4955a3 to 8711fbe Compare September 10, 2024 16:31

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

rewantsoni force-pushed the rpc branch from 8711fbe to 7174fef Compare September 19, 2024 05:25

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

rewantsoni force-pushed the rpc branch 3 times, most recently from 681a175 to 5f8a8da Compare September 19, 2024 05:39

nb-ohad requested changes Sep 29, 2024

View reviewed changes

openshift-ci bot assigned nb-ohad Sep 29, 2024

rewantsoni force-pushed the rpc branch from 5f8a8da to 7fdca77 Compare October 8, 2024 13:25

rewantsoni force-pushed the rpc branch from 7fdca77 to b96e149 Compare October 8, 2024 13:43

rewantsoni changed the title ~~Add RPC calls for onboarding/offboarding secondary provider~~ Add RPC calls for peering storageCluster Oct 8, 2024

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

rewantsoni added 4 commits October 9, 2024 16:46

Add rpc calls related to peering of storageClusters

81a20e5

Signed-off-by: Rewant Soni <[email protected]>

Add generated code for rpc calls

0ff3f52

ran make gen-protobuf to generate the code Signed-off-by: Rewant Soni <[email protected]>

Add client side code for rpc calls

ae3f82f

Signed-off-by: Rewant Soni <[email protected]>

add dummy server implementation

22cda0a

Signed-off-by: Rewant Soni <[email protected]>

rewantsoni force-pushed the rpc branch from b96e149 to 22cda0a Compare October 9, 2024 11:18

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add RPC calls for peering storageCluster #2671

Add RPC calls for peering storageCluster #2671

rewantsoni commented Jun 20, 2024 •

edited

Loading

openshift-ci bot commented Jun 20, 2024

rchikatw commented Jul 8, 2024

leelavg Jul 16, 2024

rewantsoni Jul 22, 2024

rewantsoni Jul 31, 2024

leelavg Jul 16, 2024

rewantsoni Jul 16, 2024

rewantsoni commented Jul 30, 2024

rewantsoni commented Sep 27, 2024

nb-ohad Sep 29, 2024

nb-ohad Sep 29, 2024

nb-ohad Sep 29, 2024

nb-ohad Sep 29, 2024

nb-ohad Sep 29, 2024

nb-ohad Sep 29, 2024

openshift-ci bot commented Oct 8, 2024

rewantsoni commented Oct 9, 2024

	func (s OCSProviderServer) GetMirroringInfo(_ context.Context, _ pb.MirroringInfoRequest) (*pb.MirroringInfoResponse, error) {
	func (s OCSProviderServer) GetPeerInfo(_ context.Context, _ pb.MirroringInfoRequest) (*pb.MirroringInfoResponse, error) {

Add RPC calls for peering storageCluster #2671

Are you sure you want to change the base?

Add RPC calls for peering storageCluster #2671

Conversation

rewantsoni commented Jun 20, 2024 • edited Loading

openshift-ci bot commented Jun 20, 2024

rchikatw commented Jul 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 Jul 30, 2024

rewantsoni commented Sep 27, 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

openshift-ci bot commented Oct 8, 2024

rewantsoni commented Oct 9, 2024

rewantsoni commented Jun 20, 2024 •

edited

Loading