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

3 files changed, 122 insertions, 4 deletions

diff --git a/nova/db/sqlalchemy/migrate_repo/versions/153_instance_type_in_system_metadata.py b/nova/db/sqlalchemy/migrate_repo/versions/153_instance_type_in_system_metadata.py
index 20e75a6eb..36545b435 100644
--- a/nova/db/sqlalchemy/migrate_repo/versions/153_instance_type_in_system_metadata.py
+++ b/nova/db/sqlalchemy/migrate_repo/versions/153_instance_type_in_system_metadata.py
@@ -38,8 +38,9 @@ def upgrade(migrate_engine):
     i = sys_meta.insert()
     for values in q.execute():
         for index in range(0, len(instance_type_props)):
+            value = values[index + 1]
             i.execute({"key": "instance_type_%s" % instance_type_props[index],
-                       "value": str(values[index + 1]),
+                       "value": None if value is None else str(value),
                        "instance_uuid": values[0]})
 
 
diff --git a/nova/db/sqlalchemy/migrate_repo/versions/161_fix_system_metadata_none_strings.py b/nova/db/sqlalchemy/migrate_repo/versions/161_fix_system_metadata_none_strings.py
new file mode 100644
index 000000000..bd8f22a97
--- /dev/null
+++ b/nova/db/sqlalchemy/migrate_repo/versions/161_fix_system_metadata_none_strings.py
@@ -0,0 +1,43 @@
+# Copyright 2013 IBM Corp.
+#
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+
+from sqlalchemy import MetaData, Table
+from nova.openstack.common import timeutils
+
+
+def upgrade(migrate_engine):
+    meta = MetaData()
+    meta.bind = migrate_engine
+    sys_meta = Table('instance_system_metadata', meta, autoload=True)
+
+    sys_meta.update().\
+        values(value=None).\
+        where(sys_meta.c.key != 'instance_type_name').\
+        where(sys_meta.c.key != 'instance_type_flavorid').\
+        where(sys_meta.c.key.like('instance_type_%')).\
+        where(sys_meta.c.value == 'None').\
+        execute()
+
+    now = timeutils.utcnow()
+    sys_meta.update().\
+        values(created_at=now).\
+        where(sys_meta.c.created_at == None).\
+        where(sys_meta.c.key.like('instance_type_%')).\
+        execute()
+
+
+def downgrade(migration_engine):
+    # This migration only touches data, and only metadata at that. No need
+    # to go through and delete old metadata items.
+    pass
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."""