Merge lp:~corey.bryant/charms/trusty/cinder/remove-missing into lp:~openstack-charmers-archive/charms/trusty/cinder/next

[Corey Bryant]

OIL has hit failures where physical volumes (from a previous deployment) are missing from the volume group on the current deployment, and vgextend fails. This patch fixes that by adding an option that requests that missing physical volumes be removed from the volume group.

[James Page (james-page)]

review: Needs Fixing

[James Page (james-page)]

Marking WIP - set back to 'Needs review' when ready for re-review.

[James Page (james-page)]

======================================================================
FAIL: It writes out all config
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/lib/python2.7/dist-packages/mock.py", line 1210, in patched
    return func(*args, **keywargs)
  File "/home/jamespage/src/charms/landing/cinder/unit_tests/test_cinder_hooks.py", line 118, in test_config_changed
    False)
  File "/usr/lib/python2.7/dist-packages/mock.py", line 844, in assert_called_with
    raise AssertionError(msg)
AssertionError: Expected call: configure_lvm_storage(['sdb'], 'cinder-volumes', False)
Actual call: configure_lvm_storage(['sdb'], 'cinder-volumes', False, False)

======================================================================
FAIL: It writes out all config
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/lib/python2.7/dist-packages/mock.py", line 1210, in patched
    return func(*args, **keywargs)
  File "/home/jamespage/src/charms/landing/cinder/unit_tests/test_cinder_hooks.py", line 133, in test_config_changed_block_devices
    True)
  File "/usr/lib/python2.7/dist-packages/mock.py", line 844, in assert_called_with
    raise AssertionError(msg)
AssertionError: Expected call: configure_lvm_storage(['sdb', '/dev/sdc', 'sde'], 'cinder-new', True)
Actual call: configure_lvm_storage(['sdb', '/dev/sdc', 'sde'], 'cinder-new', True, False)

review: Needs Fixing

[James Page (james-page)]

Probably worth adding a unit test to ensure that switching the config has the desired effect as well.

=== modified file 'config.yaml'
--- config.yaml	2014-10-08 10:42:20 +0000
+++ config.yaml	2014-10-10 20:37:29 +0000
@@ -27,12 +27,16 @@
     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 @@
     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
 
=== modified file 'hooks/cinder_hooks.py'
--- hooks/cinder_hooks.py	2014-10-01 22:15:34 +0000
+++ hooks/cinder_hooks.py	2014-10-10 20:37:29 +0000
@@ -105,7 +105,8 @@
         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)
 
=== modified file 'hooks/cinder_utils.py'
--- hooks/cinder_utils.py	2014-10-07 12:27:23 +0000
+++ hooks/cinder_utils.py	2014-10-10 20:37:29 +0000
@@ -267,9 +267,19 @@
     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 @@
     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 @@
         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:
 
=== modified file 'unit_tests/test_cinder_hooks.py'
--- unit_tests/test_cinder_hooks.py	2014-10-08 10:53:24 +0000
+++ unit_tests/test_cinder_hooks.py	2014-10-10 20:37:29 +0000
@@ -115,7 +115,7 @@
         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 @@
         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):
 
=== modified file 'unit_tests/test_cinder_utils.py'
--- unit_tests/test_cinder_utils.py	2014-04-14 14:41:11 +0000
+++ unit_tests/test_cinder_utils.py	2014-10-10 20:37:29 +0000
@@ -211,11 +211,12 @@
                         ('/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 +226,29 @@
              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 +266,21 @@
         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 +298,18 @@
         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,13 +327,19 @@
         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')
         _call.assert_called_with(['vgextend', 'test', '/dev/sdb'])