-
Notifications
You must be signed in to change notification settings - Fork 428
OCPBUGS-66211: fix(azure): preserve defaulted values in Infrastructure status #7393
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@bryan-cox: This pull request references Jira Issue OCPBUGS-66211, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughReconcileInfrastructure's Azure handling was refactored: the code now ensures Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox 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 |
|
/jira refresh |
|
@bryan-cox: This pull request references Jira Issue OCPBUGS-66211, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
support/globalconfig/infrastructure.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
support/globalconfig/infrastructure.go
|
/test e2e-aks-4-21 |
|
/test e2e-aws-upgrade-hypershift-operator |
f05d4d0 to
4b9c8d1
Compare
|
/test e2e-aks-4-21 |
|
@bryan-cox: No Jira issue is referenced in the title of this pull request. 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. |
Previously, the Azure platform reconciliation replaced the entire AzurePlatformStatus struct, which inadvertently reverted API-defaulted fields like CloudLoadBalancerConfig and IPFamily to nil/empty values. This caused the HCCO to continuously update the Infrastructure resource as the defaulting controller fought with the reconciliation loop. The fix now checks if the Azure struct exists before initializing, and only updates the specific fields we manage (CloudName and ResourceGroupName), preserving any other defaulted values. Signed-off-by: Bryan Cox <brcox@redhat.com>
4b9c8d1 to
237b375
Compare
|
@bryan-cox: This pull request references Jira Issue OCPBUGS-66211, which is invalid:
Comment 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. |
|
/lgtm |
|
/test e2e-aks-4-21 |
|
/override "ci/prow/okd-scos-images" |
|
@bryan-cox: Overrode contexts on behalf of bryan-cox: ci/prow/okd-scos-images 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. |
|
/test e2e-aks-4-21 |
| infra.Status.PlatformStatus.Azure = &configv1.AzurePlatformStatus{} | ||
| } | ||
| infra.Status.PlatformStatus.Azure.CloudName = configv1.AzurePublicCloud | ||
| cloudName := configv1.AzureCloudEnvironment(hcp.Spec.Platform.Azure.Cloud) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are not the same constants. I believe you'll need to add a mapping function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The direct type conversion is actually valid here - both types use the same underlying string values. Looking at both APIs:
- HyperShift API (
hyperv1.AzurePlatformSpec.Cloud):stringwith enumAzurePublicCloud;AzureUSGovernmentCloud;AzureChinaCloud;AzureGermanCloud;AzureStackCloud - configv1 API (
configv1.AzureCloudEnvironment):type AzureCloudEnvironment stringwith enum"";AzurePublicCloud;AzureUSGovernmentCloud;AzureChinaCloud;AzureGermanCloud;AzureStackCloud
The string values are identical between both APIs. The only difference is configv1 also allows empty string, which we handle on lines 86-88 by defaulting to AzurePublicCloud.
Since both are backed by string and use the exact same values, the type conversion configv1.AzureCloudEnvironment(hcp.Spec.Platform.Azure.Cloud) works correctly.
I did check for an existing mapping function - GetAzureCloudConfiguration in support/azureutil/azureutil.go maps to Azure SDK's cloud.Configuration, not to configv1.AzureCloudEnvironment. Happy to add an explicit mapping function if you'd prefer that for clarity, but the current approach is type-safe.
AI-assisted response via Claude Code
|
/hold |
|
/hold cancel |
|
/verified by e2e |
|
@bryan-cox: 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. |
|
/retest |
|
/jira refresh |
|
@bryan-cox: This pull request references Jira Issue OCPBUGS-66211, which is invalid:
Comment 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. |
|
/jira refresh |
|
@bryan-cox: This pull request references Jira Issue OCPBUGS-66211, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
|
/test e2e-aks |
|
@bryan-cox: 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. |
|
@bryan-cox: Jira Issue Verification Checks: Jira Issue OCPBUGS-66211 Jira Issue OCPBUGS-66211 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓 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. |
What this PR does / why we need it:
Fixes an issue where the Azure platform reconciliation in HyperShift was replacing the entire
AzurePlatformStatusstruct, which inadvertently reverted API-defaulted fields likeCloudLoadBalancerConfigandIPFamilyto nil/empty values.This caused the HCCO to continuously update the Infrastructure resource as the defaulting controller fought with the reconciliation loop, resulting in errors like:
Changes:
AzurePlatformStatusstruct if it's nil, preserving any defaulted valueshcp.Spec.Platform.Azure.Cloudinstead of hardcodingAzurePublicCloud, falling back toAzurePublicCloudif not specifiedCloudNameandResourceGroupName)Which issue(s) this PR fixes:
A part of OCPBUGS-66211
Special notes for your reviewer:
The change is minimal and targeted - instead of replacing the entire struct, we now:
Checklist: