From bbb83aff023c90cfc186459603320ad3c622a40b Mon Sep 17 00:00:00 2001 From: John Griffith Date: Mon, 11 May 2015 13:55:41 -0600 Subject: [PATCH] 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 --- cinder/tests/unit/test_volume_utils.py | 11 +++++++++++ cinder/volume/manager.py | 12 +++++++++++- cinder/volume/utils.py | 4 ++++ 3 files changed, 26 insertions(+), 1 deletion(-) diff --git a/cinder/tests/unit/test_volume_utils.py b/cinder/tests/unit/test_volume_utils.py index cd49bf6b9..ee1a550df 100644 --- 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 821ccc865..a87c8d3ce 100644 --- 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 405c8ef16..e0735dbcb 100644 --- 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) -- 2.45.2