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

Add support to Azure StackHCI vms in the inventory plugin #1620

Merged
merged 6 commits into from
Oct 8, 2024

Conversation

TiTi
Copy link
Contributor

@TiTi TiTi commented Jul 2, 2024

SUMMARY

Add ability to list Azure Stack HCI vms

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

azure_rm inventory plugin

ADDITIONAL INFORMATION
  • Azure Stack HCI vms are Azure Arc VMs but with most of the fields like an azure vm
  • By default, it will NOT list hci vms unless include_hcivm_resource_groups is specified to [*] or specific resource groups.
    This is done to prevent impact on existing inventories, even if AzureStackHCI + Ansible is somewhat new & niche

TiTi added 2 commits July 2, 2024 18:45
Prevents impacting existing inventories, even if AzureStackHCI + Ansible is somewhat new & niche
@TiTi
Copy link
Contributor Author

TiTi commented Jul 2, 2024

Working sample here:
image

@Fred-sun
Copy link
Collaborator

Fred-sun commented Jul 9, 2024

@TiTi Please fix the following sanity error! Thank you very much!

ERROR: plugins/inventory/azure_rm.py:34:161: E501: line too long (191 > 160 characters)
ERROR: plugins/inventory/azure_rm.py:426:161: E501: line too long (161 > 160 characters)
ERROR: plugins/inventory/azure_rm.py:572:108: E261: at least two spaces before inline comment
ERROR: plugins/inventory/azure_rm.py:575:39: E128: continuation line under-indented for visual indent
ERROR: plugins/inventory/azure_rm.py:576:39: E128: continuation line under-indented for visual indent
ERROR: plugins/inventory/azure_rm.py:611:52: E231: missing whitespace after ','
ERROR: plugins/inventory/azure_rm.py:611:73: E261: at least two spaces before inline comment
ERROR: plugins/inventory/azure_rm.py:646:84: E261: at least two spaces before inline comment
ERROR: plugins/inventory/azure_rm.py:646:161: E501: line too long (186 > 160 characters)
ERROR: plugins/inventory/azure_rm.py:649:43: E225: missing whitespace around operator
ERROR: plugins/inventory/azure_rm.py:649:81: E231: missing whitespace after ','
ERROR: plugins/inventory/azure_rm.py:650:53: E225: missing whitespace around operator
ERROR: plugins/inventory/azure_rm.py:651:55: E225: missing whitespace around operator

@Fred-sun Fred-sun added medium_priority Medium priority work in In trying to solve, or in working with contributors question Further information is requested labels Jul 9, 2024
ansible-test sanity plugins/inventory/azure_rm.py --color --junit -v
@TiTi
Copy link
Contributor Author

TiTi commented Jul 9, 2024

Ok fixed, sorry about that.
I also checked with: ansible-test sanity plugins/inventory/azure_rm.py --color --junit -v

@TiTi
Copy link
Contributor Author

TiTi commented Jul 26, 2024

ping @Fred-sun

@par-texx
Copy link

This would be really useful for us. @Fred-sun , can this be looked at please?

@Fred-sun
Copy link
Collaborator

@TiTi Okay, I'll complete the review and move forward with the merger as soon as possible. Thank you!

@par-texx
Copy link

@Fred-sun , Appreciate it. Looking forward to getting HCI tied into our ansible system

@Fred-sun Fred-sun added ready_for_review The PR has been modified and can be reviewed and merged and removed work in In trying to solve, or in working with contributors labels Sep 20, 2024
@par-texx
Copy link

par-texx commented Oct 2, 2024

@xuzhang3 , this is a fairly large blocker for a project. Can I help in getting this reviewed in any way? thanks

next_link = response.get('nextLink')

if next_link:
self._enqueue_get(url=next_link, api_version=self._compute_api_version, handler=self._on_arcvm_page_response)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this be:

self._enqueue_get(url=next_link, api_version=self._hybridcompute_api_version, handler=self._on_arcvm_page_response)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch, will fix that

@p3ck
Copy link
Collaborator

p3ck commented Oct 4, 2024

This is great work. I have a couple questions though.

1 - We have a request to add ARC support to ansible, as you know the AzureStackHCI hosts are themselves ARC machines. So I've updated your PR to include those in the inventory. #1735
2 - Do you need ssh proxy support to allow you to connect to the hosts returned by this inventory?

I haven't done this yet, but plan to add support to the above PR to setup the ssh config which you would get when running the following azure cli command:

az ssh config --vm-name ....

Thanks,
Bill

Seen by p3ck in code review
@TiTi
Copy link
Contributor Author

TiTi commented Oct 4, 2024

Ok thx for catching invalid api_version on next link, fixed.

1 - Arc object are different and have less properties overall so yeah it makes sens to create a dedicated class ArcHost(object):
2 - In my case no. Depends on your network configuration / bastion / vms.
For windows machines, i use WinRM. For linux machines, SSH.
However I'd like to share a trick i'm doing:

---

plugin: azure.azcollection.azure_rm
#...
hostvar_expressions:
  # kerberos needs FQDN ; zscaler (remote) needs dns
  ansible_host: (private_ipv4_addresses | first) if os_profile.system == 'linux' else computer_name | default(name, true)
#...

By defining ansible_host as an hostvar expression, i can enforce FQDN rather than ip or reverse.
This can help in some scenarios (kerberos auth, zscaler client, ...)

@xuzhang3 xuzhang3 merged commit a0c50b2 into ansible-collections:dev Oct 8, 2024
Justwmz pushed a commit to Justwmz/azure that referenced this pull request Nov 4, 2024
…llections#1620)

* Add support to Azure StackHCI vms in the inventory plugin

* Change default behavior to no HCI vm fetch

Prevents impacting existing inventories, even if AzureStackHCI + Ansible is somewhat new & niche

* Fix sanity errors

ansible-test sanity plugins/inventory/azure_rm.py --color --junit -v

* Improve comments

* Handle when a guest agent is disabled (it brokes the inventory plugin otherwise)

* Fix api_version on next page

Seen by p3ck in code review
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
medium_priority Medium priority question Further information is requested ready_for_review The PR has been modified and can be reviewed and merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants