Skip to content
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

*_virtual_machine_scale_set: Do not force recreation on rolling_upgrade_policy and health_probe_id update. #10856

Merged
merged 9 commits into from
Mar 10, 2021
Merged

*_virtual_machine_scale_set: Do not force recreation on rolling_upgrade_policy and health_probe_id update. #10856

merged 9 commits into from
Mar 10, 2021

Conversation

favoretti
Copy link
Collaborator

@favoretti favoretti commented Mar 5, 2021

Fixes #10851

An execution plan has been generated and is shown below.
Resource actions are indicated with the following symbols:
  ~ update in-place

Terraform will perform the following actions:

  # azurerm_linux_virtual_machine_scale_set.example will be updated in-place
  ~ resource "azurerm_linux_virtual_machine_scale_set" "example" {
        id                                                = "/subscriptions/4e094144-afbc-420a-8d74-a4a0da618a3b/resourceGroups/vlad-test-resources/providers/Microsoft.Compute/virtualMachineScaleSets/vlad-test-vmss"
        name                                              = "vlad-test-vmss"
        tags                                              = {}
        # (23 unchanged attributes hidden)




      ~ rolling_upgrade_policy {
          ~ max_batch_instance_percent              = 30 -> 20
          ~ max_unhealthy_instance_percent          = 30 -> 20
          ~ max_unhealthy_upgraded_instance_percent = 30 -> 20
          ~ pause_time_between_batches              = "PT0S" -> "PT1S"
        }

        # (4 unchanged blocks hidden)
    }

Plan: 0 to add, 1 to change, 0 to destroy.


Warning: "resource_group_name": [DEPRECATED] This field is no longer used and will be removed in the next major version of the Azure Provider

  on vmss_test.tf line 42, in resource "azurerm_lb_backend_address_pool" "example":
  42: resource "azurerm_lb_backend_address_pool" "example" {


Do you want to perform these actions?
  Terraform will perform the actions described above.
  Only 'yes' will be accepted to approve.

  Enter a value: yes

azurerm_linux_virtual_machine_scale_set.example: Modifying... [id=/subscriptions/4e094144-afbc-420a-8d74-a4a0da618a3b/resourceGroups/vlad-test-resources/providers/Microsoft.Compute/virtualMachineScaleSets/vlad-test-vmss]
azurerm_linux_virtual_machine_scale_set.example: Still modifying... [id=/subscriptions/4e094144-afbc-420a-8d74-...virtualMachineScaleSets/vlad-test-vmss, 10s elapsed]
[...]
azurerm_linux_virtual_machine_scale_set.example: Still modifying... [id=/subscriptions/4e094144-afbc-420a-8d74-...virtualMachineScaleSets/vlad-test-vmss, 3m50s elapsed]
azurerm_linux_virtual_machine_scale_set.example: Modifications complete after 3m51s [id=/subscriptions/4e094144-afbc-420a-8d74-a4a0da618a3b/resourceGroups/vlad-test-resources/providers/Microsoft.Compute/virtualMachineScaleSets/vlad-test-vmss]

Apply complete! Resources: 0 added, 1 changed, 0 destroyed.

@favoretti favoretti added the service/vmss Virtual Machine Scale Sets label Mar 5, 2021
@favoretti favoretti changed the title *_virtual_machine_scale_set: Do not force recreation on rolling_upgrade_policy update. *_virtual_machine_scale_set: Do not force recreation on rolling_upgrade_policy and health_probe_id update. Mar 5, 2021
@IamBread
Copy link

IamBread commented Mar 8, 2021

Hey @jackofallops,
Can we get this merged as it's a minor change and fixes crucial configuration issues?
Regards

@favoretti favoretti requested a review from tombuildsstuff March 8, 2021 11:20
Copy link
Member

@jackofallops jackofallops left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @favoretti - Thanks for this PR. It's off to a good start. Could you add tests that cover the update behaviour of these properties now that the can updated in place?
Thanks!

@favoretti
Copy link
Collaborator Author

@jackofallops certainly, will do so ASAP.

@ghost ghost added size/L and removed size/XS labels Mar 9, 2021
@favoretti favoretti requested a review from jackofallops March 9, 2021 17:38
Copy link
Member

@jackofallops jackofallops left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @favoretti - Thanks for adding the tests, unfortunately it appears the test you've used as the base is actually broken at the destroy stage due to dependency graph not having information to know when to delete the azurerm_lb_rule (it's on our list to fix these tests). I've added a suggestion that should allow the tests to pass for now. The same will be needed in the Windows test.

I didn't see a test for updating health_probe_id, could you add that also?

Thanks!

admin_username = "adminuser"
admin_password = "P@ssword1234!"

disable_password_authentication = false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This property is for Linux VMSS only.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My apologies, should've ran all the tests locally. Doing this now and applying fixes.

@ghost ghost added size/XL and removed size/L labels Mar 10, 2021
@favoretti
Copy link
Collaborator Author

$ make acctests SERVICE='compute' TESTARGS='-run=.*Linux.*otherHealthProbeUpdate' TESTTIMEOUT='600m'
==> Checking that code complies with gofmt requirements...
==> Checking that Custom Timeouts are used...
==> Checking that acceptance test packages are used...
TF_ACC=1 go test -v ./azurerm/internal/services/compute -run=.*Linux.*otherHealthProbeUpdate -timeout 600m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
2021/03/10 11:07:51 [DEBUG] not using binary driver name, it's no longer needed
2021/03/10 11:07:51 [DEBUG] not using binary driver name, it's no longer needed
=== RUN   TestAccLinuxVirtualMachineScaleSet_otherHealthProbeUpdate
=== PAUSE TestAccLinuxVirtualMachineScaleSet_otherHealthProbeUpdate
=== CONT  TestAccLinuxVirtualMachineScaleSet_otherHealthProbeUpdate
--- PASS: TestAccLinuxVirtualMachineScaleSet_otherHealthProbeUpdate (486.56s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/compute	488.121s
$ make acctests SERVICE='compute' TESTARGS='-run=TestAccLinuxVirtualMachineScaleSet_otherRollingUpgradePolicyUpdate' TESTTIMEOUT='600m'
==> Checking that code complies with gofmt requirements...
==> Checking that Custom Timeouts are used...
==> Checking that acceptance test packages are used...
TF_ACC=1 go test -v ./azurerm/internal/services/compute -run=TestAccLinuxVirtualMachineScaleSet_otherRollingUpgradePolicyUpdate -timeout 600m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
2021/03/10 12:40:24 [DEBUG] not using binary driver name, it's no longer needed
2021/03/10 12:40:24 [DEBUG] not using binary driver name, it's no longer needed
=== RUN   TestAccLinuxVirtualMachineScaleSet_otherRollingUpgradePolicyUpdate
=== PAUSE TestAccLinuxVirtualMachineScaleSet_otherRollingUpgradePolicyUpdate
=== CONT  TestAccLinuxVirtualMachineScaleSet_otherRollingUpgradePolicyUpdate
--- PASS: TestAccLinuxVirtualMachineScaleSet_otherRollingUpgradePolicyUpdate (615.53s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/compute	616.872s

I assume windows tests would work too... Haven't ran them though, sorry.

@favoretti favoretti requested a review from jackofallops March 10, 2021 11:57
@favoretti
Copy link
Collaborator Author

$ make acctests SERVICE='compute' TESTARGS='-run=.*Windows.*otherHealthProbeUpdate' TESTTIMEOUT='600m'
==> Checking that code complies with gofmt requirements...
==> Checking that Custom Timeouts are used...
==> Checking that acceptance test packages are used...
TF_ACC=1 go test -v ./azurerm/internal/services/compute -run=.*Windows.*otherHealthProbeUpdate -timeout 600m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
2021/03/10 13:25:26 [DEBUG] not using binary driver name, it's no longer needed
2021/03/10 13:25:26 [DEBUG] not using binary driver name, it's no longer needed
=== RUN   TestAccWindowsVirtualMachineScaleSet_otherHealthProbeUpdate
=== PAUSE TestAccWindowsVirtualMachineScaleSet_otherHealthProbeUpdate
=== CONT  TestAccWindowsVirtualMachineScaleSet_otherHealthProbeUpdate
--- PASS: TestAccWindowsVirtualMachineScaleSet_otherHealthProbeUpdate (566.97s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/compute	568.404s

@favoretti
Copy link
Collaborator Author

$ make acctests SERVICE='compute' TESTARGS='-run=TestAccWindowsVirtualMachineScaleSet_otherRollingUpgradePolicyUpdate' TESTTIMEOUT='600m'
==> Checking that code complies with gofmt requirements...
==> Checking that Custom Timeouts are used...
==> Checking that acceptance test packages are used...
TF_ACC=1 go test -v ./azurerm/internal/services/compute -run=TestAccWindowsVirtualMachineScaleSet_otherRollingUpgradePolicyUpdate -timeout 600m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
2021/03/10 14:29:06 [DEBUG] not using binary driver name, it's no longer needed
2021/03/10 14:29:06 [DEBUG] not using binary driver name, it's no longer needed
=== RUN   TestAccWindowsVirtualMachineScaleSet_otherRollingUpgradePolicyUpdate
=== PAUSE TestAccWindowsVirtualMachineScaleSet_otherRollingUpgradePolicyUpdate
=== CONT  TestAccWindowsVirtualMachineScaleSet_otherRollingUpgradePolicyUpdate
--- PASS: TestAccWindowsVirtualMachineScaleSet_otherRollingUpgradePolicyUpdate (742.41s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/compute	744.236s
$ make acctests SERVICE='compute' TESTARGS='-run=.*Windows.*otherHealthProbeUpdate' TESTTIMEOUT='600m'
==> Checking that code complies with gofmt requirements...
==> Checking that Custom Timeouts are used...
==> Checking that acceptance test packages are used...
TF_ACC=1 go test -v ./azurerm/internal/services/compute -run=.*Windows.*otherHealthProbeUpdate -timeout 600m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
2021/03/10 14:28:58 [DEBUG] not using binary driver name, it's no longer needed
2021/03/10 14:28:58 [DEBUG] not using binary driver name, it's no longer needed
=== RUN   TestAccWindowsVirtualMachineScaleSet_otherHealthProbeUpdate
=== PAUSE TestAccWindowsVirtualMachineScaleSet_otherHealthProbeUpdate
=== CONT  TestAccWindowsVirtualMachineScaleSet_otherHealthProbeUpdate
--- PASS: TestAccWindowsVirtualMachineScaleSet_otherHealthProbeUpdate (581.08s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/compute	582.717s

@jackofallops jackofallops added this to the v2.51.0 milestone Mar 10, 2021
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @favoretti - LGTM 👍

@katbyte katbyte merged commit d73de96 into hashicorp:master Mar 10, 2021
katbyte added a commit that referenced this pull request Mar 10, 2021
@ghost
Copy link

ghost commented Mar 12, 2021

This has been released in version 2.51.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.51.0"
}
# ... other configuration ...

@ghost
Copy link

ghost commented Apr 10, 2021

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 ghost locked as resolved and limited conversation to collaborators Apr 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for mutable rolling upgrade properties
4 participants