feat(gcp): add GCP PSC conditions to metrics and add platform-specific gauges - #9255
Closed
apahim wants to merge 4 commits into
Closed
feat(gcp): add GCP PSC conditions to metrics and add platform-specific gauges#9255apahim wants to merge 4 commits into
apahim wants to merge 4 commits into
Conversation
Add a tri-state CredentialStatus type (Valid=0, Invalid=1, Unknown=2) and GetCredentialStatus() function to the GCP platform package, mirroring the existing pattern in the AWS platform package. GetCredentialStatus checks ValidGCPWorkloadIdentity and ValidGCPCredentials conditions and returns: - CredentialStatusValid (0) if both are True - CredentialStatusInvalid (1) if either is explicitly False - CredentialStatusUnknown (2) if either is missing or Unknown Non-GCP clusters naturally return CredentialStatusUnknown since neither condition will be present on their HostedCluster status. Signed-off-by: Amador Pahim <apahim@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
Add GCPEndpointAvailable and GCPServiceAttachmentAvailable to the GCP case in ExpectedHCConditions(). These conditions are already set on the HostedCluster by computeGCPPSCCondition but were not tracked as expected conditions, so PSC failures did not appear in hypershift_hostedclusters_failure_conditions. Both GCP endpoint access modes (GCPEndpointAccessPrivate and GCPEndpointAccessPublicAndPrivate) use PSC, so no EndpointAccess gate is needed — unlike AWS which has a Public-only mode. Covers GA checklist item 6.1. Signed-off-by: Amador Pahim <apahim@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
Add GCPEndpointAvailable and GCPServiceAttachmentAvailable to the condition list in collectTransitionDurationMetrics(). PSC setup latency is now tracked in the hypershift_hosted_cluster_transition_seconds histogram. Covers GA checklist item 6.2. Signed-off-by: Amador Pahim <apahim@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
Add a hypershift_cluster_invalid_gcp_creds Prometheus gauge metric that reports GCP credential validity for all HostedClusters: 0 = valid (both ValidGCPWorkloadIdentity and ValidGCPCredentials are True) 1 = invalid (either condition is explicitly False) 2 = unknown (either condition is missing or Unknown) The metric is emitted unconditionally for all clusters. Non-GCP clusters report Unknown (2) because neither GCP condition will be present on them, matching the behavior of hypershift_cluster_invalid_aws_creds. Covers GA checklist item 6.3. Signed-off-by: Amador Pahim <apahim@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
Contributor
📝 WalkthroughWalkthroughThe PR adds GCP credential states and evaluates workload identity and credential conditions on each HostedCluster. The metrics collector exposes the result through a Prometheus gauge with valid, invalid, and unknown values. GCP endpoint and service-attachment conditions are included in transition-duration metrics. Tests cover credential evaluation, metric collection, and GCP condition combinations. Sequence Diagram(s)sequenceDiagram
participant HostedCluster
participant MetricsCollector
participant GCPPlatform
participant Prometheus
MetricsCollector->>HostedCluster: Read GCP conditions
MetricsCollector->>GCPPlatform: GetCredentialStatus
GCPPlatform-->>MetricsCollector: Return credential status
MetricsCollector->>Prometheus: Emit credential gauge
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
area/control-plane-operator
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: apahim 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 |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
hypershift-operator/controllers/hostedcluster/internal/platform/gcp/gcp_conditions_test.go (1)
140-255: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the required test-case description format.
Rename each table-case
namevalue to useWhen ... it should ....
hypershift-operator/controllers/hostedcluster/internal/platform/gcp/gcp_conditions_test.go#L140-L255: Addit shouldbefore the expected credential-status result.hypershift-operator/controllers/hostedcluster/metrics/metrics_test.go#L656-L695: Addit shouldbefore the expected metric result.hypershift-operator/controllers/hostedcluster/metrics/metrics_test.go#L1607-L1653: Addit shouldbefore the expected transition-duration observation result.As per coding guidelines, unit-test descriptions must use
When ... it should ....🤖 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/internal/platform/gcp/gcp_conditions_test.go` around lines 140 - 255, The table-case descriptions use inconsistent wording. Rename every affected test case name to follow the required “When ... it should ...” format: update gcp_conditions_test.go lines 140-255 for credential-status results, metrics_test.go lines 656-695 for metric results, and metrics_test.go lines 1607-1653 for transition-duration observations; change only the name strings and preserve each test’s scenario and expected result.Source: Coding guidelines
🤖 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.
Nitpick comments:
In
`@hypershift-operator/controllers/hostedcluster/internal/platform/gcp/gcp_conditions_test.go`:
- Around line 140-255: The table-case descriptions use inconsistent wording.
Rename every affected test case name to follow the required “When ... it should
...” format: update gcp_conditions_test.go lines 140-255 for credential-status
results, metrics_test.go lines 656-695 for metric results, and metrics_test.go
lines 1607-1653 for transition-duration observations; change only the name
strings and preserve each test’s scenario and expected result.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 867e5142-e796-429c-9e2a-3ac0199f5899
📒 Files selected for processing (5)
hypershift-operator/controllers/hostedcluster/internal/platform/gcp/gcp.gohypershift-operator/controllers/hostedcluster/internal/platform/gcp/gcp_conditions_test.gohypershift-operator/controllers/hostedcluster/metrics/metrics.gohypershift-operator/controllers/hostedcluster/metrics/metrics_test.gosupport/conditions/conditions.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9255 +/- ##
==========================================
+ Coverage 44.81% 45.04% +0.22%
==========================================
Files 775 778 +3
Lines 97219 97576 +357
==========================================
+ Hits 43572 43949 +377
+ Misses 50644 50598 -46
- Partials 3003 3029 +26
... and 37 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Contributor
|
@apahim: 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements GCP-959 — Add GCP PSC conditions to metrics and add platform-specific gauges (GA checklist items 6.1, 6.2, 6.3).
Problem
PSC conditions (
GCPEndpointAvailable,GCPServiceAttachmentAvailable) are set on the HostedCluster bycomputeGCPPSCConditionbut were not tracked as expected conditions, so PSC failures did not appear inhypershift_hostedclusters_failure_conditionsor the transition duration histogram. Additionally, there was no GCP equivalent ofhypershift_cluster_invalid_aws_creds.Changes
Commit 1:
feat(gcp): add CredentialStatus type and GetCredentialStatus functionCredentialStatustype (Valid=0, Invalid=1, Unknown=2) andGetCredentialStatus()to the GCP platform package, mirroring the AWS pattern.Unknownsince neither GCP condition will be present on their HostedCluster.Commit 2:
feat(gcp): add PSC conditions to ExpectedHCConditions(GA 6.1)GCPEndpointAvailableandGCPServiceAttachmentAvailableto the GCP case inExpectedHCConditions()unconditionally — noEndpointAccessgate needed since both GCP access modes (Private,PublicAndPrivate) use PSC (unlike AWS which has aPublic-only mode).hypershift_hostedclusters_failure_conditions.Commit 3:
feat(metrics): add GCP PSC conditions to transition duration metric(GA 6.2)GCPEndpointAvailableandGCPServiceAttachmentAvailabletocollectTransitionDurationMetrics().hypershift_hosted_cluster_transition_seconds.Commit 4:
feat(metrics): add hypershift_cluster_invalid_gcp_creds gauge(GA 6.3)hypershift_cluster_invalid_gcp_credsPrometheus gauge (0=valid, 1=invalid, 2=unknown) following thehypershift_cluster_invalid_aws_credspattern.Tests
TestGetCredentialStatus— 9 cases covering all tri-state combinationsTestReportInvalidGcpCreds— 7 cases mirroringTestReportInvalidAwsCredsTestReportTransitionDurationForGCPEndpointConditions— 5 cases mirroringTestReportTransitionDurationForAWSEndpointConditionsAcceptance Criteria
hypershift_hostedclusters_failure_conditionsmetrichypershift_hostedclusters_transition_durationhistogramSummary by CodeRabbit
New Features
Bug Fixes
Tests