[MS] Added support for custom images with VMSS + Managed Disks #203

echuvyrov · 2017-07-31T05:30:15Z

Adding custom image support for virtual machine scale sets with managed disks

tombuildsstuff

Thanks for this PR - I've taken a look and left some comments inline, my main question is around the need for a state migration - but this is off to a good start :)

Thanks!

tombuildsstuff · 2017-07-31T12:36:01Z

azurerm/resource_arm_virtual_machine_scale_set.go

+	}
+	if image.ID != nil {
+		result["id"] = *image.ID
+	}


I have a feeling it shouldn't - but does this change the hash? i.e. when having previously built a resource using the VMSS resource in master, then running terraform plan against this branch - are there any changes shown?

Hey @tombuildsstuff thanks for a quick turnaround on this; I provisioned VMSS using TF v. 0.9.11, then ran terraform plan using my branch. Got the message

No changes. Infrastructure is up-to-date.

I believe this is expected behavior.

Awesome - thanks for checking/confirming that :)

tombuildsstuff · 2017-07-31T12:36:20Z

azurerm/resource_arm_virtual_machine_scale_set.go

+	}
+	if m["id"] != nil {
+		buf.WriteString(fmt.Sprintf("%s-", m["id"].(string)))
+	}


(same as above - does this require a state migration?)

(same as above) + I also ran a test where I created an Image resource and then set the id to point to that resource. Running terraform plan gave me the message

Plan: 3 to add, 0 to change, 1 to destroy.

which is also expected I believe (recreating VMSS).

tombuildsstuff · 2017-07-31T12:40:09Z

azurerm/resource_arm_virtual_machine_scale_set.go

+			Offer:     &offer,
+			Sku:       &sku,
+			Version:   &version,
+		}


tbh given this object is initialised above, we could we just assign these values here?

tombuildsstuff · 2017-07-31T13:02:20Z

website/docs/r/virtual_machine_scale_set.html.markdown

@@ -346,6 +346,27 @@ The following arguments are supported:

 `storage_profile_image_reference` supports the following:

+* `id` - (Optional) Specifies the ID of the (custom) image to use to create the virtual 
+machine scale set, for example:


I'd argue we might be better linking to an example for this, rather than having this in-line - what do you think?

I gave it a shot based on how I understood it - take a look and let me know if that's not what you had in mind.

Thanks for making those changes - I was thinking more of linking to an example within the Examples directory on Github - but on reflection perhaps that's a future task once this has been merged, and this is fine for the moment?

tombuildsstuff · 2017-07-31T13:02:50Z

azurerm/resource_arm_image_test.go

+
+		resp, err := vmssClient.Get(resourceGroup, vmssName)
+		if err != nil {
+			return fmt.Errorf("Bad: Get on vmssClient: %s", err)


can we make this formatting argument %+v to return the full error?

tombuildsstuff · 2017-07-31T13:03:29Z

website/docs/r/virtual_machine.html.markdown

@@ -275,7 +275,7 @@ resource "azurerm_virtual_machine" "test" {
  ...

 	storage_image_reference {
-		image_id = "${azurerm_image.test.id}"
+		id = "${azurerm_image.test.id}"


👍 good spot

echuvyrov · 2017-07-31T22:01:14Z

Hey @tombuildsstuff I made the changes requested (hope I got them all right :)), reran the tests and checked in the code.

make testacc TESTARGS='-run=TestAccAzureRMVirtualMachineScaleSet_basic'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run=TestAccAzureRMVirtualMachineScaleSet_basic -timeout 120m
?   	github.com/terraform-providers/terraform-provider-azurerm	[no test files]
=== RUN   TestAccAzureRMVirtualMachineScaleSet_basic
--- PASS: TestAccAzureRMVirtualMachineScaleSet_basic (451.08s)
=== RUN   TestAccAzureRMVirtualMachineScaleSet_basicLinux_managedDisk
--- PASS: TestAccAzureRMVirtualMachineScaleSet_basicLinux_managedDisk (316.75s)
=== RUN   TestAccAzureRMVirtualMachineScaleSet_basicLinux_managedDiskNoName
--- PASS: TestAccAzureRMVirtualMachineScaleSet_basicLinux_managedDiskNoName (336.32s)
=== RUN   TestAccAzureRMVirtualMachineScaleSet_basicLinux_disappears
--- PASS: TestAccAzureRMVirtualMachineScaleSet_basicLinux_disappears (415.10s)
PASS

make testacc TESTARGS='-run=TestAccAzureRMImageVMSS_customImageVMSSFromVHD'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run=TestAccAzureRMImageVMSS_customImageVMSSFromVHD -timeout 120m
?   	github.com/terraform-providers/terraform-provider-azurerm	[no test files]
=== RUN   TestAccAzureRMImageVMSS_customImageVMSSFromVHD
--- PASS: TestAccAzureRMImageVMSS_customImageVMSSFromVHD (810.90s)
PASS

tombuildsstuff · 2017-08-01T20:34:23Z

azurerm/resource_arm_virtual_machine_scale_set.go

+						"id": {
+							Type:     schema.TypeString,
+							Optional: true,
+							ForceNew: true,


isn't it possible to change the image on a VMSS (and then either roll the VMSS instances automatically or manually)? As such, does this want to be without the ForceNew?

(same question for Publisher/Offer/Sku/Version)

you are right @tombuildsstuff the update operation should not force new VMSS. I have updated my code to reflect that.

tombuildsstuff

Hey @echuvyrov

Thanks for pushing the latest changes - I've pushed a small commit to update the documentation for the publisher, offer, sku and product fields to be Optional to match the last change, but this otherwise LGTM :)

Thanks!

ghost · 2020-04-01T15:31: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 👉 [email protected]. Thanks!

echuvyrov added 2 commits July 30, 2017 22:27

Added support for custom images with VMSS + Managed Disks

85126e3

Updated documentation for managed disks with custom images

8021f98

tombuildsstuff reviewed Jul 31, 2017

View reviewed changes

tombuildsstuff added enhancement waiting-response labels Jul 31, 2017

echuvyrov added 2 commits July 31, 2017 13:52

Made changes based on initial review

f1ef150

Fixed minor typo in link

281b152

tombuildsstuff removed the waiting-response label Aug 1, 2017

tombuildsstuff reviewed Aug 1, 2017

View reviewed changes

echuvyrov and others added 2 commits August 1, 2017 17:03

Removed ForceNew properties on VMSS Image ID

bd4c001

optional fields are now shown as optional

76cb47f

tombuildsstuff approved these changes Aug 2, 2017

View reviewed changes

tombuildsstuff merged commit 0cdc415 into hashicorp:master Aug 2, 2017

tombuildsstuff added a commit that referenced this pull request Aug 2, 2017

Updating to include #203

0dce0c6

ghost locked and limited conversation to collaborators Apr 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MS] Added support for custom images with VMSS + Managed Disks #203

[MS] Added support for custom images with VMSS + Managed Disks #203

echuvyrov commented Jul 31, 2017

tombuildsstuff left a comment

tombuildsstuff Jul 31, 2017

echuvyrov Jul 31, 2017

tombuildsstuff Aug 1, 2017

tombuildsstuff Jul 31, 2017

echuvyrov Jul 31, 2017

tombuildsstuff Jul 31, 2017

echuvyrov Jul 31, 2017

tombuildsstuff Jul 31, 2017

echuvyrov Jul 31, 2017

tombuildsstuff Aug 1, 2017

tombuildsstuff Jul 31, 2017

echuvyrov Jul 31, 2017

tombuildsstuff Jul 31, 2017

echuvyrov commented Jul 31, 2017 •

edited

Loading

tombuildsstuff Aug 1, 2017

echuvyrov Aug 2, 2017

tombuildsstuff left a comment

ghost commented Apr 1, 2020

[MS] Added support for custom images with VMSS + Managed Disks #203

[MS] Added support for custom images with VMSS + Managed Disks #203

Conversation

echuvyrov commented Jul 31, 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

echuvyrov commented Jul 31, 2017 • edited Loading

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

ghost commented Apr 1, 2020

echuvyrov commented Jul 31, 2017 •

edited

Loading