provider-server: enable csi drivers for ocs-client-op #3195
Conversation
rohan47
force-pushed
the
drivers
branch
2 times, most recently
from
388a43b to
6b18f94
Compare
|
/retest-required |
| @@ -118,13 +118,30 @@ message StorageConfigResponse{ | |||
| SystemAttributes systemAttributes = 3; | |||
| } | |||
|
|
|||
| message RbdDriverRequirements { | |||
| bool generateOmapInfo = 1; | |||
There was a problem hiding this comment.
You are surfacing requirements so it does not make sense to have something named as a verb (generate)
| bool generateOmapInfo = 1; | |
| bool omapInfo = 1; |
| } | ||
|
|
||
| message CsiDrivers { | ||
| RbdDriverRequirements DesiredRbdDriver = 1; |
There was a problem hiding this comment.
Let make sure we are naming the field in accordance to our terminology. As we decided to call these "requirements" I would argue the name of the field should be RbdDriverRequirments
| // GetDesiredClientStateResponse holds the response for the GetDesiredClientState API request | ||
| message GetDesiredClientStateResponse{ | ||
| repeated bytes kubeResources = 1; | ||
| string desiredStateHash = 2; | ||
| string clientOperatorChannel = 3; | ||
| bool maintenanceMode = 4; | ||
| bool mirrorEnabled = 5; | ||
| CsiDrivers CsiDrivers = 6; |
There was a problem hiding this comment.
Why do we need a substruct here? Can we just have the requirement sections on the root struct?
| desiredClientConfigHash := getDesiredClientConfigHash( | ||
| channelName, | ||
| storageConsumer, | ||
| isEncryptionInTransitEnabled(storageCluster.Spec.Network), | ||
| inMaintenanceMode, | ||
| isConsumerMirrorEnabled, | ||
| isNfsDriverEnabled, |
There was a problem hiding this comment.
Why only NFS? what about the other drivers?
services/provider/server/server.go
Outdated
| response.CsiDrivers.DesiredRbdDriver = &pb.RbdDriverRequirements{} | ||
| response.CsiDrivers.DesiredRbdDriver.GenerateOmapInfo = isConsumerMirrorEnabled | ||
| response.CsiDrivers.DesiredCephFsDriver = &pb.CephFsDriverRequirements{} |
There was a problem hiding this comment.
Why are we assuming these are always enabled?
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: rohan47 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 |
Signed-off-by: Rohan Gupta <[email protected]>
| @@ -118,13 +118,26 @@ message StorageConfigResponse{ | |||
| SystemAttributes systemAttributes = 3; | |||
| } | |||
|
|
|||
| message RbdDriverRequirements { | |||
| bool omapInfo = 1; | |||
There was a problem hiding this comment.
@nb-ohad, do we want to move this to operatorConfig Spec instead of Driver Spec and always deploy it?
openshift-merge-robot
added
the
needs-rebase
|
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Send information of desired csi drivers to enable to ocs-client-operator