ARO-26057: add ADX Grafana datasource provisioning#4878
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds ARO-HCP support to provision and clean up Azure Data Explorer (Kusto) data sources in the shared Managed Grafana workspace, integrated into the existing geography rollout flow.
Changes:
- Extend Kusto IaC to optionally grant Grafana’s managed identity DB-level Viewer access and output the cluster URI.
- Add geography pipeline steps to fetch global Grafana outputs and provision/update ADX Grafana datasources via Azure CLI.
- Introduce configuration flags/schema + docs, and update the Kusto delete cleanup to remove the matching Grafana datasource.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/monitoring.md | Documents ADX/Kusto datasource provisioning gates, naming, validation, and teardown behavior. |
| dev-infrastructure/templates/kusto.bicep | Threads optional Grafana resource ID into the Kusto module and exposes kustoUri output. |
| dev-infrastructure/modules/logs/kusto/main.bicep | Grants Grafana MI Viewer on ServiceLogs when provided and surfaces cluster URI output. |
| dev-infrastructure/modules/logs/kusto/cluster.bicep | Exposes Kusto cluster uri as an output. |
| dev-infrastructure/geography-pipeline.yaml | Adds global output step and a shell step to create/update ADX Grafana datasources. |
| dev-infrastructure/configurations/kusto.tmpl.bicepparam | Adds grafanaResourceId parameter placeholder for pipeline substitution. |
| dev-infrastructure/cleanup/delete.kusto.instance.sh | Extends cleanup to also delete the corresponding Grafana datasource. |
| dev-infrastructure/cleanup/delete.kusto.instance.pipeline.yaml | Wires Grafana resource ID from global outputs into the cleanup script run. |
| config/rendered/dev/swft/uksouth.yaml | Adds default monitoring flags for ADX datasource provisioning (disabled). |
| config/rendered/dev/prow/westus3.yaml | Adds default monitoring flags for ADX datasource provisioning (disabled). |
| config/rendered/dev/pers/westus3.yaml | Adds default monitoring flags for ADX datasource provisioning (disabled). |
| config/rendered/dev/perf/westus3.yaml | Adds default monitoring flags for ADX datasource provisioning (disabled). |
| config/rendered/dev/dev/westus3.yaml | Adds default monitoring flags for ADX datasource provisioning (disabled). |
| config/rendered/dev/cspr/westus3.yaml | Adds default monitoring flags for ADX datasource provisioning (disabled). |
| config/config.yaml | Introduces new monitoring defaults for ADX provisioning flags. |
| config/config.schema.json | Defines schema/validation for the new monitoring configuration knobs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/hold |
|
Thanks for the feedback, I'll move this to On the "update once" concern: the existing |
8d14303 to
a80a076
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/unhold Updated to use grafanactl. |
| go run ../tooling/grafanactl/main.go modify datasource reconcile-adx \ | ||
| --grafana-resource-id "${GRAFANA_RESOURCE_ID}" \ | ||
| --cluster-url "${ADX_CLUSTER_URL}" \ | ||
| --default-database "${ADX_DATABASE}" \ | ||
| --datasource-name "${DATASOURCE_NAME}" \ | ||
| --data-consistency strongconsistency |
There was a problem hiding this comment.
This will not work in EV2, since we need the binary pre-built
There was a problem hiding this comment.
Agreed. This path has been reworked to use the typed GrafanaDatasources action instead of rollout-time go run; the downstream sdp-pipelines PR packages the prebuilt grafanactl binary and emits the modify datasource reconcile --adx-* command. Current ARO-HCP CI is still expected to fail until Azure/ARO-Tools#228 lands and this PR bumps the ARO-Tools pin.
Replace the temporary shell datasource script with a direct grafanactl invocation and keep cleanup datasource deletion out of this rollout scope. Depends on Azure/ARO-Tools#228.
5d069ae to
c091e9c
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: swiencki 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 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…peline Per PR #4878 review. Drops the new GrafanaDatasources call from geography-pipeline.yaml and extends the existing add-grafana-datasource in region-pipeline.yaml with the ADX block. clusterUrl sources from regional kusto-lookup output. AzureMonitor reconcile preserved via the ARO-Tools default-true. Triple-AND gate keeps the three feature flags agreeing by construction. No Go/Bicep/config/test changes.
…peline Per PR #4878 review. Drops the new GrafanaDatasources call from geography-pipeline.yaml and extends the existing add-grafana-datasource in region-pipeline.yaml with the ADX block. clusterUrl sources from regional kusto-lookup output. AzureMonitor reconcile preserved via the ARO-Tools default-true. Triple-AND gate keeps the three feature flags agreeing by construction. No Go/Bicep/config/test changes.
ef9d6e8 to
656403e
Compare
|
@swiencki: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
ARO-26057
What
Adds ADX/Kusto Grafana datasource provisioning through the geography rollout using the typed
GrafanaDatasourcesaction. The step forwards ADX desired state tografanactl modify datasource reconcile, grants the Grafana managed identity Kusto Viewer access to ServiceLogs, and removes the prior rollout-timego runshell path.Why
This follows the existing typed/prebuilt-binary pattern for Grafana datasource rollout while enabling SRE dashboards to query Kusto data from Grafana.
Testing
GOWORK=/tmp/aro-hcp-adx-local.work go test ./pkg/pipelinefromtooling/templatizegit diff --checkSpecial notes for your reviewer
Depends on Azure/ARO-Tools#228. Downstream sdp-pipelines rollout packaging is https://dev.azure.com/msazure/AzureRedHatOpenShift/_git/sdp-pipelines/pullrequest/15393796.
Required merge order: ARO-Tools first, then this ARO-HCP PR with the ARO-Tools pin bump, then sdp-pipelines with the ARO-Tools/ARO-HCP pin bumps and
make -C hcp/ updaterun twice.The Kusto role-assignment rename fixes a name collision between ingest and viewer grants. After first deployment, old
grant-${guid(...)}Kusto role assignments may remain as harmless duplicates; ops can do a one-time cleanup if desired.