diff --git a/changelogs/fragments/6981-proxmox-fix-vm-creation-when-only-name-provided.yml b/changelogs/fragments/6981-proxmox-fix-vm-creation-when-only-name-provided.yml new file mode 100644 index 0000000000..0dbd617734 --- /dev/null +++ b/changelogs/fragments/6981-proxmox-fix-vm-creation-when-only-name-provided.yml @@ -0,0 +1,2 @@ +bugfixes: + - proxmox_kvm - when ``name`` option is provided without ``vmid`` and VM with that name already exists then no new VM will be created (https://github.com/ansible-collections/community.general/issues/6911, https://github.com/ansible-collections/community.general/pull/6981). diff --git a/plugins/modules/proxmox_kvm.py b/plugins/modules/proxmox_kvm.py index 238191f6a1..45b042316a 100644 --- a/plugins/modules/proxmox_kvm.py +++ b/plugins/modules/proxmox_kvm.py @@ -287,8 +287,9 @@ options: type: int name: description: - - Specifies the VM name. Only used on the configuration web interface. + - Specifies the VM name. Name could be non-unique across the cluster. - Required only for O(state=present). + - With O(state=present) if O(vmid) not provided and VM with name exists in the cluster then no changes will be made. type: str nameservers: description: @@ -1289,10 +1290,14 @@ def main(): # the cloned vm name or retrieve the next free VM id from ProxmoxAPI. if not vmid: if state == 'present' and not update and not clone and not delete and not revert and not migrate: - try: - vmid = proxmox.get_nextvmid() - except Exception: - module.fail_json(msg="Can't get the next vmid for VM {0} automatically. Ensure your cluster state is good".format(name)) + existing_vmid = proxmox.get_vmid(name, ignore_missing=True) + if existing_vmid: + vmid = existing_vmid + else: + try: + vmid = proxmox.get_nextvmid() + except Exception: + module.fail_json(msg="Can't get the next vmid for VM {0} automatically. Ensure your cluster state is good".format(name)) else: clone_target = clone or name vmid = proxmox.get_vmid(clone_target, ignore_missing=True) diff --git a/tests/unit/plugins/modules/test_proxmox_kvm.py b/tests/unit/plugins/modules/test_proxmox_kvm.py index c99cfa37a7..4e2cf032c1 100644 --- a/tests/unit/plugins/modules/test_proxmox_kvm.py +++ b/tests/unit/plugins/modules/test_proxmox_kvm.py @@ -12,14 +12,17 @@ import sys import pytest -proxmoxer = pytest.importorskip('proxmoxer') +proxmoxer = pytest.importorskip("proxmoxer") mandatory_py_version = pytest.mark.skipif( sys.version_info < (2, 7), - reason='The proxmoxer dependency requires python2.7 or higher' + reason="The proxmoxer dependency requires python2.7 or higher", ) from ansible_collections.community.general.plugins.modules import proxmox_kvm -from ansible_collections.community.general.tests.unit.compat.mock import patch +from ansible_collections.community.general.tests.unit.compat.mock import ( + patch, + DEFAULT, +) from ansible_collections.community.general.tests.unit.plugins.modules.utils import ( AnsibleExitJson, AnsibleFailJson, @@ -36,10 +39,19 @@ class TestProxmoxKvmModule(ModuleTestCase): self.module = proxmox_kvm self.connect_mock = patch( "ansible_collections.community.general.plugins.module_utils.proxmox.ProxmoxAnsible._connect" - ) - self.connect_mock.start() + ).start() + self.get_node_mock = patch.object( + proxmox_utils.ProxmoxAnsible, "get_node" + ).start() + self.get_vm_mock = patch.object(proxmox_utils.ProxmoxAnsible, "get_vm").start() + self.create_vm_mock = patch.object( + proxmox_kvm.ProxmoxKvmAnsible, "create_vm" + ).start() def tearDown(self): + self.create_vm_mock.stop() + self.get_vm_mock.stop() + self.get_node_mock.stop() self.connect_mock.stop() super(TestProxmoxKvmModule, self).tearDown() @@ -58,18 +70,16 @@ class TestProxmoxKvmModule(ModuleTestCase): "node": "pve", } ) - with patch.object(proxmox_utils.ProxmoxAnsible, "get_vm") as get_vm_mock: - get_vm_mock.return_value = [{"vmid": "100"}] - with pytest.raises(AnsibleExitJson) as exc_info: - self.module.main() + self.get_vm_mock.return_value = [{"vmid": "100"}] + with pytest.raises(AnsibleExitJson) as exc_info: + self.module.main() - assert get_vm_mock.call_count == 1 - result = exc_info.value.args[0] - assert result["changed"] is False - assert result["msg"] == "VM with vmid <100> already exists" + assert self.get_vm_mock.call_count == 1 + result = exc_info.value.args[0] + assert result["changed"] is False + assert result["msg"] == "VM with vmid <100> already exists" - @patch.object(proxmox_kvm.ProxmoxKvmAnsible, "create_vm") - def test_vm_created_when_vmid_not_exist_but_name_already_exist(self, create_vm_mock): + def test_vm_created_when_vmid_not_exist_but_name_already_exist(self): set_module_args( { "api_host": "host", @@ -80,23 +90,79 @@ class TestProxmoxKvmModule(ModuleTestCase): "node": "pve", } ) - with patch.object(proxmox_utils.ProxmoxAnsible, "get_vm") as get_vm_mock: - with patch.object(proxmox_utils.ProxmoxAnsible, "get_node") as get_node_mock: - get_vm_mock.return_value = None - get_node_mock.return_value = {"node": "pve", "status": "online"} - with pytest.raises(AnsibleExitJson) as exc_info: - self.module.main() + self.get_vm_mock.return_value = None + with pytest.raises(AnsibleExitJson) as exc_info: + self.module.main() - assert get_vm_mock.call_count == 1 - assert get_node_mock.call_count == 1 - result = exc_info.value.args[0] - assert result["changed"] is True - assert result["msg"] == "VM existing.vm.local with vmid 100 deployed" + assert self.get_vm_mock.call_count == 1 + assert self.get_node_mock.call_count == 1 + result = exc_info.value.args[0] + assert result["changed"] is True + assert result["msg"] == "VM existing.vm.local with vmid 100 deployed" + + def test_vm_not_created_when_name_already_exist_and_vmid_not_set(self): + set_module_args( + { + "api_host": "host", + "api_user": "user", + "api_password": "password", + "name": "existing.vm.local", + "node": "pve", + } + ) + with patch.object(proxmox_utils.ProxmoxAnsible, "get_vmid") as get_vmid_mock: + get_vmid_mock.return_value = { + "vmid": 100, + "name": "existing.vm.local", + } + with pytest.raises(AnsibleExitJson) as exc_info: + self.module.main() + + assert get_vmid_mock.call_count == 1 + result = exc_info.value.args[0] + assert result["changed"] is False + + def test_vm_created_when_name_doesnt_exist_and_vmid_not_set(self): + set_module_args( + { + "api_host": "host", + "api_user": "user", + "api_password": "password", + "name": "existing.vm.local", + "node": "pve", + } + ) + self.get_vm_mock.return_value = None + with patch.multiple( + proxmox_utils.ProxmoxAnsible, get_vmid=DEFAULT, get_nextvmid=DEFAULT + ) as utils_mock: + utils_mock["get_vmid"].return_value = None + utils_mock["get_nextvmid"].return_value = 101 + with pytest.raises(AnsibleExitJson) as exc_info: + self.module.main() + + assert utils_mock["get_vmid"].call_count == 1 + assert utils_mock["get_nextvmid"].call_count == 1 + result = exc_info.value.args[0] + assert result["changed"] is True + assert result["msg"] == "VM existing.vm.local with vmid 101 deployed" def test_parse_mac(self): - assert proxmox_kvm.parse_mac("virtio=00:11:22:AA:BB:CC,bridge=vmbr0,firewall=1") == "00:11:22:AA:BB:CC" + assert ( + proxmox_kvm.parse_mac("virtio=00:11:22:AA:BB:CC,bridge=vmbr0,firewall=1") + == "00:11:22:AA:BB:CC" + ) def test_parse_dev(self): - assert proxmox_kvm.parse_dev("local-lvm:vm-1000-disk-0,format=qcow2") == "local-lvm:vm-1000-disk-0" - assert proxmox_kvm.parse_dev("local-lvm:vm-101-disk-1,size=8G") == "local-lvm:vm-101-disk-1" - assert proxmox_kvm.parse_dev("local-zfs:vm-1001-disk-0") == "local-zfs:vm-1001-disk-0" + assert ( + proxmox_kvm.parse_dev("local-lvm:vm-1000-disk-0,format=qcow2") + == "local-lvm:vm-1000-disk-0" + ) + assert ( + proxmox_kvm.parse_dev("local-lvm:vm-101-disk-1,size=8G") + == "local-lvm:vm-101-disk-1" + ) + assert ( + proxmox_kvm.parse_dev("local-zfs:vm-1001-disk-0") + == "local-zfs:vm-1001-disk-0" + )