Improve Proxmox volume handling (#8542)

* proxmox: basic linting

using black via trunk.io

* proxmox: refactor mount handling (#8407)

- make mount creation idempotent: Mounts created using the special syntax "<storage>:<size>" no longer create a new volume each time
- add new keys for easier mount creation & management

* proxmox: add changelog fragment

* proxmox(fix): fix occasional syntax error

* Update changelogs/fragments/8542-fix-proxmox-volume-handling.yml

Link to pull request

Co-authored-by: Felix Fontein <felix@fontein.de>

* Update documentation

- Fix options defined as values
- Document mutual exclusivity
- Fix option hierarchy
- Add version_added tag

* Revert "proxmox: basic linting"

This reverts commit ca7214f60e.

* proxmox: Fix documentation

* Fix list identifier in documentation

* pass volume options as dict instead of list

* Update plugins/modules/proxmox.py

Update documentation wording

Co-authored-by: Felix Fontein <felix@fontein.de>

* Update plugins/modules/proxmox.py

Update documentation wording

Co-authored-by: Felix Fontein <felix@fontein.de>

* proxmox: ensure values of `disk_volume` and `mount_volumes.*` dicts are strings

* proxmox(fix): correct indentation

* Apply suggestions from code review: punctuation

Add suggested punctuation to documentation

Co-authored-by: Alexei Znamensky <103110+russoz@users.noreply.github.com>

* Update plugins/modules/proxmox.py: vol_string building

Accept suggested review change

Co-authored-by: Alexei Znamensky <103110+russoz@users.noreply.github.com>

* proxmox: Use better string check and conversion

---------

Co-authored-by: Felix Fontein <felix@fontein.de>
Co-authored-by: Alexei Znamensky <103110+russoz@users.noreply.github.com>

changelogs/fragments/8542-fix-proxmox-volume-handling.yml

@@ -0,0 +1,5 @@
+bugfixes:
+  - proxmox - fix idempotency on creation of mount volumes using Proxmox' special ``<storage>:<size>`` syntax (https://github.com/ansible-collections/community.general/issues/8407, https://github.com/ansible-collections/community.general/pull/8542).
+minor_changes:
+  - proxmox - add ``disk_volume`` and ``mount_volumes`` keys for better readability (https://github.com/ansible-collections/community.general/pull/8542).
+  - proxmox - translate the old ``disk`` and ``mounts`` keys to the new handling internally (https://github.com/ansible-collections/community.general/pull/8542).

plugins/modules/proxmox.py

@@ -49,8 +49,44 @@ options:
         comma-delimited list C([volume=]<volume> [,acl=<1|0>] [,mountoptions=<opt[;opt...]>] [,quota=<1|0>]
         [,replicate=<1|0>] [,ro=<1|0>] [,shared=<1|0>] [,size=<DiskSize>])."
       - See U(https://pve.proxmox.com/wiki/Linux_Container) for a full description.
-      - Should not be used in conjunction with O(storage).
+      - This option is mutually exclusive with O(storage) and O(disk_volume).
     type: str
+  disk_volume:
+    description:
+      - Specify a hash/dictionary of the C(rootfs) disk.
+      - See U(https://pve.proxmox.com/wiki/Linux_Container#pct_mount_points) for a full description.
+      - This option is mutually exclusive with O(storage) and O(disk).
+    type: dict
+    version_added: 9.2.0
+    suboptions:
+      storage:
+        description:
+          - O(disk_volume.storage) is the storage identifier of the storage to use for the C(rootfs).
+          - Mutually exclusive with O(disk_volume.host_path).
+        type: str
+      volume:
+        description:
+          - O(disk_volume.volume) is the name of an existing volume.
+          - If not defined, the module will check if one exists. If not, a new volume will be created.
+          - If defined, the volume must exist under that name.
+          - Required only if O(disk_volume.storage) is defined and mutually exclusive with O(disk_volume.host_path).
+        type: str
+      size:
+        description:
+          - O(disk_volume.size) is the size of the storage to use.
+          - The size is given in GB.
+          - Required only if O(disk_volume.storage) is defined and mutually exclusive with O(disk_volume.host_path).
+        type: int
+      host_path:
+        description:
+          - O(disk_volume.host_path) defines a bind or device path on the PVE host to use for the C(rootfs).
+          - Mutually exclusive with O(disk_volume.storage), O(disk_volume.volume), and O(disk_volume.size).
+        type: path
+      options:
+        description:
+          - O(disk_volume.options) is a dict of extra options.
+          - The value of any given option must be a string, for example V("1").
+        type: dict
   cores:
     description:
       - Specify number of cores per socket.
@@ -89,7 +125,55 @@ options:
     version_added: 8.5.0
   mounts:
     description:
-      - specifies additional mounts (separate disks) for the container. As a hash/dictionary defining mount points
+      - Specifies additional mounts (separate disks) for the container. As a hash/dictionary defining mount points as strings.
+      - This Option is mutually exclusive with O(mount_volumes).
+    type: dict
+  mount_volumes:
+    description:
+      - Specify additional mounts (separate disks) for the container. As a hash/dictionary defining mount points.
+      - See U(https://pve.proxmox.com/wiki/Linux_Container#pct_mount_points) for a full description.
+      - This Option is mutually exclusive with O(mounts).
+    type: list
+    elements: dict
+    version_added: 9.2.0
+    suboptions:
+      id:
+        description:
+          - O(mount_volumes[].id) is the identifier of the mount point written as C(mp[n]).
+        type: str
+        required: true
+      storage:
+        description:
+          - O(mount_volumes[].storage) is the storage identifier of the storage to use.
+          - Mutually exclusive with O(mount_volumes[].host_path).
+        type: str
+      volume:
+        description:
+          - O(mount_volumes[].volume) is the name of an existing volume.
+          - If not defined, the module will check if one exists. If not, a new volume will be created.
+          - If defined, the volume must exist under that name.
+          - Required only if O(mount_volumes[].storage) is defined and mutually exclusive with O(mount_volumes[].host_path).
+        type: str
+      size:
+        description:
+          - O(mount_volumes[].size) is the size of the storage to use.
+          - The size is given in GB.
+          - Required only if O(mount_volumes[].storage) is defined and mutually exclusive with O(mount_volumes[].host_path).
+        type: int
+      host_path:
+        description:
+          - O(mount_volumes[].host_path) defines a bind or device path on the PVE host to use for the C(rootfs).
+          - Mutually exclusive with O(mount_volumes[].storage), O(mount_volumes[].volume), and O(mount_volumes[].size).
+        type: path
+      mountpoint:
+        description:
+          - O(mount_volumes[].mountpoint) is the mount point of the volume.
+        type: path
+        required: true
+      options:
+        description:
+          - O(mount_volumes[].options) is a dict of extra options.
+          - The value of any given option must be a string, for example V("1").
         type: dict
   ip_address:
     description:
@@ -101,8 +185,8 @@ options:
     type: bool
   storage:
     description:
-      - target storage
+      - Target storage.
-      - Should not be used in conjunction with O(disk).
+      - This Option is mutually exclusive with O(disk) and O(disk_volume).
     type: str
     default: 'local'
   ostype:
@@ -248,6 +332,20 @@ EXAMPLES = r'''
     ostemplate: 'local:vztmpl/ubuntu-14.04-x86_64.tar.gz'
     disk: 'local-lvm:20'
 
+- name: Create new container with minimal options specifying disk storage location and size via disk_volume
+  community.general.proxmox:
+    vmid: 100
+    node: uk-mc02
+    api_user: root@pam
+    api_password: 1q2w3e
+    api_host: node1
+    password: 123456
+    hostname: example.org
+    ostemplate: 'local:vztmpl/ubuntu-14.04-x86_64.tar.gz'
+    disk_volume:
+      storage: local
+      size: 20
+
 - name: Create new container with hookscript and description
   community.general.proxmox:
     vmid: 100
@@ -329,6 +427,22 @@ EXAMPLES = r'''
     ostemplate: 'local:vztmpl/ubuntu-14.04-x86_64.tar.gz'
     mounts: '{"mp0":"local:8,mp=/mnt/test/"}'
 
+- name: Create new container with minimal options defining a mount with 8GB using mount_volumes
+  community.general.proxmox:
+    vmid: 100
+    node: uk-mc02
+    api_user: root@pam
+    api_password: 1q2w3e
+    api_host: node1
+    password: 123456
+    hostname: example.org
+    ostemplate: 'local:vztmpl/ubuntu-14.04-x86_64.tar.gz'
+    mount_volumes:
+      - id: mp0
+        storage: local
+        size: 8
+        mountpoint: /mnt/test
+
 - name: Create new container with minimal options defining a cpu core limit
   community.general.proxmox:
     vmid: 100
@@ -476,7 +590,9 @@ import time
 from ansible_collections.community.general.plugins.module_utils.version import LooseVersion
 
 from ansible.module_utils.basic import AnsibleModule
-from ansible.module_utils.common.text.converters import to_native
+from ansible.module_utils.six import string_types
+from ansible.module_utils.common.text.converters import to_native, to_text
+
 
 from ansible_collections.community.general.plugins.module_utils.proxmox import (
     ansible_to_proxmox_bool, proxmox_auth_argument_spec, ProxmoxAnsible)
@@ -501,6 +617,124 @@ class ProxmoxLxcAnsible(ProxmoxAnsible):
                 msg="Updating configuration is only supported for LXC enabled proxmox clusters.",
             )
 
+        def parse_disk_string(disk_string):
+            # Example strings:
+            #  "acl=0,thin1:base-100-disk-1,size=8G"
+            #  "thin1:10,backup=0"
+            #  "local:20"
+            #  "volume=local-lvm:base-100-disk-1,size=20G"
+            #  "/mnt/bindmounts/shared,mp=/shared"
+            #  "volume=/dev/USB01,mp=/mnt/usb01"
+            args = disk_string.split(",")
+            # If the volume is not explicitly defined but implicit by only passing a key,
+            # add the "volume=" key prefix for ease of parsing.
+            args = ["volume=" + arg if "=" not in arg else arg for arg in args]
+            # Then create a dictionary from the arguments
+            disk_kwargs = dict(map(lambda item: item.split("="), args))
+
+            VOLUME_PATTERN = r"""(?x)
+              (?:(?P<storage>[\w\-.]+):
+                (?:(?P<size>\d+)|
+                (?P<volume>[^,\s]+))
+              )|
+              (?P<host_path>[^,\s]+)
+            """
+            # DISCLAIMER:
+            # There are two things called a "volume":
+            # 1. The "volume" key which describes the storage volume, device or directory to mount into the container.
+            # 2. The storage volume of a storage-backed mount point in the PVE storage sub system.
+            # In this section, we parse the "volume" key and check which type of mount point we are dealing with.
+            pattern = re.compile(VOLUME_PATTERN)
+            match_dict = pattern.match(disk_kwargs.pop("volume")).groupdict()
+            match_dict = {k: v for k, v in match_dict.items() if v is not None}
+
+            if "storage" in match_dict and "volume" in match_dict:
+                disk_kwargs["storage"] = match_dict["storage"]
+                disk_kwargs["volume"] = match_dict["volume"]
+            elif "storage" in match_dict and "size" in match_dict:
+                disk_kwargs["storage"] = match_dict["storage"]
+                disk_kwargs["size"] = match_dict["size"]
+            elif "host_path" in match_dict:
+                disk_kwargs["host_path"] = match_dict["host_path"]
+
+            # Pattern matching only available in Python 3.10+
+            # match match_dict:
+            #     case {"storage": storage, "volume": volume}:
+            #         disk_kwargs["storage"] = storage
+            #         disk_kwargs["volume"] = volume
+
+            #     case {"storage": storage, "size": size}:
+            #         disk_kwargs["storage"] = storage
+            #         disk_kwargs["size"] = size
+
+            #     case {"host_path": host_path}:
+            #         disk_kwargs["host_path"] = host_path
+
+            return disk_kwargs
+
+        def convert_mounts(mount_dict):
+            return_list = []
+            for mount_key, mount_value in mount_dict.items():
+                mount_config = parse_disk_string(mount_value)
+                return_list.append(dict(id=mount_key, **mount_config))
+
+            return return_list
+
+        def build_volume(
+            key,
+            storage=None,
+            volume=None,
+            host_path=None,
+            size=None,
+            mountpoint=None,
+            options=None,
+            **kwargs
+        ):
+            if size is not None and isinstance(size, str):
+                size = size.strip("G")
+            # 1. Handle volume checks/creation
+            # 1.1 Check if defined volume exists
+            if volume is not None:
+                storage_content = self.get_storage_content(node, storage, vmid=vmid)
+                vol_ids = [vol["volid"] for vol in storage_content]
+                volid = "{storage}:{volume}".format(storage=storage, volume=volume)
+                if volid not in vol_ids:
+                    self.module.fail_json(
+                        changed=False,
+                        msg="Storage {storage} does not contain volume {volume}".format(
+                            storage=storage,
+                            volume=volume,
+                        ),
+                    )
+                vol_string = "{storage}:{volume},size={size}G".format(
+                    storage=storage, volume=volume, size=size
+                )
+            # 1.2 If volume not defined (but storage is), check if it exists
+            elif storage is not None:
+                api_node = self.proxmox_api.nodes(
+                    node
+                )  # The node must exist, but not the LXC
+                try:
+                    vol = api_node.lxc(vmid).get("config").get(key)
+                    volume = parse_disk_string(vol).get("volume")
+                    vol_string = "{storage}:{volume},size={size}G".format(
+                        storage=storage, volume=volume, size=size
+                    )
+
+                # If not, we have proxmox create one using the special syntax
+                except Exception:
+                    vol_string = "{storage}:{size}".format(storage=storage, size=size)
+
+            # 1.3 If we have a host_path, we don't have storage, a volume, or a size
+            vol_string = ",".join(
+                ([] if host_path is None else [host_path]) +
+                ([] if mountpoint is None else ["mp={0}".format(mountpoint)]) +
+                ([] if options is None else [map("=".join, options.items())]) +
+                ([] if not kwargs else [map("=".join, kwargs.items())])
+            )
+
+            return {key: vol_string}
+
         # Version limited features
         minimum_version = {"tags": "6.1", "timezone": "6.3"}
         proxmox_node = self.proxmox_api.nodes(node)
@@ -518,22 +752,35 @@ class ProxmoxLxcAnsible(ProxmoxAnsible):
                 )
 
         # Remove all empty kwarg entries
-        kwargs = dict((k, v) for k, v in kwargs.items() if v is not None)
+        kwargs = dict((key, val) for key, val in kwargs.items() if val is not None)
 
         if cpus is not None:
             kwargs["cpulimit"] = cpus
         if disk is not None:
-            kwargs["rootfs"] = disk
+            kwargs["disk_volume"] = parse_disk_string(disk)
+        if "disk_volume" in kwargs:
+            if not all(isinstance(val, string_types) for val in kwargs["disk_volume"].values()):
+                self.module.warn("All disk_volume values must be strings. Converting non-string values to strings.")
+                kwargs["disk_volume"] = {key: to_text(val) for key, val in kwargs["disk_volume"].items()}
+            disk_dict = build_volume(key="rootfs", **kwargs.pop("disk_volume"))
+            kwargs.update(disk_dict)
         if memory is not None:
             kwargs["memory"] = memory
         if swap is not None:
             kwargs["swap"] = swap
         if "netif" in kwargs:
-            kwargs.update(kwargs["netif"])
+            kwargs.update(kwargs.pop("netif"))
-            del kwargs["netif"]
         if "mounts" in kwargs:
-            kwargs.update(kwargs["mounts"])
+            kwargs["mount_volumes"] = convert_mounts(kwargs.pop("mounts"))
-            del kwargs["mounts"]
+        if "mount_volumes" in kwargs:
+            mounts_list = kwargs.pop("mount_volumes")
+            for mount_config in mounts_list:
+                if not all(isinstance(val, string_types) for val in mount_config.values()):
+                    self.module.warn("All mount_volumes values must be strings. Converting non-string values to strings.")
+                    mount_config = {key: to_text(val) for key, val in mount_config.items()}
+                key = mount_config.pop("id")
+                mount_dict = build_volume(key=key, **mount_config)
+                kwargs.update(mount_dict)
         # LXC tags are expected to be valid and presented as a comma/semi-colon delimited string
         if "tags" in kwargs:
             re_tag = re.compile(r"^[a-z0-9_][a-z0-9_\-\+\.]*$")
@@ -735,12 +982,53 @@ def main():
         hostname=dict(),
         ostemplate=dict(),
         disk=dict(type='str'),
+        disk_volume=dict(
+            type="dict",
+            options=dict(
+                storage=dict(type="str"),
+                volume=dict(type="str"),
+                size=dict(type="int"),
+                host_path=dict(type="path"),
+                options=dict(type="dict"),
+            ),
+            required_together=[("storage", "size")],
+            required_by={
+                "volume": ("storage", "size"),
+            },
+            mutually_exclusive=[
+                ("host_path", "storage"),
+                ("host_path", "volume"),
+                ("host_path", "size"),
+            ],
+        ),
         cores=dict(type='int'),
         cpus=dict(type='int'),
         memory=dict(type='int'),
         swap=dict(type='int'),
         netif=dict(type='dict'),
         mounts=dict(type='dict'),
+        mount_volumes=dict(
+            type="list",
+            elements="dict",
+            options=dict(
+                id=(dict(type="str", required=True)),
+                storage=dict(type="str"),
+                volume=dict(type="str"),
+                size=dict(type="int"),
+                host_path=dict(type="path"),
+                mountpoint=dict(type="path", required=True),
+                options=dict(type="dict"),
+            ),
+            required_together=[("storage", "size")],
+            required_by={
+                "volume": ("storage", "size"),
+            },
+            mutually_exclusive=[
+                ("host_path", "storage"),
+                ("host_path", "volume"),
+                ("host_path", "size"),
+            ],
+        ),
         ip_address=dict(),
         ostype=dict(default='auto', choices=[
             'auto', 'debian', 'devuan', 'ubuntu', 'centos', 'fedora', 'opensuse', 'archlinux', 'alpine', 'gentoo', 'nixos', 'unmanaged'
@@ -776,11 +1064,17 @@ def main():
             # either clone a container or create a new one from a template file.
             ('state', 'present', ('clone', 'ostemplate', 'update'), True),
         ],
-        required_together=[
+        required_together=[("api_token_id", "api_token_secret")],
-            ('api_token_id', 'api_token_secret')
+        required_one_of=[("api_password", "api_token_id")],
+        mutually_exclusive=[
+            (
+                "clone",
+                "ostemplate",
+                "update",
+            ),  # Creating a new container is done either by cloning an existing one, or based on a template.
+            ("disk", "disk_volume", "storage"),
+            ("mounts", "mount_volumes"),
         ],
-        required_one_of=[('api_password', 'api_token_id')],
-        mutually_exclusive=[('clone', 'ostemplate', 'update')],  # Creating a new container is done either by cloning an existing one, or based on a template.
     )
 
     proxmox = ProxmoxLxcAnsible(module)
@@ -821,7 +1115,9 @@ def main():
                                               cores=module.params["cores"],
                                               hostname=module.params["hostname"],
                                               netif=module.params["netif"],
+                                              disk_volume=module.params["disk_volume"],
                                               mounts=module.params["mounts"],
+                                              mount_volumes=module.params["mount_volumes"],
                                               ip_address=module.params["ip_address"],
                                               onboot=ansible_to_proxmox_bool(module.params["onboot"]),
                                               cpuunits=module.params["cpuunits"],
@@ -876,7 +1172,9 @@ def main():
                                     hostname=module.params['hostname'],
                                     ostemplate=module.params['ostemplate'],
                                     netif=module.params['netif'],
+                                    disk_volume=module.params["disk_volume"],
                                     mounts=module.params['mounts'],
+                                    mount_volumes=module.params["mount_volumes"],
                                     ostype=module.params['ostype'],
                                     ip_address=module.params['ip_address'],
                                     onboot=ansible_to_proxmox_bool(module.params['onboot']),