-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
azurerm_orchestrated_virtual_machine_scale_set
- add sku_profile
to support instance mix feature
#27599
azurerm_orchestrated_virtual_machine_scale_set
- add sku_profile
to support instance mix feature
#27599
Conversation
azurerm_orchestrated_virtual_machine_scale_set
- add sku_profile
to support mix instance featureazurerm_orchestrated_virtual_machine_scale_set
- add sku_profile
to support instance mix feature
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ms-zhenhua - Thanks for this PR. A few things to take a look at below. It also looks like there's inconsistency between the schema and the code for the new block, can you take a look?
Thanks
// currently only supported in EastUS, WestUS, EastUS2, and WestUS2 | ||
data.Locations.Primary = "eastus2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we update this to be a TODO so we know it needs to be removed when the Preview is rolled out to our normal testing region(s).
Can we get an estimate from the Service team as to when this will be deployed to the normal regions?
// currently only supported in EastUS, WestUS, EastUS2, and WestUS2 | |
data.Locations.Primary = "eastus2" | |
// TODO - Remove this override when Preview is rolled out to westeurope - currently only supported in EastUS, WestUS, EastUS2, and WestUS2 | |
data.Locations.Primary = "eastus2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated. I am still confirming with the service team when will westeurope
be available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the current rollout which will take approximately 2 weeks will enable the feature in all regions
// currently only supported in EastUS, WestUS, EastUS2, and WestUS2 | ||
data.Locations.Primary = "eastus2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above
// currently only supported in EastUS, WestUS, EastUS2, and WestUS2 | |
data.Locations.Primary = "eastus2" | |
// TODO - Remove this override when Preview is rolled out to westeurope - currently only supported in EastUS, WestUS, EastUS2, and WestUS2 | |
data.Locations.Primary = "eastus2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated. thanks.
"sku_profile": { | ||
Type: pluginsdk.TypeList, | ||
Optional: true, | ||
ForceNew: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given this is ForceNew
what is the default value here? Are we going to trigger replacements for existing resources?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sku_profile
can be set only when sku_name="Mix"
and it cannot be added/removed through update
. However, its sub-properties allocation_strategy
and vm_sizes
can be changed through Update
.
@@ -351,6 +380,14 @@ func resourceOrchestratedVirtualMachineScaleSetCreate(d *pluginsdk.ResourceData, | |||
props.Sku = sku | |||
} | |||
|
|||
if v, ok := d.GetOk("sku_profile"); ok { | |||
if props.Sku == nil || pointer.From(props.Sku.Name) != "Mix" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be implemented as a customiseDiff to catch this configuration issue at plan time rather than erroring out here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved the logic into customiseDiff
. Thanks.
@@ -351,6 +380,14 @@ func resourceOrchestratedVirtualMachineScaleSetCreate(d *pluginsdk.ResourceData, | |||
props.Sku = sku | |||
} | |||
|
|||
if v, ok := d.GetOk("sku_profile"); ok { | |||
if props.Sku == nil || pointer.From(props.Sku.Name) != "Mix" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, can we have this ("Mix"
) defined as a const
rather than using magic string everywhere?
maybe:
if props.Sku == nil || pointer.From(props.Sku.Name) != "Mix" { | |
if props.Sku == nil || pointer.From(props.Sku.Name) != skuNameMix { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
defined it in a const variable. Thanks.
@@ -732,6 +769,11 @@ func resourceOrchestratedVirtualMachineScaleSetUpdate(d *pluginsdk.ResourceData, | |||
} | |||
} | |||
|
|||
if d.HasChange("sku_profile") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block is ForceNew
in the schema, why are we checking for updates?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
answered in the above comment. Though it's defined ForceNew
, its sub-properties can be changed through Update
as long as sku_profile
is not removed.
…er-azurerm into flexible-vmss-instance-mix
Hi @jackofallops, thank you for your review. I have resolved the comments. For testcases, I will create another PR to remove the region limitation once Azure supports it. Could you please help take another review? --- PASS: TestAccOrchestratedVirtualMachineScaleSet_skuProfileErrorConfiguration (32.10s) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ms-zhenhua - Thanks for making those changes, this LGTM now 👍
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
Community Note
Description
MS-DOC
PR Checklist
For example: “
resource_name_here
- description of change e.g. adding propertynew_property_name_here
”Changes to existing Resource / Data Source
Testing
Change Log
Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.
azurerm_orchestrated_virtual_machine_scale_set
- addsku_profile
to support mix instance featureThis is a (please select all that apply):
Related Issue(s)
Fixes #0000
Note
If this PR changes meaningfully during the course of review please update the title and description as required.