From 7feafe808df07e3edf5269b09103ad0dab98ee16 Mon Sep 17 00:00:00 2001
From: Gorka Eguileor <geguileo@redhat.com>
Date: Thu, 29 Oct 2020 12:28:46 +0100
Subject: [PATCH] Fix unnecessary migration on retype

Retyping a volume with different QoS entries will require a migration,
even if both QoS entries have the exact same data.

This is caused by an incorrect comparison of the QoS information where
we are comparing unnecessary fields: created_at, updated_at, and
deleted_at.

The reason why the unit tests were not picking this up is because the
test was using 'created_at' as one of the spec keys (something artificial
that would not happen in the real world) and it was replacing the value
from the QoS entity.

This patch removes those 3 fields from the QoS comparison, avoiding
unnecessary migrations on retyping.

Close-Bug: #1901188
Change-Id: I979d4db90ee40b4c0a37cd198d7dfcfd7f8d75f7
---
 cinder/tests/unit/test_volume_types.py                   | 6 ++++--
 cinder/volume/volume_types.py                            | 9 +++++----
 ...unnecessary-migration-on-retype-67cedb1bd8e4c4b2.yaml | 6 ++++++
 3 files changed, 15 insertions(+), 6 deletions(-)
 create mode 100644 releasenotes/notes/fix-unnecessary-migration-on-retype-67cedb1bd8e4c4b2.yaml

diff --git a/cinder/tests/unit/test_volume_types.py b/cinder/tests/unit/test_volume_types.py
index 413476cb434..fe726a6f806 100644
--- a/cinder/tests/unit/test_volume_types.py
+++ b/cinder/tests/unit/test_volume_types.py
@@ -469,7 +469,7 @@ class VolumeTypeTestCase(test.TestCase):
         self.assertEqual(('val1', 'val0'), diff['extra_specs']['key1'])
 
         # qos_ref 1 and 2 have the same specs, while 3 has different
-        qos_keyvals1 = {'k1': 'v1', 'k2': 'v2', 'k3': 'v3', 'created_at': 'v4'}
+        qos_keyvals1 = {'k1': 'v1', 'k2': 'v2', 'k3': 'v3', 'k4': 'v4'}
         qos_keyvals2 = {'k1': 'v0', 'k2': 'v2', 'k3': 'v3'}
         qos_ref1 = qos_specs.create(self.ctxt, 'qos-specs-1', qos_keyvals1)
         qos_ref2 = qos_specs.create(self.ctxt, 'qos-specs-2', qos_keyvals1)
@@ -483,6 +483,8 @@ class VolumeTypeTestCase(test.TestCase):
         diff, same = volume_types.volume_types_diff(self.ctxt, type_ref1['id'],
                                                     type_ref2['id'])
         self.assertTrue(same)
+        for k in ('id', 'name', 'created_at', 'updated_at', 'deleted_at'):
+            self.assertNotIn(k, diff['qos_specs'])
         self.assertEqual(('val1', 'val1'), diff['extra_specs']['key1'])
         self.assertEqual(('v1', 'v1'), diff['qos_specs']['k1'])
         qos_specs.disassociate_qos_specs(self.ctxt, qos_ref2['id'],
@@ -525,7 +527,7 @@ class VolumeTypeTestCase(test.TestCase):
                           'k1': (None, 'v1'),
                           'k2': (None, 'v2'),
                           'k3': (None, 'v3'),
-                          'created_at': (None, 'v4')}, diff['qos_specs'])
+                          'k4': (None, 'v4')}, diff['qos_specs'])
         self.assertEqual({'cipher': (None, 'c1'),
                           'control_location': (None, 'front-end'),
                           'deleted': (None, False),
diff --git a/cinder/volume/volume_types.py b/cinder/volume/volume_types.py
index 14c028cfb0b..2fb241b35b5 100644
--- a/cinder/volume/volume_types.py
+++ b/cinder/volume/volume_types.py
@@ -36,8 +36,9 @@ from cinder import utils
 CONF = cfg.CONF
 LOG = logging.getLogger(__name__)
 QUOTAS = quota.QUOTAS
-ENCRYPTION_IGNORED_FIELDS = ['volume_type_id', 'created_at', 'updated_at',
-                             'deleted_at', 'encryption_id']
+ENCRYPTION_IGNORED_FIELDS = ('volume_type_id', 'created_at', 'updated_at',
+                             'deleted_at', 'encryption_id')
+QOS_IGNORED_FIELDS = ('id', 'name', 'created_at', 'updated_at', 'deleted_at')
 DEFAULT_VOLUME_TYPE = "__DEFAULT__"
 
 MIN_SIZE_KEY = "provisioning:min_vol_size"
@@ -360,8 +361,8 @@ def volume_types_diff(context, vol_type_id1, vol_type_id2):
     """
     def _fix_qos_specs(qos_specs):
         if qos_specs:
-            qos_specs.pop('id', None)
-            qos_specs.pop('name', None)
+            for key in QOS_IGNORED_FIELDS:
+                qos_specs.pop(key, None)
             qos_specs.update(qos_specs.pop('specs', {}))
 
     def _fix_encryption_specs(encryption):
diff --git a/releasenotes/notes/fix-unnecessary-migration-on-retype-67cedb1bd8e4c4b2.yaml b/releasenotes/notes/fix-unnecessary-migration-on-retype-67cedb1bd8e4c4b2.yaml
new file mode 100644
index 00000000000..531020524b5
--- /dev/null
+++ b/releasenotes/notes/fix-unnecessary-migration-on-retype-67cedb1bd8e4c4b2.yaml
@@ -0,0 +1,6 @@
+---
+fixes:
+  - |
+    `Bug #1901188 <https://bugs.launchpad.net/cinder/+bug/1901188>`_: Fix
+    unnecessary migration on retype when QoS has the same elements in both
+    types.