From 315c20cb7a7ae54e284861534cb09cf8d3d4845e Mon Sep 17 00:00:00 2001 From: Abhimanyu Saharan Date: Thu, 17 Nov 2022 04:27:34 +0530 Subject: [PATCH] Added virtual machine filtering (#255) * Added vm filters * Fixed vm property listing * Added vm filtering by host_name * Fixed logic check * Fixed logic check * Fixed pre-commit issues * Updated vm list test case * start testing vm mod * add another test * finish up tests * fixed lint issue * fix pre-commit issues * fix pre-commit issues * update to latest version of python setup in actions * make tests pass on 3.7 Co-authored-by: Wayne Werner --- .github/workflows/test.yml | 10 +- src/saltext/vmware/modules/vm.py | 21 ++- src/saltext/vmware/utils/vm.py | 56 +++++- tests/integration/modules/test_vm.py | 5 +- tests/unit/modules/test_vm.py | 262 +++++++++++++++++++++++++++ 5 files changed, 343 insertions(+), 11 deletions(-) create mode 100644 tests/unit/modules/test_vm.py diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 1fe4c376..e87f6ead 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -9,7 +9,7 @@ jobs: steps: - uses: actions/checkout@v2 - name: Set up Python - uses: actions/setup-python@v1 + uses: actions/setup-python@v4 with: python-version: 3.7 - name: Set Cache Key @@ -34,7 +34,7 @@ jobs: - uses: actions/checkout@v2 - name: Set up Python 3.7 For Nox - uses: actions/setup-python@v1 + uses: actions/setup-python@v4 with: python-version: 3.7 @@ -74,7 +74,7 @@ jobs: - uses: actions/checkout@v2 - name: Set up Python ${{ matrix.python-version }} - uses: actions/setup-python@v1 + uses: actions/setup-python@v4 with: python-version: ${{ matrix.python-version }} @@ -192,7 +192,7 @@ jobs: - uses: actions/checkout@v2 - name: Set up Python ${{ matrix.python-version }} - uses: actions/setup-python@v1 + uses: actions/setup-python@v4 with: python-version: ${{ matrix.python-version }} @@ -315,7 +315,7 @@ jobs: - uses: actions/checkout@v2 - name: Set up Python ${{ matrix.python-version }} - uses: actions/setup-python@v1 + uses: actions/setup-python@v4 with: python-version: ${{ matrix.python-version }} diff --git a/src/saltext/vmware/modules/vm.py b/src/saltext/vmware/modules/vm.py index c635671a..d441eb6d 100644 --- a/src/saltext/vmware/modules/vm.py +++ b/src/saltext/vmware/modules/vm.py @@ -27,10 +27,21 @@ def __virtual__(): return __virtualname__ -def list_(service_instance=None, profile=None): +def list_( + service_instance=None, datacenter_name=None, cluster_name=None, host_name=None, profile=None +): """ Returns virtual machines. + datacenter_name + Filter by this datacenter name (required when cluster is specified) + + cluster_name + Filter by this cluster name (optional) + + host_name + Filter by this host name (optional) + service_instance (optional) The Service Instance from which to obtain managed object references. @@ -43,10 +54,16 @@ def list_(service_instance=None, profile=None): salt '*' vmware_vm.list """ + log.debug("Running vmware_vm.list") service_instance = service_instance or connect.get_service_instance( config=__opts__, profile=profile ) - return utils_vm.list_vms(service_instance) + return utils_vm.list_vms( + service_instance=service_instance, + host_name=host_name, + cluster_name=cluster_name, + datacenter_name=datacenter_name, + ) def list_templates(service_instance=None, profile=None): diff --git a/src/saltext/vmware/utils/vm.py b/src/saltext/vmware/utils/vm.py index a81aa6d4..4066af55 100644 --- a/src/saltext/vmware/utils/vm.py +++ b/src/saltext/vmware/utils/vm.py @@ -316,18 +316,68 @@ def unregister_vm(vm_ref): raise salt.exceptions.VMwareRuntimeError(exc.msg) -def list_vms(service_instance): +def list_vms( + service_instance, + datacenter_name=None, + cluster_name=None, + host_name=None, +): """ Returns a list of VMs associated with a given service instance. service_instance The Service Instance Object from which to obtain VMs. + + datacenter_name + The datacenter name. Default is None. + + cluster_name + The cluster name - used to restrict the VMs retrieved. Only used if + the datacenter is set. This argument is optional. + + host_name + The host_name to be retrieved. Default is None. """ + properties = ["name", "config"] + + if cluster_name and not datacenter_name: + raise salt.exceptions.ArgumentValueError( + "Must specify the datacenter when specifying the cluster" + ) + + if not datacenter_name: + # Assume the root folder is the starting point + start_point = utils_common.get_root_folder(service_instance) + else: + start_point = utils_datacenter.get_datacenter(service_instance, datacenter_name) + if cluster_name: + # Retrieval to test if cluster exists. Cluster existence only makes + # sense if the datacenter has been specified + properties.append("parent") + + if host_name: + host = utils_common.get_mor_by_property(service_instance, vim.HostSystem, host_name) + properties.append("runtime.host") + log.trace("Retrieved host: %s", host) + else: + host = None + + # Search for the objects + vms = utils_common.get_mors_with_properties( + service_instance, + vim.VirtualMachine, + container_ref=start_point, + property_list=properties, + ) + items = [] - vms = utils_common.get_mors_with_properties(service_instance, vim.VirtualMachine) for vm in vms: if not vm["config"].template: - items.append(vm["name"]) + if host_name: + if host == vm["runtime.host"]: + items.append(vm["name"]) + else: + items.append(vm["name"]) return items diff --git a/tests/integration/modules/test_vm.py b/tests/integration/modules/test_vm.py index 7a5a1e02..f55420e7 100644 --- a/tests/integration/modules/test_vm.py +++ b/tests/integration/modules/test_vm.py @@ -56,7 +56,10 @@ def test_vm_list(integration_test_config, patch_salt_globals_vm): """ Test vm list_ """ - all = virtual_machine.list_() + all = virtual_machine.list_( + datacenter_name="Datacenter", + cluster_name="Cluster", + ) assert all == integration_test_config["virtual_machines"] diff --git a/tests/unit/modules/test_vm.py b/tests/unit/modules/test_vm.py new file mode 100644 index 00000000..0b4d2bb4 --- /dev/null +++ b/tests/unit/modules/test_vm.py @@ -0,0 +1,262 @@ +import itertools +import sys +import unittest.mock as mock +from collections import namedtuple + +import pytest +import salt.exceptions +import saltext.vmware.modules.vm as vm + +# TODO: why is `name` bad when it comes to Mock? Why do we need to use a namedtuple? -W. Werner, 2022-07-19 +PropSet = namedtuple("PropSet", "name,val") +non_template_config = mock.Mock(template=None) + + +@pytest.fixture +def fake_vmodl(): + with mock.patch("saltext.vmware.utils.common.vmodl") as fake_vmodl: + # TODO: Should we replace this with the actual vmodl.RuntimeFault? -W. Werner, 2022-07-21 + fake_vmodl.RuntimeFault = Exception + yield fake_vmodl + + +@pytest.fixture +def vm_property_contents(): + fake_content_1 = mock.MagicMock() + fake_content_1.propSet = [ + PropSet(name="foo", val="foo"), + PropSet(name="bar", val="barval"), + PropSet(name="config", val=non_template_config), + PropSet(name="name", val="okay?"), + PropSet(name="runtime.host", val="fnord"), + ] + fake_content_1.obj = "fnord" + + fake_content_2 = mock.MagicMock() + fake_content_2.propSet = [ + PropSet(name="foo", val="foo"), + PropSet(name="bar", val="barval"), + PropSet(name="config", val=non_template_config), + PropSet(name="name", val="foo"), + PropSet(name="runtime.host", val="fnord"), + ] + fake_content_2.obj = "fnord" + + fake_content_3 = mock.MagicMock() + fake_content_3.propSet = [ + PropSet(name="foo", val="foo"), + PropSet(name="bar", val="barval"), + PropSet(name="config", val=non_template_config), + PropSet(name="name", val="bar"), + PropSet(name="runtime.host", val="fnord"), + ] + fake_content_3.obj = "fnord" + + vm_names = ["okay?", "foo", "bar"] + return vm_names, [fake_content_1, fake_content_2, fake_content_3] + + +@pytest.fixture +def template_property_contents(): + template_config = mock.Mock(template=True) + + fake_template_1 = mock.MagicMock() + fake_template_1.propSet = [ + PropSet(name="foo", val="foo"), + PropSet(name="bar", val="barval"), + PropSet(name="config", val=template_config), + PropSet(name="name", val="template 1"), + ] + fake_template_1.obj = "fnord" + + template_names = ["template 1"] + return template_names, [fake_template_1] + + +@pytest.fixture +def vms_with_coolguy_host(quacks_like_vms): + vm_names = ["coolguy", "cooldude", "coolbro"] + coolguy_host = object() + hosty_thing = mock.MagicMock( + propSet=[ + PropSet(name="ignore", val="me"), + # This coolguy is the host.name referred to in get_mor_by_property + # or in other words, *this* is the name we search for when + # host_name is provided. + PropSet(name="name", val="coolguy"), + ], + ) + hosty_thing.obj = coolguy_host + + # The PropSet `name` should align with `vm_names`. Yes, normally we would + # just iterate over the vm_names, but when testing that can accidentally + # turn into not actually testing anything. Better to just manually do + # things here. + vms = [ + mock.MagicMock( + propSet=[ + PropSet(name="ignore", val="me"), + PropSet(name="something", val="irrelevant"), + PropSet(name="config", val=non_template_config), + PropSet(name="name", val="coolguy"), + PropSet(name="runtime.host", val=coolguy_host), + ], + ), + mock.MagicMock( + propSet=[ + PropSet(name="ignore", val="me"), + PropSet(name="something", val="irrelevant"), + PropSet(name="config", val=non_template_config), + PropSet(name="name", val="cooldude"), + PropSet(name="runtime.host", val=coolguy_host), + ], + ), + mock.MagicMock( + propSet=[ + PropSet(name="ignore", val="me"), + PropSet(name="something", val="irrelevant"), + PropSet(name="config", val=non_template_config), + PropSet(name="name", val="coolbro"), + PropSet(name="runtime.host", val=coolguy_host), + ], + ), + ] + si, *_ = quacks_like_vms + # We can't just insert stuff into the mock's `side_effect` willy-nilly. + # Instead we have to replace the side_effect with our new one. For some + # reason we have to expand with `*si.content...` the existing side_effect + # when we want to itertools.chain it. And since the existing side effect is + # our vms, we need to adjust the side_effect this way. + si.content.propertyCollector.RetrieveContents.side_effect = [ + [hosty_thing], + itertools.chain(*si.content.propertyCollector.RetrieveContents.side_effect, vms), + ] + return coolguy_host, vm_names + + +# TODO: Rename this -W. Werner, 2022-07-19 +@pytest.fixture +def quacks_like_vms(vm_property_contents, template_property_contents): + contents = [] + vm_names, vm_contents = vm_property_contents + template_names, template_contents = template_property_contents + contents.extend(vm_contents) + contents.extend(template_contents) + + fake_service_instance = mock.MagicMock() + fake_service_instance.content.propertyCollector.RetrieveContents.side_effect = [contents] + return fake_service_instance, vm_names, template_names + + +@pytest.mark.parametrize( + "datacenter_name", + [ + # To be fair, only None or "" are common - but to be comprehensive, + # let's go ahead and test the rest of the False-y things. + None, + "", + # Yep, anything below here is silly, but also still behaves the same + # way + [], + (), + {}, + False, + 0, + 0.0, + ], +) +def test_when_vm_list_is_called_with_cluster_name_but_no_datacenter_name_then_it_should_ArgumentValueError( + datacenter_name, +): + expected_message = "Must specify the datacenter when specifying the cluster" + with pytest.raises(salt.exceptions.ArgumentValueError, match=expected_message): + vm.list_( + service_instance="fnord", + datacenter_name=datacenter_name, + cluster_name="Anything that is not falsey", + ) + + +def test_when_vm_list_is_not_given_a_datacenter_or_hostname_it_should_return_expected_vms( + fake_vmodl, quacks_like_vms +): + datacenter_name = None + host_name = None + fake_service_instance, expected_vm_names, _ = quacks_like_vms + + actual_vm_names = vm.list_( + service_instance=fake_service_instance, datacenter_name=datacenter_name, host_name=host_name + ) + + assert actual_vm_names == expected_vm_names + + +def test_when_vm_list_is_given_a_hostname_then_only_vms_with_matching_runtime_host_should_be_returned( + fake_vmodl, quacks_like_vms, vms_with_coolguy_host +): + coolguy_host, expected_vm_names = vms_with_coolguy_host + fake_service_instance, _, _ = quacks_like_vms + actual_vm_names = vm.list_(service_instance=fake_service_instance, host_name="coolguy") + assert actual_vm_names == expected_vm_names + + +def test_when_vm_list_is_given_a_cluster_name_and_datacenter_name_then_parent_should_be_added_to_filter_properties( + fake_vmodl, quacks_like_vms +): + fake_service_instance, *_ = quacks_like_vms + with mock.patch( + "saltext.vmware.utils.common.get_datacenters", return_value=["fnord"], autospec=True + ): + vm.list_( + service_instance=fake_service_instance, + datacenter_name="fnord as well", + cluster_name="anything not false/empty", + ) + + if sys.version_info < (3, 8): + from pyVmomi import vim + + expected_call = mock.call( + all=False, pathSet=["name", "config", "parent"], type=vim.VirtualMachine + ) + assert fake_vmodl.query.PropertyCollector.PropertySpec.has_call(expected_call) + else: + assert ( + "parent" + in fake_vmodl.query.PropertyCollector.PropertySpec.mock_calls[0].kwargs["pathSet"] + ) + + +def test_when_vm_list_is_given_a_datacenter_name_but_no_cluster_name_then_it_should_return_expected_vms_as_filtered_by_datacenter( + fake_vmodl, quacks_like_vms +): + fake_datacenter = object() + fake_service_instance, expected_vm_names, _ = quacks_like_vms + with mock.patch( + "saltext.vmware.utils.common.get_datacenters", return_value=[fake_datacenter], autospec=True + ): + actual_vm_names = vm.list_( + service_instance=fake_service_instance, datacenter_name="some cool datacenter" + ) + + assert actual_vm_names == expected_vm_names + + if sys.version_info < (3, 8): + assert ( + fake_service_instance.content.viewManager.CreateContainerView.mock_calls[0][1][0] + is fake_datacenter + ) + else: + # Yes, these assertions are kind of gross - but we're dealing with pyvmomi and all that that entails. I don't think that there's really a great way to make these assertions. + assert ( + fake_service_instance.content.viewManager.CreateContainerView.mock_calls[0].args[0] + is fake_datacenter + ) + + assert ( + fake_vmodl.query.PropertyCollector.ObjectSpec.mock_calls[0].kwargs["obj"] + == fake_service_instance.content.viewManager.CreateContainerView.return_value + ) + assert fake_vmodl.query.PropertyCollector.FilterSpec.mock_calls[0].kwargs["objectSet"] == [ + fake_vmodl.query.PropertyCollector.ObjectSpec.return_value + ]