[MS] provider/azurerm: Virtual Machine Scale Sets with managed disk support #13717

sozercan · 2017-04-17T17:41:38Z

Adds support for VMSS with managed disks
Adds support for adding load balancer inbound nat rules in network profile
Changed storage references to align with new VM storage references

tombuildsstuff · 2017-04-18T16:50:29Z

Fixes #13307

…ence

cberner · 2017-04-20T23:54:17Z

This is awesome! Looks like it doesn't support a custom image, however. Is support for that planned?

cloudbooster · 2017-04-25T16:57:40Z

Fixes #13902

cloudbooster · 2017-04-25T17:00:25Z

@cberner We are working on support for custom image, once this is merged @echuvyrov should be able to send one in shortly.

tombuildsstuff

Hi @sozercan

Thanks for submitting this PR - apologies for the delay reviewing this! I've reviewed and left some comments in-line :)

The main issue that stands out is the name field on storage_profile_os_disk, which is set as a Required field is only set under certain conditions. Essentially, if it's a required field then it needs to be set at all times - however if it's optional then can we add some tests to cover that scenario?

Thanks! :)

tombuildsstuff · 2017-04-26T16:37:46Z

builtin/providers/azurerm/resource_arm_virtual_machine_scale_set.go

@@ -838,8 +872,8 @@ func resourceArmVirtualMachineScaleSetStorageProfileOsDiskHash(v interface{}) in
 	m := v.(map[string]interface{})
 	buf.WriteString(fmt.Sprintf("%s-", m["name"].(string)))


~~Given name won't be set unless Image/VhdContainers is set - I think this wants an if statement around it?~~

tombuildsstuff · 2017-04-26T17:44:01Z

builtin/providers/azurerm/import_arm_virtual_machine_scale_set_test.go

+	resourceName := "azurerm_virtual_machine_scale_set.test"
+
+	ri := acctest.RandInt()
+	config := fmt.Sprintf(testAccAzureRMVirtualMachineScaleSet_basic, ri, ri, ri, ri, ri, ri, ri, ri)


From what I can see - I think this line should be:

fmt.Sprintf(testAccAzureRMVirtualMachineScaleSet_basicLinux_managedDisk, ri, ri, ri, ri, ri, ri)

You might be looking at testAccAzureRMVirtualMachineScaleSet_basicLinux_managedDisk which has config := fmt.Sprintf(testAccAzureRMVirtualMachineScaleSet_basicLinux_managedDisk, ri, ri, ri, ri, ri, ri)

@sozercan so currently this test is identical to the one above it TestAccAzureRMVirtualMachineScaleSet_importBasic, as it's running the same test with the same inputs/outputs.

Instead - I believe this test was meant to use the testAccAzureRMVirtualMachineScaleSet_basicLinux_managedDisk config? If so - is it possible to switch this out as you've identified? e.g.

config := fmt.Sprintf(testAccAzureRMVirtualMachineScaleSet_basicLinux_managedDisk, ri, ri, ri, ri, ri, ri)

Thanks :)

tombuildsstuff · 2017-04-26T18:18:26Z

builtin/providers/azurerm/resource_arm_virtual_machine_scale_set_test.go

+        	name                                   = "TestIPConfiguration"
+        	subnet_id                              = "${azurerm_subnet.test.id}"
+			load_balancer_backend_address_pool_ids = [ "${azurerm_lb_backend_address_pool.test.id}" ]
+		    load_balancer_inbound_nat_rules_ids = ["${element(azurerm_lb_nat_pool.test.*.id, count.index)}"]


Given we're only creating a single Nat Pool + VM in the scale set, could we switch this to a direct reference?

tombuildsstuff · 2017-04-26T18:22:16Z

builtin/providers/azurerm/resource_arm_virtual_machine_scale_set_test.go

-    }
-  }
+resource "azurerm_lb_nat_pool" "test" {
+  count = 1


given there's only a single one of these, and the other properties aren't configured to support incrementing this right now - could we remove the count here?

tombuildsstuff · 2017-04-26T18:31:24Z

builtin/providers/azurerm/resource_arm_virtual_machine_scale_set.go

@@ -735,19 +763,25 @@ func flattenAzureRMVirtualMachineScaleSetOsProfile(profile *compute.VirtualMachi

 func flattenAzureRmVirtualMachineScaleSetStorageProfileOSDisk(profile *compute.VirtualMachineScaleSetOSDisk) []interface{} {
 	result := make(map[string]interface{})
-	result["name"] = *profile.Name


From what I can see, name still looks like a required field, as such would it make more sense to leave this here?

This one is a little tricky. Technically, name is a optional field in the schema but it is required for vhd_containers and images. If name is defined with a managed_disk_type, it gives error Status=400 Code="InvalidParameter" Message="Parameter 'osDisk.name' is not allowed."
Please let me know which one is preferred, leaving it as is or changing it to a an optional field and using validation tests.

In that case I think the simplest way to handle this is going to be:

if profile.Name != nil { result["name"] = *profile.Name }

That works, thanks! Let me know if there are any other blockers.

tombuildsstuff · 2017-04-26T18:32:10Z

builtin/providers/azurerm/resource_arm_virtual_machine_scale_set.go


 	osDisk := &compute.VirtualMachineScaleSetOSDisk{
-		Name:         &name,


see above about name being required?

Please see above reply.

I believe we should be able to assign name in every case, as if it's nil then this value won't be propagated up?

tombuildsstuff · 2017-04-26T18:39:53Z

builtin/providers/azurerm/resource_arm_virtual_machine_scale_set_test.go

+	resource.Test(t, resource.TestCase{
+		PreCheck:     func() { testAccPreCheck(t) },
+		Providers:    testAccProviders,
+		CheckDestroy: testCheckAzureRMVirtualMachineDestroy,


I think this wants to be testCheckAzureRMVirtualMachineScaleSetDestroy?

tombuildsstuff · 2017-04-26T18:47:45Z

website/source/docs/providers/azurerm/r/virtual_machine_scale_sets.html.markdown

-                       If this property is set then vhd_containers is ignored.
+                       Cannot be used when `vhd_containers` or `managed_disk_type` is specified.
+                       Cannot be used when `storage_profile_image_reference` is specified.
+                       Have to specify `os_type` when defined.


could we consolidate this into something like:

When setting this field `os_type` needs to be specified. Cannot be used when `vhd_containers`, `managed_disk_type` or `storage_profile_image_reference ` are specified.

Thanks for reviewing this @tombuildsstuff, per this I don't think Name is required for Storage Profile OS Disk, so perhaps @sozercan can mark it as such.

Thanks! Changed documentation accordingly.

tombuildsstuff · 2017-04-26T18:48:20Z

website/source/docs/providers/azurerm/r/virtual_machine_scale_sets.html.markdown

 * `create_option` - (Required) Specifies how the virtual machine should be created. The only possible option is `FromImage`.
-* `caching` - (Required) Specifies the caching requirements.
+* `caching` - (Optional) Specifies the caching requirements.


this defaults to None in the API - is it worth documenting that here?

tombuildsstuff · 2017-04-26T18:49:16Z

builtin/providers/azurerm/resource_arm_virtual_machine_scale_set.go

 						"caching": {
 							Type:     schema.TypeString,
-							Required: true,
+							Optional: true,
+							Computed: true,


Is it worth defaulting this to None rather than leaning on the default value at the API?

Thank you for reviewing @tombuildsstuff! :) This is the same implementation as virtual_machines so wanted to keep it consistent with that behavior and documented default in documentation now. Please let me know if this works or if I should add default value here.

That works :)

StephenWeatherford · 2017-04-27T00:07:48Z

website/source/docs/providers/azurerm/r/virtual_machine_scale_sets.html.markdown

+  address_prefix       = "10.0.2.0/24"
+}
+
+resource "azurerm_public_ip" "demoterraformips" {


The resource names are inconsistent, and this example gives errors - probably best to change all resource names to "test" like I've seen with most examples

resource 'azurerm_public_ip.demoterraformips' config: unknown resource 'azurerm_resource_group.demoterraform' referenced in variable azurerm_resource_group.demoterraform.name

resource 'azurerm_lb.demoterraformlb' config: unknown resource 'azurerm_resource_group.demoterraform' referenced in variable azurerm_resource_group.demoterraform.name

resource 'azurerm_lb_nat_pool.lbnatpool' config: unknown resource 'azurerm_resource_group.demoterraform' referenced in variable azurerm_resource_group.demoterraform.name

resource 'azurerm_virtual_machine_scale_set.test' config: unknown resource 'azurerm_subnet.demoterraformsubnet' referenced in variable azurerm_subnet.demoterraformsubnet.id

resource 'azurerm_lb_backend_address_pool.bpepool' config: unknown resource 'azurerm_resource_group.demoterraform' referenced in variable azurerm_resource_group.demoterraform.name

Thanks! Should be consistent with unmanaged disk tests now.

sozercan

@tombuildsstuff all comments should be addressed now. Let me know if you need anything else. Thanks!

tombuildsstuff

Hi @sozercan

Thanks for the continued effort here - sorry for the delay in re-reviewing this!
I've taken another look and replied in-line :)

Thanks!

tombuildsstuff · 2017-05-02T14:21:26Z

builtin/providers/azurerm/resource_arm_virtual_machine_scale_set.go

 		result["image"] = *profile.Image.URI
 	}

 	if profile.VhdContainers != nil {
+		result["name"] = *profile.Name


given this statement exists above, this line can be removed :)

tombuildsstuff · 2017-05-02T14:21:29Z

builtin/providers/azurerm/resource_arm_virtual_machine_scale_set.go

 	if profile.Image != nil {
+		result["name"] = *profile.Name


given this statement exists above, this line can be removed :)

tombuildsstuff · 2017-05-02T14:23:25Z

builtin/providers/azurerm/resource_arm_virtual_machine_scale_set.go


 	osDisk := &compute.VirtualMachineScaleSetOSDisk{
-		Name:         &name,


I believe we should be able to assign name in every case, as if it's nil then this value won't be propagated up?

tombuildsstuff · 2017-05-02T14:23:46Z

builtin/providers/azurerm/resource_arm_virtual_machine_scale_set.go

 		Caching:      compute.CachingTypes(caching),
 		OsType:       compute.OperatingSystemTypes(osType),
 		CreateOption: compute.DiskCreateOptionTypes(createOption),
 	}

 	if image != "" {
+		osDisk.Name = &name


(which means, I think this line can be removed?)

tombuildsstuff · 2017-05-02T14:23:56Z

builtin/providers/azurerm/resource_arm_virtual_machine_scale_set.go

 			str := v.(string)
 			vhdContainers = append(vhdContainers, str)
 		}
+		osDisk.Name = &name


(which means, I think this line can be removed?)

Unfortunately, if i leave the osDisk.name in there with managed disks, it will give error compute.VirtualMachineScaleSetsClient#CreateOrUpdate: Failure responding to request: StatusCode=400 -- Original Error: autorest/azure: Service returned an error. Status=400 Code="InvalidParameter" Message="Parameter 'osDisk.name' is not allowed." even if name is blank.

Azure quickstart templates that have managed disks don't have name in there also: https://github.com/Azure/azure-quickstart-templates/blob/master/101-vm-simple-linux/azuredeploy.json

@tombuildsstuff @sozercan @echuvyrov IMHO, I think it's best in this case to validate that the name is nil/empty in the terraform template (give an error if not), then set it to null when passing to Azure. Otherwise you'll have problems if Azure later changes and allows this parameter and you start passing in what were previously ignored and likely unintended/invalid values.

@StephenWeatherford @tombuildsstuff just updated with @StephenWeatherford's guidance above, let me know if this works. 966659e

@tombuildsstuff Tom, could you please review these last changes as soon as possible? This PR is still blocking multiple pieces of work for us. If there are still issues with the review, but it's good enough to merge, we'd be happy to make sure those issues get fixed in a separate PR.

Thanks!
Stephen

All VMSS tests passed (one failed due to network but passed on retry).

TF_ACC=1 go test ./builtin/providers/azurerm -v -test.run TestAccAzureRMVirtualMachineScaleSet_ -timeout 120m
=== RUN TestAccAzureRMVirtualMachineScaleSet_importBasic
--- PASS: TestAccAzureRMVirtualMachineScaleSet_importBasic (507.88s)
=== RUN TestAccAzureRMVirtualMachineScaleSet_importBasic_managedDisk
--- PASS: TestAccAzureRMVirtualMachineScaleSet_importBasic_managedDisk (385.23s)
=== RUN TestAccAzureRMVirtualMachineScaleSet_importLinux
--- PASS: TestAccAzureRMVirtualMachineScaleSet_importLinux (512.03s)
=== RUN TestAccAzureRMVirtualMachineScaleSet_importLoadBalancer
--- PASS: TestAccAzureRMVirtualMachineScaleSet_importLoadBalancer (472.25s)
=== RUN TestAccAzureRMVirtualMachineScaleSet_importOverProvision
--- PASS: TestAccAzureRMVirtualMachineScaleSet_importOverProvision (337.37s)
=== RUN TestAccAzureRMVirtualMachineScaleSet_importExtension
--- PASS: TestAccAzureRMVirtualMachineScaleSet_importExtension (323.97s)
=== RUN TestAccAzureRMVirtualMachineScaleSet_importMultipleExtensions
--- PASS: TestAccAzureRMVirtualMachineScaleSet_importMultipleExtensions (338.34s)
=== RUN TestAccAzureRMVirtualMachineScaleSet_basic
--- PASS: TestAccAzureRMVirtualMachineScaleSet_basic (320.88s)
=== RUN TestAccAzureRMVirtualMachineScaleSet_linuxUpdated
--- PASS: TestAccAzureRMVirtualMachineScaleSet_linuxUpdated (492.62s)
=== RUN TestAccAzureRMVirtualMachineScaleSet_basicLinux_managedDisk
--- PASS: TestAccAzureRMVirtualMachineScaleSet_basicLinux_managedDisk (444.20s)
=== RUN TestAccAzureRMVirtualMachineScaleSet_basicLinux_disappears
--- FAIL: TestAccAzureRMVirtualMachineScaleSet_basicLinux_disappears (323.27s)
testing.go:280: Step 0 error: Check failed: Check 2/2 error: Bad: Delete on vmScaleSetClient: compute.VirtualMachineScaleSetsClient#Delete: Failure sending request: StatusCode=0 -- Original Error: Get https://management.azure.com/subscriptions/e5ef2b13-6478-4887-ad57-1aa6b9475040/providers/Microsoft.Compute/locations/westus2/operations/c8570e71-ffbc-4ec9-a062-a1d2af8e873a?api-version=2016-04-30-preview: read tcp 10.138.31.172:58844->13.91.95.31:443: read: connection reset by peer
=== RUN TestAccAzureRMVirtualMachineScaleSet_loadBalancer
--- PASS: TestAccAzureRMVirtualMachineScaleSet_loadBalancer (348.82s)
=== RUN TestAccAzureRMVirtualMachineScaleSet_overprovision
--- PASS: TestAccAzureRMVirtualMachineScaleSet_overprovision (338.09s)
=== RUN TestAccAzureRMVirtualMachineScaleSet_extension
--- PASS: TestAccAzureRMVirtualMachineScaleSet_extension (399.41s)
=== RUN TestAccAzureRMVirtualMachineScaleSet_multipleExtensions
--- PASS: TestAccAzureRMVirtualMachineScaleSet_multipleExtensions (338.61s)
=== RUN TestAccAzureRMVirtualMachineScaleSet_osDiskTypeConflict
--- PASS: TestAccAzureRMVirtualMachineScaleSet_osDiskTypeConflict (74.91s)
FAIL
exit status 1
FAIL github.com/hashicorp/terraform/builtin/providers/azurerm 5957.883s
Makefile:48: recipe for target 'testacc' failed
make: *** [testacc] Error 1

=== RUN TestAccAzureRMVirtualMachineScaleSet_basicLinux_disappears
--- PASS: TestAccAzureRMVirtualMachineScaleSet_basicLinux_disappears (350.48s)
PASS
ok github.com/hashicorp/terraform/builtin/providers/azurerm 350.489s

thanks so much for testing @StephenWeatherford!

@StephenWeatherford sorry, totally missed the comment above until now. I'll take a look at this PR tomorrow, sorry for the delay!

tombuildsstuff · 2017-05-02T14:24:39Z

builtin/providers/azurerm/import_arm_virtual_machine_scale_set_test.go

+	resourceName := "azurerm_virtual_machine_scale_set.test"
+
+	ri := acctest.RandInt()
+	config := fmt.Sprintf(testAccAzureRMVirtualMachineScaleSet_basic, ri, ri, ri, ri, ri, ri, ri, ri)


@sozercan so currently this test is identical to the one above it TestAccAzureRMVirtualMachineScaleSet_importBasic, as it's running the same test with the same inputs/outputs.

Instead - I believe this test was meant to use the testAccAzureRMVirtualMachineScaleSet_basicLinux_managedDisk config? If so - is it possible to switch this out as you've identified? e.g.

config := fmt.Sprintf(testAccAzureRMVirtualMachineScaleSet_basicLinux_managedDisk, ri, ri, ri, ri, ri, ri)

Thanks :)

tombuildsstuff · 2017-05-02T14:26:05Z

builtin/providers/azurerm/resource_arm_virtual_machine_scale_set.go

 						"caching": {
 							Type:     schema.TypeString,
-							Required: true,
+							Optional: true,
+							Computed: true,


That works :)

StephenWeatherford · 2017-05-02T19:26:04Z

@tombuildsstuff, could you please check this one out again as soon as possible? We've got several other work items blocked by this merge, so we'd like to get it in quickly. Thanks!

Update: Sorry, it looks like we're currently waiting on Sertac.

stack72 · 2017-05-12T12:57:57Z

Hi @sozercan

Thanks for waiting on this - @tombuildsstuff and I have just talked about this and we are happy with the last set of changes :)

Thanks for all the work here

Paul

ghost · 2020-04-12T02:25:08Z

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.

sozercan added 5 commits April 16, 2017 20:34

added vmss with managed disk support

8aabb06

Update vmss docs

05c9119

update vmss test

7c83508

added vmss managed disk import test

edc5078

update vmss tests

64fbab8

tombuildsstuff added enhancement provider/azurerm labels Apr 17, 2017

remove unused test resources

353ed8e

reverting breaking changes on storage_os_disk and storage_image_refer…

8057c86

…ence

tombuildsstuff self-assigned this Apr 26, 2017

tombuildsstuff suggested changes Apr 26, 2017

View reviewed changes

StephenWeatherford reviewed Apr 27, 2017

View reviewed changes

sozercan added 2 commits April 27, 2017 17:01

updated vmss tests and documentation

899c31a

updated vmss flatten osdisk

11e524a

sozercan commented May 1, 2017

View reviewed changes

sozercan changed the title ~~provider/azurerm: Virtual Machine Scale Sets with managed disk support~~ [MS] provider/azurerm: Virtual Machine Scale Sets with managed disk support May 1, 2017

tombuildsstuff reviewed May 2, 2017

View reviewed changes

tombuildsstuff added the waiting-response An issue/pull request is waiting for a response from the community label May 2, 2017

updated vmss resource and import test

79e7cb2

tombuildsstuff removed the waiting-response An issue/pull request is waiting for a response from the community label May 8, 2017

sozercan added 3 commits May 9, 2017 11:32

update name in vmss osdisk

966659e

update vmss test to include a blank name

672e347

update vmss test to include a blank name

67e4bda

stack72 merged commit e0cd380 into hashicorp:master May 12, 2017

sozercan deleted the vmss-managed branch May 12, 2017 16:11

stack72 mentioned this pull request May 24, 2017

Unable to specify data disks for azurerm_virtual_machine_scale_set in storage profile section #14791

Closed

ghost locked and limited conversation to collaborators Apr 12, 2020

		@@ -838,8 +872,8 @@ func resourceArmVirtualMachineScaleSetStorageProfileOsDiskHash(v interface{}) in
		m := v.(map[string]interface{})
		buf.WriteString(fmt.Sprintf("%s-", m["name"].(string)))


		osDisk := &compute.VirtualMachineScaleSetOSDisk{
		Name: &name,

[MS] provider/azurerm: Virtual Machine Scale Sets with managed disk support #13717

[MS] provider/azurerm: Virtual Machine Scale Sets with managed disk support #13717

Conversation

sozercan commented Apr 17, 2017

tombuildsstuff commented Apr 18, 2017

cberner commented Apr 20, 2017

cloudbooster commented Apr 25, 2017

cloudbooster commented Apr 25, 2017

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sozercan left a 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

sozercan May 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sozercan May 9, 2017 • edited Loading

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 May 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StephenWeatherford commented May 2, 2017 • edited Loading

stack72 commented May 12, 2017

ghost commented Apr 12, 2020

sozercan May 5, 2017 •

edited

Loading

sozercan May 9, 2017 •

edited

Loading

tombuildsstuff May 11, 2017 •

edited

Loading

StephenWeatherford commented May 2, 2017 •

edited

Loading