provider: enable csi omap generator for client #2874
Conversation
rewantsoni
force-pushed
the
csi-omap
branch
2 times, most recently
from
b747ba5
to
8f2f36f
Compare
openshift-merge-robot
added
the
needs-rebase
rewantsoni
force-pushed
the
csi-omap
branch
2 times, most recently
from
bd2c11d
to
3325edc
Compare
openshift-merge-robot
removed
the
needs-rebase
rewantsoni
force-pushed
the
csi-omap
branch
2 times, most recently
from
62ff5cf
to
6a49ca6
Compare
| @@ -27,6 +27,10 @@ import ( | |||
| v1 "github.com/red-hat-storage/ocs-operator/api/v4/v1" | |||
| ) | |||
|
|
|||
| const ( | |||
| ClientMappingConfigMapName = "client-mapping-config" | |||
There was a problem hiding this comment.
'Client' and 'Mapping' are very generic terms when you consider all of ODF and not just RDR. Please choose a better name for the config map and the var
services/provider/server/server.go
Outdated
| if err != nil { | ||
| return nil, err | ||
| } | ||
| _, hasClientID := clientMappingConfig.Data[req.StorageConsumerUUID] |
There was a problem hiding this comment.
I remember the design was a mapping of client names not consumer uuids
There was a problem hiding this comment.
From provider we are sending the storageConsumerUIDs, and MCO updates this configmap with local ClientID to remote ClientID
services/provider/server/server.go
Outdated
| clientMappingConfig, err := s.getClientMapping(ctx) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
why would we fail this function if there is not storage client id mapping config?
There was a problem hiding this comment.
This function is failing in cases expect isNotFound
services/provider/server/server.go
Outdated
| clientMappingConfig, err := s.getClientMapping(ctx) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
This function is failing in cases expect isNotFound
services/provider/server/server.go
Outdated
| if err != nil { | ||
| return nil, err | ||
| } | ||
| _, hasClientID := clientMappingConfig.Data[string(consumerResource.UID)] |
There was a problem hiding this comment.
This is not good enough, what if the mapping is broken? For example the key exists but not a value
I would suggest doing this instead
| _, hasClientID := clientMappingConfig.Data[string(consumerResource.UID)] | |
| clientIsMapped := clientMappingConfig.Data[string(consumerResource.UID)] != "" |
services/provider/server/server.go
Outdated
| extR = append(extR, &pb.ExternalResource{ | ||
| Name: "ocs-ceph-csi-config", | ||
| Kind: "ConfigMap", | ||
| Data: mustMarshal(map[string]string{ | ||
| "CSI_ENABLE_OMAP_GENERATOR": strconv.FormatBool(hasClientID), | ||
| }), | ||
| }) |
There was a problem hiding this comment.
What is this ocs-ceph-csi-config? I believe that ceph csi operator is using a CR for that
There was a problem hiding this comment.
The CR is created in operator_configmap controller, it is not sent from provider to the client.
The rpc calls are limited to storageClient/storageClaim controller. In order to pass information from storageClient to operator_configmap controller, creating this new configMap. operator_configMap will load this configMap and then set the fields as required
services/provider/server/server.go
Outdated
| @@ -910,6 +941,17 @@ func (s *OCSProviderServer) getOCSSubscriptionChannel(ctx context.Context) (stri | |||
| return subscription.Spec.Channel, nil | |||
| } | |||
|
|
|||
| func (s *OCSProviderServer) getClientMapping(ctx context.Context) (*corev1.ConfigMap, error) { | |||
There was a problem hiding this comment.
What return a config map and not just map[string]string. You are not interested in the resource, just in the mapping.
Also, please rename this func to something more specific
send a configMap from provider to client to enable csi-omap generator required for DR on client cluster Signed-off-by: Rewant Soni <[email protected]>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: rewantsoni The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
send a configMap from provider to client to enable csi-omap generator required for DR on client cluster