AzureRM Managed Disks #12455
AzureRM Managed Disks #12455
Conversation
changed location to West US 2
refactored managed disk acceptance tests
# Conflicts: # vendor/vendor.json
|
Any news on this? |
| "github.com/hashicorp/terraform/helper/acctest" | ||
| "github.com/hashicorp/terraform/helper/resource" | ||
| "github.com/hashicorp/terraform/terraform" | ||
| //"regexp" |
There was a problem hiding this comment.
minor: can we remove this commented include?
There was a problem hiding this comment.
removed, thanks!
| @@ -190,7 +208,24 @@ func resourceArmVirtualMachine() *schema.Resource { | |||
|
|
|||
| "vhd_uri": { | |||
| Type: schema.TypeString, | |||
| Required: true, | |||
| Optional: true, | |||
There was a problem hiding this comment.
Hey @brandontosch
Thanks for pushing those updates - sorry for the delay in getting back to you here.
The issue was that it tries to lookup the conflicts from the root of the schema using the keys provided inside ConflictsWith. So trying to locate storage_data_disk.vhd_uri would never succeed since it doesn't contain the index and storage_data_disk is a list. This would cause validation to always succeed and lead to an error when trying to create the resource.
Ah, apologies - I wasn't aware ConflictsWith didn't support this.
IMHO it makes sense to expect storage_data_disk.vhd_uri to work for ConflictsWith so I added some code during the validation to check the key that is being validated for the index (since it is there) and then add it to the conflicting key being searched for so that it can check for a value appropriately. Added some tests in there too for various scenarios to make sure it doesn't garble anything up for other cases.
I'd agree that the behaviour is expected, so thanks for adding support for this! Given that's a change to the Core it'd need further discussion, so that we're not blocking this PR on that (as it otherwise looks good to merge!) - can we pull the ConflictsWith changes out into a separate PR?
Thanks!
|
+1 love this @brandontosch ! |
There was a problem hiding this comment.
Hey @brandontosch
Thanks for the continued effort on this - it's really close now!
I've run the tests and all but one of the tests are passing - TestAccAzureRMManagedDisk_import is failing as the disk_size_gb field isn't set. I've tried updating this to 45 (to match the disk size) and then the tests pass as expected - is it possible to update this?
Thanks!
| resource_group_name = "${azurerm_resource_group.test.name}" | ||
| storage_account_type = "Standard_LRS" | ||
| create_option = "Import" | ||
| source_uri = "${azurerm_storage_account.test.primary_blob_endpoint}${azurerm_storage_container.test.name}/myosdisk1.vhd" |
There was a problem hiding this comment.
The TestAccAzureRMManagedDisk_import test is currently failing since disk_size_gb isn't specified. Setting this value to 45 lets the test pass, thus is it possible to update this?
There was a problem hiding this comment.
Doh! I'll update and do a full re-run as well just in case.
There was a problem hiding this comment.
Ok @tombuildsstuff, all green on my side. Thanks!
|
👋 Hi @brandontosch Thanks for pushing the latest changes - I've taken another look through and it looks good to me :) I've run the tests and (ignoring an unrelated test failure) everything looks great: Thanks again for all your effort here :) |
|
Awesome, thanks for your help getting this merged in @tombuildsstuff! |
|
@brandontosch thanks for this great work on managed disks! One other gap that I have noticed working with Terraform in Azure is lack of App Gateway support, and the inability to link a virtual machine scale set or network interface to the app gateway (currently you can only link the VMSS/NI to a load balancer). There is already a merge request but it seems to have stalled (last commit was end of November) and it doesn't include the tweaks to the VMSS or NI. If you have time to finish that up, I know I would greatly appreciate it and I'm sure many others would as well. |
|
@bpoland thanks for the suggestion! I think we were actually going to need app gateway support as well so I'll try and hop over there and take a look when I can. |
|
I tried to use your managed disk work with custom images (use a vhd instead of an azure provided image) and i get:
The code is in: https://gist.github.com/dobrerazvan/b4a6bfbf1c6648b20ae0aad39dd6ac74 Thanks |
|
@dobrerazvan Actually, I don't think there is a path available to make this work on the azure api side of things.. Looks like building a VM's OS disk from a custom VHD is not supported for managed disks right now. Hopefully if they add that support then the config you specified should work because it looks like it should in my opinion. |
|
Thanks for the reply. I tried the solution you suggested with standalone managed disk and use the Import option for create options. Seems to create the volume with the custom os on top. I wasn't able to find a way through azure portal to spin up a vm from that managed disk with the os. The error i get is: gist: https://gist.github.com/dobrerazvan/df4fb12a22303f885856c0be6b571007 |
|
@dobrerazvan looks like an issue has been opened to track this #13932 and I think I might have a fix for it. I'll open up a PR with what I think can work and it would be much appreciated if you could try testing it out to see if it works for your scenario as well. |
|
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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
ghost
locked and limited conversation to collaborators
Implemented support for Managed Disks (#11874) as standalone resources and as part of a virtual machine. My team is in the middle of a migration to Azure and needed this feature so decided I would try and help out with getting it implemented. Hope this helps!
Acceptance test results