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

Force recreation if storage_data_disk.creation_option changes #218

Closed
wants to merge 0 commits into from

Conversation

thiagocaiubi
Copy link
Contributor

I'm not sure how to test this change, please provide any feedback to help me with it.

Azure Resource Manager API doesn't support changes to this field. It
should recreate the virtual machine and attach the disks again.

Actual Behaviour:

module.xpto.azurerm_virtual_machine.prometheus_virtual_machine:
Modifying... (ID: /subscriptions/xxxxx...ute/virtualMachines/vm)
delete_data_disks_on_termination: "" => "false"
delete_os_disk_on_termination: "" => "true"
storage_data_disk.0.create_option: "Empty" => "Attach"
storage_data_disk.1.create_option: "Empty" => "Attach"
tags.%: "0" => "1"
tags.role: "" => "xpto"
Error applying plan:

1 error(s) occurred:

  • module.xpto.azurerm_virtual_machine.vm: 1 error(s) occurred:

  • azurerm_virtual_machine.prometheus_virtual_machine:
    compute.VirtualMachinesClient#CreateOrUpdate: Failure responding to
    request: StatusCode=409 -- Original Error: autorest/azure: Service
    returned an error. Status=409 Code="PropertyChangeNotAllowed"
    Message="Changing property 'dataDisk.createOption' is not allowed."

@tombuildsstuff
Copy link
Contributor

Hey @thiagocaiubi

Thanks for opening this PR :)

I'm not sure how to test this change, please provide any feedback to help me with it.

So the best way to do this is to add an acceptance test which has multiple phases - for instance in this example. Essentially, we'd need to create a Virtual Machine with creation_option set to Empty - and then in an updated Terraform configuration switch that field to Attach. With regards to verifying that the machine has been recreated - there's an example of that here :)

Those tests can then be run via:

TF_ACC=1 go test ./azurerm -v -timeout 120m -run TestAccAzureRMPostgreSQLConfiguration_backslashQuote

.. where TestAccAzureRMPostgreSQLConfiguration_backslashQuote is the test name. Running the tests presumes you have the environment variables set, as defined here - and will complain if not.

I'm hoping that makes sense? However this change looks sensible to me - I'd actually run into this earlier today too :)

Thanks!

@thiagocaiubi
Copy link
Contributor Author

Hi @tombuildsstuff, thanks for the very informative explanation.
I've update the PR based on the examples that you pointed out unfortunately I'm facing the following issue:

TF_ACC=1 go test ./azurerm -v -timeout 120m -run TestAccAzureRMVirtualMachine_changeStorageDataDiskCreationOption
=== RUN   TestAccAzureRMVirtualMachine_changeStorageDataDiskCreationOption
--- FAIL: TestAccAzureRMVirtualMachine_changeStorageDataDiskCreationOption (495.48s)
        testing.go:428: Step 1 error: Error applying: 1 error(s) occurred:

                * azurerm_virtual_machine.test: 1 error(s) occurred:

                * azurerm_virtual_machine.test: compute.VirtualMachinesClient#CreateOrUpdate: Failure responding to request: StatusCode=404 -- Original Error: autorest/azure: Service returned an error. Status=404 Code="NotFound" Message="The entity was not found."
FAIL
exit status 1
FAIL    github.com/terraform-providers/terraform-provider-azurerm/azurerm       495.597s

@tombuildsstuff
Copy link
Contributor

Hey @thiagocaiubi

Thanks for pushing those updates :)

I may be wrong, but taking a quick glance I think the issue may be that the disks are being deleted when the Original VM is - would it be possible to check by removing those two fields? Terraform will automatically clear up the config once it's finished the test, so they shouldn't be needed :)

Thanks!

@thiagocaiubi
Copy link
Contributor Author

Hi @tombuildsstuff , thanks! 😄 👍

TF_ACC=1 go test ./azurerm -v -timeout 120m -run TestAccAzureRMVirtualMachine_changeStorageDataDiskCreationOption
=== RUN   TestAccAzureRMVirtualMachine_changeStorageDataDiskCreationOption
--- PASS: TestAccAzureRMVirtualMachine_changeStorageDataDiskCreationOption (791.58s)
PASS
ok      github.com/terraform-providers/terraform-provider-azurerm/azurerm       791.691s

@tombuildsstuff
Copy link
Contributor

@thiagocaiubi apologies, I pushed a commit to this branch (to switch the OS version to Ubuntu 16 after #237) which apparently closed this PR.. I'll re-open it, sorry!

@tombuildsstuff
Copy link
Contributor

tombuildsstuff commented Aug 11, 2017

@thiagocaiubi I can't see how to re-open this PR unfortunately (the button's greyed out). I've opened #240 which includes your commits + git authorship history where we'll merge this, I'm really sorry about that!

@thiagocaiubi
Copy link
Contributor Author

@tombuildsstuff there is nothing to apologize for. The more important is making terraform better each day. I'm very happy to contribute back every now and then. 👍

@ghost
Copy link

ghost commented Apr 1, 2020

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 and limited conversation to collaborators Apr 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants