feat(gcp): add GCP PSC conditions to metrics and add platform-specific gauges (GCP-959) - #9242
feat(gcp): add GCP PSC conditions to metrics and add platform-specific gauges (GCP-959)#9242apahim wants to merge 6 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 AWS pattern in platform/aws. GetCredentialStatus checks ValidGCPWorkloadIdentity and ValidGCPCredentials conditions: returns Invalid if either is False, Valid if both are True, and Unknown otherwise (missing or Unknown conditions). The Unknown state also covers non-GCP clusters naturally, matching AWS behavior. This is a prerequisite for the hypershift_cluster_invalid_gcp_creds Prometheus gauge introduced in a subsequent commit. Signed-off-by: Amador Pahim <apahim@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
… to ExpectedHCConditions Both GCP endpoint access modes (Private and PublicAndPrivate) use Private Service Connect, so PSC conditions are included unconditionally in the GCP case — there is no GCPEndpointAccessPublic constant unlike AWS. This causes PSC failures to appear in the hypershift_hostedclusters_failure_conditions metric. Signed-off-by: Amador Pahim <apahim@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
…trics Track GCP Private Service Connect setup latency and credential validity in Prometheus, mirroring existing AWS metric patterns. Transition duration (hypershift_hosted_cluster_transition_seconds): - Add GCPEndpointAvailable and GCPServiceAttachmentAvailable to the condition slice in collectTransitionDurationMetrics so PSC setup latency is captured in the histogram. Credentials gauge (hypershift_cluster_invalid_gcp_creds): - New gauge emitting 0=valid, 1=invalid, 2=unknown per HostedCluster. - Calls gcp.GetCredentialStatus; non-GCP clusters report Unknown (2), consistent with the AWS collectAwsCredsMetric pattern. - Wired unconditionally into collectPerClusterMetrics. Signed-off-by: Amador Pahim <apahim@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change adds tri-state GCP credential status evaluation for Sequence Diagram(s)sequenceDiagram
participant MetricsCollector
participant HostedCluster
participant GCPPlatform
participant Prometheus
MetricsCollector->>HostedCluster: read GCP conditions
MetricsCollector->>GCPPlatform: call GetCredentialStatus
GCPPlatform-->>MetricsCollector: return valid, invalid, or unknown
MetricsCollector->>Prometheus: emit credential gauge and transition metrics
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
|
[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}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/internal/platform/gcp/gcp_conditions_test.go`:
- Around line 141-215: Rename every affected table-test case to follow the
required “When ... it should ...” format: update all cases in
hypershift-operator/controllers/hostedcluster/internal/platform/gcp/gcp_conditions_test.go
lines 141-215, each credential metric case in
hypershift-operator/controllers/hostedcluster/metrics/metrics_test.go lines
656-695, and the no-condition case in lines 1607-1662. Preserve the existing
test behavior and descriptions.
- Around line 134-215: Add both mixed condition cases to the credential-status
table in
hypershift-operator/controllers/hostedcluster/internal/platform/gcp/gcp_conditions_test.go:
one with ValidGCPWorkloadIdentity false and ValidGCPCredentials unknown, and the
reverse, expecting CredentialStatusInvalid. Add the same two cases to the metric
table in hypershift-operator/controllers/hostedcluster/metrics/metrics_test.go,
expecting gauge value 1.
🪄 Autofix
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: 421b163a-b38d-4591-ace4-c695438e5ad9
📒 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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9242 +/- ##
==========================================
+ Coverage 44.81% 45.03% +0.22%
==========================================
Files 775 778 +3
Lines 97219 97481 +262
==========================================
+ Hits 43572 43904 +332
+ Misses 50644 50550 -94
- Partials 3003 3027 +24
... and 34 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:
|
- Rename all new GCP test case names to follow the required "When ..., it should ..." format per TESTING.md conventions. - Add two mixed False/Unknown condition test cases to TestGetCredentialStatus to cover the contract that False overrides Unknown, producing Invalid status. - Add matching mixed cases to TestReportInvalidGcpCreds expecting gauge value 1. - Fix TestReportTransitionDurationForGCPEndpointConditions case names to follow the same convention. Signed-off-by: Amador Pahim <apahim@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
Add support/conditions/conditions_test.go to cover the GCP-specific conditions added to ExpectedHCConditions. The file had 0% patch coverage as reported by Codecov; these tests bring the GCP case to full coverage. Tests verify that all four GCP-specific conditions are present and set to True for GCP clusters, and absent for non-GCP clusters: - ValidGCPWorkloadIdentity - ValidGCPCredentials - GCPEndpointAvailable - GCPServiceAttachmentAvailable Signed-off-by: Amador Pahim <apahim@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
There was a problem hiding this comment.
🧹 Nitpick comments (1)
support/conditions/conditions_test.go (1)
93-94: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid shadowing the outer test handle.
Line 93 declares a second
tthat shadows thetparameter fromTestExpectedHCConditionsGCPPlatform. Rename the callback parameter tosubtestand pass it toNewWithT.Proposed fix
- t.Run(tt.name, func(t *testing.T) { - g := NewWithT(t) + t.Run(tt.name, func(subtest *testing.T) { + g := NewWithT(subtest)🤖 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 `@support/conditions/conditions_test.go` around lines 93 - 94, Rename the t parameter in the t.Run callback within TestExpectedHCConditionsGCPPlatform to subtest, and pass subtest to NewWithT to avoid shadowing the outer test handle.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 `@support/conditions/conditions_test.go`:
- Around line 93-94: Rename the t parameter in the t.Run callback within
TestExpectedHCConditionsGCPPlatform to subtest, and pass subtest to NewWithT to
avoid shadowing the outer test handle.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 35192938-f6ed-44b5-88a8-b2b89370acaf
📒 Files selected for processing (1)
support/conditions/conditions_test.go
Signed-off-by: Amador Pahim <apahim@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
|
@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. |
1 participant
Summary
Implements GCP-959 — GA checklist items 6.1, 6.2, 6.3.
GCP PSC conditions (
GCPEndpointAvailable,GCPServiceAttachmentAvailable) are already 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.Changes
Commit 1:
CredentialStatustype +GetCredentialStatus()(GCP platform package)CredentialStatustype (Valid=0, Invalid=1, Unknown=2) mirroring the AWS patternGetCredentialStatus(hc)checkingValidGCPWorkloadIdentityandValidGCPCredentialsconditionsTestGetCredentialStatuswith 9 test casesCommit 2: PSC conditions in
ExpectedHCConditions()GCPEndpointAvailableandGCPServiceAttachmentAvailableunconditionally to the GCP caseEndpointAccessgate needed: GCP only definesPrivateandPublicAndPrivate— both use PSChypershift_hostedclusters_failure_conditionsCommit 3: Metrics — transition duration + GCP creds gauge
GCPEndpointAvailableandGCPServiceAttachmentAvailabletocollectTransitionDurationMetrics()so PSC setup latency is tracked inhypershift_hosted_cluster_transition_secondshypershift_cluster_invalid_gcp_credsgauge (0=valid, 1=invalid, 2=unknown), emitted unconditionally for all clusters (non-GCP clusters report Unknown=2, consistent with the AWS pattern)TestReportInvalidGcpCreds(7 cases) andTestReportTransitionDurationForGCPEndpointConditions(5 cases)Testing
All 21 new tests pass. No existing tests broken.
Summary by CodeRabbit