-
Notifications
You must be signed in to change notification settings - Fork 95
Fix for slow "vmdkops_admin vm-group ls" #1108
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.
A simple solution is always the best solution! :-)
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.
LGTM! depends on the CI run
@@ -274,26 +277,25 @@ def get_vm_uuid_by_name(vm_name): | |||
""" Returns vm_uuid for given vm_name, or None """ | |||
si = vmdk_ops.get_si() | |||
try: | |||
vm = [d for d in si.content.rootFolder.childEntity[0].vmFolder.childEntity if d.config.name == vm_name] | |||
return vm[0].config.uuid | |||
vm = FindChild(GetVmFolder(), vm_name) |
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.
Is GetVmFolder() defined in the ESX service, searched but not able to find the description for this. Which VM folder does this return data for?
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.
No, it's from pyVim.invt
. See L33.
It's Host Agent VM Folder;
>>> c = GetVmFolder()
>>> print c
'vim.Folder:ha-folder-vm'
except: | ||
return None | ||
|
||
def get_vm_name_by_uuid(vm_uuid): | ||
""" Returns vm_name for given vm_uuid, or None """ | ||
si = vmdk_ops.get_si() | ||
try: | ||
vm = [d for d in si.content.rootFolder.childEntity[0].vmFolder.childEntity if d.config.uuid == vm_uuid] | ||
return vm[0].config.name | ||
return vmdk_ops.vm_uuid2name(vm_uuid) |
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.
May be refactoring issue, but get VM UUID from name and get name from UUID should be in the vmdk_utils.py code. In this file we have the get UUID from name and the reverse is in vmdk_ops.py. Instead keep both variants here in vmdk_utils.py.
Not this issue but code is mixed.
From vmdk_ops,.py:
def findVmByUuid(vm_uuid):
si = get_si()
vm = si.content.searchIndex.FindByUuid(None, vm_uuid, True, False)
return vm
def vm_uuid2name(vm_uuid):
vm = findVmByUuid(vm_uuid)
if not vm or not vm.config:
return None
return vm.config.name
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.
Good catch! Yes, I noticed that too. Rather code in vmdk_ops.py triggered light bulbs! I wanted to separate bug fix from code cleanup so didnt touch vm_uuid2name
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.
Ship it. Nice improvement.
2f9dac0
to
82a02cb
Compare
Use SearchIndex and Find* methods for VM-Name <-> UUID conversion.
Testing Done:
Manually tested on Anup's setup.
Without this change: 3+ minutes
With this change: 5 Secs!
Fixes #1104
This will also improve CI tests.