From c2ec2d2474fa0482cc558d6b6d3a2462f0506f1e Mon Sep 17 00:00:00 2001 From: Arun Kumar Mohan Date: Wed, 13 Dec 2023 19:53:03 +0530 Subject: [PATCH 1/4] Imroved error message for ceph blockpool metric collection failure Added the received image health status message from the cluster. Helps user in debugging. Signed-off-by: Arun Kumar Mohan --- metrics/internal/collectors/ceph-block-pool.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/metrics/internal/collectors/ceph-block-pool.go b/metrics/internal/collectors/ceph-block-pool.go index 1c7cf9bdf2..83899bc739 100644 --- a/metrics/internal/collectors/ceph-block-pool.go +++ b/metrics/internal/collectors/ceph-block-pool.go @@ -133,7 +133,7 @@ func (c *CephBlockPoolCollector) collectMirroringImageHealth(cephBlockPools []*c cephBlockPool.Name, cephBlockPool.Namespace) default: - klog.Errorf("Invalid image health for pool %s. Must be OK, UNKNOWN, WARNING or ERROR", cephBlockPool.Name) + klog.Errorf("Invalid image health, %q, for pool %s. Must be OK, UNKNOWN, WARNING or ERROR.", imageHealth, cephBlockPool.Name) } } } From c739804b082b4ec3201408f8b9353beb0eb97a0b Mon Sep 17 00:00:00 2001 From: Arun Kumar Mohan Date: Wed, 13 Dec 2023 17:43:32 +0530 Subject: [PATCH 2/4] Fixed a minor resource permission issue with exporter Remove an accidental role permissions added to prometheus. Add the same (missing) permissions to metrics exporter. This resolves the following error messages in metrics exporter, persistentvolumeclaims "db-noobaa-db-pg-0" is forbidden: User "system:serviceaccount:openshift-storage:ocs-metrics-exporter" cannot get resource "persistentvolumeclaims" in API group "" in the namespace "openshift-storage" pods is forbidden: User "system:serviceaccount:openshift-storage:ocs-metrics-exporter" cannot list resource "pods" in API group "" in the namespace "openshift-storage" Signed-off-by: Arun Kumar Mohan --- controllers/storagecluster/exporter.go | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/controllers/storagecluster/exporter.go b/controllers/storagecluster/exporter.go index 5850c6b8a2..695c2c5bd5 100644 --- a/controllers/storagecluster/exporter.go +++ b/controllers/storagecluster/exporter.go @@ -579,11 +579,6 @@ const expectedPrometheusK8RoleJSON = ` "apiGroups":[""], "resources":["services","endpoints","pods"], "verbs":["get","list","watch"] - }, - { - "apiGroups":[""], - "resources":["persistentvolumeclaims","pods","configmaps","secrets"], - "verbs":["get","list","watch"] } ] } @@ -597,7 +592,7 @@ const expectedMetricExporterRoleJSON = ` "rules":[ { "apiGroups":[""], - "resources":["secrets","configmaps"], + "resources":["secrets","configmaps","persistentvolumeclaims","pods"], "verbs":["get","list","watch"] }, { From 10f1b17b5f28212a2fcdabf9a6cf13a2338a7217 Mon Sep 17 00:00:00 2001 From: Arun Kumar Mohan Date: Thu, 14 Dec 2023 10:24:50 +0530 Subject: [PATCH 3/4] A minor refactor of exporter code A small optization to exporter role and rolebinding functions to remove duplicate codes. Signed-off-by: Arun Kumar Mohan --- controllers/storagecluster/exporter.go | 235 ++++++++++++------------- 1 file changed, 110 insertions(+), 125 deletions(-) diff --git a/controllers/storagecluster/exporter.go b/controllers/storagecluster/exporter.go index 695c2c5bd5..46173eb932 100644 --- a/controllers/storagecluster/exporter.go +++ b/controllers/storagecluster/exporter.go @@ -21,7 +21,8 @@ import ( const ( metricsExporterName = "ocs-metrics-exporter" - metricsExporterRoleName = "ocs-metrics-svc" + prometheusRoleName = "ocs-metrics-svc" + metricsExporterRoleName = metricsExporterName portMetrics = "metrics" portExporter = "exporter" metricsPath = "/metrics" @@ -621,61 +622,45 @@ const expectedMetricExporterRoleJSON = ` func createMetricsExporterRoles(ctx context.Context, r *StorageClusterReconciler, instance *ocsv1.StorageCluster) error { - // create/update prometheus server needed roles - var expectedRole = new(rbacv1.Role) - err := json.Unmarshal([]byte(expectedPrometheusK8RoleJSON), expectedRole) - if err != nil { - r.Log.Error(err, "an unexpected error occurred while unmarshalling prometheus role") - return err - } - currentRole := new(rbacv1.Role) - expectedRole.Name, currentRole.Name = metricsExporterRoleName, metricsExporterRoleName - expectedRole.Namespace, currentRole.Namespace = instance.Namespace, instance.Namespace - _, err = controllerutil.CreateOrUpdate(ctx, r.Client, currentRole, func() error { - expectedRulesLen := len(expectedRole.Rules) - if len(currentRole.Rules) != expectedRulesLen { - currentRole.Rules = make([]rbacv1.PolicyRule, expectedRulesLen) + var jsonRoles = map[string]string{ + prometheusRoleName: expectedPrometheusK8RoleJSON, + metricsExporterRoleName: expectedMetricExporterRoleJSON, + } + + for roleName, jsonRole := range jsonRoles { + // create expected roles + var expectedRole = new(rbacv1.Role) + err := json.Unmarshal([]byte(jsonRole), expectedRole) + if err != nil { + r.Log.Error(err, + "an unexpected error occurred while unmarshalling following JSON role", + "JSONRoleName", roleName, + "JSONRole", jsonRole) + return err } - copy(currentRole.Rules, expectedRole.Rules) - currentRole.OwnerReferences = []metav1.OwnerReference{{ - APIVersion: instance.APIVersion, - Kind: instance.Kind, - Name: instance.Name, - UID: instance.UID, - }} - return nil - }) - if err != nil { - r.Log.Error(err, "failed to create/update prometheus roles") - return err - } - - // create/update metrics exporter roles - expectedRole = new(rbacv1.Role) - err = json.Unmarshal([]byte(expectedMetricExporterRoleJSON), expectedRole) - if err != nil { - r.Log.Error(err, "an unexpected error occurred while unmarshalling metrics exporter role") - return err - } - currentRole = new(rbacv1.Role) - expectedRole.Name, currentRole.Name = metricsExporterName, metricsExporterName - expectedRole.Namespace, currentRole.Namespace = instance.Namespace, instance.Namespace - _, err = controllerutil.CreateOrUpdate(ctx, r.Client, currentRole, func() error { - expectedRulesLen := len(expectedRole.Rules) - if len(currentRole.Rules) != expectedRulesLen { - currentRole.Rules = make([]rbacv1.PolicyRule, expectedRulesLen) + currentRole := new(rbacv1.Role) + expectedRole.Name, currentRole.Name = roleName, roleName + expectedRole.Namespace, currentRole.Namespace = instance.Namespace, instance.Namespace + _, err = controllerutil.CreateOrUpdate(ctx, r.Client, currentRole, func() error { + expectedRulesLen := len(expectedRole.Rules) + if len(currentRole.Rules) != expectedRulesLen { + currentRole.Rules = make([]rbacv1.PolicyRule, expectedRulesLen) + } + copy(currentRole.Rules, expectedRole.Rules) + currentRole.OwnerReferences = []metav1.OwnerReference{{ + APIVersion: instance.APIVersion, + Kind: instance.Kind, + Name: instance.Name, + UID: instance.UID, + }} + return nil + }) + if err != nil { + r.Log.Error(err, "failed to create/update exporter role", "RoleName", roleName) + return err } - copy(currentRole.Rules, expectedRole.Rules) - currentRole.OwnerReferences = []metav1.OwnerReference{{ - APIVersion: instance.APIVersion, - Kind: instance.Kind, - Name: instance.Name, - UID: instance.UID, - }} - return nil - }) - - return err + } + return nil } const expectedPrometheusK8RoleBindingJSON = ` @@ -687,14 +672,7 @@ const expectedPrometheusK8RoleBindingJSON = ` "apiGroup":"rbac.authorization.k8s.io", "kind":"Role", "name":"ocs-metrics-svc" - }, - "subjects":[ - { - "kind":"ServiceAccount", - "name":"prometheus-k8s", - "namespace":"openshift-monitoring" - } - ] + } }` // expectedMetricsExporterRoleBindingJSON rolebindings for metrics exporter @@ -713,73 +691,80 @@ const expectedMetricsExporterRoleBindingJSON = ` func createMetricsExporterRolebindings(ctx context.Context, r *StorageClusterReconciler, instance *ocsv1.StorageCluster) error { - // rolebinding for prometheus - var expectedRoleBinding = new(rbacv1.RoleBinding) - err := json.Unmarshal([]byte(expectedPrometheusK8RoleBindingJSON), expectedRoleBinding) - if err != nil { - r.Log.Error(err, - "an unexpected error occurred while unmarshalling prometheus rolebinding") - return err + var roleBindings = []string{ + expectedPrometheusK8RoleBindingJSON, expectedMetricsExporterRoleBindingJSON, } - currentRoleBinding := new(rbacv1.RoleBinding) - expectedRoleBinding.Name, currentRoleBinding.Name = metricsExporterRoleName, metricsExporterRoleName - expectedRoleBinding.Namespace, currentRoleBinding.Namespace = instance.Namespace, instance.Namespace - _, err = controllerutil.CreateOrUpdate(ctx, r.Client, currentRoleBinding, func() error { - currentRoleBinding.RoleRef.APIGroup = expectedRoleBinding.RoleRef.APIGroup - currentRoleBinding.RoleRef.Kind = expectedRoleBinding.RoleRef.Kind - currentRoleBinding.RoleRef.Name = expectedRoleBinding.RoleRef.Name - expectedSubjectsLen := len(expectedRoleBinding.Subjects) - if len(currentRoleBinding.Subjects) != expectedSubjectsLen { - currentRoleBinding.Subjects = make([]rbacv1.Subject, expectedSubjectsLen) - } - copy(currentRoleBinding.Subjects, expectedRoleBinding.Subjects) - currentRoleBinding.OwnerReferences = []metav1.OwnerReference{{ - APIVersion: instance.APIVersion, - Kind: instance.Kind, - Name: instance.Name, - UID: instance.UID, - }} - return nil - }) - if err != nil { - r.Log.Error(err, "error while create/update prometheus rolebindings") - return err + var roleBindingNames = []string{ + // rolebindings have the same names as the roles + prometheusRoleName, metricsExporterRoleName, } - - // rolebinding for metrics exporter - expectedRoleBinding = new(rbacv1.RoleBinding) - err = json.Unmarshal([]byte(expectedMetricsExporterRoleBindingJSON), expectedRoleBinding) - if err != nil { - r.Log.Error(err, - "an unexpected error occurred while unmarshalling metrics exporter rolebinding") - return err + var roleBindingSubjects = []rbacv1.Subject{ + // subject for prometheus-k8 rolebinding + { + Kind: "ServiceAccount", + Name: "prometheus-k8s", + Namespace: "openshift-monitoring", + }, + // subject for metrics exporter rolebinding + { + Kind: "ServiceAccount", + Name: metricsExporterName, + Namespace: instance.Namespace, + }, } - currentRoleBinding = new(rbacv1.RoleBinding) - expectedRoleBinding.Name, currentRoleBinding.Name = metricsExporterName, metricsExporterName - expectedRoleBinding.Namespace, currentRoleBinding.Namespace = instance.Namespace, instance.Namespace - - expectedRoleBinding.Subjects = make([]rbacv1.Subject, 1) - expectedRoleBinding.Subjects[0].Kind = "ServiceAccount" - expectedRoleBinding.Subjects[0].Name = metricsExporterName - expectedRoleBinding.Subjects[0].Namespace = instance.Namespace - - _, err = controllerutil.CreateOrUpdate(ctx, r.Client, currentRoleBinding, func() error { - currentRoleBinding.RoleRef.APIGroup = expectedRoleBinding.RoleRef.APIGroup - currentRoleBinding.RoleRef.Kind = expectedRoleBinding.RoleRef.Kind - currentRoleBinding.RoleRef.Name = expectedRoleBinding.RoleRef.Name - expectedSubjectsLen := len(expectedRoleBinding.Subjects) - if len(currentRoleBinding.Subjects) != expectedSubjectsLen { + + for rbIndx, roleBinding := range roleBindings { + var roleBindingName = roleBindingNames[rbIndx] + var roleBindingSubject = roleBindingSubjects[rbIndx] + + var expectedRoleBinding = new(rbacv1.RoleBinding) + err := json.Unmarshal([]byte(roleBinding), expectedRoleBinding) + if err != nil { + r.Log.Error(err, + "an unexpected error occurred while unmarshalling rolebinding", + "RoleBindingName", roleBindingName, + "RoleBindingJSON", roleBinding) + return err + } + currentRoleBinding := new(rbacv1.RoleBinding) + // name + expectedRoleBinding.Name = roleBindingName + currentRoleBinding.Name = roleBindingName + // namespace + expectedRoleBinding.Namespace = instance.Namespace + currentRoleBinding.Namespace = instance.Namespace + // expected role reference name + // PS: we use the same name for both roles and rolebindings + expectedRoleBinding.RoleRef.Name = roleBindingName + // expected subjects + expectedRoleBinding.Subjects = []rbacv1.Subject{roleBindingSubject} + + _, err = controllerutil.CreateOrUpdate(ctx, r.Client, currentRoleBinding, func() error { + // add expected role reference + currentRoleBinding.RoleRef = expectedRoleBinding.RoleRef + + // add expected subjects + expectedSubjectsLen := len(expectedRoleBinding.Subjects) currentRoleBinding.Subjects = make([]rbacv1.Subject, expectedSubjectsLen) + copy(currentRoleBinding.Subjects, expectedRoleBinding.Subjects) + + // add owner references + currentRoleBinding.OwnerReferences = []metav1.OwnerReference{{ + APIVersion: instance.APIVersion, + Kind: instance.Kind, + Name: instance.Name, + UID: instance.UID, + }} + + return nil + }) + if err != nil { + r.Log.Error(err, + "error while create/update metrics exporter rolebinding", + "RoleBindingName", roleBindingName) + return err } - copy(currentRoleBinding.Subjects, expectedRoleBinding.Subjects) - currentRoleBinding.OwnerReferences = []metav1.OwnerReference{{ - APIVersion: instance.APIVersion, - Kind: instance.Kind, - Name: instance.Name, - UID: instance.UID, - }} - return nil - }) + } - return err + return nil } From a63c97a234d8031425621044994f9bdfd2b93be6 Mon Sep 17 00:00:00 2001 From: Arun Kumar Mohan Date: Thu, 14 Dec 2023 12:32:13 +0530 Subject: [PATCH 4/4] Expect either PV or it's pointer type as arg to PVStore 'Add()' method We were getting following error message while adding a PV to PVStore as we were expecting only pointer to PV as arg to PVStore Add() method. err: unexpected object of type v1.PersistentVolume Signed-off-by: Arun Kumar Mohan --- metrics/internal/cache/pv.go | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/metrics/internal/cache/pv.go b/metrics/internal/cache/pv.go index fe25d42cee..e5d8e91e17 100644 --- a/metrics/internal/cache/pv.go +++ b/metrics/internal/cache/pv.go @@ -94,8 +94,14 @@ func appendIfNotExists(slice []string, value string) []string { // Add inserts to the PersistentVolumeStore. func (p *PersistentVolumeStore) Add(obj interface{}) error { - pv, ok := obj.(*corev1.PersistentVolume) - if !ok { + var pv *corev1.PersistentVolume + // obj can be of PV type or a pointer to it + switch pvFromObj := obj.(type) { + case *corev1.PersistentVolume: + pv = pvFromObj + case corev1.PersistentVolume: + pv = &pvFromObj + default: return fmt.Errorf("unexpected object of type %T", obj) }