Merge lp:~jason-hobbs/charms/trusty/cinder/force-vgreduce into lp:~openstack-charmers-archive/charms/trusty/cinder/next

[Jason Hobbs]

Add 'remove-missing-force' config option.

[uosci-testing-bot (uosci-testing-bot)]

charm_lint_check #6566 cinder-next for jason-hobbs mp265448
    LINT FAIL: lint-test failed

LINT Results (max last 2 lines):
make: *** [lint] Error 1
ERROR:root:Make target returned non-zero.

Full lint test output: http://paste.ubuntu.com/11916404/
Build: http://10.245.162.77:8080/job/charm_lint_check/6566/

[uosci-testing-bot (uosci-testing-bot)]

charm_unit_test #6198 cinder-next for jason-hobbs mp265448
    UNIT OK: passed

Build: http://10.245.162.77:8080/job/charm_unit_test/6198/

[uosci-testing-bot (uosci-testing-bot)]

charm_amulet_test #5243 cinder-next for jason-hobbs mp265448
    AMULET OK: passed

Build: http://10.245.162.77:8080/job/charm_amulet_test/5243/

[Billy Olsen (billy-olsen)]

Jason, thanks for the code submission! There were some lint errors which need to be corrected and I've also added a few comments inline.

review: Needs Fixing

[Corey Bryant (corey.bryant)]

Hey Jason, One comment from me below.

[lp:~jason-hobbs/charms/trusty/cinder/force-vgreduce]

107. By Corey Bryant on 2015-07-22

[jason-hobbs,r=corey.bryant] Add LVM logging when configuring LVM.

[Jason Hobbs (jason-hobbs)]

> Jason, thanks for the code submission! There were some lint errors which need
> to be corrected and I've also added a few comments inline.

Thanks for the review. I fixed up the lint issues and got rid of the force_reduce_lvm_volume_group_missing(). reduce_lvm_volume_group_missing() now takes an optional list of extra args to pass to vgreduce.

[Jason Hobbs (jason-hobbs)]

> Hey Jason, One comment from me below.

Thanks Corey. I changed it so that force-remove-missing overrides remove-missing.

[Jason Hobbs (jason-hobbs)]

All of the review comments have been addressed - this is ready for another look.

[uosci-testing-bot (uosci-testing-bot)]

charm_lint_check #6634 cinder-next for jason-hobbs mp265448
    LINT OK: passed

Build: http://10.245.162.77:8080/job/charm_lint_check/6634/

[uosci-testing-bot (uosci-testing-bot)]

charm_unit_test #6261 cinder-next for jason-hobbs mp265448
    UNIT OK: passed

Build: http://10.245.162.77:8080/job/charm_unit_test/6261/

[Corey Bryant (corey.bryant)]

LGTM. Just want to wait on amulet results to be posted.

review: Approve

[uosci-testing-bot (uosci-testing-bot)]

charm_amulet_test #5264 cinder-next for jason-hobbs mp265448
    AMULET OK: passed

Build: http://10.245.162.77:8080/job/charm_amulet_test/5264/

[Corey Bryant (corey.bryant)]

Ok Amulet passes. Billy, look ok to you?

[Billy Olsen (billy-olsen)]

Comment below, not a big enough of an issue to hold up this MP, but I'd like to revisit this in the near future.

review: Approve

[lp:~jason-hobbs/charms/trusty/cinder/force-vgreduce]

108. By Billy Olsen <wolsen@blackwell> on 2015-07-23

[jason-hobbs,r=corecb,billy-olsen] Add remove-missing-force config option.

Adds an option to force remove physical volumes from a volume group even
when a logical volume is allocated on top of the physical volume.

[Jason Hobbs (jason-hobbs)]

Thanks Billy - I responded to your comment inline blow.

=== modified file 'config.yaml'
--- config.yaml	2015-07-10 14:14:14 +0000
+++ config.yaml	2015-07-22 19:16:40 +0000
@@ -101,6 +101,13 @@
     description: |
       If True, charm will attempt to remove missing physical volumes from
       volume group, if logical volumes are not allocated on them.
+  remove-missing-force:
+    default: False
+    type: boolean
+    description: |
+      If True, charm will attempt to remove missing physical volumes from
+      volume group, even when logical volumes are allocated on them. This
+      option overrides 'remove-missing' when set.
   database-user:
     default: cinder
     type: string

=== modified file 'hooks/cinder_hooks.py'
--- hooks/cinder_hooks.py	2015-06-09 09:57:24 +0000
+++ hooks/cinder_hooks.py	2015-07-22 19:16:40 +0000
@@ -129,7 +129,8 @@
         configure_lvm_storage(block_devices,
                               conf['volume-group'],
                               conf['overwrite'] in ['true', 'True', True],
-                              conf['remove-missing'])
+                              conf['remove-missing'],
+                              conf['remove-missing-force'])
 
     if git_install_requested():
         if config_value_changed('openstack-origin-git'):

=== modified file 'hooks/cinder_utils.py'
--- hooks/cinder_utils.py	2015-07-22 17:05:42 +0000
+++ hooks/cinder_utils.py	2015-07-22 19:16:40 +0000
@@ -333,14 +333,19 @@
     return list(set(_services))
 
 
-def reduce_lvm_volume_group_missing(volume_group):
+def reduce_lvm_volume_group_missing(volume_group, extra_args=None):
     '''
     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.
+    :param extra_args: list: List of extra args to pass to vgreduce
     '''
-    subprocess.check_call(['vgreduce', '--removemissing', volume_group])
+    if extra_args is None:
+        extra_args = []
+
+    command = ['vgreduce', '--removemissing'] + extra_args + [volume_group]
+    subprocess.check_call(command)
 
 
 def extend_lvm_volume_group(volume_group, block_device):
@@ -362,7 +367,7 @@
 
 
 def configure_lvm_storage(block_devices, volume_group, overwrite=False,
-                          remove_missing=False):
+                          remove_missing=False, remove_missing_force=False):
     ''' Configure LVM storage on the list of block devices provided
 
     :param block_devices: list: List of whitelisted block devices to detect
@@ -371,6 +376,9 @@
                             not already in-use
     :param remove_missing: bool: Remove missing physical volumes from volume
                            group if logical volume not allocated on them
+    :param remove_missing_force: bool: Remove missing physical volumes from
+                           volume group even if logical volumes are allocated
+                           on them. Overrides 'remove_missing' if set.
     '''
     log_lvm_info()
     devices = []
@@ -412,7 +420,9 @@
         new_devices.remove(new_devices[0])
 
     # Remove missing physical volumes from volume group
-    if remove_missing:
+    if remove_missing_force:
+        reduce_lvm_volume_group_missing(volume_group, extra_args=['--force'])
+    elif remove_missing:
         reduce_lvm_volume_group_missing(volume_group)
 
     if len(new_devices) > 0:

=== modified file 'unit_tests/test_cinder_hooks.py'
--- unit_tests/test_cinder_hooks.py	2015-06-10 21:37:05 +0000
+++ unit_tests/test_cinder_hooks.py	2015-07-22 19:16:40 +0000
@@ -156,7 +156,7 @@
         self.assertTrue(conf_https.called)
         self.configure_lvm_storage.assert_called_with(['sdb'],
                                                       'cinder-volumes',
-                                                      False, False)
+                                                      False, False, False)
 
     @patch.object(hooks, 'configure_https')
     @patch.object(hooks, 'git_install_requested')
@@ -174,7 +174,22 @@
         self.configure_lvm_storage.assert_called_with(
             ['sdb', '/dev/sdc', 'sde'],
             'cinder-new',
-            True, True)
+            True, True, False)
+
+    @patch.object(hooks, 'configure_https')
+    @patch.object(hooks, 'git_install_requested')
+    def test_config_changed_uses_remove_missing_force(self, git_requested,
+                                                      conf_https):
+        'It uses the remove-missing-force config option'
+        git_requested.return_value = False
+        self.openstack_upgrade_available.return_value = False
+        self.test_config.set('block-device', 'sdb')
+        self.test_config.set('remove-missing-force', True)
+        hooks.hooks.execute(['hooks/config-changed'])
+        self.configure_lvm_storage.assert_called_with(
+            ['sdb'],
+            'cinder-volumes',
+            False, False, True)
 
     @patch.object(hooks, 'configure_https')
     @patch.object(hooks, 'git_install_requested')

=== modified file 'unit_tests/test_cinder_utils.py'
--- unit_tests/test_cinder_utils.py	2015-07-21 19:09:36 +0000
+++ unit_tests/test_cinder_utils.py	2015-07-22 19:16:40 +0000
@@ -409,12 +409,36 @@
         self.assertFalse(extend_lvm.called)
         self.assertFalse(self.create_lvm_volume_group.called)
 
+    @patch('cinder_utils.log_lvm_info', Mock())
+    @patch.object(cinder_utils, 'reduce_lvm_volume_group_missing')
+    def test_configure_lvm_storage_unforced_remove_default(self, reduce_lvm):
+        """It doesn't force remove missing by default."""
+        devices = ['/dev/vdb']
+        cinder_utils.configure_lvm_storage(devices, 'test', False, True)
+        reduce_lvm.assert_called_with('test')
+
+    @patch('cinder_utils.log_lvm_info', Mock())
+    @patch.object(cinder_utils, 'reduce_lvm_volume_group_missing')
+    def test_configure_lvm_storage_force_removemissing(self, reduce_lvm):
+        """It forces remove missing when asked to."""
+        devices = ['/dev/vdb']
+        cinder_utils.configure_lvm_storage(
+            devices, 'test', False, True, remove_missing_force=True)
+        reduce_lvm.assert_called_with('test', extra_args=['--force'])
+
     @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_reduce_lvm_volume_group_missing_extra_args(self, _call):
+        cinder_utils.reduce_lvm_volume_group_missing(
+            'test', extra_args=['--arg'])
+        expected_call_args = ['vgreduce', '--removemissing', '--arg', 'test']
+        _call.assert_called_with(expected_call_args)
+
+    @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'])