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

VMware: Folder support on dynamic inventory plugin #52782

Open
wants to merge 6 commits into
base: devel
from

Conversation

Projects
None yet
4 participants
@dgmorales
Copy link

dgmorales commented Feb 22, 2019

SUMMARY

Adds folder support options to the new vmware_vm_inventory.py plugin.

  • New search_folder option: Allows for restricting the inventory search
    to a vcenter folder.
  • Added datacenter as a required param when using folder params, so we can
    always find the right vm root folder.
  • New with_folders option: Adds with_folders option to build ansible
    groups based on vcenter folders.
ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

lib/ansible/plugins/inventory/vmware_vm_inventory.py

ADDITIONAL INFORMATION

Suppose a vCenter folder layout for an AWX lab like this:

/AWX
/AWX/lab
/AWX/lab/db
/AWX/lab/workers

Setting with_folders: True (and a datacenter name) will generate an inventory like this:

@all:
  |--@awx:
  |  |--@awx_lab:
  |  |  |--@awx_lab_db:
  |  |  |  |--awx-db01_xxxx-uuid
  |  |  |--@awx_lab_workers:
  |  |  |  |--awx-worker01_xxxx-uuid
  |  |  |  |--awx-worker02_xxxx-uuid
...
...

IMPORTANT NOTE: This PR contains my code based on the current level branch. But today, while investigating an inventory caching issue with this module, I spotted some changes already committed by @Akasurde on his personal repo (sorry for the repo stalking), with a sizeable refactoring on this repo (but fixing the caching issue!). I've already rebased my code on top of those changes, but left that in another branch of mine: https://github.com/stone-payments/ansible/tree/feature/vmware-vm-inventory-folder-support-latest

Additionally, I believe it makes sense to match this PR with changes to the Vmware inventory wiki doc, which I will propose too. I also have not investigated yet if there tests around the Vmware_vm_inventory code that should be updated, but will. My goal was to release this right now to make this work visible and help merging the changes with @Akasurde ongoing work.

@Akasurde

This comment has been minimized.

Copy link
Member

Akasurde commented Feb 22, 2019

@dgmorales Thanks for the PR. I will test and add my comments. Thanks.

@dgmorales dgmorales force-pushed the stone-payments:feature/vmware-vm-inventory-folder-support branch from bd76da2 to 6344868 Feb 23, 2019

@dgmorales

This comment has been minimized.

Copy link
Author

dgmorales commented Feb 23, 2019

I saw that the upstream devel branch was updated with @Akasurde changes and rebased to devel (this is the same code now as the alternate branch I mentioned above).

@Akasurde
Copy link
Member

Akasurde left a comment

I have added few comments let me know if it looks good to you. Also, Could you please add integration test for this change - here

Show resolved Hide resolved lib/ansible/plugins/inventory/vmware_vm_inventory.py
Show resolved Hide resolved lib/ansible/plugins/inventory/vmware_vm_inventory.py
if not self.root_folder:
raise AnsibleError("Specified datacenter '%s' was not found on Inventory." % self.datacenter)
else:
self.root_folder = self.content.rootFolder

This comment has been minimized.

@Akasurde

Akasurde Feb 25, 2019

Member

I am thinking about -

Suggested change
self.root_folder = self.content.rootFolder
elif self.datacenter:
# User wants specific datacenter's VM
datacenter_obj = self.content.searchIndex.FindByInventoryPath('%s' % self.datacenter)
if datacenter_obj:
self.root_folder = self.datacenter_obj.vmFolder
else:
raise AnsibleError("Unable to find the datacenter specified using %s" % self.datacenter)
else:
# User has not specified datacenter nor search_folder, with_folders
self.root_folder = self.content.rootFolder

This comment has been minimized.

@dgmorales

dgmorales Mar 13, 2019

Author

That would be a nice addition. Then the datacenter param wouldn't be ignored anymore on absence of the folder params (requiring changes in the doc and the related test). I'll try it out to make sure it works as expected.

This comment has been minimized.

@dgmorales

dgmorales Mar 13, 2019

Author

Actually, I've just realised there's a shorter way. See my comment on line 179 (elif self.with_folders)

if (self.with_folders or self.search_folder) and not self.datacenter:
raise AnsibleError("You must set the 'datacenter' param when setting either 'search_folder' "
"or 'with_folders'.")

def _get_managed_objects_properties(self, vim_type, properties=None):

This comment has been minimized.

@Akasurde

Akasurde Feb 25, 2019

Member

How about using folder value as parameter in this method rather than passing to __init__

def _get_managed_objects_properties(self, vim_type, properties=None, folder=None):

This comment has been minimized.

@dgmorales

dgmorales Mar 13, 2019

Author

I've moved the "root folder deciding and finding" out of _get_managed_objects because I was now calling it two times (when with_folders=True) and it didn't seems necessary to redo those operations every time. When you set datacenter or search_folder for the inventory, everything will be restricted for that subtree, at least the way it is now.

Do you foresee any need to being able to get managed objects from several different inventory subtrees? If not, I would think leaving the way I proposed is quite alright. But maybe I've missed the point.

This comment has been minimized.

@dgmorales

dgmorales Mar 16, 2019

Author

I just saw your changes to this file on PR #50645. I will check in a few days how to reconcile with those changes here.

def _get_managed_objects_properties(self, vim_type, properties=None):
"""
Look up a Managed Object Reference in vCenter / ESXi Environment
:param vim_type: Type of vim object e.g, for datacenter - vim.Datacenter
:param properties: List of properties related to vim object e.g. Name
:return: local content object
"""
# Get Root Folder
root_folder = self.content.rootFolder

This comment has been minimized.

@Akasurde

Akasurde Feb 25, 2019

Member
if folder:
   root_folder = folder
else:
   root_folder = self.content.rootFolder
@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Mar 5, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Mar 13, 2019

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

lib/ansible/plugins/inventory/vmware_vm_inventory.py:32:33: W291 trailing whitespace

click here for bot help

@ansibot ansibot added ci_verified and removed stale_ci labels Mar 13, 2019

@dgmorales
Copy link
Author

dgmorales left a comment

Committed a few suggestions, will push another changes based on these suggestions, hopefully later or tomorrow.

Show resolved Hide resolved lib/ansible/plugins/inventory/vmware_vm_inventory.py
if not self.root_folder:
raise AnsibleError("Specified datacenter '%s' was not found on Inventory." % self.datacenter)
else:
self.root_folder = self.content.rootFolder

This comment has been minimized.

@dgmorales

dgmorales Mar 13, 2019

Author

That would be a nice addition. Then the datacenter param wouldn't be ignored anymore on absence of the folder params (requiring changes in the doc and the related test). I'll try it out to make sure it works as expected.

if not self.root_folder:
raise AnsibleError("Specified datacenter '%s' was not found on Inventory." % self.datacenter)
else:
self.root_folder = self.content.rootFolder

This comment has been minimized.

@dgmorales

dgmorales Mar 13, 2019

Author

Actually, I've just realised there's a shorter way. See my comment on line 179 (elif self.with_folders)

if not self.root_folder:
raise AnsibleError("Specified datacenter + search_folder '%s/vm/%s' were not found on Inventory." %
(self.datacenter, self.search_folder))
elif self.with_folders:

This comment has been minimized.

@dgmorales

dgmorales Mar 13, 2019

Author
Suggested change
elif self.with_folders:
elif self.datacenter:

This would achieve the same you proposed, I believe. Since with_folders requires the datacenter param anyway, and what is done here is just setting the root folder to the vm folder of the DC.

I see you searched for the DC name and got the vmFolder from it. Is that a more robust approach? Seems like it. I'll use that.

This comment has been minimized.

@dgmorales

dgmorales Mar 14, 2019

Author

@Akasurde I've pushed this change now, as my suggestion above, and amending the description doc about datacenter param.

if (self.with_folders or self.search_folder) and not self.datacenter:
raise AnsibleError("You must set the 'datacenter' param when setting either 'search_folder' "
"or 'with_folders'.")

def _get_managed_objects_properties(self, vim_type, properties=None):

This comment has been minimized.

@dgmorales

dgmorales Mar 13, 2019

Author

I've moved the "root folder deciding and finding" out of _get_managed_objects because I was now calling it two times (when with_folders=True) and it didn't seems necessary to redo those operations every time. When you set datacenter or search_folder for the inventory, everything will be restricted for that subtree, at least the way it is now.

Do you foresee any need to being able to get managed objects from several different inventory subtrees? If not, I would think leaving the way I proposed is quite alright. But maybe I've missed the point.

@ansibot ansibot added needs_rebase and removed ci_verified labels Mar 14, 2019

@dgmorales

This comment has been minimized.

Copy link
Author

dgmorales commented Mar 14, 2019

I have added few comments let me know if it looks good to you. Also, Could you please add integration test for this change - here

Done @Akasurde. Check it out.

@@ -59,7 +65,7 @@ ${PYTHON} -m pip install toml
ansible-inventory -i ${VMWARE_CONFIG} --list --toml

echo "Check if cache is working for inventory plugin"
ls "$(pwd)/inventory_cache/vmware_vm_*" > /dev/null 2>&1
ls $(pwd)/inventory_cache/vmware_vm_* > /dev/null 2>&1

This comment has been minimized.

@dgmorales

dgmorales Mar 14, 2019

Author

AFAICT, the final playbook was never being run because of this. The quotes made ls interpret the wildcard literally, failing and dropping to the cleanup code early.


echo "Debugging new instances"
curl "http://${VCENTER_HOST}:5000/govc_find"

# Creates folder structure to test inventory folder support
ansible-playbook -i 'localhost,' test_vmware_prep_folders.yml

This comment has been minimized.

@dgmorales

dgmorales Mar 14, 2019

Author

I could not figure out how to use the folder param of vcsim to get a folder structure that I could test for. It seems kindof broken to me. So I prepare a folder structure using this preparation playbook. I took care of making Ansible not to thrown a warning about wrong inventory or implicit localhost, which included the change in ansible.cfg to remove the hardcoded inventory path (which was actually not needed already) and add host_list plugin.

@dgmorales dgmorales force-pushed the stone-payments:feature/vmware-vm-inventory-folder-support branch from f637f7d to 3f33cd8 Mar 14, 2019

dgmorales and others added some commits Feb 23, 2019

VMware: Folder support on dynamic inventory plugin
    * New search_folder option: Allows for restricting the inventory search
      to a vcenter folder.
    * Added datacenter as a required param when using folder params, so we can
      always find the right vm root folder.
    * New with_folders option: Adds with_folders option to build ansible
      groups based on vcenter folders.
    * Adjusted cache logic to support nested folders
VMware: improving documentation strings for folder params on vmware_v…
…m_inventory

Co-Authored-By: dgmorales <dgmorales@gmail.com>

@dgmorales dgmorales force-pushed the stone-payments:feature/vmware-vm-inventory-folder-support branch from 3f33cd8 to 0be1c19 Mar 14, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Mar 14, 2019

The test ansible-test sanity --test shellcheck [explain] failed with 1 error:

test/integration/targets/inventory_vmware_vm_inventory/runme.sh:68:4: SC2046 Quote this to prevent word splitting.

click here for bot help

@ansibot ansibot removed the needs_rebase label Mar 14, 2019

@dgmorales dgmorales force-pushed the stone-payments:feature/vmware-vm-inventory-folder-support branch 2 times, most recently from cd29d95 to 591e1d9 Mar 14, 2019

dgmorales added some commits Mar 14, 2019

VMware: more tests for vmware_vm_inventory
Improved integration test for testing for folder support features.

@dgmorales dgmorales force-pushed the stone-payments:feature/vmware-vm-inventory-folder-support branch from 591e1d9 to e887575 Mar 15, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Mar 15, 2019

assert:
that: "'{{ item }}' in {{ groups.keys() | list }}"
that: "item in (groups.keys()|list)"

This comment has been minimized.

@dgmorales

dgmorales Mar 15, 2019

Author

Python 2.7 was failing with the original line.

@Akasurde

This comment has been minimized.

Copy link
Member

Akasurde commented Mar 25, 2019

@samccann @acozine Could you please take a look at documentation part of this PR ? Thanks.

@agowa338

This comment has been minimized.

Copy link
Contributor

agowa338 commented Mar 25, 2019

I tried this and I'm seeing a lot of these warnings:

[DEPRECATION WARNING]: Ignoring invalid character(s) "{' ', '&', '-'}" in group name (foo&bar-foobar). This feature will be removed in version 2.12. Deprecation warnings can be disabled by setting deprecation_warnings=False in ansible.cfg.

Can you please take a look at the difference in the supported characters? VMware is fine with these chars, but ansible is apparently going to complain about them, simply removing them will cause problems, too, e.g. if somebody has the folders:

  • foo-bar
  • foobar
    Ansible would not be able to differentiate, if it is only removed, but replacing could cause the same issue...

And also this PR does not seam to do anything. The inventory plugin output still appears to be grouped by operating system, even though with_folder is True, search_folder and datacenter are specified. I'm trying with ansible-inventory -i vmware.yaml --list -y.

I looked a bit closer at the structure, and it appears, that the structure is different than the one listed in the starting post. The structure is actually:

all:
  children:
    poweredOn:
      hosts:
        VM0001-name:
          {... list of config items ...}
        VM0002-name:
          {... list of config items ...}
    folder001-01:
      children:
        subfolder001-01:
          hosts:
            VM003: {}
      hosts:
        VM004: {}
        VM005: {}
    ubuntu64Guest:
      hosts:
        VM006: {}

only the hosts listed within poweredOn, do seam to have any usable information. All others are just listed with there name and id.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.