Support health extension for rolling ugrade mode #9136
Conversation
There was a problem hiding this comment.
Hi @ArcturusZhang
Thanks for this PR.
I've left a few comments below based on our conversations offline.
This will need to wait for us to promote the in-line extensions out of beta before being merged as until then the behaviours won't be consistent/predictable with externally defined extensions. We're expecting to review that soon, I'll update here and re-review at that point, but will mark this as blocked for now.
azurerm/internal/services/compute/windows_virtual_machine_scale_set_resource.go
Outdated
Show resolved
Hide resolved
|
Hi @jackofallops I think this might be a part of the extension beta shipping out. When the beta flag is not set, the flag I added will always be will be exactly the same as |
ArcturusZhang
force-pushed
the
vmss-health-extension
branch
from
4522fd6
to
14713cb
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Hi @ArcturusZhang - Apologies for the long delay in returning to this review. I've left some comments below regarding changes required, if you can address those I'll jump back on it asap.
Thanks!
azurerm/internal/services/compute/linux_virtual_machine_scale_set_extensions_test.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/compute/linux_virtual_machine_scale_set_extensions_test.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/compute/windows_virtual_machine_scale_set_extensions_test.go
Outdated
Show resolved
Hide resolved
| @@ -433,6 +434,18 @@ func resourceLinuxVirtualMachineScaleSetCreate(d *schema.ResourceData, meta inte | |||
| virtualMachineProfile.ExtensionProfile.ExtensionsTimeBudget = utils.String(v.(string)) | |||
| } | |||
|
|
|||
| // otherwise the service return the error: | |||
| // Automatic OS Upgrade is not supported for this Virtual Machine Scale Set because a health probe or health extension was not specified. | |||
| if upgradeMode == compute.Automatic && len(automaticOSUpgradePolicyRaw) > 0 && (healthProbeId == "" && !hasHealthExtension) { | |||
There was a problem hiding this comment.
I don't think we'll get here, line 352 above will kick us out first if health_probe_id is unset, so that will need investigating for refactoring.
There was a problem hiding this comment.
Yeah... something is messed up during the merging from master. I need to check these carefully again.
| @@ -449,6 +450,18 @@ func resourceWindowsVirtualMachineScaleSetCreate(d *schema.ResourceData, meta in | |||
| virtualMachineProfile.ExtensionProfile.ExtensionsTimeBudget = utils.String(v.(string)) | |||
| } | |||
|
|
|||
| // otherwise the service return the error: | |||
| // Automatic OS Upgrade is not supported for this Virtual Machine Scale Set because a health probe or health extension was not specified. | |||
| if upgradeMode == compute.Automatic && len(automaticOSUpgradePolicyRaw) > 0 && (healthProbeId == "" && !hasHealthExtension) { | |||
There was a problem hiding this comment.
As with the Linux resource, we'll not get here if health_probe_id is not set, Line 368.
|
Hi @jackofallops I have resolved the comments. please take a look again |
ArcturusZhang
force-pushed
the
vmss-health-extension
branch
from
883d45f
to
09726f3
Compare
There was a problem hiding this comment.
Hi @ArcturusZhang - Thanks for the updates, this LGTM now 👍 I'll get the tests run asap.
|
Tests looking fine (Failures unrelated)
|
|
This has been released in version 2.56.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example: provider "azurerm" {
version = "~> 2.56.0"
}
# ... other configuration ... |
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks! |
ghost
locked as resolved and limited conversation to collaborators
Fixes #8120