[coreycb,r=james-page] Add option to remove missing physical volumes from volume group.

config.yaml

@@ -27,12 +27,16 @@ options:
     description: |
       The block devices on which to create LVM volume group.
 
-      May also be set to None for deployments that will not need local
+      May be set to None for deployments that will not need local
       storage (eg, Ceph/RBD-backed volumes).
 
       This can also be a space delimited list of block devices to attempt
       to use in the cinder LVM volume group - each block device detected
       will be added to the available physical volumes in the volume group.
+
+      May be set to the path and size of a local file
+      (/path/to/file.img|$sizeG), which will be created and used as a
+      loopback device (for testing only).  $sizeG defaults to 5G
   ceph-osd-replication-count:
     default: 3
     type: int
@@ -51,8 +55,14 @@ options:
     default: "false"
     type: string
     description: |
-      If true, charm will attempt to overwrite block devices containin
+      If true, charm will attempt to overwrite block devices containing
       previous filesystems or LVM, assuming it is not in use.
+  remove-missing:
+    default: False
+    type: boolean
+    description: |
+      If True, charm will attempt to remove missing physical volumes from
+      volume group, if logical volumes are not allocated on them.
   database-user:
     default: cinder
     type: string

hooks/cinder_hooks.py

@@ -105,7 +105,8 @@ def config_changed():
         block_devices = conf['block-device'].split()
         configure_lvm_storage(block_devices,
                               conf['volume-group'],
-                              conf['overwrite'] in ['true', 'True', True])
+                              conf['overwrite'] in ['true', 'True', True],
+                              conf['remove-missing'])
 
     if openstack_upgrade_available('cinder-common'):
         do_openstack_upgrade(configs=CONFIGS)

hooks/cinder_utils.py

@@ -267,9 +267,19 @@ def services():
     return list(set(_services))
 
 
+def reduce_lvm_volume_group_missing(volume_group):
+    '''
+    Remove all missing physical volumes from the volume group, if there
+    are no logical volumes allocated on them.
+
+    :param volume_group: str: Name of volume group to reduce.
+    '''
+    subprocess.check_call(['vgreduce', '--removemissing', volume_group])
+
+
 def extend_lvm_volume_group(volume_group, block_device):
     '''
-    Extend and LVM volume group onto a given block device.
+    Extend an LVM volume group onto a given block device.
 
     Assumes block device has already been initialized as an LVM PV.
 
@@ -279,13 +289,16 @@ def extend_lvm_volume_group(volume_group, block_device):
     subprocess.check_call(['vgextend', volume_group, block_device])
 
 
-def configure_lvm_storage(block_devices, volume_group, overwrite=False):
+def configure_lvm_storage(block_devices, volume_group, overwrite=False,
+                          remove_missing=False):
     ''' Configure LVM storage on the list of block devices provided
 
     :param block_devices: list: List of whitelisted block devices to detect
                                 and use if found
     :param overwrite: bool: Scrub any existing block data if block device is
                             not already in-use
+    :param remove_missing: bool: Remove missing physical volumes from volume
+                           group if logical volume not allocated on them
     '''
     devices = []
     for block_device in block_devices:
@@ -320,6 +333,10 @@ def configure_lvm_storage(block_devices, volume_group, overwrite=False):
         create_lvm_volume_group(volume_group, new_devices[0])
         new_devices.remove(new_devices[0])
 
+    # Remove missing physical volumes from volume group
+    if remove_missing:
+        reduce_lvm_volume_group_missing(volume_group)
+
     if len(new_devices) > 0:
         # Extend the volume group as required
         for new_device in new_devices:

unit_tests/test_cinder_hooks.py

@@ -115,7 +115,7 @@ class TestChangedHooks(CharmTestCase):
         self.assertTrue(conf_https.called)
         self.configure_lvm_storage.assert_called_with(['sdb'],
                                                       'cinder-volumes',
-                                                      False)
+                                                      False, False)
 
     @patch.object(hooks, 'configure_https')
     def test_config_changed_block_devices(self, conf_https):
@@ -124,13 +124,14 @@ class TestChangedHooks(CharmTestCase):
         self.test_config.set('block-device', 'sdb /dev/sdc sde')
         self.test_config.set('volume-group', 'cinder-new')
         self.test_config.set('overwrite', 'True')
+        self.test_config.set('remove-missing', True)
         hooks.hooks.execute(['hooks/config-changed'])
         self.assertTrue(self.CONFIGS.write_all.called)
         self.assertTrue(conf_https.called)
         self.configure_lvm_storage.assert_called_with(
             ['sdb', '/dev/sdc', 'sde'],
             'cinder-new',
-            True)
+            True, True)
 
     @patch.object(hooks, 'configure_https')
     def test_config_changed_upgrade_available(self, conf_https):

unit_tests/test_cinder_utils.py

@@ -211,11 +211,13 @@ class TestCinderUtils(CharmTestCase):
                         ('/mnt/loop0', cinder_utils.DEFAULT_LOOPBACK_SIZE))
 
     @patch.object(cinder_utils, 'clean_storage')
+    @patch.object(cinder_utils, 'reduce_lvm_volume_group_missing')
     @patch.object(cinder_utils, 'extend_lvm_volume_group')
-    def test_configure_lvm_storage(self, extend_lvm, clean_storage):
+    def test_configure_lvm_storage(self, extend_lvm, reduce_lvm,
+                                   clean_storage):
         devices = ['/dev/vdb', '/dev/vdc']
         self.is_lvm_physical_volume.return_value = False
-        cinder_utils.configure_lvm_storage(devices, 'test', True)
+        cinder_utils.configure_lvm_storage(devices, 'test', True, True)
         clean_storage.assert_has_calls(
             [call('/dev/vdb'),
              call('/dev/vdc')]
@@ -225,24 +227,29 @@ class TestCinderUtils(CharmTestCase):
              call('/dev/vdc')]
         )
         self.create_lvm_volume_group.assert_called_with('test', '/dev/vdb')
+        reduce_lvm.assert_called_with('test')
         extend_lvm.assert_called_with('test', '/dev/vdc')
 
     @patch.object(cinder_utils, 'clean_storage')
+    @patch.object(cinder_utils, 'reduce_lvm_volume_group_missing')
     @patch.object(cinder_utils, 'extend_lvm_volume_group')
-    def test_configure_lvm_storage_loopback(self, extend_lvm, clean_storage):
+    def test_configure_lvm_storage_loopback(self, extend_lvm, reduce_lvm,
+                                            clean_storage):
         devices = ['/mnt/loop0|10']
         self.ensure_loopback_device.return_value = '/dev/loop0'
         self.is_lvm_physical_volume.return_value = False
-        cinder_utils.configure_lvm_storage(devices, 'test', True)
+        cinder_utils.configure_lvm_storage(devices, 'test', True, True)
         clean_storage.assert_called_with('/dev/loop0')
         self.ensure_loopback_device.assert_called_with('/mnt/loop0', '10')
         self.create_lvm_physical_volume.assert_called_with('/dev/loop0')
         self.create_lvm_volume_group.assert_called_with('test', '/dev/loop0')
+        reduce_lvm.assert_called_with('test')
         self.assertFalse(extend_lvm.called)
 
     @patch.object(cinder_utils, 'clean_storage')
+    @patch.object(cinder_utils, 'reduce_lvm_volume_group_missing')
     @patch.object(cinder_utils, 'extend_lvm_volume_group')
-    def test_configure_lvm_storage_existing_vg(self, extend_lvm,
+    def test_configure_lvm_storage_existing_vg(self, extend_lvm, reduce_lvm,
                                                clean_storage):
         def pv_lookup(device):
             devices = {
@@ -260,19 +267,21 @@ class TestCinderUtils(CharmTestCase):
         devices = ['/dev/vdb', '/dev/vdc']
         self.is_lvm_physical_volume.side_effect = pv_lookup
         self.list_lvm_volume_group.side_effect = vg_lookup
-        cinder_utils.configure_lvm_storage(devices, 'test', True)
+        cinder_utils.configure_lvm_storage(devices, 'test', True, True)
         clean_storage.assert_has_calls(
             [call('/dev/vdc')]
         )
         self.create_lvm_physical_volume.assert_has_calls(
             [call('/dev/vdc')]
         )
+        reduce_lvm.assert_called_with('test')
         extend_lvm.assert_called_with('test', '/dev/vdc')
         self.assertFalse(self.create_lvm_volume_group.called)
 
     @patch.object(cinder_utils, 'clean_storage')
+    @patch.object(cinder_utils, 'reduce_lvm_volume_group_missing')
     @patch.object(cinder_utils, 'extend_lvm_volume_group')
-    def test_configure_lvm_storage_different_vg(self, extend_lvm,
+    def test_configure_lvm_storage_different_vg(self, extend_lvm, reduce_lvm,
                                                 clean_storage):
         def pv_lookup(device):
             devices = {
@@ -290,15 +299,18 @@ class TestCinderUtils(CharmTestCase):
         devices = ['/dev/vdb', '/dev/vdc']
         self.is_lvm_physical_volume.side_effect = pv_lookup
         self.list_lvm_volume_group.side_effect = vg_lookup
-        cinder_utils.configure_lvm_storage(devices, 'test', True)
+        cinder_utils.configure_lvm_storage(devices, 'test', True, True)
         clean_storage.assert_called_with('/dev/vdc')
         self.create_lvm_physical_volume.assert_called_with('/dev/vdc')
+        reduce_lvm.assert_called_with('test')
         extend_lvm.assert_called_with('test', '/dev/vdc')
         self.assertFalse(self.create_lvm_volume_group.called)
 
     @patch.object(cinder_utils, 'clean_storage')
+    @patch.object(cinder_utils, 'reduce_lvm_volume_group_missing')
     @patch.object(cinder_utils, 'extend_lvm_volume_group')
     def test_configure_lvm_storage_different_vg_ignore(self, extend_lvm,
+                                                       reduce_lvm,
                                                        clean_storage):
         def pv_lookup(device):
             devices = {
@@ -316,12 +328,18 @@ class TestCinderUtils(CharmTestCase):
         devices = ['/dev/vdb', '/dev/vdc']
         self.is_lvm_physical_volume.side_effect = pv_lookup
         self.list_lvm_volume_group.side_effect = vg_lookup
-        cinder_utils.configure_lvm_storage(devices, 'test', False)
+        cinder_utils.configure_lvm_storage(devices, 'test', False, False)
         self.assertFalse(clean_storage.called)
         self.assertFalse(self.create_lvm_physical_volume.called)
+        self.assertFalse(reduce_lvm.called)
         self.assertFalse(extend_lvm.called)
         self.assertFalse(self.create_lvm_volume_group.called)
 
+    @patch('subprocess.check_call')
+    def test_reduce_lvm_volume_group_missing(self, _call):
+        cinder_utils.reduce_lvm_volume_group_missing('test')
+        _call.assert_called_with(['vgreduce', '--removemissing', 'test'])
+
     @patch('subprocess.check_call')
     def test_extend_lvm_volume_group(self, _call):
         cinder_utils.extend_lvm_volume_group('test', '/dev/sdb')