Merge lp:~jason-hobbs/charms/trusty/cinder/remove-volume-group into lp:~openstack-charmers-archive/charms/trusty/cinder/next

[Jason Hobbs]

When 'overwrite' is set and volume group exists on other devices, remove it.

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

charm_lint_check #6567 cinder-next for jason-hobbs mp265461
    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/11916989/
Build: http://10.245.162.77:8080/job/charm_lint_check/6567/

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

charm_unit_test #6199 cinder-next for jason-hobbs mp265461
    UNIT OK: passed

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

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

charm_amulet_test #5244 cinder-next for jason-hobbs mp265461
    AMULET FAIL: amulet-test failed

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

Full amulet test output: http://paste.ubuntu.com/11917122/
Build: http://10.245.162.77:8080/job/charm_amulet_test/5244/

[Corey Bryant (corey.bryant)]

Hey Jason, thanks for the patch! I have a couple minor comments below. Also can you run 'make lint' and fix the results? The amulet failure above might have just been an osci hiccup.

[lp:~jason-hobbs/charms/trusty/cinder/remove-volume-group]

110. By Jason Hobbs on 2015-07-22

Corrections from code review.

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

charm_lint_check #6625 cinder-next for jason-hobbs mp265461
    LINT OK: passed

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

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

charm_unit_test #6252 cinder-next for jason-hobbs mp265461
    UNIT OK: passed

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

[Jason Hobbs (jason-hobbs)]

On 07/22/2015 08:19 AM, Corey Bryant wrote:
> Hey Jason, thanks for the patch! I have a couple minor comments below. Also can you run 'make lint' and fix the results? The amulet failure above might have just been an osci hiccup.

Thanks for the review - I fixed the lint and docstring copy/paste errors
you pointed out.

>> if vg_found is False and len(new_devices) > 0:
>> + if overwrite:
>> + ensure_lvm_volume_group_non_existent(volume_group)
>
> I wouldn't block merging on this but it seems like you could get rid of ensure_lvm_volume_group_non_existent and just add it's logic to remove_lvm_volume_group.

I'd prefer to leave it how it is - I like that 'remove_lvm_volume_group'
does just one thing - it issues the vgremove command. I also think it'd
be less clear to always call "remove_lvm_volume_group" even if it's not
there to begin with.

Jason

[Corey Bryant (corey.bryant)]

> On 07/22/2015 08:19 AM, Corey Bryant wrote:
> > Hey Jason, thanks for the patch! I have a couple minor comments below.
> Also can you run 'make lint' and fix the results? The amulet failure above
> might have just been an osci hiccup.
>
> Thanks for the review - I fixed the lint and docstring copy/paste errors
> you pointed out.
>
> >> if vg_found is False and len(new_devices) > 0:
> >> + if overwrite:
> >> + ensure_lvm_volume_group_non_existent(volume_group)
> >
> > I wouldn't block merging on this but it seems like you could get rid of
> ensure_lvm_volume_group_non_existent and just add it's logic to
> remove_lvm_volume_group.
>
> I'd prefer to leave it how it is - I like that 'remove_lvm_volume_group'
> does just one thing - it issues the vgremove command. I also think it'd
> be less clear to always call "remove_lvm_volume_group" even if it's not
> there to begin with.

Fair enough, I'm convinced.
>
> Jason

[Corey Bryant (corey.bryant)]

Just waiting on the amulet results to post before landing this.

review: Approve

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

charm_amulet_test #5254 cinder-next for jason-hobbs mp265461
    AMULET OK: passed

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

[Jason Hobbs (jason-hobbs)]

Can this be landed now?

=== modified file 'hooks/cinder_utils.py'
--- hooks/cinder_utils.py	2015-07-22 15:02:01 +0000
+++ hooks/cinder_utils.py	2015-07-22 15:02:01 +0000
@@ -355,6 +355,38 @@
     subprocess.check_call(['vgextend', volume_group, block_device])
 
 
+def lvm_volume_group_exists(volume_group):
+    """Check for the existence of a volume group.
+
+    :param volume_group: str: Name of volume group.
+    """
+    try:
+        subprocess.check_call(['vgdisplay', volume_group])
+    except subprocess.CalledProcessError:
+        return False
+    else:
+        return True
+
+
+def remove_lvm_volume_group(volume_group):
+    """Remove a volume group.
+
+    :param volume_group: str: Name of volume group to remove.
+    """
+    subprocess.check_call(['vgremove', '--force', volume_group])
+
+
+def ensure_lvm_volume_group_non_existent(volume_group):
+    """Remove volume_group if it exists.
+
+    :param volume_group: str: Name of volume group.
+    """
+    if not lvm_volume_group_exists(volume_group):
+        return
+
+    remove_lvm_volume_group(volume_group)
+
+
 def log_lvm_info():
     """Log some useful information about how LVM is setup."""
     pvscan_output = subprocess.check_output(['pvscan'])
@@ -405,8 +437,11 @@
             vg_found = True
 
     log_lvm_info()
-    
+
     if vg_found is False and len(new_devices) > 0:
+        if overwrite:
+            ensure_lvm_volume_group_non_existent(volume_group)
+
         # Create new volume group from first device
         create_lvm_volume_group(volume_group, new_devices[0])
         new_devices.remove(new_devices[0])

=== modified file 'unit_tests/test_cinder_utils.py'
--- unit_tests/test_cinder_utils.py	2015-07-22 15:02:01 +0000
+++ unit_tests/test_cinder_utils.py	2015-07-22 15:02:01 +0000
@@ -2,6 +2,7 @@
 
 from collections import OrderedDict
 import os
+import subprocess
 
 os.environ['JUJU_UNIT_NAME'] = 'cinder'
 import cinder_utils as cinder_utils
@@ -247,11 +248,12 @@
         _check.assert_called_with(['fdisk', '-l', '/dev/vdb'], stderr=-2)
 
     @patch('cinder_utils.log_lvm_info', Mock())
+    @patch.object(cinder_utils, 'ensure_lvm_volume_group_non_existent')
     @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, reduce_lvm,
-                                   clean_storage):
+                                   clean_storage, ensure_non_existent):
         devices = ['/dev/vdb', '/dev/vdc']
         self.is_lvm_physical_volume.return_value = False
         cinder_utils.configure_lvm_storage(devices, 'test', True, True)
@@ -266,6 +268,7 @@
         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')
+        ensure_non_existent.assert_called_with('test')
 
     @patch('cinder_utils.log_lvm_info', Mock())
     @patch.object(cinder_utils, 'has_partition_table')
@@ -301,11 +304,13 @@
         reduce_lvm.assert_called_with('test')
 
     @patch('cinder_utils.log_lvm_info', Mock())
+    @patch.object(cinder_utils, 'ensure_lvm_volume_group_non_existent')
     @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, reduce_lvm,
-                                            clean_storage):
+                                            clean_storage,
+                                            ensure_non_existent):
         devices = ['/mnt/loop0|10']
         self.ensure_loopback_device.return_value = '/dev/loop0'
         self.is_lvm_physical_volume.return_value = False
@@ -316,6 +321,7 @@
         self.create_lvm_volume_group.assert_called_with('test', '/dev/loop0')
         reduce_lvm.assert_called_with('test')
         self.assertFalse(extend_lvm.called)
+        ensure_non_existent.assert_called_with('test')
 
     @patch('cinder_utils.log_lvm_info', Mock())
     @patch.object(cinder_utils, 'clean_storage')
@@ -725,3 +731,47 @@
         cinder_utils.log_lvm_info()
         _check.assert_called_with(['pvscan'])
         self.juju_log.assert_called_with("pvscan: %s" % output)
+
+    @patch.object(cinder_utils, 'lvm_volume_group_exists')
+    @patch.object(cinder_utils, 'remove_lvm_volume_group')
+    def test_ensure_non_existent_removes_if_present(self,
+                                                    remove_lvm_volume_group,
+                                                    volume_group_exists):
+        volume_group = "test"
+        volume_group_exists.return_value = True
+        cinder_utils.ensure_lvm_volume_group_non_existent(volume_group)
+        remove_lvm_volume_group.assert_called_with(volume_group)
+
+    @patch.object(cinder_utils, 'lvm_volume_group_exists')
+    @patch.object(cinder_utils, 'remove_lvm_volume_group')
+    def test_ensure_non_existent_not_present(self, remove_lvm_volume_group,
+                                             volume_group_exists):
+        volume_group = "test"
+        volume_group_exists.return_value = False
+        cinder_utils.ensure_lvm_volume_group_non_existent(volume_group)
+        self.assertFalse(remove_lvm_volume_group.called)
+
+    @patch('subprocess.check_call')
+    def test_lvm_volume_group_exists_finds_volume_group(self, _check):
+        volume_group = "test"
+        _check.return_value = True
+        result = cinder_utils.lvm_volume_group_exists(volume_group)
+        self.assertTrue(result)
+        _check.assert_called_with(['vgdisplay', volume_group])
+
+    @patch('subprocess.check_call')
+    def test_lvm_volume_group_exists_finds_no_volume_group(self, _check):
+        volume_group = "test"
+
+        def raise_error(x):
+            raise subprocess.CalledProcessError(1, x)
+        _check.side_effect = raise_error
+        result = cinder_utils.lvm_volume_group_exists(volume_group)
+        self.assertFalse(result)
+        _check.assert_called_with(['vgdisplay', volume_group])
+
+    @patch('subprocess.check_call')
+    def test_remove_lvm_volume_group(self, _check):
+        volume_group = "test"
+        cinder_utils.remove_lvm_volume_group(volume_group)
+        _check.assert_called_with(['vgremove', '--force', volume_group])