feat(gcp): GCP-504 NodePool Platform Conditions#8917
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. |
openshift-ci
Bot
added
do-not-merge/needs-area
area/hypershift-operator
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: thetechnick 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 |
📝 WalkthroughWalkthroughThis change adds GCP-specific NodePool condition handling. Sequence Diagram(s)sequenceDiagram
participant NodePoolReconciler
participant HostedCluster
participant resolveGCPImage
NodePoolReconciler->>NodePoolReconciler: setPlatformConditions(GCPPlatform)
NodePoolReconciler->>HostedCluster: check Spec.Platform.GCP
alt HostedCluster GCP config missing
NodePoolReconciler-->>NodePoolReconciler: return error
else HostedCluster GCP config present
NodePoolReconciler->>resolveGCPImage: resolveGCPImage(...)
alt resolution fails
resolveGCPImage-->>NodePoolReconciler: error
NodePoolReconciler->>NodePoolReconciler: set NodePoolValidPlatformImageType=False
else resolution succeeds
resolveGCPImage-->>NodePoolReconciler: image
NodePoolReconciler->>NodePoolReconciler: set NodePoolValidPlatformImageType=True
end
end
Compact metadata
Related issues: None specified Related PRs: None specified Suggested labels: gcp, nodepool, conditions, tests Suggested reviewers: None specified Poem 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
hypershift-operator/controllers/nodepool/gcp.go (1)
32-49: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: avoid
if/elseafter an early return.Since the
ifbranch already returns, theelseblock can be flattened for readability (common Go idiom / lint preference, e.g.revive's indent-error-flow).♻️ Proposed refactor
- if img, err := resolveGCPImage(nodePool, releaseImage); err != nil { - SetStatusCondition(&nodePool.Status.Conditions, hyperv1.NodePoolCondition{ - Type: hyperv1.NodePoolValidPlatformImageType, - Status: corev1.ConditionFalse, - Reason: hyperv1.NodePoolValidationFailedReason, - Message: fmt.Sprintf("Couldn't discover a GCP machine image for release image %q: %s", nodePool.Spec.Release.Image, err.Error()), - ObservedGeneration: nodePool.Generation, - }) - return fmt.Errorf("couldn't discover a GCP machine image for release image: %w", err) - } else { - SetStatusCondition(&nodePool.Status.Conditions, hyperv1.NodePoolCondition{ - Type: hyperv1.NodePoolValidPlatformImageType, - Status: corev1.ConditionTrue, - Reason: hyperv1.AsExpectedReason, - Message: fmt.Sprintf("Bootstrap GCP machine image is %q", img), - ObservedGeneration: nodePool.Generation, - }) - } - return nil + img, err := resolveGCPImage(nodePool, releaseImage) + if err != nil { + SetStatusCondition(&nodePool.Status.Conditions, hyperv1.NodePoolCondition{ + Type: hyperv1.NodePoolValidPlatformImageType, + Status: corev1.ConditionFalse, + Reason: hyperv1.NodePoolValidationFailedReason, + Message: fmt.Sprintf("Couldn't discover a GCP machine image for release image %q: %s", nodePool.Spec.Release.Image, err.Error()), + ObservedGeneration: nodePool.Generation, + }) + return fmt.Errorf("couldn't discover a GCP machine image for release image: %w", err) + } + SetStatusCondition(&nodePool.Status.Conditions, hyperv1.NodePoolCondition{ + Type: hyperv1.NodePoolValidPlatformImageType, + Status: corev1.ConditionTrue, + Reason: hyperv1.AsExpectedReason, + Message: fmt.Sprintf("Bootstrap GCP machine image is %q", img), + ObservedGeneration: nodePool.Generation, + }) + return nil🤖 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/nodepool/gcp.go` around lines 32 - 49, Flatten the conditional in the GCP image resolution flow by removing the unnecessary else after the early return in the resolveGCPImage/nodePool status update block. Keep the error-handling branch in the existing if, return immediately on failure, and move the successful SetStatusCondition call for NodePoolValidPlatformImageType out of the else so the logic is easier to read and matches Go idioms.
🤖 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/nodepool/gcp_test.go`:
- Around line 791-907: Add a test case in
TestNodePoolReconciler_setGCPConditions that covers a GCP NodePool where
nodePool.Spec.Platform.Type is GCPPlatform but nodePool.Spec.Platform.GCP is nil
while HostedCluster.Spec.Platform.GCP is present. Update the test table and
check function to verify setGCPConditions does not panic and returns the
expected error/condition behavior for this nil GCP config path. Use the existing
NodePoolReconciler.setGCPConditions, NodePoolValidPlatformImageType, and
HostedCluster/NodePool fixture patterns to keep the case aligned with the other
scenarios.
In `@hypershift-operator/controllers/nodepool/gcp.go`:
- Around line 20-51: The setGCPConditions flow in NodePoolReconciler only guards
hcluster.Spec.Platform.GCP, but resolveGCPImage assumes
nodePool.Spec.Platform.GCP is present and can panic when it is nil. Add a nil
check for nodePool.Spec.Platform.GCP before calling resolveGCPImage, mirroring
the HostedCluster validation, and return/set a NodePoolValidPlatformImageType
failure condition with a clear message if the GCP platform config is missing.
---
Nitpick comments:
In `@hypershift-operator/controllers/nodepool/gcp.go`:
- Around line 32-49: Flatten the conditional in the GCP image resolution flow by
removing the unnecessary else after the early return in the
resolveGCPImage/nodePool status update block. Keep the error-handling branch in
the existing if, return immediately on failure, and move the successful
SetStatusCondition call for NodePoolValidPlatformImageType out of the else so
the logic is easier to read and matches Go idioms.
🪄 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: e04c8126-9e2d-4471-9b24-d70185edc364
📒 Files selected for processing (3)
hypershift-operator/controllers/nodepool/conditions.gohypershift-operator/controllers/nodepool/gcp.gohypershift-operator/controllers/nodepool/gcp_test.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8917 +/- ##
==========================================
+ Coverage 43.28% 43.30% +0.02%
==========================================
Files 771 771
Lines 95503 95531 +28
==========================================
+ Hits 41335 41367 +32
+ Misses 51284 51282 -2
+ Partials 2884 2882 -2
... and 1 file 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:
|
thetechnick
force-pushed
the
gcp-504-nodepool-platform-conditions
branch
from
36ef0b4 to
89c2751
Compare
Add GCP-specific conditions to NodePool status, to allow users to diagnose image resolution failures. This functionality is modeled after the AWS implementation.
thetechnick
force-pushed
the
gcp-504-nodepool-platform-conditions
branch
from
89c2751 to
f141100
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
@thetechnick: 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. |
| } else { | ||
| SetStatusCondition(&nodePool.Status.Conditions, hyperv1.NodePoolCondition{ | ||
| Type: hyperv1.NodePoolValidPlatformImageType, | ||
| Status: corev1.ConditionTrue, | ||
| Reason: hyperv1.AsExpectedReason, | ||
| Message: fmt.Sprintf("Bootstrap GCP machine image is %q", img), | ||
| ObservedGeneration: nodePool.Generation, | ||
| }) |
There was a problem hiding this comment.
We could probably drop the else statement here.
There was a problem hiding this comment.
Could do, but than I have to move img, err := resolveGCPImage(nodePool, releaseImage) out of the scope of the if condition. -> right now the img variable is not available outside of the else statement.
I personally have no preference for either way here.
Do you prefer it like this?
img, err := resolveGCPImage(nodePool, releaseImage)
if err != nil {
SetStatusCondition(&nodePool.Status.Conditions, hyperv1.NodePoolCondition{
Type: hyperv1.NodePoolValidPlatformImageType,
Status: corev1.ConditionFalse,
Reason: hyperv1.NodePoolValidationFailedReason,
Message: fmt.Sprintf("Couldn't discover a GCP machine image for release image %q: %s", nodePool.Spec.Release.Image, err.Error()),
ObservedGeneration: nodePool.Generation,
})
return fmt.Errorf("couldn't discover a GCP machine image for release image: %w", err)
}
SetStatusCondition(&nodePool.Status.Conditions, hyperv1.NodePoolCondition{
Type: hyperv1.NodePoolValidPlatformImageType,
Status: corev1.ConditionTrue,
Reason: hyperv1.AsExpectedReason,
Message: fmt.Sprintf("Bootstrap GCP machine image is %q", img),
ObservedGeneration: nodePool.Generation,
})
There was a problem hiding this comment.
yeah, I personally like this version better, but at this point it's just a styling issue/preference.
| if img, err := resolveGCPImage(nodePool, releaseImage); err != nil { | ||
| SetStatusCondition(&nodePool.Status.Conditions, hyperv1.NodePoolCondition{ | ||
| Type: hyperv1.NodePoolValidPlatformImageType, | ||
| Status: corev1.ConditionFalse, | ||
| Reason: hyperv1.NodePoolValidationFailedReason, | ||
| Message: fmt.Sprintf("Couldn't discover a GCP machine image for release image %q: %s", nodePool.Spec.Release.Image, err.Error()), | ||
| ObservedGeneration: nodePool.Generation, | ||
| }) | ||
| return fmt.Errorf("couldn't discover a GCP machine image for release image: %w", err) | ||
| } else { | ||
| SetStatusCondition(&nodePool.Status.Conditions, hyperv1.NodePoolCondition{ | ||
| Type: hyperv1.NodePoolValidPlatformImageType, | ||
| Status: corev1.ConditionTrue, | ||
| Reason: hyperv1.AsExpectedReason, | ||
| Message: fmt.Sprintf("Bootstrap GCP machine image is %q", img), | ||
| ObservedGeneration: nodePool.Generation, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Minor behavioral inconsistency with AWS worth considering: when a user pins their own image via nodePool.Spec.Platform.GCP.Image, resolveGCPImage returns it directly with no error, so this condition is always set to ConditionTrue - including for user-defined images that HyperShift never actually validated.
The AWS equivalent (setAWSConditions) handles this explicitly by calling removeStatusCondition for custom AMIs, avoiding any implied validation claim.
Not a blocker, but worth either aligning the behavior or adding a comment documenting the intentional difference.
There was a problem hiding this comment.
+1 on this. The condition type's own documentation in nodepool_conditions.go (line 9) explicitly says "If the image is direct user input then this condition is meaningless." Setting ConditionTrue for a user-pinned image contradicts that — aligning with the AWS removeStatusCondition pattern would be more accurate.
| releaseImage *releaseinfo.ReleaseImage | ||
| check func(t *testing.T, nodePool *hyperv1.NodePool, err error) | ||
| }{ | ||
| { |
There was a problem hiding this comment.
nit: test names here don't follow the "When...it should..." format that every other test in this file and the AWS equivalent use. e.g. "Not a GCP NodePool" -> "When NodePool platform type is not GCP, it should not set platform image condition", "success" -> "When GCP NodePool has a valid image, it should set ValidPlatformImage to true", etc.
3 participants
What this PR does / why we need it:
Add GCP-specific conditions to NodePool status, to allow users to diagnose image resolution failures.
This functionality is modeled after the AWS implementation.
Which issue(s) this PR fixes:
Fixes #GCP-504
Special notes for your reviewer:
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Tests