OCPBUGS-85296: export Azure Metadata.Region for state file serialization - #10736
OCPBUGS-85296: export Azure Metadata.Region for state file serialization#10736savio87 wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
jira/valid-reference
|
@savio87: This pull request references Jira Issue OCPBUGS-85296, 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. |
|
Hi @savio87. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
openshift-ci
Bot
added
the
needs-ok-to-test
📝 WalkthroughWalkthroughChangesAzure metadata region handling
Estimated code review effort: 1 (Trivial) | ~5 minutes 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions 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.
🧹 Nitpick comments (1)
pkg/asset/installconfig/azure/metadata.go (1)
25-25: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd a regression test for the persisted
Regioncontract.Because
Regionmust survive reentrant installs, add or extend a unit test that marshals and unmarshalsMetadata, then verifies the restored value reachesGetRegionAvailabilityZones,GetAvailabilityZones,defaultInstanceType, andGetVMCapabilities. The provided context does not include coverage for this cross-layer contract.As per coding guidelines, all Go code under
pkg/must have unit tests.🤖 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 `@pkg/asset/installconfig/azure/metadata.go` at line 25, Add a regression unit test for Metadata’s Region persistence by marshaling and unmarshaling a populated Metadata value, then verify the restored region is used by GetRegionAvailabilityZones, GetAvailabilityZones, defaultInstanceType, and GetVMCapabilities. Keep the test focused on the reentrant-install contract and ensure the relevant pkg/ code paths are covered.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 `@pkg/asset/installconfig/azure/metadata.go`:
- Line 25: Add a regression unit test for Metadata’s Region persistence by
marshaling and unmarshaling a populated Metadata value, then verify the restored
region is used by GetRegionAvailabilityZones, GetAvailabilityZones,
defaultInstanceType, and GetVMCapabilities. Keep the test focused on the
reentrant-install contract and ensure the relevant pkg/ code paths are covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 10826c24-a8b6-430b-8fc4-cb4a2b452297
📒 Files selected for processing (1)
pkg/asset/installconfig/azure/metadata.go
|
Note: The Jira bot flagged OCPBUGS-85296 as invalid because it doesn't have a target version set. I don't have permissions to update the target version on the Jira issue. Could a maintainer set the target version to 5.0.0 on OCPBUGS-85296? I can then comment /jira refresh to re-validate. |
|
This looks good to me. I didn't even realize we had a re-entry flag though. I'm willing to bet there are many more problems than just this in that case. According to commit 2b3d05a we don't necessarily want people to do this. I'm just pointing out we may want to create an epic to look at all the edge cases for re-entrant code. |
|
/assign |
|
Thanks for the review and for the context on the re-entry flag! An epic for auditing the re-entrant code paths sounds like a great idea. Let me know if there's anything else needed on this PR. |
|
/retest-required |
|
/test e2e-azure-ovn |
|
/label ok-to-test |
|
@tthvo: The label(s) 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. |
|
/ok-to-test |
openshift-ci
Bot
added
ok-to-test
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@tthvo: This pull request references Jira Issue OCPBUGS-85296, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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. |
|
@savio87: 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. |
|
/approve |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jhixson74 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
4 participants
Summary
regionfield in AzureMetadatastruct with a JSON tag so it persists in the state fileFixes https://issues.redhat.com/browse/OCPBUGS-85296
Details
The Azure
Metadata.regionfield was unexported (lowercase) with no JSON tag, so it was not serialized to.openshift_install_state.json. When resuming an interrupted install withOPENSHIFT_INSTALL_REENTRANT=true, the installer loadedInstallConfigfrom the state file butregioncame back empty. This causedVMAvailabilityZones()to query the Azure API with an empty location filter, failing withlocation information not found for Standard_D8s_v3 in.The fix follows the same pattern used by AWS metadata, which already exports
Regionwith a JSON tag.Test plan
go build ./pkg/asset/installconfig/azure/...)go test ./pkg/asset/installconfig/azure/...)Summary by CodeRabbit