Fix system_metadata "None" and created_at values

The ill-fated migration 153 converted None instance_type values to
"None" in the database instead of properly making them NULL. This
corrects that by sweeping all of the likely instance_type_% values and
converting "None" to NULL. Also, it adds a belated created_at stamp
to all of the items, which was missing in 153 as well.

It also corrects the 153 migration to avoid polluting things in the
first place for systems that haven't rolled through that yet.

Fixes bug 1152546

Change-Id: I3585463ae15abfd534f2ff070e2974f3cb51d7e8

1 files changed, 77 insertions, 3 deletions

diff --git a/nova/tests/test_migrations.py b/nova/tests/test_migrations.py
index cf5c2f509..60975c68c 100644
--- a/nova/tests/test_migrations.py
+++ b/nova/tests/test_migrations.py
@@ -785,7 +785,7 @@ class TestNovaMigrations(BaseMigrationTestCase, CommonTestsMixIn):
                  is_public=False),
             dict(id=13, name='type4', memory_mb=128, vcpus=1,
                  root_gb=10, ephemeral_gb=0, flavorid="4", swap=0,
-                 rxtx_factor=1.0, vcpu_weight=1, disabled=True,
+                 rxtx_factor=1.0, vcpu_weight=None, disabled=True,
                  is_public=True),
             dict(id=14, name='type5', memory_mb=128, vcpus=1,
                  root_gb=10, ephemeral_gb=0, flavorid="5", swap=0,
@@ -831,8 +831,14 @@ class TestNovaMigrations(BaseMigrationTestCase, CommonTestsMixIn):
             for prop in instance_type_props:
                 prop_name = 'instance_type_%s' % prop
                 self.assertIn(prop_name, inst_sys_meta)
-                self.assertEqual(str(inst_sys_meta[prop_name]),
-                                 str(inst_type[prop]))
+                if prop == "vcpu_weight":
+                    # NOTE(danms) vcpu_weight can be NULL
+                    self.assertEqual(inst_sys_meta[prop_name],
+                                     inst_type[prop] and str(inst_type[prop])
+                                     or None)
+                else:
+                    self.assertEqual(str(inst_sys_meta[prop_name]),
+                                     str(inst_type[prop]))
 
     # migration 154, add shadow tables for deleted data
     # There are 53 shadow tables but we only test one
@@ -1032,6 +1038,74 @@ class TestNovaMigrations(BaseMigrationTestCase, CommonTestsMixIn):
             for key, value in data[the_id].items():
                 self.assertEqual(value, result[key])
 
+    # migration 161, fix system_metadata "None" values should be NULL
+    def _pre_upgrade_161(self, engine):
+        fake_instances = [dict(uuid='m161-uuid1')]
+        sm_base = dict(instance_uuid='m161-uuid1', value=None)
+        now = timeutils.utcnow().replace(microsecond=0)
+        fake_sys_meta = [
+            # Should be fixed
+            dict(sm_base, key='instance_type_foo', value='None'),
+            dict(sm_base, key='instance_type_bar', value='88 mph'),
+
+            # Should be unaffected
+            dict(sm_base, key='instance_type_name', value='None'),
+            dict(sm_base, key='instance_type_flavorid', value='None'),
+            dict(sm_base, key='foo', value='None'),
+            dict(sm_base, key='instance_type_bat'),
+            dict(sm_base, key='instance_type_baz', created_at=now),
+            ]
+
+        instances = get_table(engine, 'instances')
+        sys_meta = get_table(engine, 'instance_system_metadata')
+        engine.execute(instances.insert(), fake_instances)
+
+        data = {}
+        for sm in fake_sys_meta:
+            result = sys_meta.insert().values(sm).execute()
+            sm['id'] = result.inserted_primary_key[0]
+            data[sm['id']] = sm
+
+        return data
+
+    def _check_161(self, engine, data):
+        our_ids = data.keys()
+        sys_meta = get_table(engine, 'instance_system_metadata')
+        results = sys_meta.select().where(sys_meta.c.id.in_(our_ids)).\
+                                    execute()
+        results = list(results)
+        self.assertEqual(len(our_ids), len(results))
+        for result in results:
+            the_id = result['id']
+            key = result['key']
+            value = result['value']
+            original = data[the_id]
+
+            if key == 'instance_type_baz':
+                # Neither value nor created_at should have been altered
+                self.assertEqual(result['value'], original['value'])
+                self.assertEqual(result['created_at'], original['created_at'])
+            elif key in ['instance_type_name', 'instance_type_flavorid']:
+                # These should not have their values changed, but should
+                # have corrected created_at stamps
+                self.assertEqual(result['value'], original['value'])
+                self.assertTrue(isinstance(result['created_at'],
+                                           datetime.datetime))
+            elif key.startswith('instance_type'):
+                # Values like instance_type_% should be stamped and values
+                # converted from 'None' to None where appropriate
+                self.assertEqual(result['value'],
+                                 None if original['value'] == 'None'
+                                 else original['value'])
+                self.assertTrue(isinstance(result['created_at'],
+                                           datetime.datetime))
+            else:
+                # None of the non-instance_type values should have
+                # been touched. Since we didn't set created_at on any
+                # of them, they should all still be None.
+                self.assertEqual(result['value'], original['value'])
+                self.assertEqual(result['created_at'], None)
+
 
 class TestBaremetalMigrations(BaseMigrationTestCase, CommonTestsMixIn):
     """Test sqlalchemy-migrate migrations."""