From: John Griffith Date: Mon, 11 May 2015 19:55:41 +0000 (-0600) Subject: Check volume_backend in retype X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=03562c362cfa217ec794dd98f197f623c1ffe45d;p=openstack-build%2Fcinder-build.git 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 Conflicts: cinder/volume/utils.py Closes-Bug: #1452823 (cherry picked from commit bbb83aff023c90cfc186459603320ad3c622a40b) Change-Id: Idfd7dfa2284fcea66cf23c4273efda61ee8f7eba --- diff --git a/cinder/tests/test_volume_utils.py b/cinder/tests/test_volume_utils.py index cd49bf6b9..ee1a550df 100644 --- a/cinder/tests/test_volume_utils.py +++ b/cinder/tests/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 150816f26..d26e8ef68 100644 --- a/cinder/volume/manager.py +++ b/cinder/volume/manager.py @@ -1698,7 +1698,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 d24211858..2ce361369 100644 --- a/cinder/volume/utils.py +++ b/cinder/volume/utils.py @@ -498,3 +498,7 @@ def append_host(host, pool): new_host = "#".join([host, pool]) return new_host + + +def hosts_are_equivalent(host_1, host_2): + return extract_host(host_1) == extract_host(host_2)