New resource: `azurerm_orchestrated_virtual_machine_scale_set` #6626

ArcturusZhang · 2020-04-26T07:08:36Z

This PR implements the new orchestration mode VM of VMSS by introducing the new resource azurerm_orchestrated_virtual_machine_scale_set.

For more information about the orchestration mode of VMSS, please refer this doc

Fixes #6085

…ual_machine_scale_set

tombuildsstuff

hi @ArcturusZhang

Thanks for this PR - I've taken a look through and left some comments inline, but this is off to a good start - if we can fix up the comments then we can take another look 👍

Thanks!

azurerm/internal/services/compute/orchestrated_virtual_machine_scale_set_resource.go

azurerm/internal/services/compute/tests/orchestrated_virtual_machine_scale_set_resource_test.go

azurerm/internal/services/compute/linux_virtual_machine_resource.go

tombuildsstuff · 2020-05-14T17:45:45Z

azurerm/internal/services/compute/orchestrated_virtual_machine_scale_set_data_source.go

+	"github.com/terraform-providers/terraform-provider-azurerm/azurerm/utils"
+)
+
+func dataSourceArmVirtualMachineScaleSetOrchestratorVM() *schema.Resource {


what's the purpose of making this it's own data source? we may as well reuse the existing one

Actually we currently do not have a data source for VMSS.
But I think it makes more sense to add a data source for all the types of VMSS. Removing this data source.

tombuildsstuff · 2020-05-14T18:13:14Z

azurerm/internal/services/compute/tests/orchestrated_virtual_machine_scale_set_resource_test.go

+func testAccAzureRMOrchestratedVirtualMachineScaleSet_template(data acceptance.TestData) string {
+	// in VMSS VMO mode, the `platform_fault_domain_count` has different acceptable values for different locations,
+	// therefore this location is fixed to EastUS2 to make sure the acceptance test has no issues about this value
+	location := "EastUS2"


almost all of the regions support 2 - so we can use a dynamic location and set this to 2? https://github.com/MicrosoftDocs/azure-docs/blob/master/includes/managed-disks-common-fault-domain-region-list.md

if necessary we can look to add a data source for this information that looks it up in the API - but for now it looks sufficient to hard-code this to 2?

Well, I just tried that eastus2 only accepts 5.

And I got some feedback from the service team, there is a difference when the VMSS is zonal or not. In this case, when zonal, eastus2 only supports platform_fault_domain_count = 5.
Different locations have different zone support list as well.

This is incongruous with the data in the link supplied in the docs below, which suggests 3 is the max for EastUS2? (as does querying the API).
I've done a little digging and it would appear that the only value accepted for any supported region when zones is set, is 5, could you confirm this with the service team also? (and update code/docs accordingly).

I could check with the service team about this....
But I am afraid that the problem remains: different locations support different list of zones, and the list of zones is subscription dependent.
However I am fine with replacing this hard-coded location with data.Locations.Primary, what do you think? If you prefer we move this hard-coded location away, I will do it then

And you can use this REST API https://management.azure.com/subscriptions/{subsID}/providers/Microsoft.Compute/?api-version=2019-07-01 to query the zone mappings in your subscription.
My result is:

"zoneMappings": [ { "location": "East US 2", "zones": [ "2", "3", "1" ] }, { "location": "Central US", "zones": [ "2", "3", "1" ] }, { "location": "West Europe", "zones": [ "2", "3", "1" ] }, { "location": "East US 2 EUAP", "zones": [ "2", "3", "1" ] }, { "location": "Central US EUAP", "zones": [ "1", "2" ] }, { "location": "France Central", "zones": [ "2", "3", "1" ] }, { "location": "Southeast Asia", "zones": [ "2", "3", "1" ] }, { "location": "West US 2", "zones": [ "2", "3", "1" ] }, { "location": "North Europe", "zones": [ "2", "3", "1" ] }, { "location": "East US", "zones": [ "2", "3", "1" ] }, { "location": "UK South", "zones": [ "2", "3", "1" ] }, { "location": "Japan East", "zones": [ "2", "3", "1" ] }, { "location": "Australia East", "zones": [] }, { "location": "South Africa North", "zones": [] }, { "location": "South Central US", "zones": [] }, { "location": "Canada Central", "zones": [] } ],

azurerm/internal/services/compute/orchestrated_virtual_machine_scale_set_resource.go

ArcturusZhang · 2020-05-15T10:14:18Z

Hi @tombuildsstuff I have made some changes, could you please have a look?

jackofallops

Hi @ArcturusZhang
Tom's not around for a couple of days so I've taken a quick look over to help keep things moving.

To me this looks fine for what is a preview resource. I've made a couple of additional comments below, one on the API values which could be misleading / confusing to users; and another on what looks to have been a missed test update when you made zone required when virtual_machine_scale_set_id is specified.

jackofallops · 2020-05-21T10:28:53Z

azurerm/internal/services/compute/tests/windows_virtual_machine_resource_orchestrated_test.go

+  virtual_machine_scale_set_id = azurerm_orchestrated_virtual_machine_scale_set.test.id
+}


Missing zone = here?

Oh, right! thanks for catching this.

jackofallops · 2020-05-21T11:36:41Z

azurerm/internal/services/compute/tests/orchestrated_virtual_machine_scale_set_resource_test.go

+func testAccAzureRMOrchestratedVirtualMachineScaleSet_template(data acceptance.TestData) string {
+	// in VMSS VMO mode, the `platform_fault_domain_count` has different acceptable values for different locations,
+	// therefore this location is fixed to EastUS2 to make sure the acceptance test has no issues about this value
+	location := "EastUS2"


This is incongruous with the data in the link supplied in the docs below, which suggests 3 is the max for EastUS2? (as does querying the API).
I've done a little digging and it would appear that the only value accepted for any supported region when zones is set, is 5, could you confirm this with the service team also? (and update code/docs accordingly).

ArcturusZhang · 2020-05-21T13:54:44Z

Hi @jackofallops I have fixed the tests, please have a look.

As for the platform_fault_domain_count and zones, still it is highly subscription and location dependent, this is why I have to hard code the location in the test and actually this is not enough since the zonal support is also different in different locations and subscriptions. But we could configurate it to fit your testing subcription to let the acceptance test pass (and show that this resource is working as expected.)

jackofallops · 2020-05-21T14:15:43Z

Hi @ArcturusZhang
Apologies, I didn't explain very well. I was referring to the allowed values for platform_fault_domain_count. If zones is specified in the config for the resource, the only valid value is 'platform_fault_domain_count = 5'.
From orchestration mode docs

Fault domains | Can define fault domains. 2 or 3 based on regional support and 5 for Availability zone.
If zones is omitted, the values from the table / API calls can be used.
The tests should be safe to use data.Locations.Primary for location based on this.

katbyte

Thanks @ArcturusZhang,

As the only outstanding comment is wrt testing i'm going to merge this for 2.11 - it would be great in the future to remove the hard coded location and write a data source to get the value from the API as tom suggested.

ghost · 2020-05-22T01:04:02Z

This has been released in version 2.11.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 2.11.0"
}
# ... other configuration ...

ArcturusZhang · 2020-05-22T03:58:27Z

Thanks @ArcturusZhang,

As the only outstanding comment is wrt testing i'm going to merge this for 2.11 - it would be great in the future to remove the hard coded location and write a data source to get the value from the API as tom suggested.

Thanks @katbyte ! That would be ideal, but frankly I cannot find the the REST API equivalence to get the zone mappings in go SDK.... maybe it is not in the REST API specs... But we do have ListSkus.

ghost · 2020-06-21T14:50:06Z

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!

ArcturusZhang added 23 commits April 9, 2020 11:54

Refine VM ID parse functions

b48bd4b

Some follow ups on VM ID refactor

4f30719

VMSS and extension refactor

5658d03

VM extension refactor

6facd42

Managed disk ID

94236b9

Fix CI

865f825

Fix another missed one

988a70c

go imports

48a2e32

Goimports again...

d54f13f

Add new resource of VMSS in VMO mode

b873afe

Modify VM to support create VM in VMSS VMO mode

5a4cf2c

Fix some problems

79ddbbe

Initial commit of acc test

b69881f

Merge remote-tracking branch 'origin/master' into Orchestration-mode

9d33259

Finalize tests and doc

0cb68ac

Merge remote-tracking branch 'origin/master' into Orchestration-mode

85d2761

Merge remote-tracking branch 'origin/master' into Orchestration-mode

86ea743

Finalize orchestration mode

cec54f6

Merge remote-tracking branch 'origin/master' into Orchestration-mode

8397ec4

terrafmt

aa5bb58

Resolve comments

bd796fc

gofmt

cdcfa20

terrafmt

b5bd514

ghost added size/XXL documentation labels Apr 26, 2020

ArcturusZhang requested a review from katbyte April 26, 2020 07:08

ArcturusZhang added new-data-source new-resource service/vmss Virtual Machine Scale Sets labels Apr 26, 2020

ArcturusZhang requested a review from tombuildsstuff April 28, 2020 02:25

ArcturusZhang added 2 commits May 8, 2020 10:46

Merge remote-tracking branch 'origin/master' into Orchestration-mode

4c43f60

Rename virtual_machine_scale_set_orchestrator_vm to orchestrated_virt…

23f8129

…ual_machine_scale_set

ArcturusZhang changed the title ~~New resource / data source: azurerm_virtual_machine_scale_set_orchestrator_vm~~ New resource / data source: azurerm_orchestrated_virtual_machine_scale_set May 8, 2020

tombuildsstuff suggested changes May 14, 2020

View reviewed changes

tombuildsstuff added waiting-response preview labels May 14, 2020

Resolve comments

a7abdc5

ArcturusZhang changed the title ~~New resource / data source: azurerm_orchestrated_virtual_machine_scale_set~~ New resource: azurerm_orchestrated_virtual_machine_scale_set May 15, 2020

ArcturusZhang requested a review from tombuildsstuff May 15, 2020 10:13

Fix the bad link

8351c72

ArcturusZhang removed the waiting-response label May 18, 2020

Merge remote-tracking branch 'origin/master' into Orchestration-mode

8e7a2bd

jackofallops requested changes May 21, 2020

View reviewed changes

Fix bad test cases

563322f

katbyte approved these changes May 21, 2020

View reviewed changes

katbyte added this to the v2.11.0 milestone May 21, 2020

katbyte merged commit 9982911 into hashicorp:master May 21, 2020

katbyte added a commit that referenced this pull request May 21, 2020

update CHANGELOG.md to include #6626

d1f533d

ghost locked and limited conversation to collaborators Jun 21, 2020

ArcturusZhang deleted the Orchestration-mode branch June 30, 2020 09:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New resource: `azurerm_orchestrated_virtual_machine_scale_set` #6626

New resource: `azurerm_orchestrated_virtual_machine_scale_set` #6626

ArcturusZhang commented Apr 26, 2020 •

edited

Loading

tombuildsstuff left a comment

tombuildsstuff May 14, 2020

ArcturusZhang May 15, 2020

tombuildsstuff May 14, 2020

ArcturusZhang May 15, 2020

ArcturusZhang May 20, 2020

jackofallops May 21, 2020

ArcturusZhang May 21, 2020

ArcturusZhang May 21, 2020

ArcturusZhang commented May 15, 2020

jackofallops left a comment

jackofallops May 21, 2020

ArcturusZhang May 21, 2020

jackofallops May 21, 2020

ArcturusZhang commented May 21, 2020

jackofallops commented May 21, 2020

katbyte left a comment

ghost commented May 22, 2020

ArcturusZhang commented May 22, 2020 •

edited

Loading

ghost commented Jun 21, 2020

		virtual_machine_scale_set_id = azurerm_orchestrated_virtual_machine_scale_set.test.id
		}

New resource: azurerm_orchestrated_virtual_machine_scale_set #6626

New resource: azurerm_orchestrated_virtual_machine_scale_set #6626

Conversation

ArcturusZhang commented Apr 26, 2020 • edited Loading

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

ArcturusZhang commented May 15, 2020

jackofallops 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

ArcturusZhang commented May 21, 2020

jackofallops commented May 21, 2020

katbyte left a comment

Choose a reason for hiding this comment

ghost commented May 22, 2020

ArcturusZhang commented May 22, 2020 • edited Loading

ghost commented Jun 21, 2020

New resource: `azurerm_orchestrated_virtual_machine_scale_set` #6626

New resource: `azurerm_orchestrated_virtual_machine_scale_set` #6626

ArcturusZhang commented Apr 26, 2020 •

edited

Loading

ArcturusZhang commented May 22, 2020 •

edited

Loading