storagecluster: update ConfigMap reference for initCephFn #3614
Conversation
aruniiird
left a comment
aruniiird
left a comment
There was a problem hiding this comment.
Now for the new configmap, ceph-csi-config, we may have to add RBAC permissions.
Please see the file controllers/storagecluster/exporter.go
PS: you may have to search for the old configmap name and replace it with the new one.
| } | ||
|
|
||
| if len(clusterConfig.Monitors) == 0 { | ||
| if len(clusterConfigs[0].Monitors) == 0 { |
There was a problem hiding this comment.
Just for my understanding,
we only check for Monitors in the first clusterConfig, right, and we don't want to check other configs in the list.?
There was a problem hiding this comment.
Yes, All the configs would have the monitors and hence we would check for the first one.
There was a problem hiding this comment.
in a cluster with internal and external mode configured, doesn't the Monitors change? maybe that was the reason there was a mention of Namespace in the original code?
There was a problem hiding this comment.
The configmap is still being retrieved based on the CephAuthNamespace. In the whole repo, the namespace stored as a part of the input struct was not being retrieved/utilised anywhere.
There was a problem hiding this comment.
if clusterConfigs[idx].Namespace == cephClusterNamespace {
clusterConfig = clusterConfigs[idx]
break
}I see it's being used in removed code?
| } | ||
|
|
||
| data, ok := configmap.Data["csi-cluster-config-json"] | ||
| data, ok := configmap.Data["config.json"] |
There was a problem hiding this comment.
For the new config map, ceph-csi-config, just make sure that this data-key, config.json, is correct?
There was a problem hiding this comment.
Yes that is the key
|
@aruniiird: changing LGTM is restricted to collaborators DetailsIn response to this:
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. |
|
@ShravaniVangur , |
The configmap rook-ceph-csi-config is no longer available for reference. Updating the reference to ceph-csi-config to retrieve mons for initCephFn. Signed-off-by: ShravaniVangur <shravanivangur@gmail.com>
ShravaniVangur
force-pushed
the
update-initCephFn
branch
from
b061281 to
3f0f541
Compare
|
@weirdwiz: changing LGTM is restricted to collaborators DetailsIn response to this:
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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ShravaniVangur, weirdwiz The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
If you are only interested in Monitors why not read from CephConnection CR? In external mode rook creates it directly and in internal mode ocs-op generates it & client-op creates it. |
@leelavg Although the CephConnection is the right source to read the monitor ips, it cannot be fetched through the current kubeclient. If that has to be retrieved, then dyamic clients might have to be introduced. |
5 participants
This PR updates the metrics logic to reference the
ceph-csi-configConfigMap instead of the deprecatedrook-ceph-csi-config. Metrics previously used the old ConfigMap to retrieve monitor endpoints for executing Ceph commands. Since that ConfigMap is no longer available, switching toceph-csi-configensures the metrics can correctly obtain the mons and execute Ceph commands as expected.