Repository navigation
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars.
 |
|
In order to test this in a dev environment, follow the instruction on the README. Precisely:
|
7b42c43 to
29ad362
Compare
|
Build failed (check pipeline). Post ✔️ s2i-openstack-container-content-provider SUCCESS in 5h 34m 29s |
csibbitt
left a comment
csibbitt
left a comment
There was a problem hiding this comment.
A couple minor issues, and I don't feel like I'm 100% solid enough on all the operator conventions to be the only Approver, but in general this LGTM, thanks!
a617368 to
082588c
Compare
|
Build failed (check pipeline). Post ✔️ s2i-openstack-container-content-provider SUCCESS in 8h 10m 33s |
|
Build failed (check pipeline). Post ❌ s2i-openstack-container-content-provider FAILURE in 4m 18s |
csibbitt
left a comment
csibbitt
left a comment
There was a problem hiding this comment.
Just commenting to indicate I'm up to date with the current version and I think it's good. Thank you for the changes!
I'm going to go ahead and write the integration and content for the rest of the dashboards and target it to your branch, as we discussed the other day.
e587589 to
b17f801
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: vkmc 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 |
|
Build failed (check pipeline). Post ✔️ s2i-openstack-container-content-provider SUCCESS in 4h 33m 14s |
Replace the ConfigMap-based dashboard delivery (dashboards stored as ConfigMaps in the shared openshift-config-managed namespace plus a managed UIPlugin) with perses.dev PersesDatasource and PersesDashboard objects created in the openstack namespace and owned by the MetricStorage CR. The MetricStorage controller reconciles a PersesDatasource pointing at the MetricStorage Prometheus and a PersesDashboard for each implemented, non-disabled dashboard, removes the legacy dashboard ConfigMaps on upgrade, and lets administrators disable individual dashboards by name via MetricStorage.spec.disabledDashboards. UIPlugin management is removed from the operator. The Perses backend (the Perses instance and its UIPlugin) is managed outside the telemetry-operator. The perses-operator's attachment state is surfaced on the dashboard conditions' messages (and logged on change) but does not gate MetricStorage/Telemetry readiness, since the backend is an external prerequisite the operator does not own. Only the openstack-cloud dashboard is implemented, as a minimal placeholder; conversion of the remaining Grafana dashboards is delivered separately. Related-To: OSPRH-37817 Signed-off-by: Victoria Martinez de la Cruz <victoria@redhat.com> Assisted-By: Claude Opus 4.8 <noreply@anthropic.com>
- Make dashboards.PersesDashboards() the single source of truth for available T-O dashboards - Iterate the registry in a stable (sorted) order so the reported unattached dashboard, and the derived condition message and log, are deterministic regardless of Go map iteration order. - Move the persesResourceAvailable helper into api/v1beta1/conditions.go as the exported PersesResourceAvailable, and relocate its unit test. - Restructure the metricstorage kuttl step 07 to assert the dashboards that should exist instead of asserting the absence of a dashboard no code path creates Related-To: OSPRH-37817 Signed-off-by: Victoria Martinez de la Cruz <victoria@redhat.com> Assisted-By: Claude Opus 4.8 <noreply@anthropic.com>
When MetricStorage.spec.prometheusTls is enabled, the Prometheus
datasource is served over HTTPS, but Perses-proxied queries failed with
"x509: certificate signed by unknown authority".
The perses-operator creates a Perses Secret ("<datasource>-secret") from
the datasource's client.tls config, but does not wire it into the prox.
The builder set client.tls (so the CA secret was created) yet never referenced
it from the proxy, so Perses queried Prometheus without the CA.
Set the datasource proxy's secret to that Perses Secret when Prometheus
TLS is enabled, so Perses verifies the Prometheus serving certificate.
Related-To: OSPRH-37817
Signed-off-by: Victoria Martinez de la Cruz <victoria@redhat.com>
Assisted-By: Claude Opus 4.8 <noreply@anthropic.com>
The DisabledDashboards field doc previously listed example dashboard names (openstack-cloud, openstack-node, openstack-vm). A later commit replaced them with generic wording. Restore the examples, which are more helpful, per review feedback. Regenerate the CRD manifests accordingly. Related-To: OSPRH-37817 Signed-off-by: Victoria Martinez de la Cruz <victoria@redhat.com> Assisted-By: Claude Opus 4.8 <noreply@anthropic.com>
Keep dashboard definitions as perses.dev/v1alpha2 PersesDashboard JSON, the format emitted by the Grafana-to-Perses conversion tooling and exported by the Perses UI, while still creating perses.dev/v1alpha1 objects, the API served by the supported perses-operator v0.2.0. - Add dashboardFromJSON, which adapts a v1alpha2 definition to v1alpha1 by moving spec.config into the v1alpha1 inline spec. Definitions with no spec.config section are rejected instead of yielding an empty dashboard. - Builders now return an error, which the controller surfaces through the CreateOrPatch mutate function. - Store the openstack-cloud placeholder dashboard as JSON. - Add unit tests for the registry builders and the adapter. Related-To: OSPRH-37817 Assisted-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Victoria Martinez de la Cruz <victoria@redhat.com>
c2bc2f3 to
0623747
Compare
Restore the behaviour of the legacy ConfigMap delivery, where the ceilometer-ipmi dashboard is only created when at least one dataplane nodeset runs the telemetry-power-monitoring service. - Add a placeholder openstack-ceilometer-ipmi PersesDashboard, stored as v1alpha2 JSON like openstack-cloud. Its content is delivered with the rest of the dashboard conversion. - Treat the dashboard as disabled when power monitoring is not deployed, so it is skipped, and removed if power monitoring is later dropped. - Extend the metricstorage kuttl test: the ipmi dashboard is absent before the power monitoring EDPM resources are deployed (01) and when dashboards are disabled (02), present once they are (07), and kept when another dashboard is disabled by name (08). Related-To: OSPRH-37817 Assisted-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Victoria Martinez de la Cruz <victoria@redhat.com>
- Return a wrapped static ErrDashboardNoSpecConfig error instead of a dynamic one when a dashboard JSON definition has no spec.config section (err113). - Use a named gomega import in the dashboards tests (revive dot-imports). Related-To: OSPRH-37817 Assisted-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Victoria Martinez de la Cruz <victoria@redhat.com>
The PersesDatasource validated the Prometheus serving certificate against a hard-coded combined-ca-bundle secret, while the MonitoringStack uses spec.prometheusTls.caBundleSecretName. With a differently named CA bundle secret, the perses-operator cannot find the CA and dashboard queries to Prometheus fail. Use the same CA bundle secret and key as the MonitoringStack, falling back to the ca.crt in the Prometheus serving certificate secret when no CA bundle secret is configured. Add unit tests for the datasource. Related-To: OSPRH-37817 Assisted-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Victoria Martinez de la Cruz <victoria@redhat.com>
The Perses objects use generic names (e.g. the "prometheus" datasource) in the MetricStorage namespace. The disable paths deleted them by name regardless of who created them, so an administrator's own PersesDatasource named "prometheus" was deleted on every reconcile while dashboards were disabled, and the create path silently took over and overwrote it. - Add EnsureControlledDeleted, which only deletes a Perses object controlled by the MetricStorage, and use it when dashboards are disabled, when a dashboard is disabled by name and on CR deletion. - Add EnsureAdoptable and call it from the PersesDatasource and PersesDashboard mutate functions, so an existing object not controlled by the MetricStorage is reported as an error instead of adopted. - Add unit tests for both helpers. Related-To: OSPRH-37817 Assisted-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Victoria Martinez de la Cruz <victoria@redhat.com>
The legacy ConfigMap-based dashboards live in openshift-config-managed with no owner references, and were only removed from reconcileNormal, after several steps that can return early. If the MetricStorage was deleted after an upgrade before reconcileNormal got that far, the ConfigMaps were orphaned. Remove them from reconcileDelete as well. Related-To: OSPRH-37817 Assisted-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Victoria Martinez de la Cruz <victoria@redhat.com>
The kuttl assertions that the legacy ConfigMap-based dashboards are absent passed vacuously, since nothing created them and the operator no longer does. Pre-create all the legacy ConfigMaps in step 00, before the MetricStorage is deployed, to simulate an upgrade, and assert in 01-errors that the operator removes them. Drop the redundant ConfigMap assertions from 02-errors and 07-errors. Related-To: OSPRH-37817 Assisted-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Victoria Martinez de la Cruz <victoria@redhat.com>
Any error while inspecting the dataplane nodesets for the telemetry-power-monitoring service, e.g. a transient list failure or one host without a resolvable address, marked the ceilometer-ipmi dashboard as disabled, deleting it until the next successful reconcile. The legacy delivery only skipped creating it. Skip reconciling the dashboard when the nodesets cannot be inspected, leaving any existing object untouched, and only remove it when no nodeset runs power monitoring. Related-To: OSPRH-37817 Assisted-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Victoria Martinez de la Cruz <victoria@redhat.com>
When a PersesDatasource or PersesDashboard with the same name already exists and is not controlled by the MetricStorage, the error was returned from createDashboardObjects, so reconcileNormal stopped before the TLS input validation, network attachments and Prometheus readiness, and kept retrying with backoff. Report the conflict on the dashboard conditions and keep reconciling instead. On a datasource conflict the dashboards, which would reference the foreign datasource, are skipped; on a dashboard conflict only that dashboard is skipped. Also fix markDashboardObjectReady logging the Perses attachment state on every reconcile: it compared against the current conditions, which Init resets at the start of each reconcile. Compare against the conditions saved before Init instead. Related-To: OSPRH-37817 Assisted-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Victoria Martinez de la Cruz <victoria@redhat.com>
The PersesDatasource URL hardcoded the metric-storage-prometheus Service, while the Prometheus Service is named after the MetricStorage instance (<name>-prometheus). With a MetricStorage not named metric-storage, e.g. in the kuttl tests, the dashboards queried a Service that does not exist. Build the URL from the instance name, which also matches the subject name requested for the Prometheus serving certificate. Related-To: OSPRH-37817 Assisted-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Victoria Martinez de la Cruz <victoria@redhat.com>
When an existing Perses object with a managed name is not controlled by the MetricStorage, the reported error now tells the administrator to delete or rename it so the operator can manage it. Related-To: OSPRH-37817 Assisted-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Victoria Martinez de la Cruz <victoria@redhat.com>
The metricstorage kuttl test only asserted the Perses objects exist, so it would still pass if the operator stopped setting the controller reference that their garbage collection and the ownership-aware deletion rely on. Assert the PersesDatasource and the openstack-cloud and ceilometer-ipmi PersesDashboards are controlled by the MetricStorage. Related-To: OSPRH-37817 Assisted-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Victoria Martinez de la Cruz <victoria@redhat.com>
|
Build failed (check pipeline). Post ✔️ s2i-openstack-container-content-provider SUCCESS in 8h 10m 07s Warning: |
|
Build failed (check pipeline). Post ✔️ s2i-openstack-container-content-provider SUCCESS in 3h 10m 05s Warning: |
Replace the ConfigMap-based dashboard delivery with perses.dev PersesDatasource and PersesDashboard objects in the openstack namespace and owned by the MetricStorage CR.
The MetricStorage controller reconciles a PersesDatasource pointing at the MetricStorage Prometheus and a PersesDashboard for each implemented, non-disabled dashboard, removes the legacy dashboard ConfigMaps on upgrade, and lets administrators disable individual dashboards by name via MetricStorage.spec.disabledDashboards. UIPlugin management is removed from the operator.
The Perses backend (the Perses instance and its UIPlugin) is managed outside the telemetry-operator. The perses-operator's attachment state is surfaced on the dashboard conditions' messages (and logged on change) but does not gate MetricStorage/Telemetry readiness, since the backend is an external prerequisite the operator does not own.
Dashboard definitions are stored as perses.dev/v1alpha2 PersesDashboard JSON. The objects are created as perses.dev/v1alpha1, the API served by the supported perses-operator v0.2.0: a small adapter moves the v1alpha2 spec.config content into the v1alpha1 inline spec, so no Perses dependency bump is needed.
Only the openstack-cloud and openstack-ceilometer-ipmi dashboards are implemented, as minimal placeholders; conversion of the Grafana dashboards content is delivered separately. As with the legacy delivery, the ceilometer-ipmi dashboard is only created when at least one dataplane nodeset runs the telemetry-power-monitoring service.
Related-To: OSPRH-37817