Fix unit suffix and add no_suffix option.

In Cinder we've been using gibibytes, however
we have code in some places using Gigabytes, the brick
LVM code was one of those places.

This change sets the default suffix to gibibytes/mibibytes (1024 based)
and also provides an option to omit the suffix from the response now
that we can say that we're consistent in what is expected.

Change-Id: Id6274ba732bbdf484c5544e005155aebd68eaf2f

diff --git a/cinder/brick/local_dev/lvm.py b/cinder/brick/local_dev/lvm.py
index 55dfb3f7a8c4939ca32d8f4c25bf141fddfc24b8..72870fcd4210cd3ddb69ef43fc100a57971a1c68 100644 (file)
--- a/cinder/brick/local_dev/lvm.py
+++ b/cinder/brick/local_dev/lvm.py
@@ -70,8 +70,9 @@ class LVM(object):
         self.vg_free_space = 0
         self.vg_lv_count = 0
         self.vg_uuid = None
-        self._execute = executor
         self.vg_thin_pool = None
+        self.vg_thin_pool_size = 0
+        self._execute = executor
 
         if create_vg and physical_volumes is not None:
             self.pv_list = physical_volumes
@@ -157,14 +158,19 @@ class LVM(object):
         return False
 
     @staticmethod
-    def get_all_volumes(vg_name=None):
+    def get_all_volumes(vg_name=None, no_suffix=True):
         """Static method to get all LV's on a system.
 
         :param vg_name: optional, gathers info for only the specified VG
+        :param no_suffix: optional, reports sizes in g with no suffix
         :returns: List of Dictionaries with LV info
 
         """
         cmd = ['lvs', '--noheadings', '--unit=g', '-o', 'vg_name,name,size']
+
+        if no_suffix:
+            cmd += ['--nosuffix']
+
         if vg_name is not None:
             cmd += [vg_name]
 
@@ -199,10 +205,11 @@ class LVM(object):
                 return r
 
     @staticmethod
-    def get_all_physical_volumes(vg_name=None):
+    def get_all_physical_volumes(vg_name=None, no_suffix=True):
         """Static method to get all PVs on a system.
 
         :param vg_name: optional, gathers info for only the specified VG
+        :param no_suffix: optional, reports sizes in g with no suffix
         :returns: List of Dictionaries with PV info
 
         """
@@ -210,6 +217,9 @@ class LVM(object):
                '--unit=g',
                '-o', 'vg_name,name,size,free',
                '--separator', ':']
+        if no_suffix:
+            cmd += ['--nosuffix']
+
         if vg_name is not None:
             cmd += [vg_name]
 
@@ -237,10 +247,11 @@ class LVM(object):
         return self.pv_list
 
     @staticmethod
-    def get_all_volume_groups(vg_name=None):
+    def get_all_volume_groups(vg_name=None, no_suffix=True):
         """Static method to get all VGs on a system.
 
         :param vg_name: optional, gathers info for only the specified VG
+        :param no_suffix: optional, reports sizes in g with no suffix
         :returns: List of Dictionaries with VG info
 
         """
@@ -248,6 +259,10 @@ class LVM(object):
                '--unit=g', '-o',
                'name,size,free,lv_count,uuid',
                '--separator', ':']
+
+        if no_suffix:
+            cmd += ['--nosuffix']
+
         if vg_name is not None:
             cmd += [vg_name]
 
@@ -285,10 +300,11 @@ class LVM(object):
         self.vg_free_space = vg_list[0]['available']
         self.vg_lv_count = vg_list[0]['lv_count']
         self.vg_uuid = vg_list[0]['uuid']
-        if self.vg_thin_pool is not None:
-            self.vg_size = self.vg_size
 
-        return vg_list[0]
+        if self.vg_thin_pool is not None:
+            for lv in self.get_all_volumes(self.vg_name):
+                if lv[1] == self.vg_thin_pool:
+                    self.vg_thin_pool_size = lv[2]
 
     def create_thin_pool(self, name=None, size_str=0):
         """Creates a thin provisioning pool for this VG.
@@ -321,9 +337,9 @@ class LVM(object):
         pool_path = '%s/%s' % (self.vg_name, name)
         cmd = ['lvcreate', '-T', '-L', size_str, pool_path]
 
-        putils.execute(*cmd,
-                       root_helper='sudo',
-                       run_as_root=True)
+        self._execute(*cmd,
+                      root_helper='sudo',
+                      run_as_root=True)
         self.vg_thin_pool = pool_path
 
     def create_volume(self, name, size_str, lv_type='default', mirror_count=0):
@@ -336,8 +352,6 @@ class LVM(object):
 
         """
 
-        size_str = self._size_str(size_str)
-        cmd = ['lvcreate', '-n', name, self.vg_name]
         if lv_type == 'thin':
             pool_path = '%s/%s' % (self.vg_name, self.vg_thin_pool)
             cmd = ['lvcreate', '-T', '-V', size_str, '-n', name, pool_path]
@@ -353,9 +367,16 @@ class LVM(object):
                 #             http://red.ht/U2BPOD
                 cmd += ['-R', str(rsize)]
 
-        self._execute(*cmd,
-                      root_helper='sudo',
-                      run_as_root=True)
+        try:
+            self._execute(*cmd,
+                          root_helper='sudo',
+                          run_as_root=True)
+        except putils.ProcessExecutionError as err:
+            LOG.exception(_('Error creating Volume'))
+            LOG.error(_('Cmd     :%s') % err.cmd)
+            LOG.error(_('StdOut  :%s') % err.stdout)
+            LOG.error(_('StdErr  :%s') % err.stderr)
+            raise
 
     def create_lv_snapshot(self, name, source_lv_name, lv_type='default'):
         """Creates a snapshot of a logical volume.
@@ -373,11 +394,18 @@ class LVM(object):
                '--snapshot', '%s/%s' % (self.vg_name, source_lv_name)]
         if lv_type != 'thin':
             size = source_lvref['size']
-            cmd += ['-L', size]
-
-        self._execute(*cmd,
-                      root_helper='sudo',
-                      run_as_root=True)
+            cmd += ['-L', '%sg' % (size)]
+
+        try:
+            self._execute(*cmd,
+                          root_helper='sudo',
+                          run_as_root=True)
+        except putils.ProcessExecutionError as err:
+            LOG.exception(_('Error creating snapshot'))
+            LOG.error(_('Cmd     :%s') % err.cmd)
+            LOG.error(_('StdOut  :%s') % err.stdout)
+            LOG.error(_('StdErr  :%s') % err.stderr)
+            raise
 
     def delete(self, name):
         """Delete logical volume or snapshot.

diff --git a/cinder/tests/brick/test_brick_lvm.py b/cinder/tests/brick/test_brick_lvm.py
index c245245e81c658cfee705a906557c696760442a9..96775cad8b484d3c0deb2102610128be28caada0 100644 (file)
--- a/cinder/tests/brick/test_brick_lvm.py
+++ b/cinder/tests/brick/test_brick_lvm.py
@@ -39,10 +39,14 @@ class BrickLvmTestCase(test.TestCase):
         self.configuration = mox.MockObject(conf.Configuration)
         self.configuration.volume_group_name = 'fake-volumes'
         super(BrickLvmTestCase, self).setUp()
+
+        #Stub processutils.execute for static methods
         self.stubs.Set(processutils, 'execute',
                        self.fake_execute)
         self.vg = brick.LVM(self.configuration.volume_group_name,
-                            False, None, 'default', self.fake_execute)
+                            False, None,
+                            'default',
+                            self.fake_execute)
 
     def failed_fake_execute(obj, *cmd, **kwargs):
         return ("\n", "fake-error")
@@ -56,7 +60,10 @@ class BrickLvmTestCase(test.TestCase):
     def fake_execute(obj, *cmd, **kwargs):
         cmd_string = ', '.join(cmd)
         data = "\n"
-        if 'vgs, --noheadings, -o, name' == cmd_string:
+
+        if 'vgs, --noheadings, --unit=g, -o, name' == cmd_string:
+            data = "  fake-volumes\n"
+        elif 'vgs, --noheadings, -o, name' == cmd_string:
             data = "  fake-volumes\n"
         if 'vgs, --version' in cmd_string:
             data = "  LVM version:     2.02.95(2) (2012-03-06)\n"
@@ -116,13 +123,16 @@ class BrickLvmTestCase(test.TestCase):
         self.assertEqual(len(self.vg.get_all_volume_groups()), 3)
         self.assertEqual(len(self.vg.get_all_volume_groups('fake-volumes')), 1)
 
-    def test_update_vg_info(self):
-        self.assertEqual(self.vg.update_volume_group_info()['name'],
-                         'fake-volumes')
-
     def test_thin_support(self):
+        # lvm.supports_thin() is a static method and doesn't
+        # use the self._executor fake we pass in on init
+        # so we need to stub proessutils.execute appropriately
+
         self.stubs.Set(processutils, 'execute', self.fake_execute)
         self.assertTrue(self.vg.supports_thin_provisioning())
 
+        self.stubs.Set(processutils, 'execute', self.fake_pretend_lvm_version)
+        self.assertTrue(self.vg.supports_thin_provisioning())
+
         self.stubs.Set(processutils, 'execute', self.fake_old_lvm_version)
         self.assertFalse(self.vg.supports_thin_provisioning())