Check volume_backend in retype

The retype command will always attempt a call
to the driver.retype method.  In *most* cases this
will hit the default impl which returns False because
most drivers don't implement any retype (most, a few do).

The problem is that the drivers that do implement this in
most cases will iterate through the settings and just make
the changes that are valid and ignore the rest, and then
return True. I think this is "ok" for the drivers to do,
drivers should be allowed to be somewhat dumb WRT Cinder
state management and placement info.  If we give them an
invalid command (which we're doing here) then it's on us
higher up the chain IMO.

The result is that for example if you're trying to retype
from backend-a to backend-b and backend-a implements retype
it can return True telling the manager that the volume was
succesfully retyped, even when it wasn't.

There's a lot of confusion around this bug, YES the
filter scheduler is used to determine if the retype is
valid and to what host.  That's not the issue, the issue
is that regardless of the source and destination host settings
that are provided from the filter-scheduler, we always make the
call to the driver, introducing the opportunity for a false
success status being reported back.

This patch adds a very simple check between the source and
destination host settings as provided by the scheduler and in
the case that the two are "different"(not including pool designations)
we skip calling the driver.retype method altogether and fall through
to the migrate process.

This introduces a new hosts_are_equivalent method in
cinder.volume.utils

Change-Id: Idfd7dfa2284fcea66cf23c4273efda61ee8f7eba
Closes-Bug: #1452823

diff --git a/cinder/tests/unit/test_volume_utils.py b/cinder/tests/unit/test_volume_utils.py
index cd49bf6b93954e9b740de4679fd9a303e3e24968..ee1a550df48ecba1c5a9dbecd04b3e2393393cbb 100644 (file)
--- a/cinder/tests/unit/test_volume_utils.py
+++ b/cinder/tests/unit/test_volume_utils.py
@@ -655,3 +655,14 @@ class VolumeUtilsTestCase(test.TestCase):
         expected = None
         self.assertEqual(expected,
                          volume_utils.append_host(host, pool))
+
+    def test_compare_hosts(self):
+        host_1 = 'fake_host@backend1'
+        host_2 = 'fake_host@backend1#pool1'
+        self.assertTrue(volume_utils.hosts_are_equivalent(host_1, host_2))
+
+        host_2 = 'fake_host@backend1'
+        self.assertTrue(volume_utils.hosts_are_equivalent(host_1, host_2))
+
+        host_2 = 'fake_host2@backend1'
+        self.assertFalse(volume_utils.hosts_are_equivalent(host_1, host_2))

diff --git a/cinder/volume/manager.py b/cinder/volume/manager.py
index 821ccc865b234a633c0cc16a3b54e7feca2723b9..a87c8d3ce21360b4f93647f7c7b7f1f1153d279e 100644 (file)
--- a/cinder/volume/manager.py
+++ b/cinder/volume/manager.py
@@ -1671,7 +1671,17 @@ class VolumeManager(manager.SchedulerDependentManager):
 
         # Call driver to try and change the type
         retype_model_update = None
-        if not retyped:
+
+        # NOTE(jdg): Check to see if the destination host is the same
+        # as the current.  If it's not don't call the driver.retype
+        # method, otherwise drivers that implement retype may report
+        # success, but it's invalid in the case of a migrate.
+
+        # We assume that those that support pools do this internally
+        # so we strip off the pools designation
+        if (not retyped and
+                vol_utils.hosts_are_equivalent(self.driver.host,
+                                               host['host'])):
             try:
                 new_type = volume_types.get_volume_type(context, new_type_id)
                 ret = self.driver.retype(context,

diff --git a/cinder/volume/utils.py b/cinder/volume/utils.py
index 405c8ef1610061f38b9acc3537ff6614906b11ff..e0735dbcb18a71309f083feccac5e2bedd58118f 100644 (file)
--- a/cinder/volume/utils.py
+++ b/cinder/volume/utils.py
@@ -503,3 +503,7 @@ def matching_backend_name(src_volume_type, volume_type):
             volume_type.get('volume_backend_name')
     else:
         return False
+
+
+def hosts_are_equivalent(host_1, host_2):
+    return extract_host(host_1) == extract_host(host_2)