OCPBUGS-78995: Azure MachinePool: Accept OSImageID - #2931
Conversation
openshift-ci-robot
added
jira/severity-critical
|
@2uasimojo: This pull request references Jira Issue OCPBUGS-78995, which is valid. 3 validation(s) were run on this bug
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
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (10)
💤 Files with no reviewable changes (2)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughAzure MachinePool now accepts an Azure managed or gallery image resource ID through ChangesAzure OS image selection
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant MachinePool
participant GenerateMachineSets
participant installazure.MachineSets
MachinePool->>GenerateMachineSets: provide OSImage and OSImageID
GenerateMachineSets->>GenerateMachineSets: apply OSImageID precedence
GenerateMachineSets->>installazure.MachineSets: pass image ID and generated image
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
| // An image was not provided so check if the installer created a "gen2" image | ||
| // to determine if we should allow resultant machinesets to consume a "gen2" image. | ||
| gen2ImageExists, err := a.gen2ImageExists(ic.Platform.Azure.ClusterResourceGroupName(cd.Spec.ClusterMetadata.InfraID)) | ||
| if err != nil { | ||
| return nil, false, err | ||
| } | ||
| if !gen2ImageExists { | ||
| // Modify capabilities to ensure that a V1 image is chosen by installazure.MachineSets() | ||
| // because a V2 image does not exist. | ||
| // The HyperVGeneration is germane to the instance/disk type and affects the image used | ||
| // for the instance. "-gen-2" will be appended to the image name by installazure.MachineSets() | ||
| // when the HyperVGenerations capability (comma separated list of HyperVGenerations) includes "V2". | ||
| // | ||
| // capabilities := map[string]string{ | ||
| // "HyperVGenerations": "V1,V2", | ||
| // } | ||
| // | ||
| if _, ok := capabilities["HyperVGenerations"]; ok { | ||
| capabilities["HyperVGenerations"] = "V1" |
There was a problem hiding this comment.
This was checking for managed (not gallery) images, and erroneously restricting the capabilities to V1 even when no such images were found. However, in recent versions of the vendored installer code, that capability wasn't even being consulted. TL;DR: this chunk of code was a no-op.
With this PR, we could have added a bunch of logic to try to paper over the MAO bug -- look for gallery images, look for managed images, validate the generation of the discovered image(s) against the VM size -- but decided it would be too difficult to prove coverage of all edge cases.
There was a problem hiding this comment.
Note: This is here so we can validate the fix, but will cause verify to fail. Ultimately we'll pick up this change via a legit revendor.
![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
🤖 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_test.go`:
- Line 39: Add test cases in the existing Azure actuator tests covering
OSImageID: verify OSImageID alone sets the final azureProvider.Image to the
explicit ID, and verify setting both OSImage and OSImageID logs the precedence
warning and does not apply the OSImage fields. Remove the TODO once both
scenarios are covered.
In `@pkg/controller/machinepool/azureactuator.go`:
- Line 129: Remove the placeholder comment `// TODO: le big comment here` from
the precedence logic in the Azure machine pool actuator, or replace it with
concise documentation that accurately describes the surrounding logic; do not
leave unfinished TODO text in shipped code.
🪄 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: de541464-ee42-459b-ba9c-c41373e90bef
📒 Files selected for processing (7)
apis/hive/v1/azure/machinepool.gopkg/azureclient/client.gopkg/azureclient/mock/client_generated.gopkg/controller/machinepool/azureactuator.gopkg/controller/machinepool/azureactuator_test.govendor/github.com/openshift/hive/apis/hive/v1/azure/machinepool.govendor/github.com/openshift/installer/pkg/asset/machines/azure/azuremachines.go
💤 Files with no reviewable changes (2)
- pkg/azureclient/mock/client_generated.go
- pkg/azureclient/client.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2931 +/- ##
==========================================
- Coverage 50.51% 50.38% -0.14%
==========================================
Files 282 282
Lines 34485 34454 -31
==========================================
- Hits 17420 17359 -61
- Misses 15707 15737 +30
Partials 1358 1358
🚀 New features to boost your workflow:
|
6754cfd to
c570d0a
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/controller/machinepool/azureactuator_test.go (1)
363-389: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrecedence test doesn't assert the "ignoring OSImage" warning log.
This case verifies the resulting image reflects
OSImageID, but doesn't setexpectedLogsto confirmazureactuator.go'slogger.Warn("ignoring OSImage since OSImageID was provided")is actually emitted when both fields are set — this was part of the original ask for this precedence scenario.♻️ Proposed addition
expectedImage: &machineapi.Image{ ResourceID: "/resourceGroups/bar/providers/Microsoft.Compute/galleries/gallery_baz/images/blah", }, + expectedLogs: []string{"ignoring OSImage since OSImageID was provided"}, },🤖 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_test.go` around lines 363 - 389, The “osImageID takes precedence over osImage” case should also verify the warning emitted when both image fields are configured. Update this test case’s expectedLogs to include the “ignoring OSImage since OSImageID was provided” warning, while preserving its existing image and replica assertions.
🤖 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/controller/machinepool/azureactuator_test.go`:
- Around line 363-389: The “osImageID takes precedence over osImage” case should
also verify the warning emitted when both image fields are configured. Update
this test case’s expectedLogs to include the “ignoring OSImage since OSImageID
was provided” warning, while preserving its existing image and replica
assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d49ea3e9-2f8c-428e-8078-63173ee33f49
📒 Files selected for processing (8)
apis/hive/v1/azure/machinepool.goconfig/crds/hive.openshift.io_machinepools.yamlhack/app-sre/saas-template.yamlpkg/azureclient/client.gopkg/azureclient/mock/client_generated.gopkg/controller/machinepool/azureactuator.gopkg/controller/machinepool/azureactuator_test.govendor/github.com/openshift/hive/apis/hive/v1/azure/machinepool.go
💤 Files with no reviewable changes (2)
- pkg/azureclient/client.go
- pkg/azureclient/mock/client_generated.go
🚧 Files skipped from review as they are similar to previous changes (2)
- vendor/github.com/openshift/hive/apis/hive/v1/azure/machinepool.go
- pkg/controller/machinepool/azureactuator.go
Previously hive optionally accepted `OSImage`, a marketplace image spec, in an Azure MachinePool. If omitted, the installer's MachineSets() generator would leave the image specifier blank and the machine-api-operator would attempt to inject a reasonable default. But in several cases, MAO didn't do a good job of this, resulting in failure to boot the Machines. In cases where marketplace images were appropriate, this could be worked around by simply filling in the `OSImage` field. However, in environments with no access to the marketplace, or where RHCOS images were not present there, and the managed/gallery image defaults didn't work, the user would be stuck with no recourse but to abandon the MachinePool and manage MachineSets directly. Here we add a new field to the Azure MachinePool API: `OSImageID`, a string which expects the resource path of an image in either managed or gallery format. This gives the user the ability to work around any defaulting failure.
c570d0a to
cfc76ae
Compare
|
@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. |
3 participants
Previously hive optionally accepted
OSImage, a marketplace image spec, in an Azure MachinePool. If omitted, the installer's MachineSets() generator would leave the image specifier blank and the machine-api-operator would attempt to inject a reasonable default. But in several cases, MAO didn't do a good job of this, resulting in failure to boot the Machines.In cases where marketplace images were appropriate, this could be worked around by simply filling in the
OSImagefield. However, in environments with no access to the marketplace, or where RHCOS images were not present there, and the managed/gallery image defaults didn't work, the user would be stuck with no recourse but to abandon the MachinePool and manage MachineSets directly.Here we add a new field to the Azure MachinePool API:
OSImageID, a string which expects the URN of an image in any of the (three: marketplace, managed, gallery) supported formats. This gives the user the ability to work around any defaulting failure.Summary by CodeRabbit
osImageIDusing a managed or gallery image resource ID.osImageandosImageIDare mutually exclusive: when both are provided,osImageIDtakes precedence; when neither is set, a default image is injected.osImageIDdirectly and removed unnecessary image-discovery behavior.osImageIDoption.