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

Change the dynamically obtained public IP address to a list #1214

Merged
merged 4 commits into from
Dec 12, 2023

Conversation

Fred-sun
Copy link
Collaborator

SUMMARY

Change the dynamically obtained public IP address to a list

ISSUE TYPE
  • Bugfix Pull Request
  • Docs Pull Request
  • Feature Pull Request
  • New Module Pull Request
COMPONENT NAME

azure_rm.py

ADDITIONAL INFORMATION

@Fred-sun Fred-sun added the work in In trying to solve, or in working with contributors label Jul 24, 2023
@Fred-sun Fred-sun added ready_for_review The PR has been modified and can be reviewed and merged medium_priority Medium priority and removed work in In trying to solve, or in working with contributors labels Jul 31, 2023
@xuzhang3 xuzhang3 merged commit 48b9a1a into ansible-collections:dev Dec 12, 2023
@epopisces
Copy link

epopisces commented Dec 14, 2023

Looks like there may be an bug in the chain() function, when there is no public IP address instead of simply returning the private IP address an index out of range error causes the entire execution to fail. Unfortunately this change breaks our dynamic inventories, because most of our hosts do not have a public IP address assigned.

The error we now get (with emphasis in bold):

ansible-inventory [core 2.15.8]
  config file = /runner/project/ansible.cfg
  configured module search path = ['/runner/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /usr/local/lib/python3.9/site-packages/ansible
  ansible collection location = /runner/requirements_collections:/runner/.ansible/collections:/usr/share/ansible/collections:/usr/share/automation-controller/collections
  executable location = /usr/local/bin/ansible-inventory
  python version = 3.9.18 (main, Sep  7 2023, 00:00:00) [GCC 11.4.1 20230605 (Red Hat 11.4.1-2)] (/usr/bin/python3)
  jinja version = 3.1.2
  libyaml = True
Using /runner/project/ansible.cfg as config file
redirecting (type: inventory) ansible.builtin.azure_rm to azure.azcollection.azure_rm
[WARNING]:  * Failed to parse
/runner/project/inventories/azure/all_running_hosts.azure_rm.yml with
ansible_collections.azure.azcollection.plugins.inventory.azure_rm plugin: list
index out of range
  File "/usr/local/lib/python3.9/site-packages/ansible/inventory/manager.py", line 293, in parse_source
    plugin.parse(self._inventory, self._loader, source, cache=cache)
  File "/runner/requirements_collections/ansible_collections/azure/azcollection/plugins/inventory/azure_rm.py", line 232, in parse
    self._get_hosts()
  File "/runner/requirements_collections/ansible_collections/azure/azcollection/plugins/inventory/azure_rm.py", line 319, in _get_hosts
    next(chain([h.hostvars['public_ip_address'][0]['ipv4_address']], h.hostvars['private_ipv4_addresses']), None))
[WARNING]: Unable to parse
/runner/project/inventories/azure/all_running_hosts.azure_rm.yml as an
inventory source
ERROR! No inventory was parsed, please check your configuration and options.

Will open an Issue.

Issue 1378 opened to track the troubleshooting, additional details in that Issue.

@Fred-sun Fred-sun deleted the inventory-publicip-error branch December 15, 2023 01:36
@Fred-sun
Copy link
Collaborator Author

@epopisces Yes, what was previously returned was a list with a string element, and now it is returned as a list with a dictionary element. Is there really a problem with linking the way before? I will consider how to solve this problem, thank you!

@epopisces
Copy link

epopisces commented Dec 15, 2023

@Fred-sun that is what it appears to be--our dynamic inventories were working previously, but now fail with the above error.

This line:
/runner/project/inventories/azure/all_running_hosts.azure_rm.yml with ansible_collections.azure.azcollection.plugins.inventory.azure_rm plugin: list index out of range

Seems to suggest it is this change:

# prior code
next(chain(h.hostvars['public_ipv4_addresses'], h.hostvars['private_ipv4_addresses']), None))

# after the merge
next(chain([h.hostvars['public_ip_address'][0]['ipv4_address']], h.hostvars['private_ipv4_addresses']), None))

Trying to use debug and other mechanisms to determine what the value of h.hostvars['public_ip_address'] is during an Inventories > Sources > sync operation, but it is tricky working in AWX vs on a local development workstation (I don't have the same perms that our AWX runners have). If the value of h.hostvars['public_ip_address'] is equal to "" (empty), then attempting to use h.hostvars['public_ip_address'][0][...] would result in a list index out of range error. If the value was False, None, or a string then the result would have been either a different error or a single char, for instance.

We have one inventory source (a small Azure subscription) that has a single host with a public IP address--that is the only one for which the dynamic inventory sync process doesn't fail: all the others fail with the above error, but all of those have VMs that do not have public IPs.

Quite possibly this may be related to us using an older version of AWX while also using the latest azure_rm collection, so I would be curious to see if the results were the same on a newer version of AWX. I went through the release notes since our version up until current and didn't see anything that appeared to be impacting. . .

We are on:
AWX Tower version: 21.4.0

@epopisces
Copy link

epopisces commented Dec 15, 2023

Upgrading to AWX Tower version 23.5.1 (using latest operator) did not resolve, same error occurs (so doesn't appear to be related to the old AWX version specifically). Which makes sense, the execution environment is separate from the AWX control plane.

@Fred-sun
Copy link
Collaborator Author

@epopisces I am very sorry for the trouble caused to you. I have fixed this problem in #1379. Thank you!

@epopisces
Copy link

Appreciate the fast responses! I'll watch the merge, and will test and report back once the new version is available.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
medium_priority Medium priority 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.

3 participants