saltstack / salt-ext-modules-vmware

Salt Extension Modules for VMware
Apache License 2.0
20 stars 36 forks source link

[Feat] - Get tags of VM from vSphere SDK Client #335

Open oijkn opened 1 year ago

oijkn commented 1 year ago

With the previous code, the tags did not appear when I did the salt-call vmware_vm.info <minion> command.

By adding the vSphere SDK Client, the tags are ok in the return of the command.

Important: it is mandatory to install the new dependencies.

pip3 install --upgrade git+https://github.com/vmware/vsphere-automation-sdk-python.git
vmwclabot commented 1 year ago

@oijkn, you must sign our contributor license agreement before your changes are merged. Click here to sign the agreement. If you are a VMware employee, read this for further instruction.

vmwclabot commented 1 year ago

@oijkn, we have received your signed contributor license agreement. It will be reviewed by VMware shortly. Another comment will be added to the pull request to notify you when the merge can proceed.

vmwclabot commented 1 year ago

@oijkn, VMware has approved your signed contributor license agreement.

waynew commented 1 year ago

@oijkn thanks for the PR!

I was under the impression that tags were get-able with our current approach but maybe that's a specific version of vSphere or something.

A couple of things - first, tests are required for all code contributions. Meaningful unit tests are fine, but unit+integration are better. If you need help with writing tests - attend one of the Test Clinics on Twitch, and we'll be happy to help you get started :+1:. We also want a changelog entry

But more importantly, the vSphere SDK is a very finicky dependency. It has a known issue about pip-installing for a very long time. Almost a decade long.

I've seen that dependency cause a number of issues in upstream Salt, which gives me very very strong reservations about making it a dependency of our project.

I'm OK with making it an optional dependency that's basically completely isolated. Something to this effect:

import utils.vsphere_sdk

...

    tags = utils.vsphere_sdk.get_tags(vm)

Then it could do something like this:

try:
    from com.vmware.vapi.std_client import DynamicID
    HAS_VSPHERE_SDK = True
except ImportError:
    HAS_VSPHERE_SDK = False

def get_tags(vm):
    if HAS_VSPHERE_SDK:
        try:
            vm_mid = vm._moId
            ...
            tags = [client.tagging.Tag.get(tag).name for tag in tags_list]
        except Exception:  # not sure if this try/except is necessary
            tags = []
    else:
        tags = [tag.name for tag in vm.tag]
    return tags

The create_vsphere_client would also need to be similarly protected. Honestly, I wouldn't put the api_client in connect - I'd keep it completely isolated in the vsphere_sdk module. That's just because of the number of issues I've seen using the SDK.

If we can't get the tags any other way :shrug: we can go ahead and provide the ability here. But if someone can't install the SDK, or it gives some issues, then we shouldn't be broken -- we should just carry on as normal, though we can log at an info or warning level or something, if that's useful.