Use disk API to load managed disk info when VM is stopped. #1100

JunyiYi · 2018-04-10T01:42:20Z

When VM is stopped, vmClient won't return disks information.
In this pull request, I've done the following steps to resolve such issues similar to #555 :

Check the status of VM using InstanceView
When it is stopped, use diskClient to query the managed disk information
Use the information to update the VM schema like "disk_size_gb" and "managed_disk_type"

metacpp · 2018-04-13T17:30:49Z

azurerm/resource_arm_virtual_machine.go

+	stopped := false
+	if instance.Statuses != nil {
+		for _, status := range *instance.Statuses {
+			if status.Code != nil && *status.Code == "PowerState/deallocated" {


Please do experimentation or check with service team: != running or == deallocated.

Tested for "PowerState/deallocating", "PowerState/deallocated" and "PowerState/starting". Other statuses didn't appear, but I would still add them for safety.

metacpp · 2018-04-13T17:55:29Z

azurerm/resource_arm_virtual_machine.go

+	return &resp, nil
+}
+
+func flattenAzureRmVirtualMachineDataDisk(disks *[]compute.DataDisk, stopped bool, meta interface{}) (interface{}, error) {


This function is taking more responsibility, we may make it simpler and only do flatten work.

metacpp · 2018-04-13T17:55:49Z

azurerm/resource_arm_virtual_machine.go

@@ -1064,7 +1120,7 @@ func flattenAzureRmVirtualMachineOsProfileLinuxConfiguration(config *compute.Lin
 	return []interface{}{result}
 }

-func flattenAzureRmVirtualMachineOsDisk(disk *compute.OSDisk) []interface{} {
+func flattenAzureRmVirtualMachineOsDisk(disk *compute.OSDisk, stopped bool, meta interface{}) ([]interface{}, error) {


same suggestion as above.

tombuildsstuff

hi @JunyiYi

Thanks for this PR :)

I've taken a look through and left some comments in-line - but there's two things which need resolving to proceed with this PR:

We need to add an acceptance test which verifies this works as expected. We can do this by applying the configuration, then shutting down the machine and then verifying that the appropriate fields are set on the os_disk and data_disk objects.
We should pull the data out into the objects returned from the Azure API prior to flattening, rather than flattening twice

Thanks!

tombuildsstuff · 2018-04-10T10:30:15Z

azurerm/resource_arm_virtual_machine.go

+	stopped := false
+	if instance.Statuses != nil {
+		for _, status := range *instance.Statuses {
+			if status.Code != nil && *status.Code == "PowerState/deallocated" {


Azure's API's have a tendency to return things in different casings (particularly for older resources which haven't been modified recently), as such can we check this in a case insensitive manner?

tombuildsstuff · 2018-04-10T10:32:13Z

azurerm/resource_arm_virtual_machine.go

@@ -976,9 +1018,23 @@ func flattenAzureRmVirtualMachineDataDisk(disks *[]compute.DataDisk) interface{}
 		}
 		l["lun"] = *disk.Lun

+		if stopped && disk.ManagedDisk != nil && disk.ManagedDisk.ID != nil {
+			diskInfo, err := getStoppedVMManagedDiskInfo(*disk.ManagedDisk.ID, meta)


thinking about it - would it be worth pulling this regardless of the VM state if managed disks are used?

tombuildsstuff · 2018-04-10T10:33:00Z

azurerm/resource_arm_virtual_machine.go

@@ -1083,7 +1139,22 @@ func flattenAzureRmVirtualMachineOsDisk(disk *compute.OSDisk) []interface{} {
 	}
 	result["os_type"] = string(disk.OsType)

-	return []interface{}{result}
+	if stopped && disk.ManagedDisk != nil && disk.ManagedDisk.ID != nil {
+		diskInfo, err := getStoppedVMManagedDiskInfo(*disk.ManagedDisk.ID, meta)


(as above) I think we could probably retrieve this from this API all the time, rather than just when it's stopped?

tombuildsstuff · 2018-04-14T08:11:26Z

azurerm/resource_arm_virtual_machine.go

@@ -1598,3 +1597,91 @@ func resourceArmVirtualMachineStorageImageReferenceHash(v interface{}) int {
 	}
 	return hashcode.String(buf.String())
 }
+
+func resourceArmVirtualMachineReviseStorageInfo(d *schema.ResourceData, meta interface{}) error {


can we merge this back in with the standard Read method?

tombuildsstuff · 2018-04-14T08:12:36Z

azurerm/resource_arm_virtual_machine.go

+		}
+	}
+
+	if stopped {


if this API is the source of truth for this data - we may as well just call it all the time? otherwise we're going to get into some fun edge cases here (whilst this'll cause a couple of extra api calls, it's not that big a deal imo)

tombuildsstuff · 2018-04-16T13:53:39Z

azurerm/resource_arm_virtual_machine.go

+			}
+		}
+		if dataDisks, ok := d.GetOk("storage_data_disk"); ok {
+			if err := resourceArmVirtualMachineUpdateManagedDisk(dataDisks.([]interface{}), meta); err != nil {


updating the state twice isn't ideal here - we should assign this to the objects used prior to the flatten methods e.g. https://github.com/terraform-providers/terraform-provider-azurerm/blob/master/azurerm/resource_arm_virtual_machine.go#L960

JunyiYi · 2018-04-16T20:18:56Z

@tombuildsstuff , I agree with adding test cases. But are there any clean ways to shut down the virtual machine (without raw Azure REST API calls) in a multi-step test case?

tombuildsstuff · 2018-04-19T18:07:10Z

@JunyiYi we do this in many tests where we call the Azure API from within a Test Case - see the TestAccAzureRMResourceGroup_disappears test for instance where we delete the Resource Group and ensure that Terraform detects it; so we should be able to achieve the same thing by calling the Stop API and Start API's in turn :)

JunyiYi · 2018-04-20T03:07:00Z

@tombuildsstuff , sure, I have added the test cases.

tombuildsstuff

A few minor comments - but this otherwise LGTM - I'll kick off the tests now..

tombuildsstuff · 2018-04-23T09:53:11Z

azurerm/resource_arm_virtual_machine_managed_disks_test.go

+			{
+				Config:  config,
+				Destroy: true,
+			},


minor we actually don't need this last step, the test framework takes care of that for us :)

tombuildsstuff · 2018-04-23T09:54:42Z

azurerm/resource_arm_virtual_machine_managed_disks_test.go

+        disable_password_authentication = false
+    }
+}
+`, rInt, location)


can we switch this to using %d rather than by index (e.g. %[1]d) - there's an issue in golint where incorrect types aren't caught using the index method - so we instead split the variables out

tombuildsstuff · 2018-04-23T10:09:03Z

azurerm/resource_arm_virtual_machine.go

+		}
+		diskResp, err := client.Get(ctx, diskID.ResourceGroup, diskID.Path["disks"])
+		if err != nil {
+			return nil, err


minor for consistency purposes and in order to return a more useful error message - can we update this to be:

id, err := parseAzureResourceID(*disk.ID) if err != nil { return nil, fmt.Errorf("Error parsing Disk ID %q: %+v", *disk.ID, err) } resourceGroup := id.ResourceGroup name := id.Path["disks"] diskResp, err := client.Get(ctx, resourceGroup, name) if err != nil { return nil, fmt.Errorf("Error retrieving Disk %q (Resource Group %q): %+v", name, resourceGroup, err) }

``` $ acctests azurerm TestAccAzureRMVirtualMachine_hasDiskInfoWhenStopped === RUN TestAccAzureRMVirtualMachine_hasDiskInfoWhenStopped --- PASS: TestAccAzureRMVirtualMachine_hasDiskInfoWhenStopped (729.95s) PASS ok github.com/terraform-providers/terraform-provider-azurerm/azurerm 729.986s ```

tombuildsstuff

@JunyiYi I've pushed a commit to resolve the issues I raised above, and this now LGTM 👍

tombuildsstuff · 2018-04-23T11:17:58Z

VM Tests pass:

ghost · 2020-03-31T15:09:02Z

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 👉 hashibot-feedback@hashicorp.com. Thanks!

Use disk API to load managed disk info when VM is stopped.

30d7d0a

JunyiYi requested review from tombuildsstuff and metacpp April 10, 2018 01:42

tombuildsstuff added enhancement service/virtual-machine labels Apr 10, 2018

metacpp added the service/disks label Apr 12, 2018

metacpp added this to the 1.4.0 milestone Apr 12, 2018

metacpp reviewed Apr 13, 2018

View reviewed changes

metacpp approved these changes Apr 13, 2018

View reviewed changes

Junyi Yi added 2 commits April 13, 2018 15:01

Abstract the storage revise procedures according to the comments.

913171e

Add more status codes to check for deallocation.

ed82631

tombuildsstuff modified the milestones: 1.4.0, Soon Apr 16, 2018

tombuildsstuff requested changes Apr 16, 2018

View reviewed changes

tombuildsstuff removed the service/disks label Apr 16, 2018

tombuildsstuff assigned tombuildsstuff and unassigned tombuildsstuff Apr 16, 2018

metacpp assigned JunyiYi Apr 18, 2018

Junyi Yi added 2 commits April 19, 2018 16:29

Update managed disk information when flattening.

d7fe03d

Add test case for disk info of a deallocated virtual machine

b9d9347

tombuildsstuff reviewed Apr 23, 2018

View reviewed changes

tombuildsstuff approved these changes Apr 23, 2018

View reviewed changes

tombuildsstuff merged commit 4cc1381 into master Apr 23, 2018

tombuildsstuff deleted the fix_stoppedvm_storageinfo branch April 23, 2018 11:18

tombuildsstuff modified the milestones: Soon, 1.4.0 Apr 23, 2018

tombuildsstuff added a commit that referenced this pull request Apr 23, 2018

Updating to include #1100

a15d68e

hashicorp locked and limited conversation to collaborators Mar 31, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use disk API to load managed disk info when VM is stopped. #1100

Use disk API to load managed disk info when VM is stopped. #1100

JunyiYi commented Apr 10, 2018

metacpp Apr 13, 2018 •

edited

JunyiYi Apr 13, 2018

metacpp Apr 13, 2018

metacpp Apr 13, 2018

tombuildsstuff left a comment

tombuildsstuff Apr 10, 2018

tombuildsstuff Apr 10, 2018

tombuildsstuff Apr 10, 2018

tombuildsstuff Apr 14, 2018

tombuildsstuff Apr 14, 2018

tombuildsstuff Apr 16, 2018

JunyiYi commented Apr 16, 2018

tombuildsstuff commented Apr 19, 2018

JunyiYi commented Apr 20, 2018

tombuildsstuff left a comment

tombuildsstuff Apr 23, 2018

tombuildsstuff Apr 23, 2018

tombuildsstuff Apr 23, 2018

tombuildsstuff left a comment

tombuildsstuff commented Apr 23, 2018

ghost commented Mar 31, 2020

Use disk API to load managed disk info when VM is stopped. #1100

Use disk API to load managed disk info when VM is stopped. #1100

Conversation

JunyiYi commented Apr 10, 2018

metacpp Apr 13, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tombuildsstuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JunyiYi commented Apr 16, 2018

tombuildsstuff commented Apr 19, 2018

JunyiYi commented Apr 20, 2018

tombuildsstuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tombuildsstuff left a comment

Choose a reason for hiding this comment

tombuildsstuff commented Apr 23, 2018

ghost commented Mar 31, 2020

metacpp Apr 13, 2018 •

edited