[MS] Adding support for Image Resources and Custom Images with Managed Disks #8

echuvyrov · 2017-06-13T02:01:56Z

Support for Image ARM Resource and Custom Images with Managed Disks (porting from v0.9.8)

tombuildsstuff

Thanks for pushing those updates and porting this over to the new repo - I've reviewed and left some comments in-line but this is looking good :)

Thanks!

tombuildsstuff · 2017-06-13T09:27:05Z

azurerm/resource_arm_image_test.go

+	return nil
+}
+
+var testAccAzureRMImage_standaloneImage_setup = `


in newer resources, we're switching these for functions which return the formatted strings - can we update this to match?

tombuildsstuff · 2017-06-13T09:33:09Z

azurerm/resource_arm_image_test.go

+var testAccAzureRMImage_standaloneImage_setup = `
+resource "azurerm_resource_group" "test" {
+    name = "acctestRG-%[1]d"
+    location = "West Central US"


for consistency, could we keep all of these resources in West US?

tombuildsstuff · 2017-06-13T09:33:26Z

azurerm/resource_arm_image_test.go

+var testAccAzureRMImage_standaloneImage_provision = `
+resource "azurerm_resource_group" "test" {
+	name = "acctestRG-%[1]d"
+	location = "West Central US"


for consistency, could we keep all of these resources in West US?

tombuildsstuff · 2017-06-14T15:42:21Z

azurerm/resource_arm_image.go

+				ConflictsWith: []string{"os_disk_os_type"},
+			},
+
+			"os_disk_os_type": {


is it worth placing these fields in an os_disk hash to match data_disk / the other resources?

tombuildsstuff · 2017-06-14T15:42:35Z

azurerm/resource_arm_image.go

+
+			"os_disk_size_gb": {
+				Type:     schema.TypeInt,
+				Optional: true,


can this also be Computed?

tombuildsstuff · 2017-06-16T10:14:54Z

website/docs/r/image.html.markdown

+* `os_disk_blob_uri` - (Optional) Specifies the URI in Azure storage of the blob that you want to use to create the image.
+* `os_disk_caching` - (Optional) Specifies the caching mode as 'readonly', 'readwrite', or 'none'. The default is none.
+* `os_disk_size_gb` - (Optional) Specifies the size of the image to be created. The target size can't be smaller than the source size.
+* `data_disk` - (Optional) The properties of the data images that 


We'd tend to phrase this "One or more data_disk elements as defined below"

tombuildsstuff · 2017-06-16T10:18:55Z

website/docs/r/image.html.markdown

+    the image. Changing this forces a new resource to be created.
+* `location` - (Required) Specified the supported Azure location where the resource exists.
+    Changing this forces a new resource to be created.
+* `source_virtual_machine_id` - (Required when creating image from existing VM) 


We'd tend to phrase this as:

* `source_virtual_machine_id` - (Optional) The Virtual Machine ID from which to create the image

Tried to match your style there as well

tombuildsstuff · 2017-06-16T10:19:31Z

website/docs/r/image.html.markdown

+* `source_virtual_machine_id` - (Required when creating image from existing VM) 
+    ID of an existing VM from which the image
+    will be created. VM must be generalized prior to creating image.
+* `os_disk_os_type` - (Required) Specifies the type of operating system contained in the the virtual machine image. Possible values are: Windows or Linux.


as above: can we split this out into an os_disk block?

tombuildsstuff · 2017-06-16T10:25:11Z

azurerm/import_arm_image_test.go

+	password := "Password1234!"
+	hostName := "tftestcustomimagesrc"
+	dnsName := fmt.Sprintf("%[1]s.westcentralus.cloudapp.azure.com", hostName)
+	sshPort := "22"


I think we can safely default this field within the testGeneralizeVMImage function?

Not sure if we could. I missed the random seed needed to avoid collisions between host names initially, but now that I put it in, the random seed is needed to generalize the VM since the DNS Name is one of the params to that function now. Open to different ideas here as well.

That's true, however we should be able to pass in only the username, password, hostname and region - as we can infer the rest from that?

tombuildsstuff · 2017-06-16T10:29:52Z

azurerm/resource_arm_image_test.go

+			ssh.Password(password),
+		},
+		//line below for the newer versions of SSH Client
+		//HostKeyCallback: ssh.InsecureIgnoreHostKey(),


should we remove this line for the moment?

…form-provider-azurerm into customimage

echuvyrov · 2017-06-20T16:55:42Z

@tombuildsstuff I refactored the dnsName out of individual funcs, let me know if these changes look good to you.

YuriyKishchenko · 2017-06-27T13:45:42Z

Hi @echuvyrov
Could you please apply the same changes to the virtual_machine_scale_set as many parts seems to be copied from virtual_machine and image uri is also applicable here.

echuvyrov · 2017-06-27T15:35:04Z

Hi @YuriyKischenko - that's the plan :)! I wanted to split the work into smaller chunks, but VMSS support is right behind. I don't think it'll be nearly as much work, since the Image resource would be already done.

tombuildsstuff

Hey @echuvyrov

Apologies for the delay in reviewing this!

Thanks for the continued effort here - I've taken another look and left some comments in-line. The biggest issue I see is around changing the Hash's, which may require a State Migration given the underlying value has changed?

Thanks!

tombuildsstuff · 2017-06-27T17:16:49Z

azurerm/resource_arm_image.go

+			"source_virtual_machine_id": {
+				Type:          schema.TypeString,
+				Optional:      true,
+				ConflictsWith: []string{"os_disk.os_type"},


I might be wrong, but I don't think this syntax is supported?

I used an example from virtual_machine as a reference (such as below) - let me know if I should change:

"managed_disk_id": {
Type: schema.TypeString,
Optional: true,
ForceNew: true,
Computed: true,
ConflictsWith: []string{"storage_os_disk.vhd_uri"},
},

So this syntax isn't supported right now - it was merged in order to progress with the Managed Disks PR (with the anticipation of hashicorp/terraform#13019 being merged, but we've not had the time to clean it up yet (hence these comments)

For the moment it'd be good to remove these if possible?

tombuildsstuff · 2017-06-27T17:20:25Z

azurerm/resource_arm_image.go

+
+						"os_type": {
+							Type:     schema.TypeString,
+							Optional: true,


the documentation doesn't make this clear - but I think this can be Required within the optional os_disk block?

should this also be ForceNew?

When I made this field required, I got the following error:

"1 error(s) occurred:\n\n* resource azurerm_image: source_virtual_machine_id: ConflictsWith cannot contain Required attribute (os_disk.os_type)"}"

So I left the field as optional, but added ForceNew

tombuildsstuff · 2017-06-27T17:20:54Z

azurerm/resource_arm_image.go

+
+						"os_state": {
+							Type:     schema.TypeString,
+							Optional: true,


the documentation doesn't make this clear - but I think this can be Required within the optional os_disk block?

should this also be ForceNew?

same limitation as above - conflict between "Required" and "ConflictsWith"

tombuildsstuff · 2017-06-27T17:21:34Z

azurerm/resource_arm_image.go

+						"caching": {
+							Type:     schema.TypeString,
+							Optional: true,
+							Default:  "None",


could we make this string(compute.None)

tombuildsstuff · 2017-06-27T17:27:39Z

azurerm/resource_arm_image.go

+		}
+	}
+
+	//either source VM or storage profile can be specified, but not both


is it worth checking this and returning an error if so?

tombuildsstuff · 2017-06-27T17:45:32Z

azurerm/resource_arm_image_test.go

+				Destroy: false,
+				Check: resource.ComposeTestCheckFunc(
+					testCheckAzureVMExists("azurerm_virtual_machine.testsource", true),
+					testGeneralizeVMImage(fmt.Sprintf("acctestRG-%[1]d", ri), "testsource",


minor: if we're only formatting a single value, there's no need for the [1] here?

tombuildsstuff · 2017-06-27T17:47:19Z

azurerm/resource_arm_virtual_machine.go

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


if this is changing the value of the hash, is this going to need a state migration?

tombuildsstuff · 2017-06-27T17:47:59Z

azurerm/resource_arm_virtual_machine.go

-	result["publisher"] = *image.Publisher
-	result["sku"] = *image.Sku
-
+	if image.Publisher != nil {


(same) if this is changing the value of the hash - is this going to need a state migration?

tombuildsstuff · 2017-06-27T17:48:37Z

azurerm/resource_arm_virtual_machine.go

-	offer := storageImageRef["offer"].(string)
-	sku := storageImageRef["sku"].(string)
-	version := storageImageRef["version"].(string)
+	if storageImageRef["image_id"] != "" {


it'd probably be better for us to error on both being set?

tombuildsstuff · 2017-06-27T17:48:53Z

azurerm/resource_arm_virtual_machine.go

-		Sku:       &sku,
-		Version:   &version,
-	}, nil
+	if imageReference.ID != nil && imageReference.Publisher != nil {


(oh) can we move this above? :)

echuvyrov · 2017-06-29T06:57:06Z

Hi @tombuildsstuff, I made all the changes requested, except for state migration recommendation. I think I need to understand this better - in my mind, I don't think we need state migration since we have a custom hash function. This custom hash function should not change the hash value for existing resources, since the fields for publisher, offer, sku, version should still all be present for non-custom images. Would appreciate you clarifying the thoughts behind the need for state migration please.

Here's the validation I performed for state, let me know if any of those are incorrect:
-Downloaded current public release of TF (0.9.9) and created basic VM + managed disk config (no custom image). Ran 'terraform plan' and the plan is showing what I expected to see. Ran 'terraform apply' to provision infra and persist state.
-Compiled terraform 0.10.0-dev with my changes. Ran 'terraform plan' on the same config - no changes detected, as expected.
-Changed sku in the config, 'terraform plan' shows 1 resource to destroy, 1 to create. As expected, since OS type has 'ForceNew' on it.
-Added the new image resource and changed VM config to use that image resource with custom image. 'terraform plan' shows 3 resources to create, 1 to destroy. I am not sure why 3, since I was expecting just 2 (1 image, 1 VM), but I am not sure if this has to do with hashing/state migration.

Thanks a ton as always!

re-reviewing

tombuildsstuff

Hey @echuvyrov

Sorry for the delay in reviewing this!

Thanks for the continued effort here - I've taken another look through and have commented on a couple of minor issues - but this is looking good :)

Thanks!

tombuildsstuff · 2017-07-04T18:02:50Z

azurerm/resource_arm_image.go

+			"source_virtual_machine_id": {
+				Type:          schema.TypeString,
+				Optional:      true,
+				ConflictsWith: []string{"os_disk.os_type"},


So this syntax isn't supported right now - it was merged in order to progress with the Managed Disks PR (with the anticipation of hashicorp/terraform#13019 being merged, but we've not had the time to clean it up yet (hence these comments)

For the moment it'd be good to remove these if possible?

tombuildsstuff · 2017-07-07T20:17:33Z

azurerm/resource_arm_image.go

+
+						"blob_uri": {
+							Type:     schema.TypeString,
+							Optional: true,


is this also computed?

I don't think it should be - same as blob_uri for os_disk above IMHO

tombuildsstuff · 2017-07-07T20:18:00Z

azurerm/resource_arm_image.go

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


is this also computed?

I don't think this should be computed, if I understood the "Computed" property correctly. In the Developer's guide, "any Resource or Data Source property that should be stored in state but that the user should not be able to configure should have its Computed property set to true."

Per API spec here blobUri "specifies the URI in Azure storage of the blob that you want to use to create the virtual machine image" which indicates it should be changeable by the user.

tombuildsstuff · 2017-07-07T20:18:15Z

azurerm/resource_arm_image.go

+						"caching": {
+							Type:     schema.TypeString,
+							Optional: true,
+							Default:  "None",


is this also computed?

I don't think it should be - "You can specify the caching mode of the disks as readonly, readwrite, or none, by adding hostCaching. The default is none." Unless I misunderstood the Computed property of course :)

tombuildsstuff · 2017-07-07T20:18:21Z

azurerm/resource_arm_image.go

+
+						"size_gb": {
+							Type:     schema.TypeInt,
+							Optional: true,


is this also computed?

I don't think it should be - "you can resize an operating system disk or the data disk in the virtual machine image as you create it by adding diskSizeGB." Unless I misunderstood the Computed property of course :)

so by Computed I mean we will store the remote value in the state regardless of whether it's specified or not (but it depends if it's retuned from the API) :)

tombuildsstuff · 2017-07-07T20:24:22Z

azurerm/resource_arm_image.go

+
+		if managedDiskID != "" {
+			managedDisk := &compute.SubResource{}
+			managedDisk.ID = &managedDiskID


could we combine these two lines?

tombuildsstuff · 2017-07-07T20:33:18Z

azurerm/resource_arm_virtual_machine.go

@@ -96,21 +96,27 @@ func resourceArmVirtualMachine() *schema.Resource {
 				MaxItems: 1,
 				Elem: &schema.Resource{
 					Schema: map[string]*schema.Schema{
+						"image_id": {


🤔 whilst I'm not opposed to this - would it make more sense to call this id to match the API Documentation / ARM Template?

tombuildsstuff · 2017-07-07T20:38:45Z

azurerm/resource_arm_virtual_machine.go

+		imageID := storageImageRef["image_id"].(string)
+		imageReference = compute.ImageReference{
+			ID: &imageID,
+		}


we could simplify this to use the existing imageReference object defined above:

if v, ok := storageImageRef["image_id"]; ok { imageReference.ID = azure.String(v.(string)) } else { .. }

tombuildsstuff · 2017-07-07T20:39:35Z

website/azurerm.erb

@@ -174,6 +174,15 @@
              </ul>
            </li>

+            <li<%= sidebar_current("docs-azurerm-resource-image") %>>
+              <a href="#">Image Resources</a>


is it worth placing this in the storage sub-section?

related: I think we should probably reorganise the sidebar in general

I put it under "Managed Disk" to match the sidebar here, hope that's Ok; agree that we should do a bit of re-organization of the side bar

tombuildsstuff · 2017-07-07T20:44:44Z

website/docs/r/image.html.markdown

+    Changing this forces a new resource to be created.
+* `source_virtual_machine_id` - (Optional) The Virtual Machine ID from which to create the image.
+* `os_disk` - (Optional) One or more os_disk elements as defined below.
+* `data_disk` - (Optional) One or more data_disk elements as defined below.


we'd normally wrap this in quote's: One or more `data_disk` elements as defined below.

echuvyrov · 2017-07-11T21:18:12Z

Hey @tombuildsstuff I made the blob_uri, caching and size_gb properties computed and made the tests pass again :). Let me know if the changes look good to you - thanks!

tombuildsstuff

Hey @echuvyrov

Thanks for pushing those updates - I've noticed (and pushed a fix for) a couple of minor issues - but this LGTM! :)

Thanks!

tombuildsstuff · 2017-07-18T23:37:49Z

website/docs/r/image.html.markdown

+* `lun` - (Required) Specifies the logical unit number of the data disk.
+* `managed_disk_id` - (Optional) Specifies the ID of the managed disk resource that you want to use to create the image.
+* `blob_uri` - (Optional) Specifies the URI in Azure storage of the blob that you want to use to create the image.
+* `caching` - (Optional) Specifies the caching mode as readonly, readwrite, or none. The default is none.


we tend to put quotes around these values, and match the casing in Azure, e.g. ReadWrite - can we update this to match?

tombuildsstuff · 2017-07-18T23:37:56Z

website/docs/r/image.html.markdown

+* `os_state` - (Required) Specifies the state of the operating system contained in the blob. Currently, the only value is Generalized.
+* `managed_disk_id` - (Optional) Specifies the ID of the managed disk resource that you want to use to create the image.
+* `blob_uri` - (Optional) Specifies the URI in Azure storage of the blob that you want to use to create the image.
+* `caching` - (Optional) Specifies the caching mode as 'readonly', 'readwrite', or 'none'. The default is none.


we tend to put quotes around these values, and match the casing in Azure, e.g. ReadWrite - can we update this to match?

tombuildsstuff · 2017-07-18T23:39:49Z

website/docs/r/image.html.markdown

+  os_disk_os_type = "Linux"
+  os_disk_os_state = "Generalized"
+  os_disk_blob_uri = "{blob_uri}"
+  os_disk_size_gb = 30


can we update this example, as os_disk is now a block rather than a prefix for these fields :)

tombuildsstuff · 2017-07-19T10:52:26Z

azurerm/resource_arm_image_test.go

+		}
+
+		if resp.StatusCode != http.StatusNotFound {
+			return fmt.Errorf("Managed Disk still exists: \n%#v", resp.Properties)


Managed Disk -> Managed Image

tombuildsstuff · 2017-07-19T13:40:52Z

Tests pass (ignoring a subscription-specific test failure):

Merge from upstream

ghost · 2020-04-01T15:32:50Z

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!

Adding support for Image Resources and Custom Images with Managed Disks

081d440

echuvyrov mentioned this pull request Jun 13, 2017

[MS] Added ARM Image resource and made VM managed disks worked with custom images hashicorp/terraform#14588

Closed

Adding random values to the host names in tests to ensure uniqueness

edbccac

tombuildsstuff added enhancement new-resource labels Jun 16, 2017

tombuildsstuff suggested changes Jun 16, 2017

View reviewed changes

echuvyrov added 3 commits June 19, 2017 23:47

Changed os_disk to a block, other changes requested for a PR merge

19c9450

Merge branch 'master' of https://github.com/terraform-providers/terra…

8ba7c33

…form-provider-azurerm into customimage

Minor refactoring of DNS name inside the deprovisionVM func

0d19808

tombuildsstuff previously requested changes Jun 27, 2017

View reviewed changes

Better formatting and validation

d0ec942

tombuildsstuff reviewed Jul 7, 2017

View reviewed changes

echuvyrov added 2 commits July 9, 2017 12:56

Changed properties, simplified code after review

457bb06

Minor changes to Image properties

af84909

tombuildsstuff added 3 commits July 19, 2017 14:18

Fixing the Creating from VHD example

2790ca2

Managed Disk -> Managed Image

bf1adf3

Updating the casings to match azure

8f91395

tombuildsstuff approved these changes Jul 19, 2017

View reviewed changes

tombuildsstuff merged commit ed3a1a3 into hashicorp:master Jul 19, 2017

tombuildsstuff added a commit that referenced this pull request Jul 19, 2017

Updating to include #8

1ece8b1

tombuildsstuff pushed a commit that referenced this pull request Oct 5, 2017

Merge pull request #8 from terraform-providers/master

dd7b283

Merge from upstream

siddthota mentioned this pull request May 31, 2018

Issue with spinning-up SQL Server 2012 running on Windows Server 2012 R2 Datacenter VM? #1325

Closed

ghost locked and limited conversation to collaborators Apr 1, 2020

[MS] Adding support for Image Resources and Custom Images with Managed Disks #8

[MS] Adding support for Image Resources and Custom Images with Managed Disks #8

Conversation

echuvyrov commented Jun 13, 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

echuvyrov commented Jun 20, 2017

YuriyKishchenko commented Jun 27, 2017

echuvyrov commented Jun 27, 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

echuvyrov commented Jun 29, 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

echuvyrov commented Jul 11, 2017

tombuildsstuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment