New Resource: `azurerm_virtual_machine_data_disk_attachment` #1207

tombuildsstuff · 2018-05-06T19:31:46Z

This PR creates a new resource which can be used for attaching disks to Virtual Machine. The acceptance tests for this need some more thought and this needs some extensive testing, in general.

After spending a lot of time investigating/thinking about this - I've ended up adapting this resource such that it only supports attaching existing Managed Disks. This makes supporting this resource considerably simpler (since supporting creating new Managed Disks involves manipulating the create_option field); unfortunately it appears this approach isn't possible for Unmanaged Disks due to this bug - as such I've dropped support for Unmanaged Disks at this time.

Fixes #795

amcguign · 2018-05-06T22:20:48Z

Great to see this - thanks @tombuildsstuff ! 👍

metacpp · 2018-05-06T23:24:28Z

azurerm/provider.go

-			"azurerm_virtual_network_gateway":             resourceArmVirtualNetworkGateway(),
-			"azurerm_virtual_network_gateway_connection":  resourceArmVirtualNetworkGatewayConnection(),
-			"azurerm_virtual_network_peering":             resourceArmVirtualNetworkPeering(),
+			"azurerm_application_gateway":                  resourceArmApplicationGateway(),


why did you change so many lines?

Because one space is added to align with each other.

on that note: you can use ?w=1 to ignore whitespace changes in reviews in Github, it's really handy for things like this :)

metacpp · 2018-05-06T23:37:47Z

azurerm/resource_arm_virtual_machine_data_disk_attachment.go

+		dataDisks := make([]compute.DataDisk, 0)
+		for _, dataDisk := range disks {
+			if !strings.EqualFold(*dataDisk.Name, *expandedDisk.Name) {
+				disks = append(disks, dataDisk)


It seems that dataDisks is never used since initialization. I guess you were using it store the left disks. Then I think you should do:

dataDisks = append(dataDisks, dataDisk)

JunyiYi

I have some concerns of introducing multiples ways to do one thing. It might cause updating issues similar to azurerm_network_security_rule.

We need to carefully think of it if we are not able to mitigate this kind of thing.

JunyiYi · 2018-05-29T22:57:43Z

azurerm/disks.go

+	"strings"
+)
+
+type UnmanagedDiskMetadata struct {


Any rules on custom types, like should it always be public, and the naming convention?

they should be private, however this is a WIP before refactoring (and placing this somewhere in helpers/)

(also, we tend to rely directly on the SDK Structs rather than composing them ourselves, since this is information which should be provided by the SDK but isn't at this point)

JunyiYi · 2018-05-30T00:47:58Z

azurerm/provider.go

-			"azurerm_virtual_network_gateway":             resourceArmVirtualNetworkGateway(),
-			"azurerm_virtual_network_gateway_connection":  resourceArmVirtualNetworkGatewayConnection(),
-			"azurerm_virtual_network_peering":             resourceArmVirtualNetworkPeering(),
+			"azurerm_application_gateway":                  resourceArmApplicationGateway(),


Because one space is added to align with each other.

tombuildsstuff · 2018-05-30T09:34:07Z

I have some concerns of introducing multiples ways to do one thing. It might cause updating issues similar to azurerm_network_security_rule.

@JunyiYi indeed, this is why this is still WIP whilst I've been thinking about how's best to implement this/chatting internally about it. We should be keeping both resources actually - the nested resource is beneficial as it allows the user to explicitly say "I only want these disks attached" vs the attachment resource for "I'm not concerned with what other disks are attached, just attach this given disk".

After mulling on this for a while, I'm coming around to the idea of forcing users to create a separate managed disk ahead of time, which allows us to remove the create_option field altogether (which is a huge pain-point), and also handles disk cleanup (by the azurerm_managed_disk resource being deleted); which would allow us to infer the details from the disk, making the attachment resource look something like this:

resource "azurerm_managed_disk" "test" {
  name                 = "tomdev-disk25"
  location             = "${azurerm_resource_group.test.location}"
  resource_group_name  = "${azurerm_resource_group.test.name}"
  storage_account_type = "Standard_LRS"
  create_option        = "Empty"
  disk_size_gb         = 10
}

resource "azurerm_virtual_machine_data_disk_attachment" "second" {
  name               = "some-name" # perhaps we could infer this from the Disk Name?
  virtual_machine_id = "${azurerm_virtual_machine.windows.id}"
  managed_disk_id    = "${azurerm_managed_disk.test.id}"
  lun                = 33
  caching            = "ReadOnly"
}

rather than this:

resource "azurerm_managed_disk" "test" {
  name                 = "tomdev-disk25"
  location             = "${azurerm_resource_group.test.location}"
  resource_group_name  = "${azurerm_resource_group.test.name}"
  storage_account_type = "Standard_LRS"
  create_option        = "Empty"
  disk_size_gb         = 10
}

resource "azurerm_virtual_machine_data_disk_attachment" "existing_disk" {
  name               = "existing-disk"
  virtual_machine_id = "${azurerm_virtual_machine.windows.id}"
  managed_disk_id    = "${azurerm_managed_disk.test.id}"
  managed_disk_type  = "Standard_LRS"
  disk_size_gb       = "${azurerm_managed_disk.test.disk_size_gb}"
  lun                = 33
}

The outstanding question is how we'd achieve the same thing with unmanaged disks, which I'm looking into at the moment :)

dismissing stale review

rohrerb · 2018-06-01T17:23:07Z

Excited to see this merged. :D

tombuildsstuff · 2018-06-01T20:19:25Z

I've been doing some initial exploratory testing against this and it seems ok, but it needs some more (in particular verifying disks created inline -> split out are handled as expected, which I'll pick up when I'm back). I've also pushed a WIP commit which should prevent disk attachments created using the independent resource from being impacted when changes are made to the parent VM resource which uses the value of the computed fields, but this needs additional testing.

rohrerb · 2018-06-18T12:10:32Z

Hi @tombuildsstuff ,

Any chance this will get merged soon? Need any help testing?

-Brandon

tombuildsstuff · 2018-06-18T14:01:14Z

👋 @rohrerb I've been out for a couple of weeks, so I've not been working on this; but I'm working through the edge-case testing at the moment and hope to finish that this week. That said - we're definitely interested to hear your feedback if you're able to test this :)

Thanks!

katbyte

Hey @tombuildsstuff,

Outside a couple minor comments this LGTM 👍

katbyte · 2018-06-26T22:59:34Z

azurerm/resource_arm_virtual_machine_data_disk_attachment.go

+				Required:         true,
+				ForceNew:         true,
+				DiffSuppressFunc: ignoreCaseDiffSuppressFunc,
+			},


This could be validated with validate.ValidateResourceId

katbyte · 2018-06-26T23:14:15Z

azurerm/resource_arm_virtual_machine_data_disk_attachment.go

+
+	dataDisks := make([]compute.DataDisk, 0)
+	for _, dataDisk := range *virtualMachine.StorageProfile.DataDisks {
+		// since this field isn't (and shouldn't be) case-sensitive; we're deliberately not using `strings.Equals`


Did you mean strings.EqualFold()

katbyte · 2018-06-26T23:19:32Z

website/docs/r/virtual_machine.html.markdown

@@ -1,7 +1,7 @@
 ---
 layout: "azurerm"
 page_title: "Azure Resource Manager: azurerm_virtual_machine"
-sidebar_current: "docs-azurerm-resource-compute-virtual-machine"
+sidebar_current: "docs-azurerm-resource-compute-virtual-machine-x"


Was the -x supposed to be added?

Yep, there's an incorrect highlight on the sidebar at the moment

ghost · 2020-03-30T15:29:21Z

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!

tombuildsstuff added new-resource service/virtual-machine labels May 6, 2018

tombuildsstuff mentioned this pull request May 6, 2018

Feature Request: new resource to attach disks to virtual machines #795

Closed

tombuildsstuff added this to the Soon milestone May 6, 2018

metacpp self-requested a review May 6, 2018 23:23

metacpp reviewed May 6, 2018

View reviewed changes

metacpp previously requested changes May 6, 2018

View reviewed changes

This was referenced May 10, 2018

Difference in behavior of managed vs unmanaged data disk parsing on azurerm_virtual_machine #136

Closed

azurerm - Terraform wants to recreate VM when attaching existing managed data disk #85

Closed

tombuildsstuff force-pushed the vm-data-disk-attachment branch 2 times, most recently from 105d2e1 to 957cb32 Compare May 15, 2018 00:18

katbyte mentioned this pull request May 18, 2018

storage_data_disk needs to support count #476

Closed

tombuildsstuff force-pushed the vm-data-disk-attachment branch from 957cb32 to 54fd5b0 Compare May 25, 2018 15:32

r7vme mentioned this pull request May 29, 2018

Support OS disk size or storage type changes #1313

Closed

metacpp requested a review from JunyiYi May 29, 2018 15:14

JunyiYi previously requested changes May 30, 2018

View reviewed changes

tombuildsstuff force-pushed the vm-data-disk-attachment branch from 07fd1ab to ca4b335 Compare May 31, 2018 15:33

tombuildsstuff force-pushed the vm-data-disk-attachment branch 2 times, most recently from 46c0f7c to bf3598b Compare May 31, 2018 17:43

tombuildsstuff changed the title ~~[WIP] New Resource: azurerm_virtual_machine_data_disk_attachment~~ New Resource: azurerm_virtual_machine_data_disk_attachment Jun 1, 2018

tombuildsstuff changed the title ~~New Resource: azurerm_virtual_machine_data_disk_attachment~~ [WIP] New Resource: azurerm_virtual_machine_data_disk_attachment Jun 1, 2018

tombuildsstuff removed this from the Soon milestone Jun 19, 2018

tombuildsstuff added this to the 1.8.0 milestone Jun 19, 2018

tombuildsstuff force-pushed the vm-data-disk-attachment branch from d239408 to 32953a0 Compare June 20, 2018 12:51

tombuildsstuff changed the title ~~[WIP] New Resource: azurerm_virtual_machine_data_disk_attachment~~ New Resource: azurerm_virtual_machine_data_disk_attachment Jun 20, 2018

tombuildsstuff requested a review from a team June 20, 2018 13:23

rohrerb mentioned this pull request Jun 25, 2018

data_disk_attachment implementation help #1441

Closed

katbyte approved these changes Jun 26, 2018

View reviewed changes

tombuildsstuff added 11 commits June 27, 2018 09:35

New Resource: azurerm_virtual_machine_data_disk_attachment

04cb867

Disclaimers about not using both resources at the same time

66b2605

Fixing an incorrect assignment

99914d1

Documentation fixes

226891a

Fixing some broken tests

3101d83

Removing the create_option field

364401d

Moving to support only managed disks

569173d

Refactoring to switch back to how it was

fa00452

Explaining why we're deliberately ignoring an error

b18dc42

Adding missing validation for the create_option

162f27f

Required -> Optional

ab9171f

tombuildsstuff force-pushed the vm-data-disk-attachment branch from 92c522e to ab9171f Compare June 27, 2018 07:35

Fixing issues identified in PR Review

9a4f6a4

radeksimko approved these changes Jun 27, 2018

View reviewed changes

tombuildsstuff merged commit 10af28f into master Jun 27, 2018

tombuildsstuff deleted the vm-data-disk-attachment branch June 27, 2018 08:40

tombuildsstuff added a commit that referenced this pull request Jun 27, 2018

Updating to include #1207

e471648

rohrerb mentioned this pull request Jun 28, 2018

Count: removing an item from a list forces unexpected changes hashicorp/terraform#18349

Closed

ghost mentioned this pull request Jun 28, 2018

data_disk_attachment implementation help #1461

Closed

ghost locked and limited conversation to collaborators Mar 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Resource: `azurerm_virtual_machine_data_disk_attachment` #1207

New Resource: `azurerm_virtual_machine_data_disk_attachment` #1207

tombuildsstuff commented May 6, 2018 •

edited

Loading

amcguign commented May 6, 2018

metacpp May 6, 2018

JunyiYi May 30, 2018

tombuildsstuff May 30, 2018

metacpp May 6, 2018

JunyiYi left a comment

JunyiYi May 29, 2018

tombuildsstuff May 30, 2018

tombuildsstuff May 30, 2018

JunyiYi May 30, 2018

tombuildsstuff commented May 30, 2018

rohrerb commented Jun 1, 2018

tombuildsstuff commented Jun 1, 2018

rohrerb commented Jun 18, 2018 •

edited

Loading

tombuildsstuff commented Jun 18, 2018

katbyte left a comment

katbyte Jun 26, 2018

katbyte Jun 26, 2018

katbyte Jun 26, 2018

tombuildsstuff Jun 27, 2018

ghost commented Mar 30, 2020

New Resource: azurerm_virtual_machine_data_disk_attachment #1207

New Resource: azurerm_virtual_machine_data_disk_attachment #1207

Conversation

tombuildsstuff commented May 6, 2018 • edited Loading

amcguign commented May 6, 2018

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 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

tombuildsstuff commented May 30, 2018

rohrerb commented Jun 1, 2018

tombuildsstuff commented Jun 1, 2018

rohrerb commented Jun 18, 2018 • edited Loading

tombuildsstuff commented Jun 18, 2018

katbyte 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

ghost commented Mar 30, 2020

New Resource: `azurerm_virtual_machine_data_disk_attachment` #1207

New Resource: `azurerm_virtual_machine_data_disk_attachment` #1207

tombuildsstuff commented May 6, 2018 •

edited

Loading

rohrerb commented Jun 18, 2018 •

edited

Loading