Slight refactor on vmware_guest to fix path searching #26511

jctanner · 2017-07-07T00:37:07Z

SUMMARY

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

vmware_guest

ANSIBLE VERSION

devel

…tibility. Fixes #25011

ansibot · 2017-07-07T00:54:32Z

The test ansible-test sanity --test pep8 failed with the following errors:

lib/ansible/module_utils/vmware.py:238:9: E265 block comment should start with '# '
lib/ansible/module_utils/vmware.py:240:9: E265 block comment should start with '# '
lib/ansible/modules/cloud/vmware/vmware_guest.py:484:13: E303 too many blank lines (2)
lib/ansible/modules/cloud/vmware/vmware_guest.py:491:33: E702 multiple statements on one line (semicolon)
lib/ansible/modules/cloud/vmware/vmware_guest.py:1107:13: E265 block comment should start with '# '
lib/ansible/modules/cloud/vmware/vmware_guest.py:1110:9: E265 block comment should start with '# '
lib/ansible/modules/cloud/vmware/vmware_guest.py:1115:17: E702 multiple statements on one line (semicolon)
lib/ansible/modules/cloud/vmware/vmware_guest.py:1124:17: E702 multiple statements on one line (semicolon)
lib/ansible/modules/cloud/vmware/vmware_guest.py:1134:17: E702 multiple statements on one line (semicolon)
lib/ansible/modules/cloud/vmware/vmware_guest.py:1135:17: E702 multiple statements on one line (semicolon)

click here for bot help

ansibot · 2017-07-07T01:04:26Z

The test ansible-test sanity --test pep8 failed with the following errors:

lib/ansible/modules/cloud/vmware/vmware_guest.py:1103:13: E265 block comment should start with '# '
lib/ansible/modules/cloud/vmware/vmware_guest.py:1106:9: E265 block comment should start with '# '
lib/ansible/modules/cloud/vmware/vmware_guest.py:1111:17: E702 multiple statements on one line (semicolon)
lib/ansible/modules/cloud/vmware/vmware_guest.py:1120:17: E702 multiple statements on one line (semicolon)
lib/ansible/modules/cloud/vmware/vmware_guest.py:1130:17: E702 multiple statements on one line (semicolon)
lib/ansible/modules/cloud/vmware/vmware_guest.py:1131:17: E702 multiple statements on one line (semicolon)

click here for bot help

jctanner · 2017-07-07T01:44:17Z

@nerzhul @dav1x @dericcrago @Akasurde @dagwieers

Can you guys test this please?

abadger · 2017-07-07T02:16:27Z

lib/ansible/module_utils/vmware.py

+        result = eval(expression)
+    except:
+        pass
+    return result


Something like this instead:

def _get_vm_prop(vm, attributes): result = vm for attribute in attributes: try: result = getattr(result, attribute) except AttributeError: return None return result _get_vm_props(vm, ('guest', 'toolsRunningStatus'))

I think, using pass instead of returning None in AttributeError as return None fails to retrieve snapshots. Snapshot data works perfect with pass.

pass doesn't advance result to the next layer of the object, though. Imagine a data structure like this:

class Alice: foo = 1 class Bob: bar = Alice() foo = 2 obj = Bob() del obj.bar result = _get_vm_props(obj, ('bar', 'foo') # with return None: result == None # with pass result == 2

For snapshot, what are you calling? This seems right and it seems like it would do the right thing:

snapshot = _get_vm_prop(vm, ['snapshot'])

@Akasurde I've tested this patch on cloning from snapshots, so I think it does work to some degree. Can you extrapolate on which situations you don't think it will work?

jctanner · 2017-07-07T02:23:04Z

@cigamit are you able to test this?

Akasurde · 2017-07-07T07:01:10Z

lib/ansible/modules/cloud/vmware/vmware_guest.py

-                self.module.fail_json(msg="Folder %(folder)s needs to be an absolute path, starting with '/'." % self.params)
-            searchpath = '%(datacenter)s%(folder)s' % self.params
+            # searchpaths do not need to be absolute
+            searchpath = self.params['folder']


Datacenter is required for FindByInventoryPath. Without datacenter API, fails to find the VM.

Can we move getvm method to vmware.py so that vmware_guest_facts or other modules could use it?

I have tested, and found that FindByInventoryPath does need the full path to find a folder in both my scenarios where VMs are nested like so.
/TestDCFolder/TestDC/TestCluster/TestFolder/
and
/TestDC/TestCluster/TestFolder/

In both cases, I needed to change searchpath to match the full path to the DC, then /vm/ and the actual folder. So in the above instances it was.
/TestDCFolder/TestDC/vm/TestFolder/
and
/TestDC/vm/TestFolder/

Akasurde · 2017-07-07T07:01:57Z

lib/ansible/modules/cloud/vmware/vmware_guest.py

    module.params['folder'] = module.params['folder'].rstrip('/')

    pyv = PyVmomiHelper(module)

    # Check if the VM exists before continuing
    vm = pyv.getvm(name=module.params['name'], folder=module.params['folder'], uuid=module.params['uuid'])
+    if not vm:
+        module.fail_json(msg="nope")


Could you please change this message to something descriptive about the error?

Akasurde · 2017-07-07T07:05:24Z

I have made changes and tested with vmware_guest_facts module.
With @abadger comment and searchpath = '%(datacenter)s%(folder)s' % self.params changes -

ok: [localhost] => {
    "changed": false, 
    "failed": false, 
    "instance": {
        "annotation": "", 
        "current_snapshot": null, 
        "customvalues": {}, 
        "guest_tools_status": "guestToolsNotRunning", 
        "guest_tools_version": "0", 
        "hw_eth0": {
            "addresstype": "generated", 
            "ipaddresses": null, 
            "label": "Network adapter 1", 
            "macaddress": "00:0c:29:93:a7:bc", 
            "macaddress_dash": "00-0c-29-93-a7-bc", 
            "summary": "VM Network"
        }, 
        "hw_guest_full_name": null, 
        "hw_guest_id": null, 
        "hw_interfaces": [
            "eth0"
        ], 
        "hw_memtotal_mb": 256, 
        "hw_name": "SampleVM", 
        "hw_power_status": "poweredOff", 
        "hw_processor_count": 1, 
        "hw_product_uuid": "564dbd8c-30e8-2b9f-50c9-fbc47993a7bc", 
        "ipv4": null, 
        "ipv6": null, 
        "module_hw": true, 
        "snapshots": []
    }, 
    "invocation": {
        "module_args": {
            "datacenter": "ha-datacenter", 
            "folder": "/vm", 
            "hostname": "192.168.122.68", 
            "name": "SampleVM", 
            "name_match": "first", 
            "password": "VALUE_SPECIFIED_IN_NO_LOG_PARAMETER", 
            "username": "root", 
            "uuid": null, 
            "validate_certs": false
        }
    }
}

Without datacenter value in searchpath it fails to find VM.

Akasurde · 2017-07-07T07:31:47Z

lib/ansible/modules/cloud/vmware/vmware_guest.py

-                self.module.fail_json(msg="Folder %(folder)s needs to be an absolute path, starting with '/'." % self.params)
-            searchpath = '%(datacenter)s%(folder)s' % self.params
+            # searchpaths do not need to be absolute
+            searchpath = self.params['folder']


Can we move getvm method to vmware.py so that vmware_guest_facts or other modules could use it?

cigamit

This leads me to believe that I should set my folder like so
folder: /Test/TestDC/vm/TestF
but doing that causes the clone task (vm_obj.Clone) to fail with
"A specified parameter was not correct: spec.location.folder"

It works in the scenario that the Datacenter is not nested under a folder, but Nesting it under a folder causes the error above.

jctanner · 2017-07-08T11:27:23Z

@cigamit thanks for testing. I'll try to get a a real vcenter setup with this configuration and revise the patch accordingly.

abadger · 2017-07-09T20:21:37Z

lib/ansible/modules/cloud/vmware/vmware_guest.py

+
+        # split the searchpath so we can iterate through it
+        paths = [x.replace('/', '') for x in searchpath.split('/')]
+        paths = [x.strip() for x in paths if x.strip()]


x.replace('/', '') shouldn't be necessary. searchpath.split('/') should have removed all instances of "/" from the output.

Is it valid to strip the path elements? Could you have a valid path like this: /folder1/ datacenter1 /vm?

ansibot · 2017-07-10T04:54:51Z

The test ansible-test sanity --test pep8 failed with the following errors:

lib/ansible/modules/cloud/vmware/vmware_guest.py:464:22: E231 missing whitespace after ','
lib/ansible/modules/cloud/vmware/vmware_guest.py:538:22: E231 missing whitespace after ','
lib/ansible/modules/cloud/vmware/vmware_guest.py:556:25: E702 multiple statements on one line (semicolon)
lib/ansible/modules/cloud/vmware/vmware_guest.py:561:1: E302 expected 2 blank lines, found 1
lib/ansible/modules/cloud/vmware/vmware_guest.py:1343:113: W291 trailing whitespace
lib/ansible/modules/cloud/vmware/vmware_guest.py:1388:161: E501 line too long (186 > 160 characters)

The test ansible-test sanity --test validate-modules failed with the following error:

lib/ansible/modules/cloud/vmware/vmware_guest.py:518:0: E403 Type comparison using type() found. Use isinstance() instead

click here for bot help

ansibot · 2017-07-10T15:26:13Z

The test ansible-test sanity --test validate-modules failed with the following error:

lib/ansible/modules/cloud/vmware/vmware_guest.py:518:0: E403 Type comparison using type() found. Use isinstance() instead

click here for bot help

cigamit · 2017-07-12T04:53:23Z

There still seems to be some regression involving Resource Pools, I'm attempting to debug the code currently. I can clone into my DC that is nested, but not my normal DC. I keep getting the error
A specified parameter was not correct: spec.location.host

Cloning without resource or snapshot set fails
Cloning with snapshot fails
Cloning with resource set to "Resources" fails
Cloning into a known Resource Pool succeeds

All these pass when cloning into my Nested DC though. Cross DC clones from Unnested to Nested DC passes also. Its almost like it is still pulling something from the wrong DC.

Also, when debugging the above, I found that if I don't specify a cluster, and instead use esxi_hostname but don't specify a Resource Pool, I get
TypeError was returned, please ensure to give correct inputs. For \"pool\" expected type vim.ResourcePool, but got vim.Datastore
If I specify a RP in the cluster, I get an error about it not being able to find said Pool.

cigamit · 2017-07-12T05:13:47Z

If I don't specify a location for the Disk (since I'm not making changes to it) it seems to "Auto-Select" instead of placing it in the same location as the VM I am cloning. I think this is one of the changes that was fixed where it was just inheriting the wrong properties from the template. While this could be acceptable, I can not specify a specific Datastore via disk without also specifying a size (the size seems to want to be required).

cigamit · 2017-07-12T20:22:15Z

This fix now works for me in all scenarios I have tested (Nested DCs, Unnested DCs, Cross DC Cloning, With/Without Resource Pools, Cloning from Snapshot, and Linked Clones)

) * vmware_guest: fixes for cache objects and datacenter association * find_all_objs was only looking for datastores * Clear the result if it's datacenter is not correct. * Re-enable pyvmomi installation Addresses #25011 Addresses #26511

Slight refactor on vmware_guest to fix path searching and vcsim compa…

35b0d4e

…tibility. Fixes #25011

ansibot added affects_2.4 This issue/PR affects Ansible v2.4 bugfix_pull_request c:module_utils/ cloud needs_triage Needs a first human triage before being processed. support:core This issue/PR relates to code supported by the Ansible Engineering Team. vmware VMware community labels Jul 7, 2017

ansibot added ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jul 7, 2017

Clean up pep8 errors

df16c52

Fix more pep8 issues

1303dd0

ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Jul 7, 2017

ansibot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Jul 7, 2017

Add assertions

1ac48b8

abadger reviewed Jul 7, 2017

View reviewed changes

Akasurde requested changes Jul 7, 2017

View reviewed changes

ansibot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Jul 7, 2017

Akasurde requested changes Jul 7, 2017

View reviewed changes

jctanner added 2 commits July 7, 2017 07:55

Remove debug failure

0ff44d8

Update docsting for folder with examples

64c810c

cigamit reviewed Jul 7, 2017

View reviewed changes

Revise _get_vm_prop based on suggestions

c83caa3

abadger reviewed Jul 9, 2017

View reviewed changes

More enhancements for datacenter->datacenter cloning

32a108e

ansibot added the ci_verified Changes made in this PR are causing tests to fail. label Jul 10, 2017

Fix a few pep8 issues

76b528f

ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Jul 10, 2017

ansibot added the ci_verified Changes made in this PR are causing tests to fail. label Jul 10, 2017

alikins removed the needs_triage Needs a first human triage before being processed. label Jul 10, 2017

Remove useless check for subclass of None

36eca0e

ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Jul 11, 2017

jctanner added 3 commits July 11, 2017 08:43

Fix a few suggestions

ee9a387

Move serialize_spec to a util function

e6cfdff

Group exceptions together

bac185f

jctanner merged commit 10fc441 into ansible:devel Jul 12, 2017

jctanner mentioned this pull request Jul 12, 2017

vmware_guest: fixes for cache objects and datacenter association #26719

Merged

This was referenced Jul 13, 2017

Ansible/vmware_guest "msg": "A specified parameter was not correct. spec.location.host" #25967

Closed

vmware_guest.py Clone operation not working with tree of default resource pools #24548

Closed

vmware_guest relative paths for folder #26927

Merged

Akasurde mentioned this pull request Jul 21, 2017

ansible vmware_guest "msg": "No folder matched the path" #23340

Closed

ansibot added bug This issue/PR relates to a bug. and removed bugfix_pull_request labels Mar 6, 2018

ansible locked and limited conversation to collaborators Apr 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Slight refactor on vmware_guest to fix path searching #26511

Slight refactor on vmware_guest to fix path searching #26511

jctanner commented Jul 7, 2017

ansibot commented Jul 7, 2017

ansibot commented Jul 7, 2017

jctanner commented Jul 7, 2017

abadger Jul 7, 2017

Akasurde Jul 7, 2017

abadger Jul 7, 2017

jctanner Jul 11, 2017

jctanner commented Jul 7, 2017

Akasurde Jul 7, 2017

Akasurde Jul 7, 2017

cigamit Jul 7, 2017

Akasurde Jul 7, 2017

Akasurde commented Jul 7, 2017 •

edited

Loading

Akasurde Jul 7, 2017

cigamit left a comment •

edited

Loading

jctanner commented Jul 8, 2017

abadger Jul 9, 2017

ansibot commented Jul 10, 2017

ansibot commented Jul 10, 2017

cigamit commented Jul 12, 2017

cigamit commented Jul 12, 2017

cigamit commented Jul 12, 2017

Slight refactor on vmware_guest to fix path searching #26511

Slight refactor on vmware_guest to fix path searching #26511

Conversation

jctanner commented Jul 7, 2017

SUMMARY

ISSUE TYPE

COMPONENT NAME

ANSIBLE VERSION

ansibot commented Jul 7, 2017

ansibot commented Jul 7, 2017

jctanner commented Jul 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jctanner commented Jul 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Akasurde commented Jul 7, 2017 • edited Loading

Choose a reason for hiding this comment

cigamit left a comment • edited Loading

Choose a reason for hiding this comment

jctanner commented Jul 8, 2017

Choose a reason for hiding this comment

ansibot commented Jul 10, 2017

ansibot commented Jul 10, 2017

cigamit commented Jul 12, 2017

cigamit commented Jul 12, 2017

cigamit commented Jul 12, 2017

Akasurde commented Jul 7, 2017 •

edited

Loading

cigamit left a comment •

edited

Loading