Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New resource: azurerm_orchestrated_virtual_machine_scale_set #6626

Merged
merged 29 commits into from
May 21, 2020

Conversation

ArcturusZhang
Copy link
Contributor

@ArcturusZhang ArcturusZhang commented Apr 26, 2020

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

@ArcturusZhang 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
Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

"github.com/terraform-providers/terraform-provider-azurerm/azurerm/utils"
)

func dataSourceArmVirtualMachineScaleSetOrchestratorVM() *schema.Resource {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I just tried that eastus2 only accepts 5.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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": []
                }
            ],

@ArcturusZhang 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
Copy link
Contributor Author

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

Copy link
Member

@jackofallops jackofallops left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines 176 to 177
virtual_machine_scale_set_id = azurerm_orchestrated_virtual_machine_scale_set.test.id
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing zone = here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, right! thanks for catching this.

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"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

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
Copy link
Member

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.

Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@katbyte katbyte added this to the v2.11.0 milestone May 21, 2020
@katbyte katbyte merged commit 9982911 into hashicorp:master May 21, 2020
katbyte added a commit that referenced this pull request May 21, 2020
@ghost
Copy link

ghost commented May 22, 2020

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
Copy link
Contributor Author

ArcturusZhang commented May 22, 2020

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

ghost commented Jun 21, 2020

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!

@ghost ghost locked and limited conversation to collaborators Jun 21, 2020
@ArcturusZhang ArcturusZhang deleted the Orchestration-mode branch June 30, 2020 09:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for VMSS orchestration mode
4 participants