rewrite get_resource_pool method for correct resource_pool selection #39792
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ansibot
added
bug
This comment has been minimized.
This comment has been minimized.
mattclay
added
the
ci_verified
ansibot
added
community_review
|
|
ansibot
added
affects_2.6
Akasurde
requested changes
May 22, 2018
lib/ansible/module_utils/vmware.py
Outdated
| @@ -66,6 +66,22 @@ def find_obj(content, vimtype, name, first=True): | |||
| return [obj for obj in obj_list if obj.name == name] | |||
|
|
|||
|
|
|||
| def find_objs(content, vimtypes, container=None, name=None): | |||
There was a problem hiding this comment.
Could you please re-use find_obj method for this ?
There was a problem hiding this comment.
I can reuse the find_obj if you are ok with modifying it to accept a extra container (or folder) argument as in the get_all_objs.
Basically the container or content.rootFolder part is what I need to be able to limit the search (to a specific datacenter or cluster in the case of the resource_pool).
There was a problem hiding this comment.
There's also an inconsistency in the find_obj method, if name=None it ignores the first argument and always returns the first matching object or None.
lib/ansible/module_utils/vmware.py
Outdated
| vimtypes, recursive=True) | ||
| # make sure name does not contain a path | ||
| if name: | ||
| name = name.split('/')[-1] |
There was a problem hiding this comment.
I think this is not required, name may not contain /
There was a problem hiding this comment.
That's what I expected but without it the integration tests failed.
# test/integration/targets/vmware_guest/tasks/cdrom_d1_c1_f0.yml (line 38):
# test/integration/targets/vmware_guest/tasks/vapp_d1_c1_f0.yml (line 35):
cluster: "{{ clusterlist['json'][0] }}"
# translates into
cluster: "/DC0/host/DC0_C0"So this part can be removed if the tests are updated.
cluster: "{{ clusterlist['json'][0]|basename }}"Although it doesn't hurt to keep it I think.
| def get_resource_pool(self, cluster=None, host=None, resource_pool=None): | ||
| """ Get a resource pool, filter on cluster, esxi_hostname or resource_pool if given """ | ||
|
|
||
| cluster_name = cluster or self.params['cluster'] |
There was a problem hiding this comment.
use self.params.get('cluster', None) instead of self.params['cluster'] and same for below two.
There was a problem hiding this comment.
Updated the code.
ansibot
added
needs_revision
ansibot
added
the
needs_revision
This comment has been minimized.
This comment has been minimized.
ansibot
added
merge_commit
pieteravonts
force-pushed
the
better-resource-pool-selection
branch
from
0c5857d
to
3278e72
Compare
ansibot
removed
the
merge_commit
added 7 commits
August 27, 2018 12:34
… resource_pool" This reverts commit 50293ec763c024b0eaceac5d775ccc0ad3ff8bd7.
pieteravonts
force-pushed
the
better-resource-pool-selection
branch
from
3278e72
to
661046e
Compare
ansibot
added
community_review
|
Any possibility to have this merged for 2.7? This patch has been in use inside WDC since it was contributed here in May. We use it a lot on Vsphere 5.5 en 6.5 setups where we had this problem and it solved our issues. Everything works stable for us. |
stroobl
approved these changes
Aug 29, 2018
dagwieers
approved these changes
Aug 29, 2018
|
@pdellaert @dericcrago Could you please review this so we can get this into Ansible 2.7? |
|
Closing and re-opening for CI trigger. |
Akasurde
pushed a commit
to Akasurde/ansible
that referenced
this pull request
Oct 8, 2018
…lection (ansible#39792) * rewrite get_resource_pool method for correct resource_pool selection * only keep name if path is given for cluster, esxi_hostname or resource_pool * Revert "only keep name if path is given for cluster, esxi_hostname or resource_pool" * This reverts commit 50293ec763c024b0eaceac5d775ccc0ad3ff8bd7. * if the name argument contains a path, only use the last part for matching * remove path from cluster argument in tests * remove find_objs in favour of reusing find_obj with an extra folder argument * fix find_obj ignoring first if name is not given (cherry picked from commit 1a810f8)
abadger
pushed a commit
that referenced
this pull request
Oct 9, 2018
…lection (#39792) * rewrite get_resource_pool method for correct resource_pool selection * only keep name if path is given for cluster, esxi_hostname or resource_pool * Revert "only keep name if path is given for cluster, esxi_hostname or resource_pool" * This reverts commit 50293ec763c024b0eaceac5d775ccc0ad3ff8bd7. * if the name argument contains a path, only use the last part for matching * remove path from cluster argument in tests * remove find_objs in favour of reusing find_obj with an extra folder argument * fix find_obj ignoring first if name is not given (cherry picked from commit 1a810f8)
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
SUMMARY
Fixes #33156 cluster option is ignored and #38043 random resource_pool selection when multiple resource_pools exist with the same name.
This change rewrites the get_resource_pool method to correctly select the resource pool based on all of the following parameters if given: datacenter, cluster, esxi_hostname and resource_pool.
ISSUE TYPE
COMPONENT NAME
vmware_guest
ANSIBLE VERSION