From f4a9429cd01bbcd799083c76fc8b83c1a400ebb8 Mon Sep 17 00:00:00 2001 From: John Griffith Date: Thu, 18 Jul 2013 18:30:36 -0600 Subject: [PATCH] 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 --- cinder/brick/local_dev/lvm.py | 68 ++++++++++++++++++++-------- cinder/tests/brick/test_brick_lvm.py | 22 ++++++--- 2 files changed, 64 insertions(+), 26 deletions(-) diff --git a/cinder/brick/local_dev/lvm.py b/cinder/brick/local_dev/lvm.py index 55dfb3f7a..72870fcd4 100644 --- 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 c245245e8..96775cad8 100644 --- 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()) -- 2.45.2