ARO-27360: Add ACR pull identity configured metric - #8840
ARO-27360: Add ACR pull identity configured metric#8840openshift-merge-bot[bot] merged 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
📝 WalkthroughWalkthroughAdds a new per-HostedCluster Prometheus gauge named Sequence Diagram(s)sequenceDiagram
participant HostedClusterMetrics as HostedCluster metrics collector
participant AzureConfig as HostedCluster Azure config
participant Prometheus as Prometheus gauge
HostedClusterMetrics->>AzureConfig: read ContainerRegistry.Credentials.ManagedIdentity.ResourceID
AzureConfig-->>HostedClusterMetrics: nil or configured value
HostedClusterMetrics->>Prometheus: emit hypershift_cluster_acr_pull_identity_configured
🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
openshift-ci
Bot
added
area/hypershift-operator
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
hypershift-operator/controllers/hostedcluster/metrics/metrics_test.go (1)
1319-1319: 📐 Maintainability & Code Quality | 🔵 TrivialUse
dto.MetricType_GAUGE.Enum()here instead ofdto.MetricType(1). It avoids depending on the numeric ordinal and makes the intent clearer. Also applies to line 1351.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hypershift-operator/controllers/hostedcluster/metrics/metrics_test.go` at line 1319, Update the metric type setup in metrics_test.go to use the enum helper instead of hardcoded ordinals: replace the dto.MetricType(1) construction with dto.MetricType_GAUGE.Enum() in both affected test cases. Locate the same pattern in the metrics test helper/fixtures around the metric DTO initialization and apply the same change wherever the gauge type is being set.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@hypershift-operator/controllers/hostedcluster/metrics/metrics_test.go`:
- Around line 1296-1361: The test case names in metrics_test.go do not follow
the required “When ... it should ...” convention. Rename the affected cases in
the table-driven test around AcrPullIdentityConfiguredMetricName to descriptive
phrases like “When Azure has no containerRegistry configured it should emit 0”
and “When Azure has a containerRegistry configured it should emit 1”, keeping
the same intent but matching the coding guideline.
---
Nitpick comments:
In `@hypershift-operator/controllers/hostedcluster/metrics/metrics_test.go`:
- Line 1319: Update the metric type setup in metrics_test.go to use the enum
helper instead of hardcoded ordinals: replace the dto.MetricType(1) construction
with dto.MetricType_GAUGE.Enum() in both affected test cases. Locate the same
pattern in the metrics test helper/fixtures around the metric DTO initialization
and apply the same change wherever the gauge type is being set.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: c1b55b1b-57bf-4986-a8a8-1e2bb740f27c
📒 Files selected for processing (2)
hypershift-operator/controllers/hostedcluster/metrics/metrics.gohypershift-operator/controllers/hostedcluster/metrics/metrics_test.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8840 +/- ##
=======================================
Coverage 43.45% 43.46%
=======================================
Files 771 771
Lines 95718 95733 +15
=======================================
+ Hits 41597 41612 +15
Misses 51234 51234
Partials 2887 2887
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
6939414 to
1d0289e
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
1d0289e to
6a134b0
Compare
twolff-gh
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@twolff-gh: This pull request references ARO-27360 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
Add hypershift_cluster_acr_pull_identity_configured gauge metric that indicates whether a HostedCluster has an ACR pull managed identity configured. Emitted only for Azure platform clusters; value is 1 when a ManagedIdentity ResourceID is set, 0 otherwise. Signed-off-by: Todd Wolff <twolff@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
6a134b0 to
be7bd91
Compare
|
I have all the information needed. Here is the analysis: Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe Root CauseThe root cause is a transient Quay.io registry outage returning HTTP 503 during blob fetch:
Recommendations
Evidence
|
|
/retest |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: enxebre, twolff-gh The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
/lgtm |
|
Scheduling tests matching the |
|
/lgtm |
|
@tmstff: 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. |
|
/retest-required |
|
/test e2e-v2-gke |
|
/test e2e-aks |
5 similar comments
|
/test e2e-aks |
|
/test e2e-aks |
|
/test e2e-aks |
|
/test e2e-aks |
|
/test e2e-aks |
|
/verified by @twolff-gh |
openshift-ci-robot
added
the
verified
|
@twolff-gh: This PR has been marked as verified by 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 openshift-eng/jira-lifecycle-plugin repository. |
|
@twolff-gh: all tests passed! 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. |
Backport of be7bd91 from main (PR openshift#8840), adapted for release-4.22 metrics file structure. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Backport of be7bd91 from main (PR openshift#8840), adapted for release-4.22 metrics file structure. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
5 participants
What this PR does / why we need it:
Adds a hypershift_cluster_acr_pull_identity_configured gauge to the hostedcluster metrics collector. Emits 1 when an Azure HostedCluster has containerRegistry.credentials.managedIdentity.resourceID set, 0 when unset. Only emitted for Azure platform clusters.
This metric supports observability for the ACR pull via managed identity feature (OCPSTRAT-2951), letting SRE dashboards track adoption and alert on misconfiguration.
Which issue(s) this PR fixes:
Fixes https://issues.redhat.com/browse/ARO-27360
Main feature https://issues.redhat.com/browse/ARO-24037
Special notes for your reviewer:
CPO-side metric was evaluated and skipped — configmetrics runs in the guest cluster context without access to the HostedCluster/HostedControlPlane CR. The HO gauge covers it because the relationship is deterministic: if the HC has containerRegistry set, CPO writes userAssignedIdentityID into cloud.conf.
Checklist:
Summary by CodeRabbit