diff --git a/docs/topics/configuration.md b/docs/topics/configuration.md index b24a181..f085a64 100644 --- a/docs/topics/configuration.md +++ b/docs/topics/configuration.md @@ -6,9 +6,10 @@ Set up the cloud configuration at `/etc/salt/cloud.providers` or `/etc/salt/clou ```yaml my-proxmox-config: # Required parameters + host: hypervisor.domain.tld:8006 user: myuser@pam # or myuser@pve - token: myapitoken - url: https://hypervisor.domain.tld:8006 + token_name: myapitoken_name + token_value: myapitoken_value driver: proxmox ``` diff --git a/pyproject.toml b/pyproject.toml index 3e5d3ce..2ad8ef5 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -37,7 +37,7 @@ classifiers = [ requires-python = ">= 3.8" dynamic = ["version"] dependencies = [ - "requests>=2.2.1", + "proxmoxer>=2.0.1", "salt>=3005", ] diff --git a/src/saltext/proxmox/clouds/proxmox.py b/src/saltext/proxmox/clouds/proxmox.py index b22e500..7af0d06 100644 --- a/src/saltext/proxmox/clouds/proxmox.py +++ b/src/saltext/proxmox/clouds/proxmox.py @@ -9,26 +9,26 @@ supported through this cloud module. :maintainer: EITR Technologies, LLC -:depends: requests >= 2.2.1 +:depends: proxmoxer >= 2.0.1 """ import logging import time from ipaddress import ip_interface -import salt.config as config +import salt.config import salt.utils.cloud -import salt.utils.json from salt.exceptions import SaltCloudExecutionTimeout from salt.exceptions import SaltCloudNotFound from salt.exceptions import SaltCloudSystemExit try: - import requests + from proxmoxer import ProxmoxAPI + from proxmoxer.tools import Tasks - HAS_REQUESTS = True + HAS_PROXMOXER = True except ImportError: - HAS_REQUESTS = False + HAS_PROXMOXER = False # Get logging started log = logging.getLogger(__name__) @@ -60,10 +60,10 @@ def get_configured_provider(): """ Return the first configured instance. """ - return config.is_provider_configured( + return salt.config.is_provider_configured( __opts__, _get_active_provider_name() or __virtualname__, - ("url", "user", "token"), + ("host", "user", "token_name", "token_value"), ) @@ -71,8 +71,8 @@ def get_dependencies(): """ Warn if dependencies aren't met. """ - deps = {"requests": HAS_REQUESTS} - return config.check_driver_dependencies(__virtualname__, deps) + deps = {"proxmoxer": HAS_PROXMOXER} + return salt.config.check_driver_dependencies(__virtualname__, deps) def create(vm_): @@ -82,29 +82,53 @@ def create(vm_): salt.utils.cloud.fire_event( "event", "starting create", - "salt/cloud/{}/creating".format(vm_["name"]), # pylint: disable=consider-using-f-string - args=salt.utils.cloud.filter_event( + f"salt/cloud/{vm_['name']}/creating", + # calling this via salt.utils.cloud.filter_event causes "name '__opts__' is not defined" error + args=__utils__["cloud.filter_event"]( # pylint: disable=undefined-variable "creating", vm_, ["name", "profile", "provider", "driver"] ), sock_dir=__opts__["sock_dir"], transport=__opts__["transport"], ) - type = vm_.get("technology") + try: + type = vm_["technology"] + except KeyError as e: + raise SaltCloudSystemExit( + f"The VM profile '{vm_['profile']}' is missing the 'technology' parameter." + ) from e clone_options = vm_.get("clone") - should_clone = True if clone_options else False - if should_clone: + if clone_options: clone(call="function", kwargs=clone_options) else: - _query("POST", f"nodes/{vm_['create']['node']}/{type}", vm_["create"]) + upid = _get_proxmox_client().post(f"nodes/{vm_['create']['node']}/{type}", **vm_["create"]) + _wait_for_task(upid=upid) - start(call="action", name=vm_["name"]) + # sometimes it takes proxmox a while to propagate the information about the new VM + max_retries = 5 + wait_time = 5 + for _ in range(max_retries): + try: + start(call="action", name=vm_["name"]) + break + except SaltCloudNotFound: + log.warning( + "Newly created VM '%s' is not yet listed via the API. Retrying in %s seconds...", + vm_["name"], + wait_time, + ) + time.sleep(wait_time) + else: + raise SaltCloudSystemExit( + f"Failed to start the VM '{vm_['name']}' after {max_retries} attempts." + ) # cloud.bootstrap expects the ssh_password to be set in vm_["password"] vm_["password"] = vm_.get("ssh_password") - ret = salt.utils.cloud.bootstrap(vm_, __opts__) + # calling this via salt.utils.cloud.bootstrap causes "name '__opts__' is not defined" error + ret = __utils__["cloud.bootstrap"](vm_, __opts__) # pylint: disable=undefined-variable ret.update(show_instance(call="action", name=vm_["name"])) @@ -112,7 +136,8 @@ def create(vm_): "event", "created instance", f"salt/cloud/{vm_['name']}/created", - args=salt.utils.cloud.filter_event( + # calling this via salt.utils.cloud.filter_event causes "name '__opts__' is not defined" error + args=__utils__["cloud.filter_event"]( # pylint: disable=undefined-variable "created", vm_, ["name", "profile", "provider", "driver"] ), sock_dir=__opts__["sock_dir"], @@ -142,25 +167,18 @@ def clone(kwargs=None, call=None): if call != "function": raise SaltCloudSystemExit("The clone function must be called with -f or --function.") - if not isinstance(kwargs, dict): + if kwargs is None: kwargs = {} - vmid = kwargs.get("vmid") + try: + vmid = int(kwargs["vmid"]) + except KeyError as e: + raise SaltCloudSystemExit("The required parameter 'vmid' was not given.") from e vm = _get_vm_by_id(vmid) - _query("POST", f"nodes/{vm['node']}/{vm['type']}/{vmid}/clone", kwargs) - - # TODO: optionally wait for it to exist - # timeout = 300 - # start_time = time.time() - # while time.time() < start_time + timeout: - # try: - # _get_vm_by_id(newid) - # except SaltCloudNotFound: - # log.debug("blabla") - - # raise SaltCloudExecutionTimeout("Timeout to wait for VM cloning reached") + upid = _get_proxmox_client().post(f"nodes/{vm['node']}/{vm['type']}/{vmid}/clone", **kwargs) + _wait_for_task(upid=upid) def reconfigure(name=None, kwargs=None, call=None): @@ -188,7 +206,10 @@ def reconfigure(name=None, kwargs=None, call=None): vm = _get_vm_by_name(name) - _query("PUT", f"nodes/{vm['node']}/{vm['type']}/{vm['vmid']}/config", kwargs) + if kwargs is None: + kwargs = {} + + _get_proxmox_client().put(f"nodes/{vm['node']}/{vm['type']}/{vm['vmid']}/config", **kwargs) return { "success": True, @@ -225,7 +246,10 @@ def destroy(name=None, kwargs=None, call=None): vm = _get_vm_by_name(name) - _query("DELETE", f"nodes/{vm['node']}/{vm['type']}/{vm['vmid']}", kwargs) + if kwargs is None: + kwargs = {} + + _get_proxmox_client().delete(f"nodes/{vm['node']}/{vm['type']}/{vm['vmid']}", **kwargs) salt.utils.cloud.fire_event( "event", @@ -239,7 +263,7 @@ def destroy(name=None, kwargs=None, call=None): def avail_locations(call=None): """ - Return available Proxmox datacenter locations + Return available Proxmox datacenter locations (nodes) CLI Example: @@ -253,7 +277,7 @@ def avail_locations(call=None): "-f or --function, or with the --list-locations option" ) - locations = _query("GET", "nodes") + locations = _get_proxmox_client().get("nodes") ret = {} for location in locations: @@ -289,7 +313,7 @@ def avail_images(kwargs=None, call=None): "-f or --function, or with the --list-images option" ) - if not isinstance(kwargs, dict): + if kwargs is None: kwargs = {} storage = kwargs.get("storage", "local") @@ -297,9 +321,9 @@ def avail_images(kwargs=None, call=None): ret = {} for location in avail_locations(): ret[location] = {} - for item in _query("GET", f"nodes/{location}/storage/{storage}/content"): - ret[location][item["volid"]] = item - # TODO: filter to actual images. what is an imagetype? images, vztmpl, iso + for item in _get_proxmox_client().get(f"nodes/{location}/storage/{storage}/content"): + if item["content"] in ("images", "vztmpl", "iso"): + ret[location][item["volid"]] = item return ret @@ -318,23 +342,14 @@ def list_nodes(call=None): if call == "action": raise SaltCloudSystemExit("The list_nodes function must be called with -f or --function.") - vms = _query("GET", "cluster/resources", data={"type": "vm"}) + vms = list_nodes_full(call="function") ret = {} - for vm in vms: - name = vm["name"] + for vm, props in vms.items(): + ret[vm] = {} - ret[name] = {} - ret[name]["id"] = str(vm["vmid"]) - ret[name]["image"] = "" # proxmox does not carry that information - ret[name]["size"] = "" # proxmox does not have VM sizes like AWS (e.g: t2-small) - ret[name]["state"] = str(vm["status"]) - - config = _query("GET", f"nodes/{vm['node']}/{vm['type']}/{vm['vmid']}/config") - private_ips, public_ips = _parse_ips(config, vm["type"]) - - ret[name]["private_ips"] = private_ips - ret[name]["public_ips"] = public_ips + for prop in ("id", "image", "private_ips", "public_ips", "size", "state"): + ret[vm][prop] = props[prop] return ret @@ -355,15 +370,24 @@ def list_nodes_full(call=None): "The list_nodes_full function must be called with -f or --function." ) - vms = _query("GET", "cluster/resources", data={"type": "vm"}) + vms = _get_proxmox_client().get("cluster/resources", type="vm") ret = {} for vm in vms: name = vm["name"] - config = _query("GET", f"nodes/{vm['node']}/{vm['type']}/{vm['vmid']}/config") - ret[name] = vm + config = _get_proxmox_client().get(f"nodes/{vm['node']}/{vm['type']}/{vm['vmid']}/config") + private_ips, public_ips = _parse_ips(config, vm["type"]) + + ret[name] = {} + ret[name]["id"] = str(vm["vmid"]) + ret[name]["image"] = "" # proxmox does not carry that information + ret[name]["private_ips"] = private_ips + ret[name]["public_ips"] = public_ips + ret[name]["size"] = "" # proxmox does not have VM sizes like AWS (e.g: t2-small) + ret[name]["state"] = str(vm["status"]) ret[name]["config"] = config + ret[name]["resource"] = vm return ret @@ -427,8 +451,6 @@ def start(name=None, kwargs=None, call=None): _set_vm_status(name, "start", kwargs) - _wait_for_vm_status(name, "running") - return { "success": True, "state": "running", @@ -461,8 +483,6 @@ def stop(name=None, kwargs=None, call=None): _set_vm_status(name, "stop", kwargs) - _wait_for_vm_status(name, "stopped") - return { "success": True, "state": "stopped", @@ -495,8 +515,6 @@ def shutdown(name=None, kwargs=None, call=None): _set_vm_status(name, "shutdown", kwargs) - _wait_for_vm_status(name, "stopped") - return { "success": True, "state": "stopped", @@ -504,72 +522,6 @@ def shutdown(name=None, kwargs=None, call=None): } -def _query(method, path, data=None): - """ - Query the Proxmox API - """ - base_url = _get_url() - api_token = _get_api_token() - - url = f"{base_url}/api2/json/{path}" - - headers = { - "Accept": "application/json", - "User-Agent": "salt-cloud-proxmox", - "Authorization": f"PVEAPIToken={api_token}", - } - - try: - response = None - - if method == "GET": - response = requests.get( - url=url, - headers=headers, - params=data, - timeout=10, - ) - else: - headers["Content-Type"] = "application/x-www-form-urlencoded" - response = requests.request( - method=method, - url=url, - headers=headers, - data=data, - timeout=10, - ) - - response.raise_for_status() - returned_data = response.json() - return returned_data.get("data") - - except requests.exceptions.RequestException as err: - log.error("Error in query to %s:\n%s", url, response.text) - raise SaltCloudSystemExit(err) from err - - -def _get_url(): - """ - Returns the configured Proxmox URL - """ - return config.get_cloud_config_value( - "url", get_configured_provider(), __opts__, search_global=False - ) - - -def _get_api_token(): - """ - Returns the API token for the Proxmox API - """ - username = config.get_cloud_config_value( - "user", get_configured_provider(), __opts__, search_global=False - ) - token = config.get_cloud_config_value( - "token", get_configured_provider(), __opts__, search_global=False - ) - return f"{username}!{token}" - - def _get_vm_by_name(name): """ Return VM identified by name @@ -581,7 +533,8 @@ def _get_vm_by_name(name): This function will return the first occurrence of a VM matching the given name. """ - vms = _query("GET", "cluster/resources", {"type": "vm"}) + vms = _get_proxmox_client().get("cluster/resources", type="vm") + for vm in vms: if vm["name"] == name: return vm @@ -596,7 +549,8 @@ def _get_vm_by_id(vmid): vmid The vmid of the VM. Required. """ - vms = _query("GET", "cluster/resources", {"type": "vm"}) + vms = _get_proxmox_client().get("cluster/resources", type="vm") + for vm in vms: if vm["vmid"] == vmid: return vm @@ -619,18 +573,21 @@ def _set_vm_status(name, status, kwargs=None): """ vm = _get_vm_by_name(name) - _query("POST", f"nodes/{vm['node']}/{vm['type']}/{vm['vmid']}/status/{status}", kwargs) + if kwargs is None: + kwargs = {} + upid = _get_proxmox_client().post( + f"nodes/{vm['node']}/{vm['type']}/{vm['vmid']}/status/{status}", **kwargs + ) + _wait_for_task(upid=upid) -def _wait_for_vm_status(name, status, timeout=300, interval=0.2): - """ - Wait for the VM to reach a given status - name - The name of the VM. Required. +def _wait_for_task(upid, timeout=300, interval=0.2): + """ + Wait for the task to finish successfully - status - The expected status of the VM. Required. + upid + The UPID of the task. Required. timeout The timeout in seconds on how long to wait for the task. Default: 300 seconds @@ -638,18 +595,45 @@ def _wait_for_vm_status(name, status, timeout=300, interval=0.2): interval The interval in seconds at which the API should be queried for updates. Default: 0.2 seconds """ - vm = _get_vm_by_name(name) - - start_time = time.time() - while time.time() < start_time + timeout: - response = _query("GET", f"nodes/{vm['node']}/{vm['type']}/{vm['vmid']}/status/current") + node = Tasks.decode_upid(upid=upid)["node"] + response = {"status": ""} - if response["status"] == status: - return True + start_time = time.monotonic() + while response["status"] != "stopped": + if time.monotonic() >= start_time + timeout: + raise SaltCloudExecutionTimeout(f"Timeout to wait for task '{upid}' reached.") + response = _get_proxmox_client().get(f"nodes/{node}/tasks/{upid}/status") time.sleep(interval) - raise SaltCloudExecutionTimeout("Timeout to wait for VM status reached.") + if "failed" in response["exitstatus"]: + raise SaltCloudSystemExit(f"Task did not finish successfully: {response['exitstatus']}") + + return response + + +def _get_proxmox_client(): + host = salt.config.get_cloud_config_value( + "host", get_configured_provider(), __opts__, search_global=False + ) + user = salt.config.get_cloud_config_value( + "user", get_configured_provider(), __opts__, search_global=False + ) + token_name = salt.config.get_cloud_config_value( + "token_name", get_configured_provider(), __opts__, search_global=False + ) + token_value = salt.config.get_cloud_config_value( + "token_value", get_configured_provider(), __opts__, search_global=False + ) + + return ProxmoxAPI( + host=host, + backend="https", + service="PVE", + user=user, + token_name=token_name, + token_value=token_value, + ) def _stringlist_to_dictionary(input_string): diff --git a/tests/unit/clouds/test_proxmox.py b/tests/unit/clouds/test_proxmox.py index 1154864..421c060 100644 --- a/tests/unit/clouds/test_proxmox.py +++ b/tests/unit/clouds/test_proxmox.py @@ -1,11 +1,9 @@ """ - :codeauthor: Bernhard Gally +:maintainer: EITR Technologies, LLC """ -import io - import pytest -import requests +from salt.exceptions import SaltCloudExecutionTimeout from salt.exceptions import SaltCloudNotFound from salt.exceptions import SaltCloudSystemExit @@ -29,17 +27,27 @@ def configure_loader_modules(): "sock_dir": True, "transport": True, }, + "__utils__": { + "cloud.filter_event": MagicMock(), + "cloud.bootstrap": MagicMock(), + }, "__active_provider_name__": "", } } @patch(_fqn(proxmox.show_instance)) +@patch(_fqn(proxmox._wait_for_task)) @patch(_fqn(proxmox.start)) -@patch(_fqn(proxmox._query)) -def test_create(mock__query: MagicMock, mock_start: MagicMock, mock_show_instance: MagicMock): +@patch(_fqn(proxmox._get_proxmox_client)) +def test_create( + mock__get_proxmox_client: MagicMock, + mock_start: MagicMock, + mock__wait_for_task: MagicMock, + mock_show_instance: MagicMock, +): """ - Test that `create()` is calling the correct endpoint with the correct arguments + Test that `create()` calls the correct endpoint with the correct arguments and waits for VM creation """ create_config = { "name": "my-vm", @@ -50,11 +58,33 @@ def test_create(mock__query: MagicMock, mock_start: MagicMock, mock_show_instanc }, } - with patch("salt.utils.cloud.bootstrap", MagicMock()), patch( - "salt.utils.cloud.filter_event", MagicMock() - ), patch("salt.utils.cloud.fire_event", MagicMock()): + upid = "UPID:node1:0016BEC6:568EF5F4:669FB044:qmcreate:101:user@pam!mytoken:" + mock__get_proxmox_client.return_value.post.return_value = upid + + with patch("salt.utils.cloud.fire_event", MagicMock()): + proxmox.create(create_config) + + mock__get_proxmox_client.return_value.post.assert_called_with( + "nodes/proxmox-node1/qemu", **create_config["create"] + ) + mock__wait_for_task.assert_called_once_with(upid=upid) + + +def test_create_with_missing_technology_argument(): + """ + Test that `create()` fails when the 'technology' argument is missing in the profile configuration + """ + create_config = { + "name": "my-vm", + "profile": "my-vm-profile", + "create": { + "vmid": 123, + "node": "proxmox-node1", + }, + } + + with pytest.raises(SaltCloudSystemExit), patch("salt.utils.cloud.fire_event", MagicMock()): proxmox.create(create_config) - mock__query.assert_called_with("POST", "nodes/proxmox-node1/qemu", create_config["create"]) @patch(_fqn(proxmox.show_instance)) @@ -76,18 +106,22 @@ def test_create_with_clone( }, } - with patch("salt.utils.cloud.bootstrap", MagicMock()), patch( - "salt.utils.cloud.filter_event", MagicMock() - ), patch("salt.utils.cloud.fire_event", MagicMock()): + with patch("salt.utils.cloud.fire_event", MagicMock()): proxmox.create(clone_config) + mock_clone.assert_called() +@patch(_fqn(proxmox._wait_for_task)) @patch(_fqn(proxmox._get_vm_by_id)) -@patch(_fqn(proxmox._query)) -def test_clone(mock__query: MagicMock, mock__get_vm_by_id: MagicMock): +@patch(_fqn(proxmox._get_proxmox_client)) +def test_clone( + mock__get_proxmox_client: MagicMock, + mock__get_vm_by_id: MagicMock, + mock__wait_for_task: MagicMock, +): """ - Test that `clone()` is calling the correct endpoint with the correct arguments + Test that `clone()` calls the correct endpoint with the correct arguments and waits for VM creation """ clone_config = { "vmid": 123, @@ -100,8 +134,23 @@ def test_clone(mock__query: MagicMock, mock__get_vm_by_id: MagicMock): "type": "lxc", } + upid = "UPID:node1:0016BEC6:568EF5F4:669FB044:qmclone:101:user@pam!mytoken:" + mock__get_proxmox_client.return_value.post.return_value = upid + proxmox.clone(call="function", kwargs=clone_config) - mock__query.assert_called_with("POST", "nodes/proxmox-node1/lxc/123/clone", clone_config) + + mock__get_proxmox_client.return_value.post.assert_called_with( + "nodes/proxmox-node1/lxc/123/clone", **clone_config + ) + mock__wait_for_task.assert_called_once_with(upid=upid) + + +def test_clone_with_missing_vmid_argument(): + """ + Test that `clone()` fails when the 'vmid' argument is missing + """ + with pytest.raises(SaltCloudSystemExit): + proxmox.clone(call="function") def test_clone_when_called_as_action(): @@ -113,10 +162,10 @@ def test_clone_when_called_as_action(): @patch(_fqn(proxmox._get_vm_by_name)) -@patch(_fqn(proxmox._query)) -def test_reconfigure(mock__query: MagicMock, mock__get_vm_by_name: MagicMock): +@patch(_fqn(proxmox._get_proxmox_client)) +def test_reconfigure(mock__get_proxmox_client: MagicMock, mock__get_vm_by_name: MagicMock): """ - Test that `reconfigure()` is calling the correct endpoint with the correct arguments + Test that `reconfigure()` calls the correct endpoint with the correct arguments """ reconfigure_config = {"description": "custom description to be updated"} @@ -128,7 +177,33 @@ def test_reconfigure(mock__query: MagicMock, mock__get_vm_by_name: MagicMock): } proxmox.reconfigure(call="action", name="my-proxmox-vm", kwargs=reconfigure_config) - mock__query.assert_called_with("PUT", "nodes/proxmox-node1/lxc/123/config", reconfigure_config) + + mock__get_proxmox_client.return_value.put.assert_called_with( + "nodes/proxmox-node1/lxc/123/config", **reconfigure_config + ) + + +@patch(_fqn(proxmox._get_vm_by_name)) +@patch(_fqn(proxmox._get_proxmox_client)) +def test_reconfigure_with_no_arguments( + mock__get_proxmox_client: MagicMock, + mock__get_vm_by_name: MagicMock, +): + """ + Test that `reconfigure()` calls the endpoint correctly with no arguments + """ + mock__get_vm_by_name.return_value = { + "vmid": 123, + "name": "my-proxmox-vm", + "node": "proxmox-node1", + "type": "lxc", + } + + proxmox.reconfigure(call="action", name="my-proxmox-vm") + + mock__get_proxmox_client.return_value.put.assert_called_with( + "nodes/proxmox-node1/lxc/123/config" + ) def test_reconfigure_when_called_as_function(): @@ -140,10 +215,10 @@ def test_reconfigure_when_called_as_function(): @patch(_fqn(proxmox._get_vm_by_name)) -@patch(_fqn(proxmox._query)) -def test_destroy(mock__query: MagicMock, mock__get_vm_by_name: MagicMock): +@patch(_fqn(proxmox._get_proxmox_client)) +def test_destroy(mock__get_proxmox_client: MagicMock, mock__get_vm_by_name: MagicMock): """ - Test that `clone()` is calling the correct endpoint with the correct arguments + Test that `clone()` calls the correct endpoint with the correct arguments """ destroy_config = {"force": True} @@ -154,11 +229,34 @@ def test_destroy(mock__query: MagicMock, mock__get_vm_by_name: MagicMock): "type": "lxc", } - with patch("salt.utils.cloud.bootstrap", MagicMock()), patch( - "salt.utils.cloud.filter_event", MagicMock() - ), patch("salt.utils.cloud.fire_event", MagicMock()): + with patch("salt.utils.cloud.fire_event", MagicMock()): proxmox.destroy(call="action", name="my-proxmox-vm", kwargs=destroy_config) - mock__query.assert_called_with("DELETE", "nodes/proxmox-node1/lxc/123", destroy_config) + + mock__get_proxmox_client.return_value.delete.assert_called_with( + "nodes/proxmox-node1/lxc/123", **destroy_config + ) + + +@patch(_fqn(proxmox._get_vm_by_name)) +@patch(_fqn(proxmox._get_proxmox_client)) +def test_destroy_with_no_arguments( + mock__get_proxmox_client: MagicMock, + mock__get_vm_by_name: MagicMock, +): + """ + Test that `clone()` calls the endpoint correctly with no arguments + """ + mock__get_vm_by_name.return_value = { + "vmid": 123, + "name": "my-proxmox-vm", + "node": "proxmox-node1", + "type": "lxc", + } + + with patch("salt.utils.cloud.fire_event", MagicMock()): + proxmox.destroy(call="action", name="my-proxmox-vm") + + mock__get_proxmox_client.return_value.delete.assert_called_with("nodes/proxmox-node1/lxc/123") def test_destroy_when_called_as_function(): @@ -169,17 +267,18 @@ def test_destroy_when_called_as_function(): proxmox.destroy(call="function") -@patch(_fqn(proxmox._query)) -def test_avail_locations(mock__query: MagicMock): +@patch(_fqn(proxmox._get_proxmox_client)) +def test_avail_locations(mock__get_proxmox_client: MagicMock): """ Test that only nodes with the status online are listed """ - mock__query.return_value = [ + mock__get_proxmox_client.return_value.get.return_value = [ {"node": "node1", "status": "online"}, {"node": "node2", "status": "offline"}, ] result = proxmox.avail_locations(call="function") + assert result == { "node1": { "node": "node1", @@ -197,59 +296,81 @@ def test_avail_locations_when_called_as_action(): @patch(_fqn(proxmox.avail_locations)) -@patch(_fqn(proxmox._query)) -def test_avail_images(mock__query: MagicMock, mock_avail_locations: MagicMock): +@patch(_fqn(proxmox._get_proxmox_client)) +def test_avail_images(mock__get_proxmox_client: MagicMock, mock_avail_locations: MagicMock): """ - Test that avail_images returns images in the correct data structure + Test that avail_images returns images (content type: vztmpl, images or iso) in the correct data structure """ mock_avail_locations.return_value = {"node1": {}} - mock__query.return_value = [ + mock__get_proxmox_client.return_value.get.return_value = [ { - "volid": "other_storage:vztmpl/ubuntu-20.04-standard_20.04-1_amd64.tar.zst", + "volid": "other_storage:vztmpl/ubuntu-20.04-standard_20.04-1_amd64.tar.gz", "content": "vztmpl", - "size": 129824858, }, { "volid": "other_storage:vztmpl/ubuntu-22.04-standard_22.04-1_amd64.tar.zst", "content": "vztmpl", - "size": 129824858, + }, + { + "volid": "other_storage:vm-201-disk-0", + "content": "images", + }, + { + "volid": "other_storage:iso/ubuntu-22.04.2-live-server-amd64.iso", + "content": "iso", + }, + { + "volid": "other_storage:backup/template-backup.tar.gz", + "content": "backup", }, ] result = proxmox.avail_images(call="function") + assert result == { "node1": { - "other_storage:vztmpl/ubuntu-20.04-standard_20.04-1_amd64.tar.zst": { - "volid": "other_storage:vztmpl/ubuntu-20.04-standard_20.04-1_amd64.tar.zst", + "other_storage:vztmpl/ubuntu-20.04-standard_20.04-1_amd64.tar.gz": { + "volid": "other_storage:vztmpl/ubuntu-20.04-standard_20.04-1_amd64.tar.gz", "content": "vztmpl", - "size": 129824858, }, "other_storage:vztmpl/ubuntu-22.04-standard_22.04-1_amd64.tar.zst": { "volid": "other_storage:vztmpl/ubuntu-22.04-standard_22.04-1_amd64.tar.zst", "content": "vztmpl", - "size": 129824858, + }, + "other_storage:vm-201-disk-0": { + "volid": "other_storage:vm-201-disk-0", + "content": "images", + }, + "other_storage:iso/ubuntu-22.04.2-live-server-amd64.iso": { + "volid": "other_storage:iso/ubuntu-22.04.2-live-server-amd64.iso", + "content": "iso", }, } } @patch(_fqn(proxmox.avail_locations)) -@patch(_fqn(proxmox._query)) -def test_avail_images_when_storage_given(mock__query: MagicMock, mock_avail_locations: MagicMock): +@patch(_fqn(proxmox._get_proxmox_client)) +def test_avail_images_when_storage_given( + mock__get_proxmox_client: MagicMock, mock_avail_locations: MagicMock +): """ Test that avail_images queries given storage """ mock_avail_locations.return_value = {"node1": {}} - kwargs = {"storage": "other_storage"} + proxmox.avail_images(call="function", kwargs=kwargs) - mock__query.assert_called_with("GET", "nodes/node1/storage/other_storage/content") + + mock__get_proxmox_client.return_value.get.assert_called_with( + "nodes/node1/storage/other_storage/content" + ) @patch(_fqn(proxmox.avail_locations)) -@patch(_fqn(proxmox._query)) +@patch(_fqn(proxmox._get_proxmox_client)) def test_avail_images_when_no_storage_given( - mock__query: MagicMock, mock_avail_locations: MagicMock + mock__get_proxmox_client: MagicMock, mock_avail_locations: MagicMock ): """ Test that avail_images queries storage "local" when not specifying a storage @@ -257,7 +378,10 @@ def test_avail_images_when_no_storage_given( mock_avail_locations.return_value = {"node1": {}} proxmox.avail_images(call="function") - mock__query.assert_called_with("GET", "nodes/node1/storage/local/content") + + mock__get_proxmox_client.return_value.get.assert_called_with( + "nodes/node1/storage/local/content" + ) def test_avail_images_when_called_as_action(): @@ -268,40 +392,52 @@ def test_avail_images_when_called_as_action(): proxmox.avail_images(call="action") -@patch(_fqn(proxmox._parse_ips)) -@patch(_fqn(proxmox._query)) -def test_list_nodes(mock__query: MagicMock, mock__parse_ips: MagicMock): +@patch(_fqn(proxmox.list_nodes_full)) +def test_list_nodes(mock_list_nodes_full: MagicMock): """ Test that `list_nodes()` returns a list of managed VMs with the following fields: * id - * size * image - * state * private_ips * public_ips + * size + * state """ - - mock__query.return_value = [ - { - "vmid": 100, - "status": "stopped", - "name": "my-proxmox-vm", - "node": "proxmox", - "type": "lxc", + mock_list_nodes_full.return_value = { + "my-proxmox-vm1": { + "id": "100", + "image": "", + "private_ips": ["192.168.1.2"], + "public_ips": [], + "size": "", + "state": "stopped", + "config": { + "ostype": "ubuntu", + "hostname": "my-proxmox-vm1", + "net0": "name=eth0,bridge=vmbr0,hwaddr=BA:F9:3B:F7:9E:A7,ip=192.168.1.2/24,type=veth", + }, + "resource": { + "vmid": 100, + "name": "my-proxmox-vm1", + "status": "stopped", + "node": "node1", + "type": "lxc", + "maxcpu": 2, + "maxdisk": 123456, + }, }, - ] - - mock__parse_ips.return_value = ([], []) + } result = proxmox.list_nodes() + assert result == { - "my-proxmox-vm": { + "my-proxmox-vm1": { "id": "100", - "size": "", "image": "", - "state": "stopped", - "private_ips": [], + "private_ips": ["192.168.1.2"], "public_ips": [], + "size": "", + "state": "stopped", } } @@ -314,48 +450,95 @@ def test_list_nodes_when_called_as_action(): proxmox.list_nodes(call="action") -@patch(_fqn(proxmox._query)) -def test_list_nodes_full(mock__query: MagicMock): +@patch(_fqn(proxmox._get_proxmox_client)) +def test_list_nodes_full(mock__get_proxmox_client: MagicMock): """ Test that `list_nodes_full()` returns a list of managed VMs with their respective config """ - # TODO - - _query_responses = [ - # first response (vm resources) + responses = [ + # first response (all vm resources) [ { "vmid": 100, + "name": "my-proxmox-vm1", + "status": "stopped", + "node": "node1", + "type": "lxc", + "maxcpu": 2, + "maxdisk": 123456, + }, + { + "vmid": 101, + "name": "my-proxmox-vm2", "status": "stopped", - "name": "my-proxmox-vm", - "node": "proxmox", + "node": "node1", "type": "lxc", - } + "maxcpu": 4, + "maxdisk": 234567, + }, ], - # second response (vm config) + # second response (vm configs) { "ostype": "ubuntu", - "hostname": "my-proxmox-vm", + "hostname": "my-proxmox-vm1", "net0": "name=eth0,bridge=vmbr0,hwaddr=BA:F9:3B:F7:9E:A7,ip=192.168.1.2/24,type=veth", }, + { + "ostype": "ubuntu", + "hostname": "my-proxmox-vm2", + "net0": "name=eth0,bridge=vmbr0,hwaddr=BA:F9:3B:F7:9E:A8,ip=192.168.1.3/24,type=veth", + }, ] - mock__query.side_effect = _query_responses + mock__get_proxmox_client.return_value.get.side_effect = responses result = proxmox.list_nodes_full() + assert result == { - "my-proxmox-vm": { - "vmid": 100, - "status": "stopped", - "name": "my-proxmox-vm", - "node": "proxmox", - "type": "lxc", + "my-proxmox-vm1": { + "id": "100", + "image": "", + "private_ips": ["192.168.1.2"], + "public_ips": [], + "size": "", + "state": "stopped", "config": { "ostype": "ubuntu", - "hostname": "my-proxmox-vm", + "hostname": "my-proxmox-vm1", "net0": "name=eth0,bridge=vmbr0,hwaddr=BA:F9:3B:F7:9E:A7,ip=192.168.1.2/24,type=veth", }, - } + "resource": { + "vmid": 100, + "name": "my-proxmox-vm1", + "status": "stopped", + "node": "node1", + "type": "lxc", + "maxcpu": 2, + "maxdisk": 123456, + }, + }, + "my-proxmox-vm2": { + "id": "101", + "image": "", + "private_ips": ["192.168.1.3"], + "public_ips": [], + "size": "", + "state": "stopped", + "config": { + "ostype": "ubuntu", + "hostname": "my-proxmox-vm2", + "net0": "name=eth0,bridge=vmbr0,hwaddr=BA:F9:3B:F7:9E:A8,ip=192.168.1.3/24,type=veth", + }, + "resource": { + "vmid": 101, + "name": "my-proxmox-vm2", + "status": "stopped", + "node": "node1", + "type": "lxc", + "maxcpu": 4, + "maxdisk": 234567, + }, + }, } @@ -369,7 +552,9 @@ def test_list_nodes_full_when_called_as_action(): @patch(_fqn(proxmox.list_nodes_full)) def test_show_instance(mock_list_nodes_full: MagicMock): - """ """ + """ + Test that `show_instance()` returns full information of requested node + """ mock_list_nodes_full.return_value = { "my-proxmox-vm": { "vmid": 100, @@ -386,6 +571,7 @@ def test_show_instance(mock_list_nodes_full: MagicMock): } result = proxmox.show_instance(call="action", name="my-proxmox-vm") + assert result == mock_list_nodes_full.return_value["my-proxmox-vm"] @@ -408,17 +594,17 @@ def test_show_instance_when_called_as_function(): proxmox.show_instance(call="function") -@patch(_fqn(proxmox._wait_for_vm_status)) @patch(_fqn(proxmox._set_vm_status)) -def test_start(mock__set_vm_status: MagicMock, mock__wait_for_vm_status: MagicMock): +def test_start(mock__set_vm_status: MagicMock): """ - Test that `start()` uses `_set_vm_status()` and `_wait_for_vm_status()` correctly + Test that `start()` uses `_set_vm_status()` correctly """ name = "my-proxmox-vm" kwargs = {"some-optional-argument": True} + proxmox.start(call="action", name=name, kwargs=kwargs) + mock__set_vm_status.assert_called_with(name, "start", kwargs) - mock__wait_for_vm_status.assert_called_with(name, "running") def test_start_when_called_as_function(): @@ -429,17 +615,17 @@ def test_start_when_called_as_function(): proxmox.start(call="function") -@patch(_fqn(proxmox._wait_for_vm_status)) @patch(_fqn(proxmox._set_vm_status)) -def test_stop(mock__set_vm_status: MagicMock, mock__wait_for_vm_status: MagicMock): +def test_stop(mock__set_vm_status: MagicMock): """ - Test that `stop()` uses `_set_vm_status()` and `_wait_for_vm_status()` correctly + Test that `stop()` uses `_set_vm_status() correctly """ name = "my-proxmox-vm" kwargs = {"some-optional-argument": True} + proxmox.stop(call="action", name=name, kwargs=kwargs) + mock__set_vm_status.assert_called_with(name, "stop", kwargs) - mock__wait_for_vm_status.assert_called_with(name, "stopped") def test_stop_when_called_as_function(): @@ -450,17 +636,17 @@ def test_stop_when_called_as_function(): proxmox.stop(call="function") -@patch(_fqn(proxmox._wait_for_vm_status)) @patch(_fqn(proxmox._set_vm_status)) -def test_shutdown(mock__set_vm_status: MagicMock, mock__wait_for_vm_status: MagicMock): +def test_shutdown(mock__set_vm_status: MagicMock): """ - Test that `shutdown()` uses `_set_vm_status()` and `_wait_for_vm_status()` correctly + Test that `shutdown()` uses `_set_vm_status()` correctly """ name = "my-proxmox-vm" kwargs = {"some-optional-argument": True} + proxmox.shutdown(call="action", name=name, kwargs=kwargs) + mock__set_vm_status.assert_called_with(name, "shutdown", kwargs) - mock__wait_for_vm_status.assert_called_with(name, "stopped") def test_shutdown_when_called_as_function(): @@ -471,89 +657,149 @@ def test_shutdown_when_called_as_function(): proxmox.shutdown(call="function") -@patch(_fqn(proxmox._get_url)) -@patch(_fqn(proxmox._get_api_token)) -@patch("requests.get") -def test_detailed_logging_on_http_errors( - mock_request: MagicMock, mock_get_api_token: MagicMock, mock_get_url: MagicMock, caplog -): - """ - Test detailed logging on HTTP errors. - """ - response = requests.Response() - response.status_code = 400 - response.reason = "Parameter verification failed." - response.raw = io.BytesIO( - b""" - { - "data": null, - "errors": { - "type": "value 'invalid_value' does not have a value in the enumeration 'vm, storage, node, sdn'" - } - } - """ - ) - - mock_request.return_value = response - mock_get_api_token.return_value = "api_token_value" - mock_get_url.return_value = "proxmox_url_value" - - with pytest.raises(SaltCloudSystemExit) as err: - proxmox._query("GET", "json/cluster/resources", {"type": "invalid_value"}) - - assert response.reason in str(err.value) - assert response.text in caplog.text - - -@patch(_fqn(proxmox._query)) -def test__get_vm_by_name(mock__query: MagicMock): +@patch(_fqn(proxmox._get_proxmox_client)) +def test__get_vm_by_name(mock__get_proxmox_client: MagicMock): """ Test that `_get_vm_by_name()` returns the first matching VM """ - mock__query.return_value = [ + mock__get_proxmox_client.return_value.get.return_value = [ {"vmid": 100, "name": "duplicate name vm"}, {"vmid": 200, "name": "duplicate name vm"}, ] result = proxmox._get_vm_by_name("duplicate name vm") + assert result["vmid"] == 100 -@patch(_fqn(proxmox._query)) -def test__get_vm_by_name_when_vm_not_found(mock__query: MagicMock): +@patch(_fqn(proxmox._get_proxmox_client)) +def test__get_vm_by_name_when_vm_not_found(mock__get_proxmox_client: MagicMock): """ Test that `_get_vm_by_name()` raises an error when no VM with given name exists """ - mock__query.return_value = [] + mock__get_proxmox_client.return_value.get.return_value = [] with pytest.raises(SaltCloudNotFound): proxmox._get_vm_by_name("my-proxmox-vm") -@patch(_fqn(proxmox._query)) -def test__get_vm_by_id(mock__query: MagicMock): +@patch(_fqn(proxmox._get_proxmox_client)) +def test__get_vm_by_id(mock__get_proxmox_client: MagicMock): """ Test that `_get_vm_by_id()` returns the matching VM """ - mock__query.return_value = [ + mock__get_proxmox_client.return_value.get.return_value = [ {"vmid": 100, "name": "my-proxmox-vm"}, ] result = proxmox._get_vm_by_id(100) + assert result["vmid"] == 100 -@patch(_fqn(proxmox._query)) -def test__get_vm_by_id_when_vm_not_found(mock__query: MagicMock): +@patch(_fqn(proxmox._get_proxmox_client)) +def test__get_vm_by_id_when_vm_not_found(mock__get_proxmox_client: MagicMock): """ Test that `_get_vm_by_id()` raises an error when no VM with given vmid """ - mock__query.return_value = [] + mock__get_proxmox_client.return_value.get.return_value = [] with pytest.raises(SaltCloudNotFound): proxmox._get_vm_by_id("my-proxmox-vm") +@patch(_fqn(proxmox._wait_for_task)) +@patch(_fqn(proxmox._get_vm_by_name)) +@patch(_fqn(proxmox._get_proxmox_client)) +def test__set_vm_status( + mock__get_proxmox_client: MagicMock, + mock__get_vm_by_name: MagicMock, + mock__wait_for_task: MagicMock, +): + """ + Test that _set_vm_status calls the endpoint with all arguments and waits for the task to finish + """ + mock__get_vm_by_name.return_value = { + "name": "my-vm", + "vmid": "101", + "type": "qemu", + "node": "node1", + } + + upid = "UPID:node1:0016BEC6:568EF5F4:669FB044:qmstop:101:user@pam!mytoken:" + mock__get_proxmox_client.return_value.post.return_value = upid + kwargs = {"forceStop": 1, "timeout": 30} + + proxmox._set_vm_status(name="my-vm", status="stop", kwargs=kwargs) + + mock__get_proxmox_client.return_value.post.assert_called_once_with( + "nodes/node1/qemu/101/status/stop", **kwargs + ) + mock__wait_for_task.assert_called_once_with(upid=upid) + + +@patch(_fqn(proxmox._wait_for_task)) +@patch(_fqn(proxmox._get_vm_by_name)) +@patch(_fqn(proxmox._get_proxmox_client)) +def test__set_vm_status_with_no_arguments( + mock__get_proxmox_client: MagicMock, + mock__get_vm_by_name: MagicMock, + mock__wait_for_task: MagicMock, +): + """ + Test that _set_vm_status calls the endpoint correctly with no arguments + """ + mock__get_vm_by_name.return_value = { + "name": "my-vm", + "vmid": "101", + "type": "qemu", + "node": "node1", + } + + upid = "UPID:node1:0016BEC6:568EF5F4:669FB044:qmstop:101:user@pam!mytoken:" + mock__get_proxmox_client.return_value.post.return_value = upid + + proxmox._set_vm_status(name="my-vm", status="stop") + + mock__get_proxmox_client.return_value.post.assert_called_once_with( + "nodes/node1/qemu/101/status/stop" + ) + + +@patch(_fqn(proxmox._get_proxmox_client)) +def test__wait_for_task(mock__get_proxmox_client: MagicMock): + response = {"status": "stopped", "exitstatus": "OK"} + + mock__get_proxmox_client.return_value.get.return_value = response + upid = "UPID:node1:0016BEC6:568EF5F4:669FB044:qmstart:101:user@pam!mytoken:" + + result = proxmox._wait_for_task(upid=upid, timeout=1) + + assert response == result + + +@patch(_fqn(proxmox._get_proxmox_client)) +def test__wait_for_task_when_timeout_reached(mock__get_proxmox_client: MagicMock): + response = {"status": "running"} + + mock__get_proxmox_client.return_value.get.return_value = response + upid = "UPID:node1:0016BEC6:568EF5F4:669FB044:qmstart:101:user@pam!mytoken:" + + with pytest.raises(SaltCloudExecutionTimeout): + proxmox._wait_for_task(upid=upid, timeout=1) + + +@patch(_fqn(proxmox._get_proxmox_client)) +def test__wait_for_task_when_task_failed(mock__get_proxmox_client: MagicMock): + response = {"status": "stopped", "exitstatus": "VM quit/powerdown failed - got timeout"} + + mock__get_proxmox_client.return_value.get.return_value = response + upid = "UPID:node1:0016BEC6:568EF5F4:669FB044:qmshutdown:101:user@pam!mytoken:" + + with pytest.raises(SaltCloudSystemExit): + proxmox._wait_for_task(upid=upid, timeout=1) + + def test__parse_ips_when_qemu_config(): """ Test that `_parse_ips()` handles QEMU configs correctly @@ -564,6 +810,7 @@ def test__parse_ips_when_qemu_config(): } private_ips, public_ips = proxmox._parse_ips(qemu_config, "qemu") + assert private_ips == ["192.168.1.10"] assert public_ips == ["200.200.200.200"] @@ -578,6 +825,7 @@ def test__parse_ips_when_lxc_config(): } private_ips, public_ips = proxmox._parse_ips(lxc_config, "lxc") + assert private_ips == ["192.168.1.10"] assert public_ips == ["200.200.200.200"] @@ -587,6 +835,7 @@ def test__parse_ips_when_missing_config(): Test that `_parse_ips()` handles missing IPs correctly """ private_ips, public_ips = proxmox._parse_ips({}, "lxc") + assert not private_ips assert not public_ips @@ -600,6 +849,7 @@ def test__parse_ips_when_invalid_config(): } private_ips, public_ips = proxmox._parse_ips(invalid_ip_config, "lxc") + assert not private_ips assert not public_ips @@ -609,6 +859,7 @@ def test__stringlist_to_dictionary(): Test that a valid stringlist returns a valid dict """ result = proxmox._stringlist_to_dictionary("foo=bar,some_key=some_value") + assert result == {"foo": "bar", "some_key": "some_value"} @@ -617,6 +868,7 @@ def test__stringlist_to_dictionary_when_empty(): Test that an empty stringlist returns an empty dict """ result = proxmox._stringlist_to_dictionary("") + assert result == dict() @@ -625,6 +877,7 @@ def test__stringlist_to_dictionary_when_containing_leading_or_trailing_spaces(): Test that spaces before and after "key=value" are removed """ result = proxmox._stringlist_to_dictionary("foo=bar, space_before=bar,space_after=bar ") + assert result == {"foo": "bar", "space_before": "bar", "space_after": "bar"} @@ -635,6 +888,7 @@ def test__stringlist_to_dictionary_when_containing_spaces(): result = proxmox._stringlist_to_dictionary( "foo=bar,internal key space=bar,space_in_value= internal value space" ) + assert result == { "foo": "bar", "internal key space": "bar",