Skip to content

OCPBUGS-78995: Azure MachinePool: Accept OSImageID #2931

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.

Sign up for GitHub

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

Open
2uasimojo wants to merge 1 commit into
base: master
Choose a base branch
from
Open

OCPBUGS-78995: Azure MachinePool: Accept OSImageID #2931

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion apis/hive/v1/azure/machinepool.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,18 @@ type MachinePool struct {
// OSDisk defines the storage for instance.
OSDisk `json:"osDisk"`

// OSImage defines the image to use for the OS.
// OSImage defines the marketplace image to use for the OS. Mutually exclusive with OSImageID;
// if both are specified, OSImage is ignored. If neither is specified, the managed cluster will
// inject a default.
// +optional
OSImage *OSImage `json:"osImage,omitempty"`

// OSImageID is the resource path of the managed or gallery image to be used on the VMs.
// Mutually exclusive with OSImage; if both are specified, OSImage is ignored. If neither is
// specified, the managed cluster will inject a default.
// +optional
OSImageID string `json:"osImageID,omitempty"`

// NetworkResourceGroupName specifies the network resource group that contains an existing VNet.
// Ignored unless VirtualNetwork is also specified.
// +optional
Expand Down
11 changes: 10 additions & 1 deletion config/crds/hive.openshift.io_machinepools.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,7 +255,10 @@ spec:
- diskSizeGB
type: object
osImage:
description: OSImage defines the image to use for the OS.
description: |-
OSImage defines the marketplace image to use for the OS. Mutually exclusive with OSImageID;
if both are specified, OSImage is ignored. If neither is specified, the managed cluster will
inject a default.
properties:
offer:
description: Offer is the offer of the image.
Expand Down Expand Up @@ -283,6 +286,12 @@ spec:
- sku
- version
type: object
osImageID:
description: |-
OSImageID is the resource path of the managed or gallery image to be used on the VMs.
Mutually exclusive with OSImage; if both are specified, OSImage is ignored. If neither is
specified, the managed cluster will inject a default.
type: string
outboundType:
description: OutboundType is a strategy for how egress from cluster is achieved. When not specified default is "Loadbalancer".
type: string
Expand Down
11 changes: 10 additions & 1 deletion hack/app-sre/saas-template.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8190,7 +8190,10 @@ objects:
- diskSizeGB
type: object
osImage:
description: OSImage defines the image to use for the OS.
description: |-
OSImage defines the marketplace image to use for the OS. Mutually exclusive with OSImageID;
if both are specified, OSImage is ignored. If neither is specified, the managed cluster will
inject a default.
properties:
offer:
description: Offer is the offer of the image.
Expand Down Expand Up @@ -8218,6 +8221,12 @@ objects:
- sku
- version
type: object
osImageID:
description: |-
OSImageID is the resource path of the managed or gallery image to be used on the VMs.
Mutually exclusive with OSImage; if both are specified, OSImage is ignored. If neither is
specified, the managed cluster will inject a default.
type: string
outboundType:
description: OutboundType is a strategy for how egress from cluster is achieved. When not specified default is "Loadbalancer".
type: string
Expand Down
3 changes: 0 additions & 3 deletions pkg/azureclient/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,6 @@ type Client interface {
ListAllVirtualMachines(ctx context.Context, statusOnly string) (compute.VirtualMachineListResultPage, error)
DeallocateVirtualMachine(ctx context.Context, resourceGroup, name string) (compute.VirtualMachinesDeallocateFuture, error)
StartVirtualMachine(ctx context.Context, resourceGroup, name string) (compute.VirtualMachinesStartFuture, error)

// Images
ListImagesByResourceGroup(ctx context.Context, resourceGroupName string) (ImageListResultPage, error)
}

// ResourceSKUsPage is a page of results from listing resource SKUs.
Expand Down
15 changes: 0 additions & 15 deletions pkg/azureclient/mock/client_generated.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

98 changes: 35 additions & 63 deletions pkg/controller/machinepool/azureactuator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import (
"time"

azenc "github.com/Azure/azure-sdk-for-go/profiles/latest/compute/mgmt/compute"
"github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2019-12-01/compute"
"github.com/Azure/go-autorest/autorest/azure"
"github.com/Azure/go-autorest/autorest/to"
machineapi "github.com/openshift/api/machine/v1beta1"
Expand Down Expand Up @@ -127,39 +126,42 @@ func (a *AzureActuator) GenerateMachineSets(cd *hivev1.ClusterDeployment, pool *
return nil, false, errors.Wrap(err, "error retrieving VM capabilities")
}

// There are two image "generations": V1 and V2. An image can be one xor the other. A VM "size"
// (type) can support one or both.
// There are three kinds of OS image (used to boot the VM):
// - "managed": Used for AzureStackCloud, which the current hive code does not support (we
// would need to plumb cd.spec.platform.azure.cloudName through the MachineSets() call).
// These live in a resource group, and are V1 only.
// - "gallery": This is the kind created pre OCP 4.20 by installer. Specific to a resource
// group (single spoke). May be V1 or V2.
// - "marketplace": The modern common case. These images are "public": available in most Azure
// regions, not tied to a resource group or even account/subscription. In 4.20+, installer
// makes sure they exist in the designated region and uses them. May be V1 or V2.
// If the user does not specify an image, MachineSets() leaves it blank in the providerSpec,
// and MAO tries to set a default on the spoke via a mutating webhook. However, MAO isn't very
// good at this in several releases, e.g. often using a V1 image without confirming that the
// specified VM size is capable of V1.
// We could have tried to plug that hole here by querying for the various images, validating
// them against the VM size, etc., but that would be brittle, hard to validate, and not
// future-proof.
// Instead we expose the ability for the user to specify the image explicitly for any of the
// three varieties. For marketplace images, they can fill out the OSImage. For managed and
// gallery images, they must use OSImageID, which we pass through opaquely to MachineSets().
if osImage := pool.Spec.Platform.Azure.OSImage; osImage != nil && osImage.Publisher != "" {
var plan = installertypesazure.ImageWithPurchasePlan // default value
if osImage.Plan != "" {
plan = installertypesazure.ImagePurchasePlan(osImage.Plan)
}
if pool.Spec.Platform.Azure.OSImageID != "" {
logger.Warn("ignoring OSImage since OSImageID was provided")
} else {
var plan = installertypesazure.ImageWithPurchasePlan // default value
if osImage.Plan != "" {
plan = installertypesazure.ImagePurchasePlan(osImage.Plan)
}

computePool.Platform.Azure.OSImage = installertypesazure.OSImage{
Plan: plan,
Publisher: osImage.Publisher,
Offer: osImage.Offer,
SKU: osImage.SKU,
Version: osImage.Version,
}
} else {
// 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"
Comment on lines -144 to -162

@2uasimojo 2uasimojo Jul 23, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

computePool.Platform.Azure.OSImage = installertypesazure.OSImage{
Plan: plan,
Publisher: osImage.Publisher,
Offer: osImage.Offer,
SKU: osImage.SKU,
Version: osImage.Version,
}
}
}
Expand All @@ -177,14 +179,11 @@ func (a *AzureActuator) GenerateMachineSets(cd *hivev1.ClusterDeployment, pool *
}
}

// If we leave the imageID, the image is determined by the installer
const imageID = ""

installerMachineSets, err := installazure.MachineSets(
cd.Spec.ClusterMetadata.InfraID,
installconfig.MakeAsset(ic),
computePool,
imageID,
pool.Spec.Platform.Azure.OSImageID,
workerRole,
workerUserDataName,
capabilities,
Expand Down Expand Up @@ -216,33 +215,6 @@ func (a *AzureActuator) getZones(region string, instanceType string) ([]string,
return nil, err
}

func (a *AzureActuator) getImagesByResourceGroup(resourceGroupName string) ([]compute.Image, error) {
ctx, cancel := context.WithTimeout(context.TODO(), 2*time.Minute)
defer cancel()

var images []compute.Image
var res azureclient.ImageListResultPage
var err error
for res, err = a.client.ListImagesByResourceGroup(ctx, resourceGroupName); err == nil && res.NotDone(); err = res.NextWithContext(ctx) {
images = append(images, res.Values()...)
}

return images, err
}

func (a *AzureActuator) gen2ImageExists(resourceGroupName string) (bool, error) {
images, err := a.getImagesByResourceGroup(resourceGroupName)
if err != nil {
return false, errors.Wrapf(err, "error listing images by resourceGroup: %s", resourceGroupName)
}
for _, image := range images {
if image.ImageProperties.HyperVGeneration == compute.HyperVGenerationTypesV2 {
return true, nil
}
}
return false, nil
}

// getVMCapabilities retrieves the capabilities of an instance type in a specific region. Returns these values
// in a map with the capability name as the key and the corresponding value.
func (a *AzureActuator) getVMCapabilities(ctx context.Context, instanceType, region string) (map[string]string, error) {
Expand Down
Loading