Master thin support size fix (#299)

japokorn · web-flow · commit d0e6be84c1b0 · 2022-11-01T11:21:47.000-06:00
* Fixed calculation of relative thinp sizes

- percent specified 'size' of thin pool volume is now properly
  calculated from size of parent thinpool

* Fixed size and percentage handling for thin pools

- percentage size thin volume now correctly references its parent device
  for size calculation
- percentage values are now accepted size for thin pool size
diff --git a/library/blivet.py b/library/blivet.py
@@ -679,7 +679,19 @@ def _get_device_id(self):
         return "%s-%s" % (self._blivet_pool._device.name, self._volume['name'])
 
     def _get_size(self):
-        size = super(BlivetLVMVolume, self)._get_size()
+
+        spec = self._volume['size']
+        if self._volume['thin'] and isinstance(spec, str) and '%' in spec:
+            try:
+                percentage = int(spec[:-1].strip())
+            except ValueError:
+                raise BlivetAnsibleError("invalid percentage '%s' size specified for thin volume '%s'" % (self._volume['size'], self._volume['name']))
+
+            parent = self._get_parent_thinpool_by_name(self._volume.get('thin_pool_name'))
+            size = parent.size * (percentage / 100.0)
+        else:
+            size = super(BlivetLVMVolume, self)._get_size()
+
         if isinstance(self._volume['size'], str) and '%' in self._volume['size']:
             size = self._blivet_pool._device.align(size, roundup=True)
         return size
@@ -1326,19 +1338,21 @@ def _manage_encryption(self, members):
 
     def _manage_thin_pools(self, pool_device):
 
-        def get_vol_size(volume):
-            if isinstance(volume, str) and '%' in volume:
+        def str_to_size(spec, hundredpercent=None):
+            # Convert given 'spec' string to Size. When input string can be in percent
+            # (e.g. '50%'), use the optional parameter (type Size) as a reference
+            if isinstance(spec, str) and ('%' in spec):
                 try:
-                    percentage = int(volume[:-1].strip())
+                    percentage = int(spec[:-1].strip())
                 except ValueError:
-                    raise BlivetAnsibleError("invalid percentage '%s' size specified for volume in pool '%s'" % (volume, pool_device.name))
+                    raise BlivetAnsibleError("invalid percentage '%s' size specified in pool '%s'" % (spec, pool_device.name))
 
-                size = pool_device.size * (percentage / 100.0)
+                size = Size(hundredpercent * (percentage / 100.0))
             else:
                 try:
-                    size = Size(volume)
+                    size = Size(spec)
                 except Exception:
-                    raise BlivetAnsibleError("invalid size specification for volume in pool '%s'" % pool_device.name)
+                    raise BlivetAnsibleError("invalid size specification '%s' in pool '%s'" % (spec, pool_device.name))
 
             return size
 
@@ -1360,35 +1374,39 @@ def get_vol_size(volume):
         auto_size_dev_count = 0
         reserved_space = Size(0)
 
+        # Thin pool will take 20% of VG space as a safety spare. At least 1GiB, at most 100GiB
+        thin_meta_space = DEFAULT_THPOOL_RESERVE.percent * (pool_device.size * 0.01)
+        if thin_meta_space < DEFAULT_THPOOL_RESERVE.min:
+            thin_meta_space = DEFAULT_THPOOL_RESERVE.min
+        elif thin_meta_space > DEFAULT_THPOOL_RESERVE.max:
+            thin_meta_space = DEFAULT_THPOOL_RESERVE.max
+
+        available_space = pool_device.size - thin_meta_space
+
         for volume in self._pool.get('volumes'):
             if not volume['state']:
                 continue
 
             if volume['thin']:
                 thin_name = volume.get('thin_pool_name')
                 thin_size = volume.get('thin_pool_size')
+                if thin_size is not None:
+                    thin_size = str_to_size(thin_size, available_space)
+
                 if thin_name not in [t['name'] for t in thinlvs_to_create] + [d['name'] for d in existing_thinlvs]:
                     thinlvs_to_create.append({'name': thin_name, 'size': thin_size})
-                    if thin_size is None:
-                        auto_size_dev_count += 1
-                    else:
-                        reserved_space += get_vol_size(thin_size)
+
+                if thin_size is None:
+                    auto_size_dev_count += 1
+                else:
+                    reserved_space += thin_size
             else:
                 # regular LV just take its size
                 vol_size = volume.get('size')
                 if vol_size is None:
                     auto_size_dev_count += 1
                 else:
-                    reserved_space += get_vol_size(vol_size)
-
-        # Thin pool will take 20% of VG space as a safety spare. At least 1GiB, at most 100GiB
-        thin_meta_space = DEFAULT_THPOOL_RESERVE.percent * (pool_device.size * 0.01)
-        if thin_meta_space < DEFAULT_THPOOL_RESERVE.min:
-            thin_meta_space = DEFAULT_THPOOL_RESERVE.min
-        elif thin_meta_space > DEFAULT_THPOOL_RESERVE.max:
-            thin_meta_space = DEFAULT_THPOOL_RESERVE.max
-
-        available_space = pool_device.size - thin_meta_space
+                    reserved_space += str_to_size(vol_size, pool_device.size)
 
         if auto_size_dev_count > 0:
             calculated_thinlv_size = available_space / auto_size_dev_count
@@ -1398,7 +1416,7 @@ def get_vol_size(volume):
             if thinlv['size'] is None:
                 tlv_size = Size(calculated_thinlv_size)
             else:
-                tlv_size = Size(thinlv['size'])
+                tlv_size = thinlv['size']
 
             thinlv_params = dict(thin_pool=True, size=tlv_size, parents=[pool_device])
 
diff --git a/tests/test-verify-volume-size.yml b/tests/test-verify-volume-size.yml
@@ -20,6 +20,12 @@
 - debug:
     var: storage_test_expected_size
 
+- name: Get the size of parent/pool device
+  bsize:
+    size: "{{ ansible_lvm.vgs[storage_test_pool.name].size_g + 'G' }}"
+  register: storage_test_pool_size
+  when: _storage_test_volume_present and storage_test_volume.type == "lvm"
+
 - name: Convert percentage-based size to normal size as needed
   block:
     - debug:
@@ -28,11 +34,6 @@
     - debug:
         var: storage_test_blkinfo
 
-    - name: Get the size of parent/pool device
-      bsize:
-        size: "{{ ansible_lvm.vgs[storage_test_pool.name].size_g + 'G' }}"
-      register: storage_test_pool_size
-
     - debug:
         var: storage_test_pool_size
 
@@ -41,6 +42,77 @@
         storage_test_expected_size: "{{ (storage_test_pool_size.bytes * ((storage_test_volume.size[:-1]|int)/100.0)) }}"
   when: _storage_test_volume_present and storage_test_volume.type == "lvm" and "%" in storage_test_volume.size|string
 
+- name: Process thin pool sizes when applicable
+  block:
+    - name: Default thin pool reserved space values
+      set_fact:
+        _storage_test_default_thpool_reserve_percent: "20"
+
+    - name: Default minimal thin pool reserved space size
+      bsize:
+        size: "1G"
+      register: _storage_test_default_thpool_reserve_min
+
+    - name: Default maximal thin pool reserved space size
+      bsize:
+        size: "100G"
+      register: _storage_test_default_thpool_reserve_max
+
+    - name: Calculate maximum usable space in thin pool
+      bsize:
+        size: "{{ storage_test_pool_size.bytes * (1 - (_storage_test_default_thpool_reserve_percent|int)/100.0) }}"
+      register: _storage_test_max_thin_pool_size
+
+    - name: Apply upper size limit to max usable thin pool space
+      set_fact:
+        _storage_test_max_thin_pool_size: _storage_test_default_thpool_reserve_max
+      when: storage_test_pool_size.bytes - _storage_test_max_thin_pool_size.bytes > _storage_test_default_thpool_reserve_max.bytes
+
+    - name: Apply lower size limit to max usable thin pool space
+      set_fact:
+        _storage_test_max_thin_pool_size: _storage_test_default_thpool_reserve_min
+      when: storage_test_pool_size.bytes - _storage_test_max_thin_pool_size.bytes < _storage_test_default_thpool_reserve_min.bytes
+
+    - debug:
+        var: storage_test_volume.thin_pool_size
+
+    - debug:
+        var: storage_test_volume.size
+
+    - name: Establish base value for expected thin pool size
+      set_fact:
+        storage_test_expected_thin_pool_size: "{{ storage_test_pool_size.bytes }}"
+      when:
+        - storage_test_volume.thin_pool_size is not none
+        - true and '%' not in storage_test_volume.thin_pool_size  # workaround - starting condition with '%' leads to a syntax error
+
+    - name: Calculate the expected size based on pool size and percentage value
+      set_fact:
+        storage_test_expected_thin_pool_size: "{{ (_storage_test_max_thin_pool_size.bytes * ((storage_test_volume.thin_pool_size[:-1]|int)/100.0)) }}"
+      when:
+        - storage_test_volume.thin_pool_size is not none
+        - true and '%' in storage_test_volume.thin_pool_size|string  # workaround - starting condition with '%' leads to a syntax error
+
+    - name: Establish base value for expected thin pool volume size
+      set_fact:
+        storage_test_expected_tp_volume_size: "{{ storage_test_expected_size }}"
+
+    - name: Calculate the expected thin pool volume size based on percentage value
+      set_fact:
+        storage_test_expected_tp_volume_size: "{{ ((storage_test_expected_thin_pool_size|int) * ((storage_test_volume.size[:-1]|int)/100.0)) }}"
+      when:
+        - true and '%' in storage_test_volume.size|string  # workaround - starting condition with '%' leads to a syn:tax error
+
+    - name: Replace expected volume size with calculated value
+      set_fact:
+        storage_test_expected_size: "{{ storage_test_expected_tp_volume_size }}"
+      when:
+        - storage_test_volume.thin_pool_size is not none
+
+  when:
+    - storage_test_volume.thin
+    - _storage_test_volume_present
+
 - debug:
     var: storage_test_actual_size
 
diff --git a/tests/tests_create_thinp_then_remove.yml b/tests/tests_create_thinp_then_remove.yml
@@ -6,6 +6,7 @@
     storage_use_partitions: true
     mount_location1: '/opt/test1'
     mount_location2: '/opt/test2'
+    mount_location3: '/opt/test3'
     volume1_size: '3g'
     volume2_size: '4g'
     fs_after: "{{ (ansible_distribution == 'RedHat' and ansible_distribution_major_version == '6') | ternary('ext4', 'xfs') }}"
@@ -18,7 +19,7 @@
       vars:
         max_return: 3
 
-    - name: Create a thinpool device
+    - name: create a thinpool device
       include_role:
         name: linux-system-roles.storage
       vars:
@@ -123,3 +124,41 @@
                 mount_point: "{{ mount_location1 }}"
 
     - include_tasks: verify-role-results.yml
+
+    - name: create a thinpool device using percentages
+      include_role:
+        name: linux-system-roles.storage
+      vars:
+        storage_pools:
+          - name: vg1
+            disks: "{{ unused_disks }}"
+            type: lvm
+            state: present
+            volumes:
+              - name: lv1
+                thin_pool_name: tpool1
+                thin_pool_size: '100%'
+                size: "100%"
+                thin: true
+                mount_point: "{{ mount_location1 }}"
+
+    - include_tasks: verify-role-results.yml
+
+    - name: Cleanup
+      include_role:
+        name: linux-system-roles.storage
+      vars:
+        storage_pools:
+          - name: vg1
+            disks: "{{ unused_disks }}"
+            type: lvm
+            state: absent
+            volumes:
+              - name: lv1
+                thin_pool_name: tpool1
+                thin_pool_size: '10g'
+                size: "{{ volume1_size }}"
+                thin: true
+                mount_point: "{{ mount_location1 }}"
+
+    - include_tasks: verify-role-results.yml
diff --git a/tests/tests_resize.yml b/tests/tests_resize.yml
@@ -162,7 +162,7 @@
         - name: Verify the output
           assert:
             that: "blivet_output.failed and
-                   blivet_output.msg|regex_search('invalid size.+for volume') and
+                   blivet_output.msg|regex_search('invalid.+size') and
                    not blivet_output.changed"
             msg: "Unexpected behavior w/ invalid volume size"
 
@@ -195,7 +195,7 @@
         - name: Verify the output
           assert:
             that: "blivet_output.failed and
-                   blivet_output.msg|regex_search('invalid size.+for volume') and
+                   blivet_output.msg|regex_search('invalid.+size') and
                    not blivet_output.changed"
             msg: "Unexpected behavior w/ invalid volume size"
 
