From: Curt Bruns <curt.e.bruns@intel.com>
Date: Mon, 6 Jul 2015 15:09:41 +0000 (+0000)
Subject: Add deactivate step to extend_lv
X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=314611f79afa6fe79f3d5022c814772048669bb0;p=openstack-build%2Fcinder-build.git

Add deactivate step to extend_lv

Extending a linear LVM volume that has a snapshot requires
that the LV be deactivated explicitly prior to extending.

This adds a deactivate_lv method and calls it prior to
issuing the extend call. If auto_activation_volume_list
is not defined lvm.conf, then volumes will be reactivated
automatically after the extend.  If auto_activation_volume_list
is defined, then volumes that should be automatically
reactivated should be added to the auto_activation_volume_list
or they won't be activated automatically.
NOTE: This doesn't apply to thin provisioned LVs as they don't
require the deactivation step.

DocImpact: When a user extends LVM Volumes with a snapshot, the
volumes will be deactivated.  Their re-activation is automatic,
unless auto_activation_volume_list is defined in lvm.conf.
See lvm.conf for more info.  Thin provisioned LVM Volumes
will not be deactivated as they don't require it.

Co-Authored-By: John Griffith <john.griffith8@gmail.com>
Change-Id: If746625cfe658c3528525dc7f4bf869f1c9704dc
Closes-Bug: #1470558
---

diff --git a/cinder/brick/local_dev/lvm.py b/cinder/brick/local_dev/lvm.py
index 83524856f..b830dd702 100644
--- a/cinder/brick/local_dev/lvm.py
+++ b/cinder/brick/local_dev/lvm.py
@@ -589,6 +589,21 @@ class LVM(executor.Executor):
             return name
         return '_' + name
 
+    def deactivate_lv(self, name):
+        lv_path = self.vg_name + '/' + self._mangle_lv_name(name)
+        cmd = ['lvchange', '-a', 'n']
+        cmd.append(lv_path)
+        try:
+            self._execute(*cmd,
+                          root_helper=self._root_helper,
+                          run_as_root=True)
+        except putils.ProcessExecutionError as err:
+            LOG.exception(_LE('Error deactivating LV'))
+            LOG.error(_LE('Cmd     :%s'), err.cmd)
+            LOG.error(_LE('StdOut  :%s'), err.stdout)
+            LOG.error(_LE('StdErr  :%s'), err.stderr)
+            raise
+
     def activate_lv(self, name, is_snapshot=False):
         """Ensure that logical volume/snapshot logical volume is activated.
 
@@ -696,7 +711,12 @@ class LVM(executor.Executor):
 
     def extend_volume(self, lv_name, new_size):
         """Extend the size of an existing volume."""
-
+        # Volumes with snaps have attributes 'o' or 'O' and will be
+        # deactivated, but Thin Volumes with snaps have attribute 'V'
+        # and won't be deactivated because the lv_has_snapshot method looks
+        # for 'o' or 'O'
+        if self.lv_has_snapshot(lv_name):
+            self.deactivate_lv(lv_name)
         try:
             self._execute('lvextend', '-L', new_size,
                           '%s/%s' % (self.vg_name, lv_name),
diff --git a/cinder/tests/unit/brick/test_brick_lvm.py b/cinder/tests/unit/brick/test_brick_lvm.py
index 77744c688..ab2760091 100644
--- a/cinder/tests/unit/brick/test_brick_lvm.py
+++ b/cinder/tests/unit/brick/test_brick_lvm.py
@@ -12,6 +12,7 @@
 #    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
 #    License for the specific language governing permissions and limitations
 #    under the License.
+import mock
 from mox3 import mox
 from oslo_concurrency import processutils
 
@@ -140,8 +141,12 @@ class BrickLvmTestCase(test.TestCase):
             pass
         elif 'lvcreate, -T, -V, ' in cmd_string:
             pass
+        elif 'lvcreate, -n, ' in cmd_string:
+            pass
         elif 'lvcreate, --name, ' in cmd_string:
             pass
+        elif 'lvextend, -L, ' in cmd_string:
+            pass
         else:
             raise AssertionError('unexpected command called: %s' % cmd_string)
 
@@ -351,3 +356,18 @@ class BrickLvmTestCase(test.TestCase):
 
     def test_get_mirrored_available_capacity(self):
         self.assertEqual(self.vg.vg_mirror_free_space(1), 2.0)
+
+    def test_lv_extend(self):
+        self.vg.deactivate_lv = mock.MagicMock()
+
+        # Extend lv with snapshot and make sure deactivate called
+        self.vg.create_volume("test", "1G")
+        self.vg.extend_volume("test", "2G")
+        self.vg.deactivate_lv.assert_called_once_with('test')
+        self.vg.deactivate_lv.reset_mock()
+
+        # Extend lv without snapshot so deactivate should not be called
+        self.vg.create_volume("test", "1G")
+        self.vg.vg_name = "test-volumes"
+        self.vg.extend_volume("test", "2G")
+        self.assertFalse(self.vg.deactivate_lv.called)
diff --git a/cinder/tests/unit/test_srb.py b/cinder/tests/unit/test_srb.py
index 329b8a65a..b0100c15f 100644
--- a/cinder/tests/unit/test_srb.py
+++ b/cinder/tests/unit/test_srb.py
@@ -593,7 +593,8 @@ class SRBDriverTestCase(test.TestCase):
 
     def _fake_lvchange(self):
         def check(cmd_string):
-            return 'lvchange, -a, y, --yes' in cmd_string
+            return 'lvchange, -a, y, --yes' in cmd_string or \
+                   'lvchange, -a, n' in cmd_string
 
         def act(_):
             pass