-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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.
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
Support health extension for rolling ugrade mode #9136
Support health extension for rolling ugrade mode #9136
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
883d45f
to
09726f3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ArcturusZhang - Thanks for the updates, this LGTM now 👍 I'll get the tests run asap.
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! |
Fixes #8120