Bug 2023434: Add Purchase Plan Name to Azure Machine Provider Spec #1057

patrickdillon · 2021-11-15T17:06:45Z

Edited: The original PR added support for both Marketplace and Shared Image Gallery images. I have decreased the scope to only Marketplace images, which means we just need a flag as to when to populate the purchase plan. To avoid a boolean, I have added a string field PurchasePlanName which when present should have the same value as the SKU, and would indicate we should create a plan. Using a string avoids using a boolean, and allows us space if there is ever drift in the Microsoft API where purchase plan name != SKU.

Upstream types: https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/d2b4e8ed2313adc270a0911d9d0c64e344f34cc4/api/v1beta1/types.go#L316-L398

The OpenShift cluster-api-provider-azure will need to implement logic based off the upstream (I will open a PR soon): https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/d2b4e8ed2313adc270a0911d9d0c64e344f34cc4/azure/services/virtualmachines/virtualmachines.go#L259-L287

patrickdillon · 2021-11-15T17:16:52Z

/bugzilla refresh

openshift-ci · 2021-11-15T17:17:26Z

@patrickdillon: This pull request references Bugzilla bug 2023434, which is invalid:

expected the bug to target the "4.10.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

Bug 2023434: Extend Azure Machine Provider Spec Image Support

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/test-infra repository.

patrickdillon · 2021-11-15T17:20:16Z

/bugzilla refresh

openshift-ci · 2021-11-15T17:20:25Z

@patrickdillon: This pull request references Bugzilla bug 2023434, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.10.0) matches configured target release for branch (4.10.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @sunzhaohua2

In response to this:

/bugzilla refresh

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/test-infra repository.

openshift-ci · 2021-11-15T17:25:04Z

@patrickdillon: An error was encountered querying GitHub for users with public email (zhsun@redhat.com) for bug 2023434 on the Bugzilla server at https://bugzilla.redhat.com. No known errors were detected, please see the full error message for details.

Full error message.


non-200 OK status code: 403 Forbidden body: "{\n  \"documentation_url\": \"https://docs.github.com/en/free-pro-team@latest/rest/overview/resources-in-the-rest-api#secondary-rate-limits\",\n  \"message\": \"You have exceeded a secondary rate limit. Please wait a few minutes before you try again.\"\n}\n"

Please contact an administrator to resolve this issue, then request a bug refresh with /bugzilla refresh.

In response to this:

/bugzilla refresh

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/test-infra repository.

patrickdillon · 2021-11-15T17:28:39Z

Looks like verify failed because I was using a newer version of protoc. Hopefully running the container generated update fixed it.

JoelSpeed · 2021-11-16T10:09:37Z

/lgtm

patrickdillon · 2021-11-16T14:46:00Z

/cherry-pick release-4.9

openshift-cherrypick-robot · 2021-11-16T14:46:01Z

@patrickdillon: once the present PR merges, I will cherry-pick it on top of release-4.9 in a new PR and assign it to you.

In response to this:

/cherry-pick release-4.9

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/test-infra repository.

patrickdillon · 2021-11-16T15:58:47Z

For use case, see openshift/cluster-api-provider-azure#242

deads2k · 2021-11-16T18:43:38Z

machine/v1beta1/types_azureprovider.go

+	// ThirdPartyImage indicates the image is published by a third party publisher and a Plan
+	// will be generated for it.
+	// +optional
+	ThirdPartyImage bool `json:"thirdPartyImage,omitempty"`


We avoid booleans in our API because they usually don't age well.

What would this be called if the value was ThirdParty? Is it an ImageAuthor? ImageSource? What is the alternative to third party?

ImageSource would probably be closest. From kubernetes-sigs/cluster-api-provider-azure#928 they define it as:
Add ThirdPartyImage flag to Marketplace image to indicate an image published by a third party publisher.

We already have a field called Publisher so it is not good to call it ImagePublisher, etc

If it not published by a third party, who is published by? microsoft, azure, redhat, someone else?

If it's not published by a third party, it's published by Microsoft or one of its cohorts (Canonical/Ubuntu or in a case that is relevant to OpenShift Windows Container the publisher is WindowsServer).

I can't find in-depth documentation on this. These repo docs are consistent with my reading the code that the distinguishing factor is that "third-party images" need a purchase plan. The cluster api provider has a conditional statement if third-party marketplace image then generate purchase plan.

So for our particular use case, when the azure cluster-api provider(CAPZ) is provisioning a new RHCOS node using a marketplace image, the CAPZ needs to include a purchase plan parameter, but for a first-party Windows Container node, a purchase plan must not be included.

So we expect the CAPZ to use this API to produce results like this:
RHCOS image published by Red Hat

virtualMachine := &compute.VirtualMachine{ Plan: &compute.Plan{ Publisher: to.StringPtr(image.Publisher), // redhat-rhel Name: to.StringPtr(image.SKU), // ocp-4 Product: to.StringPtr(image.Offer), // ocp-rhcos-worker }, VirtualMachineProperties: &compute.VirtualMachineProperties{ StorageProfile: &compute.StorageProfile{ ImageReference: imageReference := &compute.ImageReference{ Publisher: to.StringPtr(vmSpec.Image.Publisher), // redhat-rhel Offer: to.StringPtr(vmSpec.Image.Offer), // ocp-rhcos-worker Sku: to.StringPtr(vmSpec.Image.SKU), // ocp-4 Version: to.StringPtr(vmSpec.Image.Version), // 4.9.7 },

vs

Windows Server image

virtualMachine := &compute.VirtualMachine{ Plan: nil, VirtualMachineProperties: &compute.VirtualMachineProperties{ StorageProfile: &compute.StorageProfile{ ImageReference: imageReference := &compute.ImageReference{ Publisher: to.StringPtr(vmSpec.Image.Publisher), // MicrosoftWindowsServer Offer: to.StringPtr(vmSpec.Image.Offer), // WindowsServer Sku: to.StringPtr(vmSpec.Image.SKU), // datacenter-core-20h2-with-containers-smalldisk Version: to.StringPtr(vmSpec.Image.Version), // latest },

deads2k · 2021-11-16T18:45:23Z

machine/v1beta1/types_azureprovider.go

+	ThirdPartyImage bool `json:"thirdPartyImage,omitempty"`
+	// SharedGallery specifies an image to use from an Azure Shared Image Gallery
+	// +optional
+	SharedGallery *AzureSharedGalleryImage `json:"sharedGallery,omitempty"`


Is this related to the ThirdPartyImage? Are they specified together or just coincidental in this PR?

Is there a distinction between nil and empty? If there isn't, the configuration API (distinct from workload API) is more discoverable and less prone to error by not being a pointer.

Is this related to the ThirdPartyImage? Are they specified together or just coincidental in this PR?

ThirdPartyImage is only related to Marketplace Images. There are three types of images in the upstream API. Our API has two images types (ID & marketplace) flattened together and this is adding the third as a separate struct. I'm open to other approaches, but I believe we would have to deprecate fields.

Based on our current use case there is no distinction between nil & empty, so I'm fine with removing the pointer.

deads2k · 2021-11-16T18:45:57Z

machine/v1beta1/types_azureprovider.go

+	// will be generated for it.
+	// +optional
+	ThirdPartyImage bool `json:"thirdPartyImage,omitempty"`
+	// SharedGallery specifies an image to use from an Azure Shared Image Gallery


Why would I want to use this? What image is it overriding if present?

If you use this, you are specifying you are using an image from a Shared Gallery (rather than the Marketplace or by specifying an image ID, which is our current approach for installs).

If you are using a Shared Gallery image, publisher, sku, & offer from the parent struct will be used to populate a required purchase plan. The fields from this struct would be used to create a ref to the image.

deads2k · 2021-11-16T18:46:28Z

machine/v1beta1/types_azureprovider.go

@@ -211,6 +218,18 @@ type SecurityProfile struct {
 	EncryptionAtHost *bool `json:"encryptionAtHost,omitempty"`
 }

+// AzureSharedGalleryImage defines an image in a Shared Image Gallery to use for VM creation.
+type AzureSharedGalleryImage struct {


inside this type, is empty string a valid value that is distinct from "no opinion"?

empty string would only be valid if all the fields were empty. If AzureSharedGallery images are to be used, all fields are required.

deads2k · 2021-11-16T18:47:10Z

machine/v1beta1/types_azureprovider.go

@@ -211,6 +218,18 @@ type SecurityProfile struct {
 	EncryptionAtHost *bool `json:"encryptionAtHost,omitempty"`
 }

+// AzureSharedGalleryImage defines an image in a Shared Image Gallery to use for VM creation.
+type AzureSharedGalleryImage struct {
+	// SubscriptionID is the identifier of the subscription that contains the shared image gallery


thoughout, links to the azure concepts would be helpful.

patrickdillon · 2021-11-16T21:59:02Z

/hold

We don't need to support shared image galleries, so I can remove that and greatly simplify this API bump.

Just need to figure out name/type for the current bool ThirdPartyImage.

openshift-ci · 2021-11-17T15:29:38Z

@patrickdillon: This pull request references Bugzilla bug 2023434, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.10.0) matches configured target release for branch (4.10.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @sunzhaohua2

In response to this:

Bug 2023434: Extend Azure Machine Provider Spec Image Support

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/test-infra repository.

JoelSpeed · 2021-11-17T15:33:44Z

This seems ok to me, though while this is convertible to the upstream API should we need to for future projects (eg. if purchasePlaneName != "" { thirdPartyImage = true }), this could be confusing from an end user perspective.

@deads2k The change to make this not a bool doesn't really seem to add value to me, since we are saying the value should match the SKU (we are effectively treating it as a bool, rather than an enum). The only way I can see this being advantageous is if, somewhere down the line, the purchase plan name doesn't match the SKU.
I can see the advantage of the bool conversion for other fields we've discussed in other PRs recently (eg AutomaticRestart = true|false to RestartPolicy = RemainStopped|RestartAlways), but this one doesn't seem to add the same value, perhaps I'm missing the nuance on this one.

patrickdillon · 2021-11-17T16:18:56Z

This seems ok to me, though while this is convertible to the upstream API should we need to for future projects (eg. if purchasePlaneName != "" { thirdPartyImage = true }), this could be confusing from an end user perspective.

@deads2k The change to make this not a bool doesn't really seem to add value to me, since we are saying the value should match the SKU (we are effectively treating it as a bool, rather than an enum). The only way I can see this being advantageous is if, somewhere down the line, the purchase plan name doesn't match the SKU. I can see the advantage of the bool conversion for other fields we've discussed in other PRs recently (eg AutomaticRestart = true|false to RestartPolicy = RemainStopped|RestartAlways), but this one doesn't seem to add the same value, perhaps I'm missing the nuance on this one.

I agree with Joel. I think the boolean for thirdPartyImage makes more sense. Potentially we could change it to something like usePurchasePlan but I would lean toward aligning with upstream.

patrickdillon · 2021-11-17T17:48:35Z

/hold cancel

JoelSpeed · 2021-11-18T15:33:15Z

machine/v1beta1/types_azureprovider.go

+	// For more information about purchase plans, see:
+	// https://docs.microsoft.com/en-us/azure/virtual-machines/linux/cli-ps-findimage#check-the-purchase-plan-information
+	// +optional
+	Type string `json:"type,omitempty"`


I think this works from a description point of view, I would be tempted to add the valid values as constants and make this a typed string, this should then give us constsants to use during the implementation which should make things cleaner

type AzureImageType string const ( AzureImageTypeID AzureImageType = "ID" AzureImageTypeMarketplaceNoPlan AzureImageType = "MarketPlaceNoPlan" AzureImageTypeMarketplaceWithPlan AzureImageType = "MarketPlaceWithPlan" )

WDYT?

machine/v1beta1/types_azureprovider.go

With Azure marketplace images, some images require that the subscription accept the terms of a purchase plan before using; and that purchase plan information must be included in Azure API calls when provisioning a VM. Images that do not require purchase plans must not submit purchase plan info. This adds an image type to distinguish between Marketplace images with or without purchase plans. It could be used in the future to support images from galleries.

deads2k · 2021-11-18T20:54:49Z

/approve
/assign @JoelSpeed

JoelSpeed

/lgtm

openshift-ci · 2021-11-19T10:12:18Z

@patrickdillon: All pull requests linked via external trackers have merged:

openshift/api#1057

Bugzilla bug 2023434 has been moved to the MODIFIED state.

In response to this:

Bug 2023434: Add Purchase Plan Name to Azure Machine Provider Spec

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/test-infra repository.

openshift-ci · 2021-11-19T10:12:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, JoelSpeed, patrickdillon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [deads2k]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-cherrypick-robot · 2021-11-19T10:12:56Z

@patrickdillon: #1057 failed to apply on top of branch "release-4.9":

Applying: Add Image Type to Azure Machine Provider Spec
Using index info to reconstruct a base tree...
A	machine/v1beta1/types_azureprovider.go
A	machine/v1beta1/zz_generated.swagger_doc_generated.go
Falling back to patching base and 3-way merge...
CONFLICT (modify/delete): machine/v1beta1/zz_generated.swagger_doc_generated.go deleted in HEAD and modified in Add Image Type to Azure Machine Provider Spec. Version Add Image Type to Azure Machine Provider Spec of machine/v1beta1/zz_generated.swagger_doc_generated.go left in tree.
CONFLICT (modify/delete): machine/v1beta1/types_azureprovider.go deleted in HEAD and modified in Add Image Type to Azure Machine Provider Spec. Version Add Image Type to Azure Machine Provider Spec of machine/v1beta1/types_azureprovider.go left in tree.
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Add Image Type to Azure Machine Provider Spec
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-4.9

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/test-infra repository.

openshift-ci bot requested review from adambkaplan and alexander-demicev November 15, 2021 17:12

patrickdillon changed the title ~~Extend Azure Machine Provider Spec Image Support~~ Bug 2023434: Extend Azure Machine Provider Spec Image Support Nov 15, 2021

openshift-ci bot added bugzilla/severity-urgent Referenced Bugzilla bug's severity is urgent for the branch this PR is targeting. bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Nov 15, 2021

openshift-ci bot added bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. and removed bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Nov 15, 2021

openshift-ci bot requested a review from sunzhaohua2 November 15, 2021 17:20

patrickdillon force-pushed the azure-machine-spec branch from 5da375a to f64c426 Compare November 15, 2021 17:27

openshift-ci bot assigned JoelSpeed Nov 16, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 16, 2021

patrickdillon mentioned this pull request Nov 16, 2021

Add VM Support for Marketplace Purchase Plans openshift/cluster-api-provider-azure#242

Merged

deads2k reviewed Nov 16, 2021

View reviewed changes

patrickdillon mentioned this pull request Nov 16, 2021

Vendor: Bump API for Azure Marketplace Images openshift/installer#5393

Merged

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 16, 2021

patrickdillon force-pushed the azure-machine-spec branch from f64c426 to dadedc5 Compare November 17, 2021 15:24

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 17, 2021

patrickdillon changed the title ~~Bug 2023434: Extend Azure Machine Provider Spec Image Support~~ Bug 2023434: Add Purchase Plan Name to Azure Machine Provider Spec Nov 17, 2021

patrickdillon force-pushed the azure-machine-spec branch 2 times, most recently from be881cf to 404f366 Compare November 17, 2021 16:57

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 17, 2021

patrickdillon mentioned this pull request Nov 17, 2021

Azure Marketplace Machine Provider Bump openshift/machine-api-operator#956

Merged

patrickdillon force-pushed the azure-machine-spec branch from 404f366 to 43b6214 Compare November 18, 2021 15:19

JoelSpeed reviewed Nov 18, 2021

View reviewed changes

deads2k reviewed Nov 18, 2021

View reviewed changes

machine/v1beta1/types_azureprovider.go Show resolved Hide resolved

patrickdillon force-pushed the azure-machine-spec branch from 43b6214 to 0dd4457 Compare November 18, 2021 19:55

patrickdillon force-pushed the azure-machine-spec branch from 0dd4457 to 62d90f1 Compare November 18, 2021 20:25

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 18, 2021

JoelSpeed reviewed Nov 19, 2021

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 19, 2021

openshift-merge-robot merged commit 4e474ed into openshift:master Nov 19, 2021

patrickdillon mentioned this pull request Nov 22, 2021

Bug 2025697: API: Update Machine Provider Spec Image Type openshift/cluster-api-provider-azure#243

Merged

patrickdillon mentioned this pull request Sep 7, 2022

[CFE-465] Add additionalTrustBundlePolicy field to allow CA bundle propagation openshift/installer#6232

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 2023434: Add Purchase Plan Name to Azure Machine Provider Spec #1057

Bug 2023434: Add Purchase Plan Name to Azure Machine Provider Spec #1057

patrickdillon commented Nov 15, 2021 •

edited

Loading

patrickdillon commented Nov 15, 2021

openshift-ci bot commented Nov 15, 2021

patrickdillon commented Nov 15, 2021

openshift-ci bot commented Nov 15, 2021

openshift-ci bot commented Nov 15, 2021

patrickdillon commented Nov 15, 2021

JoelSpeed commented Nov 16, 2021

patrickdillon commented Nov 16, 2021

openshift-cherrypick-robot commented Nov 16, 2021

patrickdillon commented Nov 16, 2021

deads2k Nov 16, 2021

patrickdillon Nov 16, 2021

deads2k Nov 17, 2021

patrickdillon Nov 17, 2021 •

edited

Loading

deads2k Nov 16, 2021

patrickdillon Nov 16, 2021

deads2k Nov 16, 2021

patrickdillon Nov 16, 2021

deads2k Nov 16, 2021

patrickdillon Nov 16, 2021

deads2k Nov 16, 2021

patrickdillon commented Nov 16, 2021

openshift-ci bot commented Nov 17, 2021

JoelSpeed commented Nov 17, 2021

patrickdillon commented Nov 17, 2021

patrickdillon commented Nov 17, 2021

JoelSpeed Nov 18, 2021

deads2k commented Nov 18, 2021

JoelSpeed left a comment

openshift-ci bot commented Nov 19, 2021

openshift-ci bot commented Nov 19, 2021

openshift-cherrypick-robot commented Nov 19, 2021

Bug 2023434: Add Purchase Plan Name to Azure Machine Provider Spec #1057

Bug 2023434: Add Purchase Plan Name to Azure Machine Provider Spec #1057

Conversation

patrickdillon commented Nov 15, 2021 • edited Loading

patrickdillon commented Nov 15, 2021

openshift-ci bot commented Nov 15, 2021

patrickdillon commented Nov 15, 2021

openshift-ci bot commented Nov 15, 2021

openshift-ci bot commented Nov 15, 2021

patrickdillon commented Nov 15, 2021

JoelSpeed commented Nov 16, 2021

patrickdillon commented Nov 16, 2021

openshift-cherrypick-robot commented Nov 16, 2021

patrickdillon commented Nov 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickdillon Nov 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickdillon commented Nov 16, 2021

openshift-ci bot commented Nov 17, 2021

JoelSpeed commented Nov 17, 2021

patrickdillon commented Nov 17, 2021

patrickdillon commented Nov 17, 2021

Choose a reason for hiding this comment

deads2k commented Nov 18, 2021

JoelSpeed left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Nov 19, 2021

openshift-ci bot commented Nov 19, 2021

openshift-cherrypick-robot commented Nov 19, 2021

patrickdillon commented Nov 15, 2021 •

edited

Loading

patrickdillon Nov 17, 2021 •

edited

Loading