feat: Add support for creating multiple partitions

This adds support for specifying size when creating a partition
volume and adding multiple partition volumes in one partition
pool.

Fixes: #437

Author: vojtechtrefny

README.md

@@ -166,10 +166,6 @@ variables:
   Valid values for `type`: `lvm`, `disk`, `partition` or `raid`.
   The default is determined according to the OS and release (currently `lvm`).
 
-  __NOTE__: Support for managing partition volumes is currently very limited,
-            the role allows creating only a single partition spanning the
-            entire disk.
-
 - `state`
 
   Valid values are `present` (default behavior) or `absent`. Volumes marked as
@@ -378,6 +374,14 @@ variables:
   Size for the thin pool. `thin_pool_size` format is intended to be human-readable,
   e.g.: "30g", "50GiB".
 
+- `part_type`
+
+  This specifies partition type for newly created partitions on MSDOS/MBR partition table.
+  Supported values include `primary`, `extended` and `logical`.
+  If not specified, first three partitions on the disk will be created as primary and fourth
+  one will be created as logical inside a newly created extended partition.
+  `part_type` is ignored on GPT partition table.
+
 ### `storage_safe_mode`
 
 When true (the default), an error will occur instead of automatically removing existing devices and/or formatting.

library/blivet.py

@@ -198,6 +198,9 @@
                     vdo_pool_size:
                         description: vdo_pool_size
                         type: str
+                    part_type:
+                        description: part_type
+                        type: str
     volumes:
         description: volumes
         type: list
@@ -377,6 +380,7 @@
 import os
 import traceback
 import inspect
+import re
 
 BLIVET_PACKAGE = None
 LIB_IMP_ERR3 = ""
@@ -392,7 +396,7 @@
     from blivet3.errors import RaidError
     from blivet3.flags import flags as blivet_flags
     from blivet3.formats import fslib, get_format
-    from blivet3.partitioning import do_partitioning
+    from blivet3.partitioning import do_partitioning, parted
     from blivet3.size import Size
     from blivet3.udev import trigger
     from blivet3.util import set_up_logging
@@ -409,7 +413,7 @@
         from blivet.errors import RaidError
         from blivet.flags import flags as blivet_flags
         from blivet.formats import fslib, get_format
-        from blivet.partitioning import do_partitioning
+        from blivet.partitioning import do_partitioning, parted
         from blivet.size import Size
         from blivet.udev import trigger
         from blivet.util import set_up_logging
@@ -974,20 +978,65 @@ class BlivetPartitionVolume(BlivetVolume):
     def _type_check(self):
         return self._device.raw_device.type == 'partition'
 
-    def _get_device_id(self):
-        device_id = None
-        if self._blivet_pool._disks[0].partitioned and len(self._blivet_pool._disks[0].children) == 1:
-            device_id = self._blivet_pool._disks[0].children[0].name
+    def _update_from_device(self, param_name):
+        """ Return True if param_name's value was retrieved from a looked-up device. """
+        if param_name == 'part_type':
+            if self._device.raw_device.is_primary:
+                self._volume['part_type'] = 'primary'
+            elif self._device.raw_device.is_extended:
+                self._volume['part_type'] = 'extended'
+            elif self._device.raw_device.is_logical:
+                self._volume['part_type'] = 'logical'
+        elif param_name == 'fs_type':
+            if self._device.raw_device.is_extended:
+                self._volume['fs_type'] = 'unformatted'
+            else:
+                return super(BlivetPartitionVolume, self)._update_from_device(param_name)
+        else:
+            return super(BlivetPartitionVolume, self)._update_from_device(param_name)
 
-        return device_id
+        return True
 
-    def _resize(self):
-        pass
+    def _get_part_by_partnum(self, partnum):
+        return next((p for p in self._blivet_pool._disks[0].children if (p.parted_partition and p.parted_partition.number == partnum)),
+                    None)
+
+    def _get_device_id(self):
+        name = self._volume['name']
+        if self._blivet_pool._disks and self._blivet_pool._disks[0].partitioned:
+            if name in (p.name for p in self._blivet_pool._disks[0].children):
+                # partition is specified by its name, e.g. "sda1"
+                return name
+            elif name and name.isdigit():
+                # partition is specified by its partition number, e.g. "1"
+                part = self._get_part_by_partnum(int(name))
+                if part:
+                    return os.path.basename(part.path)
+            elif name and name[-1].isdigit():
+                # this might be something like "test1", not really supported but we used it previously
+                # with the "single partition spanning whole disk" approach so lets just keep this
+                # supported for backwards compatibility
+                match = re.search(r'\d+$', name)
+                if match:
+                    part = self._get_part_by_partnum(int(match.group()))
+                    if part:
+                        return os.path.basename(part.path)
+        return None
 
     def _manage_cache(self):
         if self._volume['cached']:
             raise BlivetAnsibleError("caching is not supported for partition volumes")
 
+    def _get_part_weight(self):
+        # XXX make sure the newly created partitions are in right order. We use the partition
+        # number as weight, otherwise blivet would create the partitions in random order breaking
+        # the idempotency
+        name = self._volume['name']
+        match = re.search(r'\d+$', name)
+        if match:
+            return -int(match.group()) * 100
+        return 0
+
     def _create(self):
         if self._device:
             return
@@ -1000,13 +1049,41 @@ def _create(self):
         if parent is None:
             raise BlivetAnsibleError("failed to find pool '%s' for volume '%s'" % (self._blivet_pool['name'], self._volume['name']))
 
-        size = Size("256 MiB")
-        maxsize = None
-        if isinstance(self._volume['size'], str) and '%' in self._volume['size']:
-            maxsize = self._get_size()
+        size = self._get_size()
+        if size == Size(0):
+            # backward compatibility with the old "single partition spanning whole disk"
+            # approach -- Size 0 means to use the entire disk
+            size = Size("256 MiB")
+            grow = True
+        else:
+            grow = False
+
+        part_type = self._volume['part_type']
+        if part_type == "primary":
+            parted_type = parted.PARTITION_NORMAL
+        elif part_type == "extended":
+            parted_type = parted.PARTITION_EXTENDED
+        elif part_type == "logical":
+            parted_type = parted.PARTITION_LOGICAL
+        else:
+            parted_type = None
+        if part_type in ("extended", "logical") and parent.format.label_type != "msdos":
+            raise BlivetAnsibleError("extended and logical partitions can be created only on MSDOS disk")
+        if part_type == "extended":
+            if self._volume['encryption']:
+                raise BlivetAnsibleError("extended partitions cannot be encrypted")
+            if self._volume['mount_point']:
+                raise BlivetAnsibleError("extended partitions cannot be mounted")
+        if part_type == "extended":
+            fmt = None
+            self._volume['fs_type'] = 'unformatted'
+        else:
+            fmt = self._get_format()
 
         try:
-            device = self._blivet.new_partition(parents=[parent], size=size, maxsize=maxsize, grow=True, fmt=self._get_format())
+            device = self._blivet.new_partition(parents=[parent], size=size, grow=grow, fmt=fmt,
+                                                weight=self._get_part_weight(),
+                                                part_type=parted_type)
         except Exception:
             raise BlivetAnsibleError("failed set up volume '%s'" % self._volume['name'])
 
@@ -1708,8 +1785,12 @@ def _type_check(self):
         return self._device.partitionable
 
     def _look_up_device(self):
-        self._look_up_disks()
-        self._device = self._disks[0]
+        device = self._blivet.devicetree.resolve_device(self._pool['name'])
+        if device is not None:
+            self._device = device
+        else:
+            self._look_up_disks()
+            self._device = self._disks[0]
 
     def _create(self):
         if self._device.format.type != "disklabel" or \
@@ -2361,6 +2442,7 @@ def run_module():
              cache_size=dict(type='str'),
              compression=dict(type='bool'),
              deduplication=dict(type='bool'),
+             part_type=dict(type='str'),
              raid_disks=dict(type='list', elements='str', default=list()),
              raid_stripe_size=dict(type='str'),
              thin_pool_name=dict(type='str'),

tests/tests_create_multiple_partitions_dos.yml

@@ -0,0 +1,109 @@
+---
+- name: Test creating multiple partitions on MSDOS/MBR
+  hosts: all
+  become: true
+  vars:
+    storage_safe_mode: false
+    volume1_size: '2g'
+    volume2_size: '3g'
+    volume3_size: '1g'
+    storage_disklabel_type: 'msdos'
+
+  tasks:
+    - name: Run the role
+      include_role:
+        name: linux-system-roles.storage
+
+    - name: Mark tasks to be skipped
+      set_fact:
+        storage_skip_checks:
+          - blivet_available
+          - "{{ (lookup('env',
+                        'SYSTEM_ROLES_REMOVE_CLOUD_INIT') in ['', 'false']) |
+                ternary('packages_installed', '') }}"
+          - service_facts
+
+    - name: Get unused disks
+      include_tasks: get_unused_disk.yml
+      vars:
+        max_return: 1
+
+    - name: Try to create mounted extended partition (expect failure)
+      include_tasks: verify-role-failed.yml
+      vars:
+        __storage_failed_exception: extended partitions cannot be mounted
+        __storage_failed_params:
+          storage_safe_mode: false
+          storage_pools:
+            - name: "{{ unused_disks[0] }}"
+              type: partition
+              disks: "{{ unused_disks }}"
+              volumes:
+                - name: 1
+                  type: partition
+                  fs_type: ext4
+                  part_type: extended
+                  size: "{{ volume1_size }}"
+                  mount_point: /opt/test
+
+    - name: Create three partitions
+      include_role:
+        name: linux-system-roles.storage
+      vars:
+        storage_pools:
+          - name: "{{ unused_disks[0] }}"
+            type: partition
+            disks: "{{ unused_disks }}"
+            volumes:
+              - name: 1
+                type: partition
+                fs_type: ext4
+                size: "{{ volume1_size }}"
+              - name: 2
+                type: partition
+                part_type: extended
+                size: "{{ volume2_size }}"
+              - name: 5
+                type: partition
+                fs_type: ext4
+                part_type: logical
+                size: "{{ volume3_size }}"
+
+    - name: Verify role results
+      include_tasks: verify-role-results.yml
+
+    - name: Repeat the previous invocation to verify idempotence
+      include_role:
+        name: linux-system-roles.storage
+      vars:
+        storage_pools:
+          - name: "{{ unused_disks[0] }}"
+            type: partition
+            disks: "{{ unused_disks }}"
+            volumes:
+              - name: 1
+                type: partition
+                fs_type: ext4
+                size: "{{ volume1_size }}"
+              - name: 2
+                type: partition
+                part_type: extended
+                size: "{{ volume2_size }}"
+              - name: 5
+                type: partition
+                fs_type: ext4
+                part_type: logical
+                size: "{{ volume3_size }}"
+
+    - name: Verify role results - 2
+      include_tasks: verify-role-results.yml
+
+    - name: Remove all partitions
+      include_role:
+        name: linux-system-roles.storage
+      vars:
+        storage_pools:
+          - name: "{{ unused_disks[0] }}"
+            type: partition
+            disks: "{{ unused_disks }}"
+            state: absent