Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[WIP] Sends V*ReplicationClass to client #2727

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

[WIP] Sends V*ReplicationClass to client #2727

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3986183
Sends V*ReplicationClass to client
raaizik Aug 1, 2024
5209514
getExternalResourceLabels
raaizik Sep 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
116 changes: 114 additions & 2 deletions services/provider/server/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package server
import (
"context"
"crypto"
"crypto/md5"
"crypto/rsa"
"crypto/sha256"
"crypto/x509"
Expand Down Expand Up @@ -668,10 +669,32 @@ func (s *OCSProviderServer) GetStorageClaimConfig(ctx context.Context, req *pb.S
return nil, status.Error(codes.Internal, msg)
}
}

// fetch storage cluster peer to indicate whether replication is enabled
scp := &ocsv1.StorageClusterPeerList{}
if err = s.client.List(ctx, scp, client.InNamespace(s.namespace)); err != nil {
return nil, status.Errorf(codes.Internal, "failed to get StorageClusterPeerList. %v", err)
}
replicationEnabled := len(scp.Items) > 1
ID := ""
if replicationEnabled {
storageClaimName, err := json.Marshal(req.StorageClaimName)
if err != nil {
klog.Errorf("failed to marshal a name for a storage claim request based on %v. %v", s, err)
panic("failed to marshal storage class request name")
}
md5Sum := md5.Sum(storageClaimName)
ID = hex.EncodeToString(md5Sum[:16])
} // todo storageID in provider is planned to be string(r.storageClaim.UID)
// SID for RamenDR
SID := req.StorageConsumerUUID
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

StorageID is used to match V*C to StorageClass, we don't need it to be storageClaim UID, we can have it as storageRequest UID as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've already agreed that it'll be string(r.storageClaim.UID) in #168. Eventually the value that will be assigned to this ID is the one from the client, so it actually doesn't matter what value is chosen on this end.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the concept for storageClaim might go away in the future because of convergence, we should pivot and not use it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even so, the final value assigned would be determined on the client.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to determine the label value on the client if we can do it from the provider itself?

Copy link
Contributor Author

@raaizik raaizik Sep 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could, I'm saying with the current state of things in these PRs this is what would happen. After this PR will get merged, the value assigned from here will be propagated to the client and changes will be reflected in the mentioned PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I agree that we could but I think as storageID is an arbitrary value used by ramen to cross-link the VRC/VSC and SC, the hash of storageRequest UID perfectly fits our use case. I don't think we need to change it further on the client. Let's wait for others to review this.


var extR []*pb.ExternalResource

storageRequestHash := getStorageRequestHash(req.StorageConsumerUUID, req.StorageClaimName)

for _, cephRes := range storageRequest.Status.CephResources {

switch cephRes.Kind {
case "CephClient":
clientSecretName, clientUserType, err := s.getCephClientInformation(ctx, cephRes.Name)
Expand Down Expand Up @@ -702,13 +725,19 @@ func (s *OCSProviderServer) GetStorageClaimConfig(ctx context.Context, req *pb.S
})

case "CephBlockPoolRadosNamespace":

rns := &rookCephv1.CephBlockPoolRadosNamespace{}
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 CephBlockPoolRadosNamespace. %v", cephRes.Name, err)
}

// Fetch mirroring flag for the current ceph resource
cbp := &rookCephv1.CephBlockPool{}
cbp.Name = rns.Spec.BlockPoolName
cbp.Namespace = s.namespace
if err = s.client.Get(ctx, client.ObjectKeyFromObject(cbp), cbp); err != nil {
return nil, status.Errorf(codes.Internal, "failed to get %s CephBlockPool. %v", cephRes.Name, err)
}
mirroringEnabled := cbp.Spec.Mirroring.Enabled
provisionerSecretName := storageClaimCephCsiSecretName("provisioner", storageRequestHash)
nodeSecretName := storageClaimCephCsiSecretName("node", storageRequestHash)
rbdStorageClassData := map[string]string{
Expand Down Expand Up @@ -745,6 +774,56 @@ func (s *OCSProviderServer) GetStorageClaimConfig(ctx context.Context, req *pb.S
Data: mustMarshal(map[string]string{
"csi.storage.k8s.io/group-snapshotter-secret-name": provisionerSecretName,
})},
&pb.ExternalResource{
Name: "ceph-rbd",
Kind: "VolumeReplicationClass",
Data: mustMarshal(map[string]string{
"replication.storage.openshift.io/replication-secret-name": provisionerSecretName,
"mirroringMode": "snapshot",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mirroringMode is not a label but a parameter under the spec (spec.parameters) section of the VolumeReplicationClass

}),
Labels: getExternalResourceLabels("VolumeReplicationClass", replicationEnabled, mirroringEnabled, false, ID, SID),
Annotations: map[string]string{
"replication.storage.openshift.io/is-default-class": "true",
},
},
&pb.ExternalResource{
Name: "ceph-rbd-flatten",
Kind: "VolumeReplicationClass",
Data: mustMarshal(map[string]string{
"replication.storage.openshift.io/replication-secret-name": provisionerSecretName,
Comment on lines +792 to +793
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For flattening VRC, we require the special parameter flattenMode: force to be added, we should add it from the provider side

"mirroringMode": "snapshot",
"flattenMode": "force",
}),
Labels: getExternalResourceLabels("VolumeReplicationClass", replicationEnabled, mirroringEnabled, true, ID, SID),
Annotations: map[string]string{
"replication.storage.openshift.io/is-default-class": "true",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from what I recall, we don't require this annotation for flatten VRC

Copy link
Contributor Author

@raaizik raaizik Sep 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure it is needed. See here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MCO is creating two VRC's one with flatten mode and one without flatten.
The one that is without flatten mode has the annotation the other one doesn't. See here and here
We are setting the annotations to be empty when creating the flatten mode VRC.

Also the same in mentioned in the design doc Changes required

},
},
&pb.ExternalResource{
Name: "ceph-rbd",
Kind: "VolumeGroupReplicationClass",
Data: mustMarshal(map[string]string{
"replication.storage.openshift.io/group-replication-secret-name": provisionerSecretName,
"mirroringMode": "snapshot",
}),
Labels: getExternalResourceLabels("VolumeGroupReplicationClass", replicationEnabled, mirroringEnabled, false, ID, SID),
Annotations: map[string]string{
"replication.storage.openshift.io/is-default-class": "true",
},
},
&pb.ExternalResource{
Name: "ceph-rbd-flatten",
Kind: "VolumeGroupReplicationClass",
Data: mustMarshal(map[string]string{
"replication.storage.openshift.io/group-replication-secret-name": provisionerSecretName,
"mirroringMode": "snapshot",
"flattenMode": "force",
}),
Labels: getExternalResourceLabels("VolumeGroupReplicationClass", replicationEnabled, mirroringEnabled, true, ID, SID),
Annotations: map[string]string{
"replication.storage.openshift.io/is-default-class": "true",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from what I recall, we don't require this annotation for flatten VGRC

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See reply

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See reply

},
},
&pb.ExternalResource{
Kind: "ClientProfile",
Name: "ceph-rbd",
Expand Down Expand Up @@ -832,6 +911,39 @@ func (s *OCSProviderServer) GetStorageClaimConfig(ctx context.Context, req *pb.S

}

func getExternalResourceLabels(kind string, isReplicationEnabled bool, isMirroringEnabled bool, isFlattenMode bool,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to differentiate between the Replication and Mirroring flag, we can have one mirroring flag (which will check the cephblockpool) and add labels based on that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my perspective, there's no other way to handle these flags. Replication is Vol*RepClass specific, while Mirroring is aimed for both Vol*RepClass and Vol*SnapClass.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your replication flag depends on storageClusterPeer, whereas the mirroring flag is derived from CephBlockPool.

Enabling mirroring on the cephblockpool is the responsibility of the storageClusterPeer. Isn't both pointing to the same field eventually?

I think we should add the labels for rbd-based VRC/VSC and SC based only on the mirroring field. It is possible that a particular blockpool is not selected for mirroring from StorageClusterPeer CR. See here

I was going through the PR and I am not able to find the labels applied for CephFS-based V*SC and SC. Do we not need them?

Copy link
Contributor Author

@raaizik raaizik Sep 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enabling mirroring on the cephblockpool is the responsibility of the storageClusterPeer.

I am not aware of such thing. How does it fetch this flag exactly? Is it merged already?

I am not able to find the labels applied for CephFS-based V*SC and SC

As discussed, all of this will be handled in another PR. This PR refers only to Vol*RepClass.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not aware of such thing. How does it fetch this flag exactly? Is it merged already?

This was discussed in the design discussion. Each StorageClusterPeer will enable mirroring on the cephblockpool based on the label selector.

For rbd we can use the mirroring field on the cephblockpool to add ramenRelated labels, we don't need another replication flag explicitly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you're misunderstanding. I am using the Mirroring flag solely for the StorageID label. It's unrelated to RepID.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need two flags for the same set of ramen labels? From what I understand we are creating a differentiation which we don't need. It will just complicate the things.

As an example what if we have 2 cephblockpool a and b the storageclusterpeer is configured only for blockpool a.

There are two clients 1 and 2. 1 uses blockpool a, 2 uses blockpool b.

If we keep the flags separate,

For client 1 we will enable both replication and storage id label

For client 2 we will enable only replication id label

In the second case we don't require any ramen related labels to be added as that blockpool is not configured for mirroring.

Hence we don't need to check for both storageclusterpeer and cephblockpool. Checking only the cephblockpool will work

replicationID string, storageID string) map[string]string {
labels := make(map[string]string)
switch kind {
case "VolumeReplicationClass", "VolumeGroupReplicationClass":
if isReplicationEnabled {
replicationID := replicationID
labels["ramendr.openshift.io/replicationid"] = replicationID
if isFlattenMode {
labels["replication.storage.openshift.io/flatten-mode"] = "force"
}
}
if isMirroringEnabled {
labels["ramendr.openshift.io/storageID"] = storageID
}
case "VolumeSnapshotClass", "VolumeGroupSnapshotClass":
if isMirroringEnabled {
labels["ramendr.openshift.io/storageID"] = storageID
}
case "StorageClass":
if isReplicationEnabled {
labels["ramendr.openshift.io/replicationid"] = replicationID
}
if isMirroringEnabled {
labels["ramendr.openshift.io/storageID"] = storageID
}

default:
panic(fmt.Sprintf("unknown storage class kind %q", kind))
}
return labels
}

// ReportStatus rpc call to check if a consumer can reach to the provider.
func (s *OCSProviderServer) ReportStatus(ctx context.Context, req *pb.ReportStatusRequest) (*pb.ReportStatusResponse, error) {
// Update the status in storageConsumer CR
Expand Down
90 changes: 86 additions & 4 deletions services/provider/server/server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"encoding/json"
"fmt"
"reflect"
"strconv"
"testing"

Expand All @@ -29,9 +30,11 @@ import (
)

type externalResource struct {
Kind string `json:"kind"`
Data any `json:"data"`
Name string `json:"name"`
Kind string `json:"kind"`
Data any `json:"data"`
Name string `json:"name"`
Labels map[string]string `json:"labels"`
Annotations map[string]string `json:"annotations"`
}

var serverNamespace = "openshift-storage"
Expand Down Expand Up @@ -350,7 +353,10 @@ func TestGetExternalResources(t *testing.T) {
data, err := json.Marshal(mockResoruce.Data)
assert.NoError(t, err)
assert.Equal(t, string(extResource.Data), string(data))

}
assert.True(t, reflect.DeepEqual(extResource.Labels, mockResoruce.Labels))
assert.True(t, reflect.DeepEqual(extResource.Annotations, mockResoruce.Annotations))
Comment on lines +358 to +359
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could use assert.Equal() it uses reflect.DeepEqual internally

Copy link
Contributor Author

@raaizik raaizik Sep 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both labels and annotations are of type map, which means that in order to use assert.Equal() I'd need to convert from map to []byte or string which is not straightforward. Therefore I've chosen this assertion instead so it's compact as a one-liner.

Copy link
Member

@rewantsoni rewantsoni Sep 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to convert the map to byte or string. See here.

assert.Equal(t, extResource.Kind, mockResoruce.Kind)
assert.Equal(t, extResource.Name, mockResoruce.Name)
}
Expand Down Expand Up @@ -388,7 +394,8 @@ func TestGetExternalResources(t *testing.T) {
assert.NoError(t, err)
assert.Equal(t, string(extResource.Data), string(data))
}

assert.True(t, reflect.DeepEqual(extResource.Labels, mockResoruce.Labels))
assert.True(t, reflect.DeepEqual(extResource.Annotations, mockResoruce.Annotations))
assert.Equal(t, extResource.Kind, mockResoruce.Kind)
assert.Equal(t, extResource.Name, mockResoruce.Name)
}
Expand Down Expand Up @@ -681,6 +688,56 @@ func TestOCSProviderServerGetStorageClaimConfig(t *testing.T) {
"csi.storage.k8s.io/group-snapshotter-secret-name": "ceph-client-provisioner-8d40b6be71600457b5dec219d2ce2d4c",
},
},
"ceph-rbd-volumereplicationclass": {
Name: "ceph-rbd",
Kind: "VolumeReplicationClass",
Data: map[string]string{
"replication.storage.openshift.io/replication-secret-name": "ceph-client-provisioner-8d40b6be71600457b5dec219d2ce2d4c",
"mirroringMode": "snapshot",
},
Labels: map[string]string{},
Annotations: map[string]string{
"replication.storage.openshift.io/is-default-class": "true",
},
},
"ceph-rbd-flatten-volumereplicationclass": {
Name: "ceph-rbd-flatten",
Kind: "VolumeReplicationClass",
Data: map[string]string{
"replication.storage.openshift.io/replication-secret-name": "ceph-client-provisioner-8d40b6be71600457b5dec219d2ce2d4c",
"mirroringMode": "snapshot",
"flattenMode": "force",
},
Labels: map[string]string{},
Annotations: map[string]string{
"replication.storage.openshift.io/is-default-class": "true",
},
},
"ceph-rbd-volumegroupreplicationclass": {
Name: "ceph-rbd",
Kind: "VolumeGroupReplicationClass",
Data: map[string]string{
"replication.storage.openshift.io/group-replication-secret-name": "ceph-client-provisioner-8d40b6be71600457b5dec219d2ce2d4c",
"mirroringMode": "snapshot",
},
Labels: map[string]string{},
Annotations: map[string]string{
"replication.storage.openshift.io/is-default-class": "true",
},
},
"ceph-rbd-flatten-volumegroupreplicationclass": {
Name: "ceph-rbd-flatten",
Kind: "VolumeGroupReplicationClass",
Data: map[string]string{
"replication.storage.openshift.io/group-replication-secret-name": "ceph-client-provisioner-8d40b6be71600457b5dec219d2ce2d4c",
"mirroringMode": "snapshot",
"flattenMode": "force",
},
Labels: map[string]string{},
Annotations: map[string]string{
"replication.storage.openshift.io/is-default-class": "true",
},
},
"ceph-client-provisioner-8d40b6be71600457b5dec219d2ce2d4c": {
Name: "ceph-client-provisioner-8d40b6be71600457b5dec219d2ce2d4c",
Kind: "Secret",
Expand Down Expand Up @@ -1047,6 +1104,18 @@ func TestOCSProviderServerGetStorageClaimConfig(t *testing.T) {
}
assert.NoError(t, client.Create(ctx, radosNamespace))

cephBlockPool := &rookCephv1.CephBlockPool{
ObjectMeta: metav1.ObjectMeta{
Name: "cephblockpool",
Namespace: server.namespace,
},
Spec: rookCephv1.NamedBlockPoolSpec{
PoolSpec: rookCephv1.PoolSpec{
Mirroring: rookCephv1.MirroringSpec{Enabled: false},
}},
}
assert.NoError(t, client.Create(ctx, cephBlockPool))

// get the storage class request config for block pool
req := pb.StorageClaimConfigRequest{
StorageConsumerUUID: string(consumerResource.UID),
Expand All @@ -1065,6 +1134,10 @@ func TestOCSProviderServerGetStorageClaimConfig(t *testing.T) {
name = fmt.Sprintf("%s-storageclass", name)
} else if extResource.Kind == "VolumeGroupSnapshotClass" {
name = fmt.Sprintf("%s-volumegroupsnapshotclass", name)
} else if extResource.Kind == "VolumeReplicationClass" {
name = fmt.Sprintf("%s-volumereplicationclass", name)
} else if extResource.Kind == "VolumeGroupReplicationClass" {
name = fmt.Sprintf("%s-volumegroupreplicationclass", name)
} else if extResource.Kind == "ClientProfile" {
name = fmt.Sprintf("%s-clientprofile", name)
}
Expand All @@ -1074,6 +1147,9 @@ func TestOCSProviderServerGetStorageClaimConfig(t *testing.T) {
data, err := json.Marshal(mockResoruce.Data)
assert.NoError(t, err)
assert.Equal(t, string(extResource.Data), string(data))
assert.Equal(t, extResource.Labels, mockResoruce.Labels)
assert.True(t, reflect.DeepEqual(extResource.Labels, mockResoruce.Labels))
assert.True(t, reflect.DeepEqual(extResource.Annotations, mockResoruce.Annotations))
assert.Equal(t, extResource.Kind, mockResoruce.Kind)
assert.Equal(t, extResource.Name, mockResoruce.Name)
}
Expand All @@ -1096,6 +1172,10 @@ func TestOCSProviderServerGetStorageClaimConfig(t *testing.T) {
name = fmt.Sprintf("%s-storageclass", name)
} else if extResource.Kind == "VolumeGroupSnapshotClass" {
name = fmt.Sprintf("%s-volumegroupsnapshotclass", name)
} else if extResource.Kind == "VolumeReplicationClass" {
name = fmt.Sprintf("%s-volumereplicationclass", name)
} else if extResource.Kind == "VolumeGroupReplicationClass" {
name = fmt.Sprintf("%s-volumegroupreplicationclass", name)
Comment on lines +1175 to +1178
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as we are not creating these resources for filesystem, we don't need them here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not dependent on whether it's for FS or not

} else if extResource.Kind == "ClientProfile" {
name = fmt.Sprintf("%s-clientprofile", name)
}
Expand All @@ -1104,6 +1184,8 @@ func TestOCSProviderServerGetStorageClaimConfig(t *testing.T) {
data, err := json.Marshal(mockResoruce.Data)
assert.NoError(t, err)
assert.Equal(t, string(extResource.Data), string(data))
assert.True(t, reflect.DeepEqual(extResource.Labels, mockResoruce.Labels))
assert.True(t, reflect.DeepEqual(extResource.Annotations, mockResoruce.Annotations))
assert.Equal(t, extResource.Kind, mockResoruce.Kind)
assert.Equal(t, extResource.Name, mockResoruce.Name)
}
Expand Down
Loading