-
Notifications
You must be signed in to change notification settings - Fork 4.9k
azurerm_orchestrated_virtual_machine_scale_set
- Fix deployment issue with specialized image
#30889
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
base: main
Are you sure you want to change the base?
azurerm_orchestrated_virtual_machine_scale_set
- Fix deployment issue with specialized image
#30889
Conversation
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.
@ms-zhenhua, thanks for the PR, I have given it a look and there are somethings that I don't understand. Maybe I don't completely understand this change, but the code does not make sense to me. Can you please respond to my comments for my edification about your changes?
OsType: pointer.To(osType), | ||
} | ||
|
||
if len(osType) > 0 { |
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.
I don't understand this, why are we now checking if it is greater than 0? I this a change in the API?
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.
Because I removed the default value of osType
. If os_profile
is not configured, osType
will be empty.
} | ||
|
||
osType := virtualmachinescalesets.OperatingSystemTypesWindows | ||
var osType virtualmachinescalesets.OperatingSystemTypes |
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 appears to be another API change? I still don't understand why this is necessary.
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.
Because for a specialized image, os_profile
is not required. In this case, in original code, osType will use a default value Windows
. But if the os of the specialized image is Linux
, the service end will return an error 'The value 'Windows' of parameter 'osDisk.osType' is not allowed. Allowed values are: Linux.'
. That's why we should not initialize osType
with a default value.
|
||
virtualMachineProfile.OsProfile = vmssOsProfile | ||
} | ||
vmssOsProfile.AllowExtensionOperations = pointer.To(extensionOperationsEnabled) |
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.
osType isn't used here, this doesn't seem right to me... why are we defining osType above, but we don't use it in the code anywhere?
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.
osType
is used by ExpandOrchestratedVirtualMachineScaleSetOSDisk(v.([]interface{}), osType)
. If linux_configuration
is set, osType
is Linux
. If windows_configuration
is set, osType
is Windows. For a specialized image, os_profile
is not allowed so that osType
will be empty.
|
||
if virtualMachineProfile.OsProfile == nil { | ||
virtualMachineProfile.OsProfile = &virtualmachinescalesets.VirtualMachineScaleSetOSProfile{} | ||
virtualMachineProfile.OsProfile = vmssOsProfile |
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.
Why are we blindly assigning this value without a null check?
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.
my mistake, just added the nil check. Thanks.
`, data.RandomInteger, data.Locations.Primary, r.natgateway_template(data)) | ||
} | ||
|
||
func (OrchestratedVirtualMachineScaleSetResource) specializedImage(data acceptance.TestData) string { |
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.
How do we know this a specialized image? Did you create this image earlier in your test and I just missed it?
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.
Yes, it's created by SharedImageVersionResource{}.imageVersionSpecializedByVM(data)
Hi @WodansSon, |
Community Note
Description
Changes:
osType
to avoid backend validation error when deployingazurerm_orchestrated_virtual_machine_scale_set
with specialized imageazurerm_orchestrated_virtual_machine_scale_set.res-0: Creating... ╷ │ Error: creating Orchestrated Virtual Machine Scale Set: (Name 'orchvm002' / Resource Group 'sc-development-migrate-eastasia-rg'): compute.VirtualMachineScaleSetsClient#CreateOrUpdate: Failure sending request: StatusCode=400 -- Original Error: Code='InvalidParameter' Message='The value 'Windows' of parameter 'osDisk.osType' is not allowed. Allowed values are: Linux.' Target='osDisk.osType' │ │ with azurerm_orchestrated_virtual_machine_scale_set.res-0, │ on main.tf line 33, in resource 'azurerm_orchestrated_virtual_machine_scale_set' 'res-0': │ 33: resource 'azurerm_orchestrated_virtual_machine_scale_set' 'res-0' { │
AllowExtensionOperations
intoif
block ofOsProfile
to avoid the following validation error#30544
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
- Fix deployment issue with specialized imageThis is a (please select all that apply):
Related Issue(s)
Fixes #30544
AI Assistance Disclosure
Rollback Plan
If a change needs to be reverted, we will publish an updated version of the provider.
Changes to Security Controls
Are there any changes to security controls (access controls, encryption, logging) in this pull request? If so, explain.
Note
If this PR changes meaningfully during the course of review please update the title and description as required.