Add ClusterDeploymentLabelSelector support #2502
Conversation
This commit adds the labelSelector customization for some metrics. This feature can be opted into via HiveConfig, and the metrics, for which this customization is configured, will only be reported if the label query matches the labels on the related clusterdeployment. To get around cyclic import cycle, the definition and registration of all supported metrics from clusterDeployment, clusterProvision and clusterDeprovision controller have been moved to metrics controller.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: suhanime The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
| // Read the metricsConfig section and fetch clusterDeploymentLabelSelectors, error out if it fails validation | ||
| err = controllerMetrics.GetClusterDeploymentLabelSelectors(hLog, origHiveConfig.Spec.MetricsConfig) | ||
| if err != nil { | ||
| hLog.WithError(err).Error("error in metricsConfig") | ||
| instance.Status.Conditions = util.SetHiveConfigCondition(instance.Status.Conditions, hivev1.HiveReadyCondition, corev1.ConditionFalse, "ErrorInMetricsConfig", err.Error()) | ||
| r.updateHiveConfigStatus(origHiveConfig, instance, hLog, false) | ||
| return reconcile.Result{}, err | ||
| } | ||
|
|
There was a problem hiding this comment.
Is this location/incorporation correct? My testing hasn't yielded fruitful results - happy to move this to metrics controller and panic if error, and tackle this feature later if time is an issue
| err := cc.client.List(context.Background(), clusterDeployments) | ||
| if err != nil { | ||
| log.WithError(err).Error("error listing cluster deployments") | ||
| ccLog.WithError(err).Error("error listing cluster deployments") |
There was a problem hiding this comment.
We were initializing ccLog but using the one without the field set to metrics controller - corrected this in the entire file
|
|
||
| // Metrics reported by ClusterDeployment controller | ||
|
|
||
| MetricInstallJobDuration = prometheus.NewHistogramVec( |
There was a problem hiding this comment.
You might notice, I changed the type of some of the metrics, especially ones declared as Histogram or HistogramOpts, for consistency.
There was a problem hiding this comment.
Yeah - couldn't think of any other way to avoid the import cycles. Eventual plan is to move all metrics' definitions to metrics/metrics_to_report, and streamline their types, handle registration and encapsulate observation.
openshift-ci
bot
added
the
do-not-merge/hold
|
@suhanime: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
2 participants
Adds the labelSelector customization for some metrics. This feature can be opted into via HiveConfig, and the metrics, for which this customization is configured, will only be reported if the label query matches the labels on the related clusterdeployment.
To get around cyclic import cycle, the definition and registration of all supported metrics from clusterDeployment, clusterProvision and clusterDeprovision controller have been moved to metrics controller.
Hive-2285