From 6cefde622cac8bf0d20203dd32e0e8fd96ba68fe Mon Sep 17 00:00:00 2001 From: JL Euler Date: Sun, 14 Jul 2024 12:07:05 +0200 Subject: [PATCH] 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 ":" 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 * Update documentation - Fix options defined as values - Document mutual exclusivity - Fix option hierarchy - Add version_added tag * Revert "proxmox: basic linting" This reverts commit ca7214f60e7b517fa681089ee55ab0a1fed44fd4. * 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 * Update plugins/modules/proxmox.py Update documentation wording Co-authored-by: Felix Fontein * 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 Co-authored-by: Alexei Znamensky <103110+russoz@users.noreply.github.com> --- .../8542-fix-proxmox-volume-handling.yml | 5 + plugins/modules/proxmox.py | 328 +++++++++++++++++- 2 files changed, 318 insertions(+), 15 deletions(-) create mode 100644 changelogs/fragments/8542-fix-proxmox-volume-handling.yml diff --git a/changelogs/fragments/8542-fix-proxmox-volume-handling.yml b/changelogs/fragments/8542-fix-proxmox-volume-handling.yml new file mode 100644 index 0000000000..9b982c0aeb --- /dev/null +++ b/changelogs/fragments/8542-fix-proxmox-volume-handling.yml @@ -0,0 +1,5 @@ +bugfixes: + - proxmox - fix idempotency on creation of mount volumes using Proxmox' special ``:`` 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). diff --git a/plugins/modules/proxmox.py b/plugins/modules/proxmox.py index 73afd952e2..67a67aec55 100644 --- a/plugins/modules/proxmox.py +++ b/plugins/modules/proxmox.py @@ -49,8 +49,44 @@ options: comma-delimited list C([volume=] [,acl=<1|0>] [,mountoptions=] [,quota=<1|0>] [,replicate=<1|0>] [,ro=<1|0>] [,shared=<1|0>] [,size=])." - 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,8 +125,56 @@ 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: - specifies the address the container will be assigned @@ -101,8 +185,8 @@ options: type: bool storage: description: - - target storage - - Should not be used in conjunction with O(disk). + - Target storage. + - 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[\w\-.]+): + (?:(?P\d+)| + (?P[^,\s]+)) + )| + (?P[^,\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"]) - del kwargs["netif"] + kwargs.update(kwargs.pop("netif")) if "mounts" in kwargs: - kwargs.update(kwargs["mounts"]) - del kwargs["mounts"] + kwargs["mount_volumes"] = convert_mounts(kwargs.pop("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=[ - ('api_token_id', 'api_token_secret') + required_together=[("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']),