[WIP] Adds metadata to storage claim response proto message #2620

raaizik · 2024-05-20T18:13:56Z

Changes

Adds metadata field to storage claim response proto message that contains a mirroring flag of CephBlockPool to be read directly from the StorageClaim's controller which will in turn label the StorageClass with the required Ramen labels that allow RBD mirroring.

RHSTOR-5754

openshift-ci · 2024-05-20T18:14:06Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: raaizik
Once this PR has been reviewed and has the lgtm label, please assign agarwal-mudit 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

raaizik · 2024-05-20T18:15:15Z

/cc @nb-ohad

raaizik · 2024-05-21T09:21:03Z

/retest

raaizik · 2024-05-23T11:37:30Z

/cc @rewantsoni

rewantsoni · 2024-05-23T11:48:03Z

services/provider/server/server.go

+			cbp := &rookCephv1.CephBlockPool{}
+			err = s.client.Get(ctx, types.NamespacedName{Name: cephRes.Name, Namespace: s.namespace}, rns)
+			if err != nil {
+				return nil, status.Errorf(codes.Internal, "failed to get %s CephBlockPool. %v", cephRes.Name, err)
+			}
+


Not sure what you are trying to achieve here, you are creating a cpb var but you are getting the radosnamespace.
Also, we should not depend on one cephBlockPool to decide if we are enabling mirroring or not. There might be a case where we might need to blacklist some blockpools for mirroring.

No, I'm tagging the CBPRN class with a flag that will signal to StorageClaim's controller whether mirroring's enabled for said class.

There might be a case where we might need to blacklist some blockpools for mirroring.

Care to elaborate? What indicates a blacklisted pool? Either way, it's something that's better off handled on the client.

err = s.client.Get(ctx, types.NamespacedName{Name: cephRes.Name, Namespace: s.namespace}, rns)

You are getting the rns here, not the cbl, is this intended?

Discussed over DMs:

CBP is the only source to fetch the mirroring flag from.

In case of blacklisted pools we can have their flag indicating that override the mirroring flag.

rewantsoni · 2024-05-23T13:15:51Z

services/provider/server/server.go

+			cbp := &rookCephv1.CephBlockPool{}
+			err = s.client.Get(ctx, types.NamespacedName{Name: cephRes.Name, Namespace: s.namespace}, rns)
+			if err != nil {
+				return nil, status.Errorf(codes.Internal, "failed to get %s CephBlockPool. %v", cephRes.Name, err)
+			}
+


err = s.client.Get(ctx, types.NamespacedName{Name: cephRes.Name, Namespace: s.namespace}, rns)

You are getting the rns here, not the cbl, is this intended?

rewantsoni · 2024-05-23T13:16:45Z

services/provider/server/server.go

+			rbdStorageClassMetaData := map[string]string{
+				"mirroring": fmt.Sprintf("%v", cbp.Spec.Mirroring.Enabled),
+			}


If I am not wrong this will always be false, as you are haven't fetched the cbp

You are getting the rns here, not the cbl, is this intended?

CBPRN and its respective CPB carry the same name.

raaizik · 2024-06-03T08:08:21Z

/retest

rewantsoni · 2024-06-03T12:46:35Z

/retest-required

Signed-off-by: raaizik <132667934+raaizik@users.noreply.github.com>

Nikhil-Ladha · 2024-06-04T14:52:12Z

/retest

rewantsoni · 2024-06-05T12:08:20Z

Changes

Adds metadata field to storage claim response proto message that contains a mirroring flag of CephBlockPool to be read directly from the StorageClaim's controller which will in turn label the StorageClass with the required Ramen labels that allow RBD mirroring.

RHSTOR-5754

I went through the PR and this works in the case of storageClaim of type block, if I am not wrong we also need to label the storageClass created by storageClaim of type filesystem as well right? How are we handling that?

raaizik · 2024-06-09T08:59:44Z

Changes

Adds metadata field to storage claim response proto message that contains a mirroring flag of CephBlockPool to be read directly from the StorageClaim's controller which will in turn label the StorageClass with the required Ramen labels that allow RBD mirroring.
RHSTOR-5754

I went through the PR and this works in the case of storageClaim of type block, if I am not wrong we also need to label the storageClass created by storageClaim of type filesystem as well right? How are we handling that?

For Volume*SnapshotClass, the SID can be applied for backed classes of both types.
For Volume*ReplicationClass, the RepID can only be applied when there are two peered EPs. Can add a Replication flag (that I'd need to figure out how to populate) along with the Mirroring flag. @rewantsoni

leelavg · 2024-06-13T03:19:10Z

services/provider/proto/provider.proto

@@ -153,6 +153,8 @@ message StorageClaimConfigRequest{
 message StorageClaimConfigResponse{
    // ExternalResource holds the configuration data of external storage cluster
    repeated ExternalResource externalResource = 1;
+    // MetaData contains additional info regarding the external cluster resources
+    bytes MetaData = 2;


no need to Title case the field as generated proto already gives us an exported field.

Could be. I will take this into account.

leelavg · 2024-06-13T03:29:09Z

services/provider/proto/provider.proto

@@ -153,6 +153,8 @@ message StorageClaimConfigRequest{
 message StorageClaimConfigResponse{
    // ExternalResource holds the configuration data of external storage cluster
    repeated ExternalResource externalResource = 1;
+    // MetaData contains additional info regarding the external cluster resources
+    bytes MetaData = 2;


after going through PR, why not bool enableMirroring = 2;? why do we need to convert bool (cbp.Spec.Mirroring.Enabled) into string (%v) and then encode to bytes (json.Marshal)

The default proto value will be false anyways, unless false is also a valid value like after enabling you could disable we don't need to use bool pointer.

See first link in the description of the attached Jira for design details, @leelavg. We're looking at a feature that would allow us to add multiple fields that are considered as metadata. Once this PR's WIP tag is removed, there will be two flags appended to the metadata field.

openshift-ci bot requested a review from nb-ohad May 20, 2024 18:15

raaizik force-pushed the RHSTOR-5754 branch from 2cd9396 to 4cbed07 Compare May 20, 2024 18:18

raaizik changed the title ~~Add mirroring flag to the block pool's data~~ Adds mirroring flag to the block pool's data May 20, 2024

raaizik force-pushed the RHSTOR-5754 branch 2 times, most recently from 92d4cf5 to 60764a0 Compare May 23, 2024 10:31

openshift-ci bot requested a review from rewantsoni May 23, 2024 11:37

rewantsoni reviewed May 23, 2024

View reviewed changes

raaizik force-pushed the RHSTOR-5754 branch from fcb68bc to 02eb878 Compare May 23, 2024 13:15

rewantsoni reviewed May 23, 2024

View reviewed changes

raaizik force-pushed the RHSTOR-5754 branch 3 times, most recently from 62ee5f8 to 97ff9bd Compare May 23, 2024 14:14

raaizik changed the title ~~Adds mirroring flag to the block pool's data~~ [WIP] Adds mirroring flag to the block pool's data May 27, 2024

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 May 27, 2024

raaizik force-pushed the RHSTOR-5754 branch from 4f2f927 to ec9cde8 Compare May 28, 2024 12:06

raaizik changed the title ~~[WIP] Adds mirroring flag to the block pool's data~~ [WIP] Adds metadata to ceph resources May 28, 2024

raaizik force-pushed the RHSTOR-5754 branch 7 times, most recently from 4652707 to a052245 Compare May 30, 2024 16:02

raaizik changed the title ~~[WIP] Adds metadata to ceph resources~~ [WIP] Adds metadata to storage storage claim response proto message May 30, 2024

raaizik changed the title ~~[WIP] Adds metadata to storage storage claim response proto message~~ [WIP] Adds metadata to storage claim response proto message May 30, 2024

raaizik force-pushed the RHSTOR-5754 branch from a052245 to 2c68ec7 Compare May 30, 2024 16:32

raaizik force-pushed the RHSTOR-5754 branch 3 times, most recently from af4ca4a to 1e7beac Compare June 3, 2024 10:24

raaizik changed the title ~~[WIP] Adds metadata to storage claim response proto message~~ Adds metadata to storage claim response proto message Jun 3, 2024

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 3, 2024

Add metadata to ceph resources

550187d

Signed-off-by: raaizik <132667934+raaizik@users.noreply.github.com>

raaizik force-pushed the RHSTOR-5754 branch from f1bd111 to 550187d Compare June 4, 2024 09:36

raaizik requested a review from rewantsoni June 5, 2024 08:26

raaizik changed the title ~~Adds metadata to storage claim response proto message~~ [WIP] Adds metadata to storage claim response proto message Jun 10, 2024

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 10, 2024

leelavg reviewed Jun 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Adds metadata to storage claim response proto message #2620

[WIP] Adds metadata to storage claim response proto message #2620

raaizik commented May 20, 2024 •

edited

openshift-ci bot commented May 20, 2024

raaizik commented May 20, 2024

raaizik commented May 21, 2024

raaizik commented May 23, 2024

rewantsoni May 23, 2024

raaizik May 23, 2024 •

edited

rewantsoni May 23, 2024

raaizik May 30, 2024

rewantsoni May 23, 2024

rewantsoni May 23, 2024

raaizik May 23, 2024 •

edited

raaizik commented Jun 3, 2024

rewantsoni commented Jun 3, 2024

Nikhil-Ladha commented Jun 4, 2024

rewantsoni commented Jun 5, 2024

Changes

raaizik commented Jun 9, 2024

Changes

leelavg Jun 13, 2024

raaizik Jun 13, 2024

leelavg Jun 13, 2024

raaizik Jun 13, 2024

[WIP] Adds metadata to storage claim response proto message #2620

Are you sure you want to change the base?

[WIP] Adds metadata to storage claim response proto message #2620

Conversation

raaizik commented May 20, 2024 • edited

Changes

openshift-ci bot commented May 20, 2024

raaizik commented May 20, 2024

raaizik commented May 21, 2024

raaizik commented May 23, 2024

Choose a reason for hiding this comment

raaizik May 23, 2024 • edited

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

raaizik May 23, 2024 • edited

Choose a reason for hiding this comment

raaizik commented Jun 3, 2024

rewantsoni commented Jun 3, 2024

Nikhil-Ladha commented Jun 4, 2024

rewantsoni commented Jun 5, 2024

Changes

raaizik commented Jun 9, 2024

Changes

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

raaizik commented May 20, 2024 •

edited

raaizik May 23, 2024 •

edited

raaizik May 23, 2024 •

edited