OCPBUGS-78995: Azure MachinePool: Fix image gallery discovery - #2930
OCPBUGS-78995: Azure MachinePool: Fix image gallery discovery#29302uasimojo wants to merge 1 commit into
Conversation
Around 4.20, installer started using marketplace images. The logic in our azure MachinePool actuator was never updated for that, which was causing problems with downstream image defaulting. This is part of a two-part fix (the other part in installer, picked up via the latest revendor) to force the installer code to use gallery images iff they exist; and otherwise use a default marketplace image.
openshift-ci-robot
added
jira/severity-critical
|
@2uasimojo: This pull request references Jira Issue OCPBUGS-78995, 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 2uasimojo 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
📝 WalkthroughWalkthroughAzure machine pool generation now detects Azure gallery images, propagates instance architecture through installer image creation, and selects marketplace RHCOS image SKUs based on ARM64 versus AMD64 and Hyper-V generation. ChangesAzure image selection
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant MachinePoolAzureActuator
participant installazure.MachineSets
participant provider
participant capzImage
MachinePoolAzureActuator->>MachinePoolAzureActuator: detect gallery images and set capabilities
MachinePoolAzureActuator->>installazure.MachineSets: pass rhcosImg and capabilities
installazure.MachineSets->>provider: generate machine image
provider->>capzImage: pass architecture and Hyper-V generation
capzImage-->>provider: return gallery or marketplace image reference
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
NOTE: Vendor changes are a temporary port of openshift/installer#10707 for testing purposes. They'll need to be pulled in via a proper revendor, on which the rest of this commit will need to be stacked. Meanwhile, they'll trigger verify failures. That's okay.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@pkg/controller/machinepool/azureactuator.go`:
- Around line 247-265: The galleryImageExistsAndIsGen2 method currently queries
managed images through getImagesByResourceGroup, so replace that lookup with the
Azure Compute gallery image/version APIs for the specified resource group.
Preserve the existing found/gen2 return semantics and error wrapping while
evaluating the returned gallery image versions’ HyperVGeneration values.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0fe2b469-6219-4420-8839-ac00cdafb3c1
📒 Files selected for processing (4)
pkg/controller/machinepool/azureactuator.govendor/github.com/openshift/installer/pkg/asset/installconfig/azure/capabilities.govendor/github.com/openshift/installer/pkg/asset/machines/azure/azuremachines.govendor/github.com/openshift/installer/pkg/asset/machines/azure/machines.go
| // galleryImageExistsAndIsGen2 looks for gallery images in the specified RG. | ||
| // The first return indicates that we found *any* such images. If this is false, the caller should | ||
| // use marketplace images. | ||
| // The second return, only relevant if the first is true, indicates whether any of the found images | ||
| // are gen2. If this is false, the caller must assume gen1, validate compatibility with the VM | ||
| // size, and request gen1 accordingly. | ||
| func (a *AzureActuator) galleryImageExistsAndIsGen2(resourceGroupName string) (bool, bool, error) { | ||
| images, err := a.getImagesByResourceGroup(resourceGroupName) | ||
| if err != nil { | ||
| return false, errors.Wrapf(err, "error listing images by resourceGroup: %s", resourceGroupName) | ||
| // We get 200/[] rather than 404 if there are none, right? | ||
| return false, false, errors.Wrapf(err, "error listing images by resourceGroup: %s", resourceGroupName) | ||
| } | ||
| found := len(images) != 0 | ||
| for _, image := range images { | ||
| if image.ImageProperties.HyperVGeneration == compute.HyperVGenerationTypesV2 { | ||
| return true, nil | ||
| return found, true, nil | ||
| } | ||
| } | ||
| return false, nil | ||
| return found, false, nil |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate relevant symbols and inspect the surrounding implementation.
rg -n "galleryImageExistsAndIsGen2|getImagesByResourceGroup|Microsoft\.Compute/images|galleries/.*/images|image version|hyperVGeneration" pkg/controller/machinepool/azureactuator.go pkg/controller/machinepool -S
printf '\n--- azureactuator.go (selected ranges) ---\n'
sed -n '1,360p' pkg/controller/machinepool/azureactuator.goRepository: openshift/hive
Length of output: 14561
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the exact API client used for image listing.
rg -n "func \(.*getImagesByResourceGroup|ImagesClient|ListByResourceGroup|ListByResourceGroupComplete|GalleryImagesClient|GalleryImageVersionsClient" pkg -SRepository: openshift/hive
Length of output: 612
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read only the implementation of the image lookup helper and its call sites.
file=pkg/controller/machinepool/azureactuator.go
nl -ba "$file" | sed -n '210,320p'
printf '\n--- call sites ---\n'
rg -n "galleryImageExistsAndIsGen2\(" pkg/controller/machinepool -SRepository: openshift/hive
Length of output: 192
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Print the relevant section with line numbers so we can inspect the caller behavior.
nl -ba pkg/controller/machinepool/azureactuator.go | sed -n '120,290p'Repository: openshift/hive
Length of output: 192
Use the gallery API here, not managed images. getImagesByResourceGroup only lists Microsoft.Compute/images, so gallery images under Microsoft.Compute/galleries/... are never found and this path falls back to marketplace selection. Wire this through the gallery image/version APIs instead.
🤖 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/controller/machinepool/azureactuator.go` around lines 247 - 265, The
galleryImageExistsAndIsGen2 method currently queries managed images through
getImagesByResourceGroup, so replace that lookup with the Azure Compute gallery
image/version APIs for the specified resource group. Preserve the existing
found/gen2 return semantics and error wrapping while evaluating the returned
gallery image versions’ HyperVGeneration values.
|
@2uasimojo: The following tests failed, say
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. |
|
/close in favor of #2931. |
|
@2uasimojo: Closed this PR. 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. |
|
@2uasimojo: This pull request references Jira Issue OCPBUGS-78995. The bug has been updated to no longer 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. |
2 participants
Around 4.20, installer started using marketplace images. The logic in our azure MachinePool actuator was never updated for that, which was causing problems with downstream image defaulting.
This is part of a two-part fix (the other part in installer, picked up via the latest revendor) to force the installer code to use gallery images iff they exist; and otherwise use a default marketplace image.
Summary by CodeRabbit
New Features
Bug Fixes