rpc: add new rpc call for mirroring #2875

rewantsoni · 2024-11-04T16:07:52Z

Add two rpc calls required for setting up mirroring for blockpool and radosnamespace

GetBlockPoolsInfo
GetClientInfo

openshift-ci · 2024-11-04T16:07:57Z

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

services/provider/api/client/client.go

services/provider/server/server.go

openshift-ci · 2024-11-11T17:16:47Z

[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

services/provider/server/server.go

nb-ohad · 2024-11-13T09:53:16Z

services/provider/server/server.go

@@ -1042,3 +1043,130 @@ func (s *OCSProviderServer) PeerStorageCluster(ctx context.Context, req *pb.Peer

 	return &pb.PeerStorageClusterResponse{}, nil
 }
+
+func (s *OCSProviderServer) GetStorageClientsInfo(ctx context.Context, req *pb.StorageClientsInfoRequest) (*pb.StorageClientsInfoResponse, error) {
+	response := &pb.StorageClientsInfoResponse{ClientsInfo: []*pb.ClientInfo{}, Error: []*pb.StorageClientInfoError{}}


Client info is an array of pointers not a pointer of arrays, it will get allocated (at size 0) automatically

Suggested change

response := &pb.StorageClientsInfoResponse{ClientsInfo: []*pb.ClientInfo{}, Error: []*pb.StorageClientInfoError{}}

response := &pb.StorageClientsInfoResponse{}

nb-ohad · 2024-11-13T09:53:30Z

services/provider/server/server.go

@@ -1042,3 +1043,130 @@ func (s *OCSProviderServer) PeerStorageCluster(ctx context.Context, req *pb.Peer

 	return &pb.PeerStorageClusterResponse{}, nil
 }
+
+func (s *OCSProviderServer) GetStorageClientsInfo(ctx context.Context, req *pb.StorageClientsInfoRequest) (*pb.StorageClientsInfoResponse, error) {
+	response := &pb.StorageClientsInfoResponse{ClientsInfo: []*pb.ClientInfo{}, Error: []*pb.StorageClientInfoError{}}


Of Error is a list it should be called Errors

nb-ohad · 2024-11-13T09:55:09Z

services/provider/server/server.go

+		if err != nil {
+			if !kerrors.IsNotFound(err) {
+				klog.Errorf("failed to get consumer with id %v: %v", req.ClientIDs[i], err)
+				response.Error = append(response.Error,
+					&pb.StorageClientInfoError{
+						ClientID: req.ClientIDs[i],
+						Code:     pb.ErrorCode_Internal,
+						Message:  err.Error(),
+					},
+				)
+			}
+			continue
+		}


you don't need to nest

Suggested change

if err != nil {

if !kerrors.IsNotFound(err) {

klog.Errorf("failed to get consumer with id %v: %v", req.ClientIDs[i], err)

response.Error = append(response.Error,

&pb.StorageClientInfoError{

ClientID: req.ClientIDs[i],

Code: pb.ErrorCode_Internal,

Message: err.Error(),

},

)

}

continue

}

if kerror.IsNotFound(err) {

continue

} else if err != nil {

klog.Errorf("failed to get consumer with id %v: %v", req.ClientIDs[i], err)

response.Error = append(response.Error,

&pb.StorageClientInfoError{

ClientID: req.ClientIDs[i],

Code: pb.ErrorCode_Internal,

Message: err.Error(),

},

)

}

nb-ohad · 2024-11-13T09:55:34Z

services/provider/server/server.go

+			continue
+		}
+		owner := util.FindOwnerRefByKind(consumer, "StorageCluster")
+


Suggested change

nb-ohad · 2024-11-13T09:56:19Z

services/provider/server/server.go

+					},
+				)
+			}
+			continue


In case we skip, it will be helpful to have a log message capturing it

nb-ohad · 2024-11-13T09:58:30Z

services/provider/server/server.go

+			response.Error = append(response.Error,
+				&pb.StorageClientInfoError{
+					ClientID: req.ClientIDs[i],
+					Code:     pb.ErrorCode_Internal,


Please use specific error codes and not generic ones

nb-ohad · 2024-11-13T10:06:28Z

services/provider/server/server.go

+					Code:     pb.ErrorCode_Internal,
+					Message:  "invalid number of radosnamespace found for the Client",
+				},
+			)


Missing a continue here (with a log message)

nb-ohad · 2024-11-13T10:09:21Z

services/provider/server/server.go

+		} else if len(rnsList.Items) == 1 {
+			response.ClientsInfo = append(response.ClientsInfo, &pb.ClientInfo{ClientID: req.ClientIDs[i], RadosNamespace: rnsList.Items[0].Name})
+		} else {
+			response.ClientsInfo = append(response.ClientsInfo, &pb.ClientInfo{ClientID: req.ClientIDs[i]})
+		}


There is no need to have 2 append statements

You should treat the "good" case as a for loop scoped code

Suggested change

} else if len(rnsList.Items) == 1 {

response.ClientsInfo = append(response.ClientsInfo, &pb.ClientInfo{ClientID: req.ClientIDs[i], RadosNamespace: rnsList.Items[0].Name})

} else {

response.ClientsInfo = append(response.ClientsInfo, &pb.ClientInfo{ClientID: req.ClientIDs[i]})

}

}

clientInfo := pb.ClientInfo{ClientID: req.ClientIDs[i]

if len(len(rnsList.Items) != 0 {

clientInfo.RadosNamesapce = rnsList[0].Name

}

response.ClientsInfo = append(response.ClientsInfo, &clientInfo)

nb-ohad · 2024-11-13T10:10:33Z

services/provider/server/server.go

+		if err != nil {
+			if !kerrors.IsNotFound(err) {
+				response.Error = append(response.Error,
+					&pb.BlockPoolInfoError{
+						BlockPoolName: cephBlockPool.Name,
+						Code:          pb.ErrorCode_Internal,
+						Message:       err.Error(),
+					},
+				)
+			}
+			continue
+		}


No need for nesting and please log a message into the log for a continue

nb-ohad · 2024-11-13T10:12:58Z

services/provider/server/server.go

+					cephBlockPool.Name,
+					err,
+				)
+				klog.Error(errMsg)


Not finding the secret should not be treated as an error from the perspective of the RPC call. It might be an error for a controller but not for the server

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

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

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

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 Nov 4, 2024

rewantsoni force-pushed the mirroring-rpc branch 3 times, most recently from 3a5aad6 to 4950f96 Compare November 6, 2024 06:38

rewantsoni marked this pull request as ready for review November 6, 2024 06:39

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 Nov 6, 2024

rewantsoni mentioned this pull request Nov 6, 2024

controllers: add a new controller to setup mirroring #2878

Open

rewantsoni force-pushed the mirroring-rpc branch 7 times, most recently from f228a04 to e069e9a Compare November 11, 2024 16:09

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

nb-ohad requested changes Nov 11, 2024

View reviewed changes

openshift-ci bot assigned nb-ohad Nov 11, 2024

rewantsoni force-pushed the mirroring-rpc branch from e069e9a to be79221 Compare November 11, 2024 17:16

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

rewantsoni force-pushed the mirroring-rpc branch 2 times, most recently from 8d7cbb8 to e250ada Compare November 12, 2024 10:23

nb-ohad requested changes Nov 12, 2024

View reviewed changes

rewantsoni force-pushed the mirroring-rpc branch from e250ada to c85b0ad Compare November 12, 2024 13:46

nb-ohad requested changes Nov 13, 2024

View reviewed changes

rewantsoni force-pushed the mirroring-rpc branch 3 times, most recently from f25b323 to 6208c00 Compare November 13, 2024 13:02

add rpc calls related to mirroring

373be0f

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

add client side implementation

a8e897f

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

rewantsoni force-pushed the mirroring-rpc branch from 6208c00 to 5a38c09 Compare November 13, 2024 17:59

add server side implementation

064fb50

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

rewantsoni force-pushed the mirroring-rpc branch from 5a38c09 to 064fb50 Compare November 13, 2024 18:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rpc: add new rpc call for mirroring #2875

rpc: add new rpc call for mirroring #2875

rewantsoni commented Nov 4, 2024

openshift-ci bot commented Nov 4, 2024

openshift-ci bot commented Nov 11, 2024

nb-ohad Nov 13, 2024

nb-ohad Nov 13, 2024

nb-ohad Nov 13, 2024

nb-ohad Nov 13, 2024

nb-ohad Nov 13, 2024

nb-ohad Nov 13, 2024

nb-ohad Nov 13, 2024

nb-ohad Nov 13, 2024

nb-ohad Nov 13, 2024

nb-ohad Nov 13, 2024

	response := &pb.StorageClientsInfoResponse{ClientsInfo: []pb.ClientInfo{}, Error: []pb.StorageClientInfoError{}}
	response := &pb.StorageClientsInfoResponse{}

rpc: add new rpc call for mirroring #2875

Are you sure you want to change the base?

rpc: add new rpc call for mirroring #2875

Conversation

rewantsoni commented Nov 4, 2024

openshift-ci bot commented Nov 4, 2024

openshift-ci bot commented Nov 11, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment