From cfc76aeb4af1d61bb7db5fe77705e167329d66d0 Mon Sep 17 00:00:00 2001 From: Eric Fried Date: Thu, 23 Jul 2026 16:33:51 -0500 Subject: [PATCH] OCPBUGS-78995: Azure MachinePool: Accept OSImageID 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. --- apis/hive/v1/azure/machinepool.go | 10 +- .../crds/hive.openshift.io_machinepools.yaml | 11 +- hack/app-sre/saas-template.yaml | 11 +- pkg/azureclient/client.go | 3 - pkg/azureclient/mock/client_generated.go | 15 --- pkg/controller/machinepool/azureactuator.go | 98 +++++--------- .../machinepool/azureactuator_test.go | 120 +++++++++++------- .../hive/apis/hive/v1/azure/machinepool.go | 10 +- .../pkg/asset/machines/azure/azuremachines.go | 3 + .../pkg/asset/machines/azure/machines.go | 4 + 10 files changed, 151 insertions(+), 134 deletions(-) diff --git a/apis/hive/v1/azure/machinepool.go b/apis/hive/v1/azure/machinepool.go index dba6304795..f5235fd453 100644 --- a/apis/hive/v1/azure/machinepool.go +++ b/apis/hive/v1/azure/machinepool.go @@ -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 diff --git a/config/crds/hive.openshift.io_machinepools.yaml b/config/crds/hive.openshift.io_machinepools.yaml index fc9cef1975..b0c763c8b4 100644 --- a/config/crds/hive.openshift.io_machinepools.yaml +++ b/config/crds/hive.openshift.io_machinepools.yaml @@ -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. @@ -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 diff --git a/hack/app-sre/saas-template.yaml b/hack/app-sre/saas-template.yaml index 74b4c31ea0..78944687e9 100644 --- a/hack/app-sre/saas-template.yaml +++ b/hack/app-sre/saas-template.yaml @@ -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. @@ -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 diff --git a/pkg/azureclient/client.go b/pkg/azureclient/client.go index e066107825..da343c2f01 100644 --- a/pkg/azureclient/client.go +++ b/pkg/azureclient/client.go @@ -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. diff --git a/pkg/azureclient/mock/client_generated.go b/pkg/azureclient/mock/client_generated.go index 12d52796d9..d26da7634f 100644 --- a/pkg/azureclient/mock/client_generated.go +++ b/pkg/azureclient/mock/client_generated.go @@ -147,21 +147,6 @@ func (mr *MockClientMockRecorder) ListAllVirtualMachines(ctx, statusOnly any) *g return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ListAllVirtualMachines", reflect.TypeOf((*MockClient)(nil).ListAllVirtualMachines), ctx, statusOnly) } -// ListImagesByResourceGroup mocks base method. -func (m *MockClient) ListImagesByResourceGroup(ctx context.Context, resourceGroupName string) (azureclient.ImageListResultPage, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "ListImagesByResourceGroup", ctx, resourceGroupName) - ret0, _ := ret[0].(azureclient.ImageListResultPage) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// ListImagesByResourceGroup indicates an expected call of ListImagesByResourceGroup. -func (mr *MockClientMockRecorder) ListImagesByResourceGroup(ctx, resourceGroupName any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ListImagesByResourceGroup", reflect.TypeOf((*MockClient)(nil).ListImagesByResourceGroup), ctx, resourceGroupName) -} - // ListRecordSetsByZone mocks base method. func (m *MockClient) ListRecordSetsByZone(ctx context.Context, resourceGroupName, zone, suffix string) (azureclient.RecordSetPage, error) { m.ctrl.T.Helper() diff --git a/pkg/controller/machinepool/azureactuator.go b/pkg/controller/machinepool/azureactuator.go index 3fbdf04e74..f22c928599 100644 --- a/pkg/controller/machinepool/azureactuator.go +++ b/pkg/controller/machinepool/azureactuator.go @@ -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" @@ -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" + computePool.Platform.Azure.OSImage = installertypesazure.OSImage{ + Plan: plan, + Publisher: osImage.Publisher, + Offer: osImage.Offer, + SKU: osImage.SKU, + Version: osImage.Version, } } } @@ -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, @@ -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) { diff --git a/pkg/controller/machinepool/azureactuator_test.go b/pkg/controller/machinepool/azureactuator_test.go index ecae9f8ac4..c84ae1dcee 100644 --- a/pkg/controller/machinepool/azureactuator_test.go +++ b/pkg/controller/machinepool/azureactuator_test.go @@ -1,7 +1,6 @@ package machinepool import ( - "context" "fmt" "testing" @@ -37,13 +36,13 @@ func TestAzureActuator(t *testing.T) { expectedErr string expectedLogs []string }{ + // TODO: Add cases for OSImageID { name: "generate single machineset for single zone", clusterDeployment: testAzureClusterDeployment(), pool: testAzurePool(), mockAzureClient: func(mockCtrl *gomock.Controller, client *mockazure.MockClient) { mockGetVMCapabilities(mockCtrl, client, "V1,V2") - mockListImagesByResourceGroup(client, []compute.Image{testAzureImage(compute.HyperVGenerationTypesV1)}) mockListResourceSKUs(mockCtrl, client, []string{"zone1"}) }, expectedMachineSetReplicas: map[string]int64{ @@ -56,7 +55,6 @@ func TestAzureActuator(t *testing.T) { pool: testAzurePool(), mockAzureClient: func(mockCtrl *gomock.Controller, client *mockazure.MockClient) { mockGetVMCapabilities(mockCtrl, client, "V1,V2") - mockListImagesByResourceGroup(client, []compute.Image{testAzureImage(compute.HyperVGenerationTypesV1)}) mockListResourceSKUs(mockCtrl, client, []string{"zone1", "zone2", "zone3"}) }, expectedMachineSetReplicas: map[string]int64{ @@ -75,7 +73,6 @@ func TestAzureActuator(t *testing.T) { }(), mockAzureClient: func(mockCtrl *gomock.Controller, client *mockazure.MockClient) { mockGetVMCapabilities(mockCtrl, client, "V1,V2") - mockListImagesByResourceGroup(client, []compute.Image{testAzureImage(compute.HyperVGenerationTypesV1)}) }, expectedMachineSetReplicas: map[string]int64{ generateAzureMachineSetName("zone1"): 1, @@ -89,7 +86,6 @@ func TestAzureActuator(t *testing.T) { pool: testAzurePool(), mockAzureClient: func(mockCtrl *gomock.Controller, client *mockazure.MockClient) { mockGetVMCapabilities(mockCtrl, client, "V1,V2") - mockListImagesByResourceGroup(client, []compute.Image{testAzureImage(compute.HyperVGenerationTypesV1)}) mockListResourceSKUs(mockCtrl, client, []string{"zone1", "zone2", "zone3"}) }, expectedMachineSetReplicas: map[string]int64{ @@ -119,7 +115,6 @@ func TestAzureActuator(t *testing.T) { }(), mockAzureClient: func(mockCtrl *gomock.Controller, client *mockazure.MockClient) { mockGetVMCapabilities(mockCtrl, client, "V1,V2") - mockListImagesByResourceGroup(client, []compute.Image{testAzureImage(compute.HyperVGenerationTypesV1)}) mockListResourceSKUs(mockCtrl, client, []string{"zone1", "zone2", "zone3"}) }, expectedMachineSetReplicas: map[string]int64{ @@ -145,7 +140,6 @@ func TestAzureActuator(t *testing.T) { }(), mockAzureClient: func(mockCtrl *gomock.Controller, client *mockazure.MockClient) { mockGetVMCapabilities(mockCtrl, client, "V1,V2") - mockListImagesByResourceGroup(client, []compute.Image{testAzureImage(compute.HyperVGenerationTypesV1)}) mockListResourceSKUs(mockCtrl, client, []string{"zone1", "zone2", "zone3"}) }, expectedMachineSetReplicas: map[string]int64{ @@ -160,7 +154,6 @@ func TestAzureActuator(t *testing.T) { pool: testAzurePool(), mockAzureClient: func(mockCtrl *gomock.Controller, client *mockazure.MockClient) { mockGetVMCapabilities(mockCtrl, client, "V1,V2") - mockListImagesByResourceGroup(client, []compute.Image{testAzureImage(compute.HyperVGenerationTypesV1)}) mockListResourceSKUs(mockCtrl, client, []string{"zone1", "zone2", "zone3", "zone4", "zone5"}) }, expectedMachineSetReplicas: map[string]int64{ @@ -177,7 +170,6 @@ func TestAzureActuator(t *testing.T) { pool: testAzurePool(), mockAzureClient: func(mockCtrl *gomock.Controller, client *mockazure.MockClient) { mockGetVMCapabilities(mockCtrl, client, "V1,V2") - mockListImagesByResourceGroup(client, []compute.Image{testAzureImage(compute.HyperVGenerationTypesV1)}) mockListResourceSKUs(mockCtrl, client, []string{}) }, expectedMachineSetReplicas: map[string]int64{ @@ -191,7 +183,6 @@ func TestAzureActuator(t *testing.T) { pool: testAzurePool(), mockAzureClient: func(mockCtrl *gomock.Controller, client *mockazure.MockClient) { mockGetVMCapabilities(mockCtrl, client, "N/A") - mockListImagesByResourceGroup(client, []compute.Image{testAzureImage(compute.HyperVGenerationTypesV1)}) page := mockazure.NewMockResourceSKUsPage(mockCtrl) client.EXPECT().ListResourceSKUs(gomock.Any(), testRegion).Return(page, nil) page.EXPECT().NotDone().Return(false) @@ -207,7 +198,6 @@ func TestAzureActuator(t *testing.T) { pool: testAzurePool(), mockAzureClient: func(mockCtrl *gomock.Controller, client *mockazure.MockClient) { mockGetVMCapabilities(mockCtrl, client, "N/A") - mockListImagesByResourceGroup(client, []compute.Image{testAzureImage(compute.HyperVGenerationTypesV1)}) page := mockazure.NewMockResourceSKUsPage(mockCtrl) client.EXPECT().ListResourceSKUs(gomock.Any(), testRegion).Return(page, nil) page.EXPECT().NotDone().Return(true) @@ -232,7 +222,6 @@ func TestAzureActuator(t *testing.T) { pool: testAzurePool(), mockAzureClient: func(mockCtrl *gomock.Controller, client *mockazure.MockClient) { mockGetVMCapabilities(mockCtrl, client, "V1,V2") - mockListImagesByResourceGroup(client, []compute.Image{testAzureImage(compute.HyperVGenerationTypesV1)}) mockListResourceSKUs(mockCtrl, client, []string{"zone1", "zone2", "zone3"}) }, expectedMachineSetReplicas: map[string]int64{ @@ -251,7 +240,6 @@ func TestAzureActuator(t *testing.T) { pool: testAzurePool(), mockAzureClient: func(mockCtrl *gomock.Controller, client *mockazure.MockClient) { mockGetVMCapabilities(mockCtrl, client, "V1") - mockListImagesByResourceGroup(client, []compute.Image{testAzureImage(compute.HyperVGenerationTypesV1), testAzureImage(compute.HyperVGenerationTypesV2)}) mockListResourceSKUs(mockCtrl, client, []string{"zone1", "zone2", "zone3"}) }, expectedMachineSetReplicas: map[string]int64{ @@ -270,7 +258,6 @@ func TestAzureActuator(t *testing.T) { pool: testAzurePool(), mockAzureClient: func(mockCtrl *gomock.Controller, client *mockazure.MockClient) { mockGetVMCapabilities(mockCtrl, client, "V1,V2") - mockListImagesByResourceGroup(client, []compute.Image{testAzureImage(compute.HyperVGenerationTypesV1), testAzureImage(compute.HyperVGenerationTypesV2)}) mockListResourceSKUs(mockCtrl, client, []string{"zone1", "zone2", "zone3"}) }, expectedMachineSetReplicas: map[string]int64{ @@ -289,7 +276,6 @@ func TestAzureActuator(t *testing.T) { pool: testAzurePool(), mockAzureClient: func(mockCtrl *gomock.Controller, client *mockazure.MockClient) { mockGetVMCapabilities(mockCtrl, client, "V2") - mockListImagesByResourceGroup(client, []compute.Image{testAzureImage(compute.HyperVGenerationTypesV1), testAzureImage(compute.HyperVGenerationTypesV2)}) mockListResourceSKUs(mockCtrl, client, []string{"zone1", "zone2", "zone3"}) }, expectedMachineSetReplicas: map[string]int64{ @@ -332,6 +318,76 @@ func TestAzureActuator(t *testing.T) { Type: "MarketplaceWithPlan", }, }, + { + name: "machinepool provides managed osImageID", + clusterDeployment: testAzureClusterDeployment(), + pool: func() *hivev1.MachinePool { + mp := testAzurePool() + mp.Spec.Platform.Azure.OSImageID = "/subscriptions/foo/resourceGroups/bar/providers/Microsoft.Compute/images/baz" + return mp + }(), + mockAzureClient: func(mockCtrl *gomock.Controller, client *mockazure.MockClient) { + mockGetVMCapabilities(mockCtrl, client, "V1,V2") + mockListResourceSKUs(mockCtrl, client, []string{"zone1", "zone2", "zone3"}) + }, + expectedMachineSetReplicas: map[string]int64{ + generateAzureMachineSetName("zone1"): 1, + generateAzureMachineSetName("zone2"): 1, + generateAzureMachineSetName("zone3"): 1, + }, + expectedImage: &machineapi.Image{ + ResourceID: "/resourceGroups/bar/providers/Microsoft.Compute/images/baz", + }, + }, + { + name: "machinepool provides gallery osImageID", + clusterDeployment: testAzureClusterDeployment(), + pool: func() *hivev1.MachinePool { + mp := testAzurePool() + mp.Spec.Platform.Azure.OSImageID = "/subscriptions/foo/resourceGroups/bar/providers/Microsoft.Compute/galleries/gallery_baz/images/blah" + return mp + }(), + mockAzureClient: func(mockCtrl *gomock.Controller, client *mockazure.MockClient) { + mockGetVMCapabilities(mockCtrl, client, "V1,V2") + mockListResourceSKUs(mockCtrl, client, []string{"zone1", "zone2", "zone3"}) + }, + expectedMachineSetReplicas: map[string]int64{ + generateAzureMachineSetName("zone1"): 1, + generateAzureMachineSetName("zone2"): 1, + generateAzureMachineSetName("zone3"): 1, + }, + expectedImage: &machineapi.Image{ + ResourceID: "/resourceGroups/bar/providers/Microsoft.Compute/galleries/gallery_baz/images/blah", + }, + }, + { + name: "osImageID takes precedence over osImage", + clusterDeployment: testAzureClusterDeployment(), + pool: func() *hivev1.MachinePool { + mp := testAzurePool() + mp.Spec.Platform.Azure.OSImage = &hivev1azure.OSImage{ + Publisher: "testpublisher", + Offer: "testoffer", + SKU: "testsku", + Version: "testversion", + } + mp.Spec.Platform.Azure.OSImageID = "/subscriptions/foo/resourceGroups/bar/providers/Microsoft.Compute/galleries/gallery_baz/images/blah" + return mp + }(), + mockAzureClient: func(mockCtrl *gomock.Controller, client *mockazure.MockClient) { + mockGetVMCapabilities(mockCtrl, client, "V1,V2") + mockListResourceSKUs(mockCtrl, client, []string{"zone1", "zone2", "zone3"}) + }, + expectedMachineSetReplicas: map[string]int64{ + generateAzureMachineSetName("zone1"): 1, + generateAzureMachineSetName("zone2"): 1, + generateAzureMachineSetName("zone3"): 1, + }, + expectedImage: &machineapi.Image{ + ResourceID: "/resourceGroups/bar/providers/Microsoft.Compute/galleries/gallery_baz/images/blah", + }, + expectedLogs: []string{"ignoring OSImage since OSImageID was provided"}, + }, { name: "getVMCapabilities: no SKUs in region", clusterDeployment: testAzureClusterDeployment(), @@ -504,38 +560,12 @@ func TestAzureActuator(t *testing.T) { }, expectedErr: "error retrieving VM capabilities: error connecting to Azure client: error fetching SKU pages: SDK error", }, - { - name: "getImagesByResourceGroup errors on initial call", - clusterDeployment: testAzureClusterDeployment(), - pool: testAzurePool(), - mockAzureClient: func(mockCtrl *gomock.Controller, client *mockazure.MockClient) { - mockGetVMCapabilities(mockCtrl, client, "N/A") - client.EXPECT().ListImagesByResourceGroup(gomock.Any(), gomock.Any()).Return(nil, fmt.Errorf("SDK error")) - - }, - expectedErr: "error listing images by resourceGroup: foo-12345-rg: SDK error", - }, - { - name: "getImagesByResourceGroup errors getting second page", - clusterDeployment: testAzureClusterDeployment(), - pool: testAzurePool(), - mockAzureClient: func(mockCtrl *gomock.Controller, client *mockazure.MockClient) { - mockGetVMCapabilities(mockCtrl, client, "N/A") - pages := mockazure.NewMockImageListResultPage(mockCtrl) - client.EXPECT().ListImagesByResourceGroup(gomock.Any(), gomock.Any()).Return(pages, nil) - pages.EXPECT().NotDone().Return(true) - pages.EXPECT().Values().Return([]compute.Image{testAzureImage(compute.HyperVGenerationTypesV1)}) - pages.EXPECT().NextWithContext(gomock.Any()).Return(fmt.Errorf("SDK error")) - }, - expectedErr: "error listing images by resourceGroup: foo-12345-rg: SDK error", - }, { name: "getZones: error on initial call", clusterDeployment: testAzureClusterDeployment(), pool: testAzurePool(), mockAzureClient: func(mockCtrl *gomock.Controller, client *mockazure.MockClient) { mockGetVMCapabilities(mockCtrl, client, "N/A") - mockListImagesByResourceGroup(client, []compute.Image{testAzureImage(compute.HyperVGenerationTypesV1)}) mockazure.NewMockResourceSKUsPage(mockCtrl) client.EXPECT().ListResourceSKUs(gomock.Any(), testRegion).Return(nil, fmt.Errorf("SDK error")) }, @@ -547,7 +577,6 @@ func TestAzureActuator(t *testing.T) { pool: testAzurePool(), mockAzureClient: func(mockCtrl *gomock.Controller, client *mockazure.MockClient) { mockGetVMCapabilities(mockCtrl, client, "N/A") - mockListImagesByResourceGroup(client, []compute.Image{testAzureImage(compute.HyperVGenerationTypesV1)}) page := mockazure.NewMockResourceSKUsPage(mockCtrl) client.EXPECT().ListResourceSKUs(gomock.Any(), testRegion).Return(page, nil) page.EXPECT().NotDone().Return(true) @@ -659,13 +688,6 @@ func mockGetVMCapabilities(mockCtrl *gomock.Controller, client *mockazure.MockCl ) } -func mockListImagesByResourceGroup(client *mockazure.MockClient, images []compute.Image) { - resultPage := compute.NewImageListResultPage(compute.ImageListResult{Value: &images}, func(context.Context, compute.ImageListResult) (compute.ImageListResult, error) { - return compute.ImageListResult{}, nil - }) - client.EXPECT().ListImagesByResourceGroup(gomock.Any(), gomock.Any()).Return(&resultPage, nil) -} - func generateAzureMachineSetName(zone string) string { return fmt.Sprintf("%s-%s-%s%s", testInfraID, testPoolName, testRegion, zone) } diff --git a/vendor/github.com/openshift/hive/apis/hive/v1/azure/machinepool.go b/vendor/github.com/openshift/hive/apis/hive/v1/azure/machinepool.go index dba6304795..f5235fd453 100644 --- a/vendor/github.com/openshift/hive/apis/hive/v1/azure/machinepool.go +++ b/vendor/github.com/openshift/hive/apis/hive/v1/azure/machinepool.go @@ -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 diff --git a/vendor/github.com/openshift/installer/pkg/asset/machines/azure/azuremachines.go b/vendor/github.com/openshift/installer/pkg/asset/machines/azure/azuremachines.go index 36249b252f..d53f95918e 100644 --- a/vendor/github.com/openshift/installer/pkg/asset/machines/azure/azuremachines.go +++ b/vendor/github.com/openshift/installer/pkg/asset/machines/azure/azuremachines.go @@ -393,6 +393,9 @@ func capzImage(osImage aztypes.OSImage, azEnv aztypes.CloudEnvironment, confiden ThirdPartyImage: false, }, } + case strings.HasPrefix(rhcosImg, "/subscriptions/"): + // An explicit non-marketplace resource path was supplied. Use it as is. + return &capz.Image{ID: &rhcosImg} case rhcosImg == "" && !confidentialVM: // hive calls the machines function, but may pass an empty // string for rhcos. In which case, allow MAO to choose default. diff --git a/vendor/github.com/openshift/installer/pkg/asset/machines/azure/machines.go b/vendor/github.com/openshift/installer/pkg/asset/machines/azure/machines.go index fd39d4db5a..4f75c22b4b 100644 --- a/vendor/github.com/openshift/installer/pkg/asset/machines/azure/machines.go +++ b/vendor/github.com/openshift/installer/pkg/asset/machines/azure/machines.go @@ -455,5 +455,9 @@ func mapiImage(osImage azure.OSImage, azEnv azure.CloudEnvironment, confidential // for MAPI, by removing the /subspcription/ prefix. func trimSubscriptionPrefix(image string) string { rgIndex := strings.Index(image, "/resourceGroups/") + // Allow invalid inputs to fail downstream + if rgIndex < 0 { + return image + } return image[rgIndex:] }