[proxmox_kvm] Don't create VM if name is used without vmid (#6981)

* [proxmox_kvm] Don't create VM if name is used without vmid

* Add changelog and unit tests

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).

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)

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"
+        )