`azurerm_orchestrated_virtual_machine_scale_set` - support for the `upgrade_mode` and `rolling_upgrade_policy` properties by ms-zhenhua · Pull Request #28354 · hashicorp/terraform-provider-azurerm

ms-zhenhua · 2024-12-20T07:35:28Z

Community Note

Please vote on this PR by adding a 👍 reaction to the original PR to help the community and maintainers prioritize for review
Please do not leave comments along the lines of "+1", "me too" or "any updates", they generate extra noise for PR followers and do not help prioritize for review

Description

Support UpgradePolicy for Flex VMSS.

PR Checklist

I have followed the guidelines in our Contributing Documentation.
I have checked to ensure there aren't other open Pull Requests for the same update/change.
I have checked if my changes close any open issues. If so please include appropriate closing keywords below.
I have updated/added Documentation as required written in a helpful and kind way to assist users that may be unfamiliar with the resource / data source.
I have used a meaningful PR title to help maintainers and other users understand this change and help prevent duplicate work.
For example: “resource_name_here - description of change e.g. adding property new_property_name_here”

Changes to existing Resource / Data Source

I have added an explanation of what my changes do and why I'd like you to include them (This may be covered by linking to an issue above, but may benefit from additional explanation).
I have written new tests for my resource or datasource changes & updated any relevent documentation.
I have successfully run tests with my changes locally. If not, please provide details on testing challenges that prevented you running the tests.
[n/a] (For changes that include a state migration only). I have manually tested the migration path between relevant versions of the provider.

Testing

My submission includes Test coverage as described in the Contribution Guide and the tests pass. (if this is not possible for any reason, please include details of why you did or could not add test coverage)

--- PASS: TestAccOrchestratedVirtualMachineScaleSet_otherUpgradePolicyErrorValidation (28.76s)
--- PASS: TestAccOrchestratedVirtualMachineScaleSet_otherRollingUpgrade (308.14s)
--- PASS: TestAccOrchestratedVirtualMachineScaleSet_otherUpgradePolicy (349.25s)
PASS

Note: Please follow this doc to run Register-AzProviderFeature -FeatureName MaxSurgeRollingUpgrade -ProviderNamespace Microsoft.Compute to enable MaxSurge upgrades to make TestAccOrchestratedVirtualMachineScaleSet_otherUpgradePolicy pass

Change Log

Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.

azurerm_orchestrated_virtual_machine_scale_set - support for the upgrade_mode and rolling_upgrade_policy properties [Support for rolling_upgrade_policy in azurerm_orchestrated_virtual_machine_scale_set #28270]

This is a (please select all that apply):

Bug Fix
New Feature (ie adding a service, resource, or data source)
Enhancement
Breaking Change

Related Issue(s)

Fixes #28270

Note

If this PR changes meaningfully during the course of review please update the title and description as required.

mimckitt · 2025-02-27T19:45:08Z

@rcskosir anything we can do to get a reviewer on this PR :)

sreallymatt

Hi @ms-zhenhua, I've left a couple comments inline for you to review, thanks!

internal/services/compute/orchestrated_virtual_machine_scale_set_resource.go

sreallymatt

Hi @ms-zhenhua, I've gone back over this PR and have left some comments, there was also a panic when I ran the tests for this PR. Would you mind taking a look?

sreallymatt · 2025-03-06T19:22:37Z

internal/services/compute/orchestrated_virtual_machine_scale_set_resource.go

+				hasHealthExtension := false
+				if v, ok := diff.GetOk("extension"); ok {
+					var err error
+					_, hasHealthExtension, err = expandOrchestratedVirtualMachineScaleSetExtensions(v.(*pluginsdk.Set).List())


When running the tests there was a panic that seems like it may be related to this call to expandOrchestratedVirtualMachineScaleSetExtensions

The test that failed: TestAccOrchestratedVirtualMachineScaleSet_extensionProtectedSettingsFromKeyVault

Could you investigate?

apologies, my mistake. I had intended to make this validation take effect in plan stage. However, the value of expandProtectedSettingsFromKeyVaultVMSS does not exist in plan stage. Seems it is unsafe to check non-primitive type of attributes in plan stage, I have moved the validation into Create method. Thank you for pointing out this issue.

sreallymatt · 2025-03-06T19:24:31Z

internal/services/compute/orchestrated_virtual_machine_scale_set_resource.go

+
+				// Virtual Machine Scale Set with Flexible Orchestration Mode and 'Rolling' upgradeMode must have Health Extension Present
+				if upgradeMode == virtualmachinescalesets.UpgradeModeRolling && !hasHealthExtension {
+					return fmt.Errorf("a health extension must be specified when `upgrade_mode` is set to Rolling")


Minor suggestion to stay consistent with how we usually format error messages, will require an update to the test on ln313 of orchestrated_virtual_machine_scale_set_resource_other_test.go as well

Suggested change

return fmt.Errorf("a health extension must be specified when `upgrade_mode` is set to Rolling")

return fmt.Errorf("a health extension must be specified when `upgrade_mode` is set to `Rolling`")

updated. The new code is in resourceOrchestratedVirtualMachineScaleSetCreate method.

sreallymatt · 2025-03-06T19:24:58Z

internal/services/compute/orchestrated_virtual_machine_scale_set_resource.go

+	rollingUpgradePolicyRaw := d.Get("rolling_upgrade_policy").([]interface{})
+	rollingUpgradePolicy, err := ExpandVirtualMachineScaleSetRollingUpgradePolicy(rollingUpgradePolicyRaw, len(zones) > 0, false)


Since it doesn't look like rollingUpgradePolicyRaw is used elsewhere in this function we can simplify this

Suggested change

rollingUpgradePolicyRaw := d.Get("rolling_upgrade_policy").([]interface{})

rollingUpgradePolicy, err := ExpandVirtualMachineScaleSetRollingUpgradePolicy(rollingUpgradePolicyRaw, len(zones) > 0, false)

rollingUpgradePolicy, err := ExpandVirtualMachineScaleSetRollingUpgradePolicy(d.Get("rolling_upgrade_policy").([]interface{}), len(zones) > 0, false)

updated. Thanks.

sreallymatt · 2025-03-06T19:26:24Z

internal/services/compute/orchestrated_virtual_machine_scale_set_resource.go

+	rollingUpgradePolicyRaw := d.Get("rolling_upgrade_policy").([]interface{})
+	rollingUpgradePolicy, err := ExpandVirtualMachineScaleSetRollingUpgradePolicy(rollingUpgradePolicyRaw, len(zones) > 0, false)
+	if err != nil {
+		return err


Could we wrap this error?

Suggested change

return err

return fmt.Errorf("expanding `rolling_upgrade_policy`: %+v", err)

updated. Thanks

sreallymatt · 2025-03-06T19:26:59Z

internal/services/compute/orchestrated_virtual_machine_scale_set_resource.go

+		if existing.Model.Properties.UpgradePolicy == nil {
+			upgradePolicy = virtualmachinescalesets.UpgradePolicy{
+				Mode: pointer.To(virtualmachinescalesets.UpgradeMode(d.Get("upgrade_mode").(string))),
+			}
+		} else {
+			upgradePolicy = *existing.Model.Properties.UpgradePolicy
+			upgradePolicy.Mode = pointer.To(virtualmachinescalesets.UpgradeMode(d.Get("upgrade_mode").(string)))
+		}


I think we could simplify this to the below, what do you think?

Suggested change

if existing.Model.Properties.UpgradePolicy == nil {

upgradePolicy = virtualmachinescalesets.UpgradePolicy{

Mode: pointer.To(virtualmachinescalesets.UpgradeMode(d.Get("upgrade_mode").(string))),

}

} else {

upgradePolicy = *existing.Model.Properties.UpgradePolicy

upgradePolicy.Mode = pointer.To(virtualmachinescalesets.UpgradeMode(d.Get("upgrade_mode").(string)))

}

if existing.Model.Properties.UpgradePolicy != nil {

upgradePolicy = *existing.Model.Properties.UpgradePolicy

}

upgradePolicy.Mode = pointer.To(virtualmachinescalesets.UpgradeMode(d.Get("upgrade_mode").(string)))

updated. Thanks.

sreallymatt · 2025-03-06T19:28:29Z

internal/services/compute/orchestrated_virtual_machine_scale_set_resource.go

+		zones := zones.ExpandUntyped(d.Get("zones").(*schema.Set).List())
+		rollingUpgradePolicy, err := ExpandVirtualMachineScaleSetRollingUpgradePolicy(rollingRaw, len(zones) > 0, false)
+		if err != nil {
+			return err


could we wrap this error message?

Suggested change

return err

return fmt.Errorf("expanding `rolling_upgrade_policy`: %+v", err)

updated. Thanks.

sreallymatt · 2025-03-06T19:30:25Z

internal/services/compute/orchestrated_virtual_machine_scale_set_resource_other_test.go

+	data.ResourceTest(t, r, []acceptance.TestStep{
+		{
+			Config:      r.otherRollingWithoutHealthExtension(data),
+			ExpectError: regexp.MustCompile("a health extension must be specified when `upgrade_mode` is set to Rolling"),


Suggested change

ExpectError: regexp.MustCompile("a health extension must be specified when `upgrade_mode` is set to Rolling"),

ExpectError: regexp.MustCompile("a health extension must be specified when `upgrade_mode` is set to `Rolling`"),

updated. Thanks.

sreallymatt · 2025-03-06T19:30:44Z

website/docs/r/orchestrated_virtual_machine_scale_set.html.markdown


 * `termination_notification` - (Optional) A `termination_notification` block as defined below.

+* `upgrade_mode` - (Optional) Specifies how Upgrades (e.g. changing the Image/SKU) should be performed to Virtual Machine Instances. Possible values are `Automatic`, `Manual` and `Rolling`. Defaults to `Manual`. Changing this forces a new resource to be created.


Suggested change

* `upgrade_mode` - (Optional) Specifies how Upgrades (e.g. changing the Image/SKU) should be performed to Virtual Machine Instances. Possible values are `Automatic`, `Manual` and `Rolling`. Defaults to `Manual`. Changing this forces a new resource to be created.

* `upgrade_mode` - (Optional) Specifies how upgrades (e.g. changing the Image/SKU) should be performed to Virtual Machine Instances. Possible values are `Automatic`, `Manual` and `Rolling`. Defaults to `Manual`. Changing this forces a new resource to be created.

updated. Thanks.

sreallymatt · 2025-03-06T19:30:51Z

website/docs/r/orchestrated_virtual_machine_scale_set.html.markdown


 * `proximity_placement_group_id` - (Optional) The ID of the Proximity Placement Group which the Virtual Machine should be assigned to. Changing this forces a new resource to be created.

+* `rolling_upgrade_policy` - (Optional) A `rolling_upgrade_policy` block as defined below. This is Required and can only be specified when `upgrade_mode` is set to `Automatic` or `Rolling`. Changing this forces a new resource to be created.


I think this might make it more clear that this cannot be specified only when upgrade_mode is manual and required otherwise?

Suggested change

* `rolling_upgrade_policy` - (Optional) A `rolling_upgrade_policy` block as defined below. This is Required and can only be specified when `upgrade_mode` is set to `Automatic` or `Rolling`. Changing this forces a new resource to be created.

* `rolling_upgrade_policy` - (Optional) A `rolling_upgrade_policy` block as defined below. This is Required when `upgrade_mode` is set to `Automatic` or `Rolling` and cannot be specified when `upgrade_mode` is set to `Manual`. Changing this forces a new resource to be created.

since it is not required by Automatic, I updated it as below:

rolling_upgrade_policy - (Optional) A rolling_upgrade_policy block as defined below. This is Required when upgrade_mode is set to Rolling and cannot be specified when upgrade_mode is set to Manual. Changing this forces a new resource to be created.

sreallymatt · 2025-03-06T19:31:00Z

website/docs/r/orchestrated_virtual_machine_scale_set.html.markdown

+
+* `max_unhealthy_upgraded_instance_percent` - (Required) The maximum percentage of upgraded virtual machine instances that can be found to be in an unhealthy state. This check will happen after each batch is upgraded. If this percentage is ever exceeded, the rolling update aborts.
+
+* `pause_time_between_batches` - (Required) The wait time between completing the update for all virtual machines in one batch and starting the next batch. The time duration should be specified in ISO 8601 format.


Suggested change

* `pause_time_between_batches` - (Required) The wait time between completing the update for all virtual machines in one batch and starting the next batch. The time duration should be specified in ISO 8601 format.

* `pause_time_between_batches` - (Required) The wait time between completing the update for all virtual machines in one batch and starting the next batch. The time duration should be specified in ISO 8601 duration format.

updated. Thanks.

ms-zhenhua · 2025-03-17T07:45:08Z

Hi @sreallymatt ,

Thank you for the review. I have resolved all the comments. Besides, I set a default value for upgrade_mode in read method to make some legacy testcases like TestAccOrchestratedVirtualMachineScaleSet_legacyxxx pass. The reason is the API response will not take upgrade_mode if the request does not have VirtualMachineProfile, which is by design of Azure backend service. Could you please have another review? Thanks.

sreallymatt

Thanks for making those changes @ms-zhenhua, I left one comment on the CustomizeDiff for your consideration

sreallymatt · 2025-03-31T19:43:11Z

internal/services/compute/orchestrated_virtual_machine_scale_set_resource.go

+				shouldHaveRollingUpgradePolicy := upgradeMode == virtualmachinescalesets.UpgradeModeAutomatic || upgradeMode == virtualmachinescalesets.UpgradeModeRolling
+				if !shouldHaveRollingUpgradePolicy && len(rollingUpgradePolicyRaw) > 0 {
+					return fmt.Errorf("a `rolling_upgrade_policy` block cannot be specified when `upgrade_mode` is set to %q", string(upgradeMode))
+				}
+				shouldHaveRollingUpgradePolicy = upgradeMode == virtualmachinescalesets.UpgradeModeRolling
+				if shouldHaveRollingUpgradePolicy && len(rollingUpgradePolicyRaw) == 0 {
+					return fmt.Errorf("a `rolling_upgrade_policy` block must be specified when `upgrade_mode` is set to %q", string(upgradeMode))
+				}


I think this might be a little more clear if we're explicit about what we're checking for rather than checking for the inverse of upgradeMode == virtualmachinescalesets.UpgradeModeAutomatic || upgradeMode == virtualmachinescalesets.UpgradeModeRolling, WDYT?

Suggested change

shouldHaveRollingUpgradePolicy := upgradeMode == virtualmachinescalesets.UpgradeModeAutomatic || upgradeMode == virtualmachinescalesets.UpgradeModeRolling

if !shouldHaveRollingUpgradePolicy && len(rollingUpgradePolicyRaw) > 0 {

return fmt.Errorf("a `rolling_upgrade_policy` block cannot be specified when `upgrade_mode` is set to %q", string(upgradeMode))

}

shouldHaveRollingUpgradePolicy = upgradeMode == virtualmachinescalesets.UpgradeModeRolling

if shouldHaveRollingUpgradePolicy && len(rollingUpgradePolicyRaw) == 0 {

return fmt.Errorf("a `rolling_upgrade_policy` block must be specified when `upgrade_mode` is set to %q", string(upgradeMode))

}

if upgradeMode == virtualmachinescalesets.UpgradeModeManual && len(rollingUpgradePolicyRaw) > 0 {

return fmt.Errorf("a `rolling_upgrade_policy` block cannot be specified when `upgrade_mode` is set to `%s`", string(upgradeMode))

}

if upgradeMode == virtualmachinescalesets.UpgradeModeRolling && len(rollingUpgradePolicyRaw) == 0 {

return fmt.Errorf("a `rolling_upgrade_policy` block must be specified when `upgrade_mode` is set to `%s`", string(upgradeMode))

}

That makes sense. Updated.

ms-zhenhua · 2025-04-01T02:59:13Z

Hi @sreallymatt ,

Thanks for your review. I have updated the code. Could you please have another review?

sreallymatt

Thanks @ms-zhenhua, LGTM!

* new changelog for 4.26.0 * Update CHANGELOG.md #29212 * Update CHANGELOG.md #28786 * Update CHANGELOG.md for #29095 * Update CHANGELOG.md #29225 * Update CHANGELOG.md #29168 * Update CHANGELOG.md #28890 * Update CHANGELOG.md for #28689 * Update CHANGELOG.md #28354 * Update CHANGELOG.md #29240 * Update CHANGELOG.md for #29211 * Update CHANGELOG.md #29214 * Update CHANGELOG.md for #29209 * Update CHANGELOG.md for 28690 * Update CHANGELOG.md for #28577 * Update for #29217 #29144 #29145 * Update CHANGELOG.md for #29185 * prep for release --------- Co-authored-by: sreallymatt <[email protected]> Co-authored-by: catriona-m <[email protected]> Co-authored-by: Wyatt Fry <[email protected]> Co-authored-by: Matthew Frahry <[email protected]> Co-authored-by: Wodans Son <[email protected]> Co-authored-by: stephybun <[email protected]>

github-actions · 2025-05-02T02:20:48Z

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

support upgrade policy to Flex VMSS

755a242

github-actions bot added enhancement documentation size/XL labels Dec 20, 2024

ms-zhenhua changed the title ~~azurerm_orchestrated_virtual_machine_scale_set - support upgrade_mode and rolling_upgrade_policy~~ azurerm_orchestrated_virtual_machine_scale_set - support for the property upgrade_mode and rolling_upgrade_policy Dec 20, 2024

ms-zhenhua changed the title ~~azurerm_orchestrated_virtual_machine_scale_set - support for the property upgrade_mode and rolling_upgrade_policy~~ azurerm_orchestrated_virtual_machine_scale_set - support for the upgrade_mode and rolling_upgrade_policy properties Dec 20, 2024

ms-zhenhua marked this pull request as ready for review December 20, 2024 08:06

ms-zhenhua requested a review from a team as a code owner December 20, 2024 08:06

rcskosir mentioned this pull request Jan 7, 2025

Support for rolling_upgrade_policy in azurerm_orchestrated_virtual_machine_scale_set #28270

Closed

1 task

sreallymatt reviewed Mar 6, 2025

View reviewed changes

internal/services/compute/orchestrated_virtual_machine_scale_set_resource.go Show resolved Hide resolved

github-actions bot added the waiting-response label Mar 6, 2025

sreallymatt requested changes Mar 6, 2025

View reviewed changes

github-actions bot assigned sreallymatt Mar 6, 2025

resolved comments

fc497cd

github-actions bot removed the waiting-response label Mar 7, 2025

resolved comments

056a698

ms-zhenhua marked this pull request as draft March 7, 2025 06:47

ms-zhenhua added 2 commits March 12, 2025 13:29

resolve legacy breaking change

1c53faf

resolve comments

e072af8

ms-zhenhua marked this pull request as ready for review March 17, 2025 07:32

ms-zhenhua requested a review from sreallymatt March 17, 2025 07:47

sreallymatt reviewed Mar 31, 2025

View reviewed changes

github-actions bot added the waiting-response label Mar 31, 2025

resolved comments

bbab085

github-actions bot removed the waiting-response label Apr 1, 2025

ms-zhenhua requested a review from sreallymatt April 1, 2025 02:59

sreallymatt approved these changes Apr 1, 2025

View reviewed changes

sreallymatt merged commit c6c6d53 into hashicorp:main Apr 1, 2025
34 checks passed

github-actions bot added this to the v4.26.0 milestone Apr 1, 2025

sreallymatt added a commit that referenced this pull request Apr 1, 2025

Update CHANGELOG.md #28354

2d9ba85

tobiasehlert mentioned this pull request Apr 10, 2025

[AVM Module Issue]: azurerm_orchestrated_virtual_machine_scale_set resource changed in AzureRM 4.26.0 Azure/terraform-azurerm-avm-res-compute-virtualmachinescaleset#131

Closed

1 task

github-actions bot locked as resolved and limited conversation to collaborators May 2, 2025

	return fmt.Errorf("a health extension must be specified when `upgrade_mode` is set to Rolling")
	return fmt.Errorf("a health extension must be specified when `upgrade_mode` is set to `Rolling`")

		rollingUpgradePolicyRaw := d.Get("rolling_upgrade_policy").([]interface{})
		rollingUpgradePolicy, err := ExpandVirtualMachineScaleSetRollingUpgradePolicy(rollingUpgradePolicyRaw, len(zones) > 0, false)

	return err
	return fmt.Errorf("expanding `rolling_upgrade_policy`: %+v", err)

	ExpectError: regexp.MustCompile("a health extension must be specified when `upgrade_mode` is set to Rolling"),
	ExpectError: regexp.MustCompile("a health extension must be specified when `upgrade_mode` is set to `Rolling`"),


		* `termination_notification` - (Optional) A `termination_notification` block as defined below.

		* `upgrade_mode` - (Optional) Specifies how Upgrades (e.g. changing the Image/SKU) should be performed to Virtual Machine Instances. Possible values are `Automatic`, `Manual` and `Rolling`. Defaults to `Manual`. Changing this forces a new resource to be created.


		* `proximity_placement_group_id` - (Optional) The ID of the Proximity Placement Group which the Virtual Machine should be assigned to. Changing this forces a new resource to be created.

		* `rolling_upgrade_policy` - (Optional) A `rolling_upgrade_policy` block as defined below. This is Required and can only be specified when `upgrade_mode` is set to `Automatic` or `Rolling`. Changing this forces a new resource to be created.


		* `max_unhealthy_upgraded_instance_percent` - (Required) The maximum percentage of upgraded virtual machine instances that can be found to be in an unhealthy state. This check will happen after each batch is upgraded. If this percentage is ever exceeded, the rolling update aborts.

		* `pause_time_between_batches` - (Required) The wait time between completing the update for all virtual machines in one batch and starting the next batch. The time duration should be specified in ISO 8601 format.

Conversation

ms-zhenhua commented Dec 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Community Note

Description

PR Checklist

Changes to existing Resource / Data Source

Testing

Change Log

Related Issue(s)

Uh oh!

mimckitt commented Feb 27, 2025

Uh oh!

sreallymatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sreallymatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ms-zhenhua Mar 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ms-zhenhua commented Mar 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sreallymatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ms-zhenhua commented Apr 1, 2025

Uh oh!

sreallymatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented May 2, 2025

Uh oh!

ms-zhenhua commented Dec 20, 2024 •

edited

Loading

ms-zhenhua Mar 7, 2025 •

edited

Loading

ms-zhenhua commented Mar 17, 2025 •

edited

Loading