Skip to content

Conversation

@TiTi
Copy link
Contributor

@TiTi TiTi commented Oct 8, 2025

SUMMARY

Fix of #2074

  • Prevents failure if inventory also contains AzureStackHCI vms (or just arc vms)
  • Remove duplicate calls to instance view REST endpoints
  • Fix original problem when retreiving scale sets vms network interface properties
    => will provide scale set private ip under private_ipv4_addresses host var
  • Simplify logic
ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

inventory plugin

ADDITIONAL INFORMATION

The original issue was : _on_nic_response was never called for scale sets nics, despite call to inventory_client._enqueue_get(url=nic['id'], being made

To reproduce issue:
az rest --url "/subscriptions/<subscription_id>/resourceGroups/<rgName>/providers/Microsoft.Compute/virtualMachineScaleSets/<scaleSetName>/virtualMachines/<vmId>/networkInterfaces/<NicName>?api-version=2024-05-01"

Which returns:
Bad Request({"error":{"code":"NoRegisteredProviderFound","message":"No registered resource provider found for location 'westeurope' and API version '2024-05-01' for type 'virtualMachineScaleSets/virtualMachines/networkInterfaces'. The supported api-versions are '2015-05-01-preview, 2015-06-15, 2016-03-30, 2016-04-30-preview, 2016-06-01, 2016-07-01, 2016-08-01, 2016-09-01, 2017-03-30, 2017-12-01, 2018-04-01, 2018-06-01, 2018-10-01, 2019-03-01, 2019-07-01, 2019-12-01, 2020-06-01, 2020-12-01, 2021-03-01, 2021-04-01, 2021-07-01, 2021-11-01, 2022-03-01, 2022-08-01, 2022-11-01, 2023-03-01, 2023-07-01, 2023-09-01, 2024-03-01, 2024-07-01, 2024-11-01, 2025-04-01'. The supported locations are 'eastus, eastus2, westus, centralus, northcentralus, southcentralus, northeurope, westeurope, eastasia, southeastasia, japaneast, japanwest, australiaeast, australiasoutheast, australiacentral, brazilsouth, southindia, centralindia, westindia, canadacentral, canadaeast, westus2, westcentralus, uksouth, ukwest, koreacentral, koreasouth, francecentral, southafricanorth, uaenorth, switzerlandnorth, germanywestcentral, norwayeast, jioindiawest, westus3, swedencentral, qatarcentral, polandcentral, italynorth, israelcentral, spaincentral, mexicocentral, malaysiawest, newzealandnorth, indonesiacentral, austriaeast, chilecentral'."}})

TiTi added 7 commits October 7, 2025 18:11
Everything is already available in self._instanceview

Note: changes os_compute_name to computer_name
to reproduce issue:
az rest --url "/subscriptions/<subscription_id>/resourceGroups/<rgName>/providers/Microsoft.Compute/virtualMachineScaleSets/<scaleSetName>/virtualMachines/777/networkInterfaces/priva5b52Nic?api-version=2025-04-01"

returns:
Bad Request({"error":{"code":"NoRegisteredProviderFound","message":"No registered resource provider found for location 'westeurope' and API version '2025-05-01' for type 'virtualMachineScaleSets/virtualMachines/networkInterfaces'. The supported api-versions are '2015-05-01-preview, 2015-06-15, 2016-03-30, 2016-04-30-preview, 2016-06-01, 2016-07-01, 2016-08-01, 2016-09-01, 2017-03-30, 2017-12-01, 2018-04-01, 2018-06-01, 2018-10-01, 2019-03-01, 2019-07-01, 2019-12-01, 2020-06-01, 2020-12-01, 2021-03-01, 2021-04-01, 2021-07-01, 2021-11-01, 2022-03-01, 2022-08-01, 2022-11-01, 2023-03-01, 2023-07-01, 2023-09-01, 2024-03-01, 2024-07-01, 2024-11-01, 2025-04-01'. The supported locations are 'eastus, eastus2, westus, centralus, northcentralus, southcentralus, northeurope, westeurope, eastasia, southeastasia, japaneast, japanwest, australiaeast, australiasoutheast, australiacentral, brazilsouth, southindia, centralindia, westindia, canadacentral, canadaeast, westus2, westcentralus, uksouth, ukwest, koreacentral, koreasouth, francecentral, southafricanorth, uaenorth, switzerlandnorth, germanywestcentral, norwayeast, jioindiawest, westus3, swedencentral, qatarcentral, polandcentral, italynorth, israelcentral, spaincentral, mexicocentral, malaysiawest, newzealandnorth, indonesiacentral, austriaeast, chilecentral'."}})
Basically a revert of PR ansible-collections#2055 now that proper api_version is specific
location=self._arcvm['location'] if self._arcvm else self._vm_model['location'],
name=self._arcvm['name'] if self._arcvm else self._vm_model['name'],
computer_name=self._vm_model['properties'].get('osProfile', {}).get('computerName'),
computer_name=self._instanceview['computerName'] if self._instanceview.get('computerName') else self._vm_model['properties'].get('osProfile', {}).get('computerName'),
Copy link
Collaborator

Choose a reason for hiding this comment

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

line too long (178 > 160 characters)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed :)

plus i've added some logging :) :) :) check out with -vvvv or debug=true in your ansible.cfg

TiTi added 2 commits October 9, 2025 20:44
Also display urls called with -vvvv
And all raw responses if debug=true in ansible.cfg
@Fred-sun Fred-sun added high_priority High priority work in In trying to solve, or in working with contributors labels Oct 13, 2025
@TiTi
Copy link
Contributor Author

TiTi commented Oct 13, 2025

i have another small patch ready to fix another 'bug' in azure stack hci inventory, but it's unrelated so i'll wait for this to be merged as some lines are common and would it would create a conflict

@TiTi
Copy link
Contributor Author

TiTi commented Oct 29, 2025

any update @Fred-sun ?

@Fred-sun
Copy link
Collaborator

@TiTi Thank you very much for your update. Could you resolve the conflict first? Am I giving it a try? Thank you!

@Fred-sun
Copy link
Collaborator

Fred-sun commented Oct 29, 2025

The normal VM and Uniform VMSS instance vm work fine. I will create a Azure Stack HCI VM for test! Thank you very much!



display = Display()

Copy link
Collaborator

Choose a reason for hiding this comment

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

expected 2 blank lines, found 1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's fixed

@TiTi
Copy link
Contributor Author

TiTi commented Nov 4, 2025

any progress?

@TiTi
Copy link
Contributor Author

TiTi commented Nov 13, 2025

Hello, is there a reason my PR is not approved?

@zunyangc
Copy link
Collaborator

zunyangc commented Nov 26, 2025

Hi @TiTi, sorry for the slow progress on this PR. As I currently still do not have access to a pre-deployed Azure Stack HCI VM that is registered via Azure Arc, I couldn't validate this PR, hope for your understanding.

If you are able, please provide HCI validation results or any sample inventory output from an HCI environment. This would help confirm the changes and ensure the fix works as intended.

Let me discuss with the team and see how we can proceed further. Thanks!

@TiTi
Copy link
Contributor Author

TiTi commented Nov 26, 2025

Hi @TiTi, sorry for the slow progress on this PR. As I currently still do not have access to a pre-deployed Azure Stack HCI VM that is registered via Azure Arc, I couldn't validate this PR, hope for your understanding.

If you are able, please provide HCI validation results or any sample inventory output from an HCI environment. This would help confirm the changes and ensure the fix works as intended.

Let me discuss with the team and see how we can proceed further. Thanks!

Ok i understand. Indeed having an HCI is a complex requirement.
I have access to a lot of HCI clusters, that's why I made this PR and how I tested it :-)

@Klaas-
Copy link
Contributor

Klaas- commented Dec 5, 2025

This also fixes #2125

but it does condense os_computer_name and computer_name into one thing. Not sure if that is wanted behavior. From my understanding:
computer_name is currently this: https://learn.microsoft.com/en-us/rest/api/compute/virtual-machines/get?view=rest-compute-2025-04-01&tabs=HTTP#osprofile
os_computer_name is https://learn.microsoft.com/en-us/rest/api/compute/virtual-machines/instance-view?view=rest-compute-2024-11-04&tabs=HTTP&tryIt=true&source=docs#virtualmachineinstanceview

so the first one is the "creation" input, the 2nd one is the actual runtime hostname as reported by walinuxagent

@Klaas-
Copy link
Contributor

Klaas- commented Dec 5, 2025

If you are able, please provide HCI validation results or any sample inventory output from an HCI environment. This would help confirm the changes and ensure the fix works as intended.

I can confirm it works for arc machines :)

@TiTi
Copy link
Contributor Author

TiTi commented Dec 5, 2025

This also fixes #2125

but it does condense os_computer_name and computer_name into one thing. Not sure if that is wanted behavior. From my understanding: computer_name is currently this: https://learn.microsoft.com/en-us/rest/api/compute/virtual-machines/get?view=rest-compute-2025-04-01&tabs=HTTP#osprofile os_computer_name is https://learn.microsoft.com/en-us/rest/api/compute/virtual-machines/instance-view?view=rest-compute-2024-11-04&tabs=HTTP&tryIt=true&source=docs#virtualmachineinstanceview

so the first one is the "creation" input, the 2nd one is the actual runtime hostname as reported by walinuxagent

Hello and thank you. Indeed i didn't say it but it also solves the perf. issue, because removing (potentially lots of) uneeded requests.

Regarding os_compute_name (not os_computer_name), your right. However this was added in the 3.9.0 version with the PR i'm fixing: https://github.com/ansible-collections/azure/pull/2055/files
I find this property unhelpful and the naming confusing. People could also pick the "wrong" one initially (computer_name) and if the os name is changed, it's not reflected in the ansible inventory....
(we also have inventory hostname and name property)
Possible to add it though, what do you guys prefer?

# Conflicts:
#	plugins/inventory/azure_rm.py
@TiTi
Copy link
Contributor Author

TiTi commented Dec 5, 2025

This is getting more and more challenging to handle conflicts as other PR are merged (and as i have other changes planned), but i managed.

Note: for hci this PR is unsufficient and a micro pr is needed to avoid errors in output if you fetch both HCI vms and others arc objects (code is ready on my machine). Error would typically be confusing:
[ERROR]: {'error': {'code': 'NoRegisteredProviderFound', 'message': "No registered resource provider found for location 'brazilsouth' and API version '2024-01-01' for type 'virtualMachineInstances'. The supported api-versions are '2023-07-01-preview, 2023-09-01-preview, 2024-01-01, 2024-02-01-preview, 2024-05-01-preview, 2024-08-01-preview, 2024-10-01-preview, 2025-02-01-preview, 2025-04-01-preview, 2025-06-01-preview, 2025-09-01-preview'. The supported locations are 'eastus, westeurope, southeastasia, australiaeast, canadacentral, southcentralus, japaneast, centralindia, uksouth'."}}

However i prefer to keep this PR as simple as possible for readability as we have already multiple changes:


@Klaas- you're right after review i'm changing behavior of computer_name compared before 3.9.0
but i believe it's better: to reflect the real computer name... looking forward you're pov

@TiTi
Copy link
Contributor Author

TiTi commented Dec 5, 2025

Fix i'm talking about is mostly replacing this:

for arcvm in response['value']:
            url = '{0}/providers/Microsoft.AzureStackHCI/virtualMachineInstances'.format(arcvm['id'])

by this

for arcvm in response['value']:
            if archcivm.get('kind') == 'HCI':
                url = '{0}/providers/Microsoft.AzureStackHCI/virtualMachineInstances'.format(arcvm['id'])

but i also want to rename 'arcvm' objects and properties to 'archcivm' because it's more explicit and differs from real ArcHost(arcvm, ...) which is non-hci
and this creates lots of replacements => i prefer another PR

@Klaas-
Copy link
Contributor

Klaas- commented Dec 5, 2025

so general answer, os_computer_name / computer_name I personally don't care too much about them because I am not using them from inventory :D from a compatibility perspective I would say don't break existing behavior unless the MSFT people here are up for a major version increase.

Now if you ask me what should happen with the inventory generally speaking, I think what we do here should not be done here :) We should not be making up our own names for variables and completely rely on the azure python sdk, I've opened a "big issue" for that #2130 . Having said all that, this is not going to be short term solution because I guess I have to first convince msft of actually documenting the batch endpoint, then getting it into the sdk, then we can start using it :)

For shortterm changes, the smaller you can make your PRs the easier it is to merge them for the MSFT people, stuff like HCI and/or ARC is a problem because they don't have this in their test environment apparently...

@TiTi
Copy link
Contributor Author

TiTi commented Dec 5, 2025

Simple example:

plugin: azure.azcollection.azure_rm
auth_source: auto
include_vm_resource_groups:
 - rg-ind4-prd-eu-riihimaki
include_hcivm_resource_groups:
  - rg-ind4-prd-eu-riihimaki
image image

More complex example with multiple kinds in same subscription:

image image
plugin: azure.azcollection.azure_rm
auth_source: auto
include_vm_resource_groups: [] # ignore azure vm
include_hcivm_resource_groups: ['*']
include_arc_resource_groups: ['*']

this also works if i show azure vms, but this would pollute my sample with too many entries^^
note that include_arc_resource_groups is mandatory as the default is [] (none)

i get this output with this PR + the fix i mentionned

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

high_priority High priority work in In trying to solve, or in working with contributors

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants