Move _make_instance_list call outside of DB transaction context

The _make_instance_list method is used to make an InstanceList object
out of database dict-like instance objects. It's possible while making
the list that the various _from_db_object methods that are called might
do their own database writes.

Currently, we're calling _make_instance_list nested inside of a 'reader'
database transaction context and we hit the error:

  TypeError: Can't upgrade a READER transaction to a WRITER
  mid-transaction

during the _make_instance_list call if anything tries to do a database
write. The scenario encountered was after an upgrade to Pike, older
service records without UUIDs were attempted to be updated with UUIDs
upon access, and that access happened to be during an instance list,
so it failed when trying to write the service UUID while nested inside
the 'reader' database transaction context.

This simply moves the _make_instance_list method call out from the
@db.select_db_reader_mode decorated _get_by_filters_impl method to the
get_by_filters method to remove the nesting.

Closes-Bug: #1746509

Change-Id: Ifadf408802cc15eb9769d2dc1fc920426bb7fc20
(cherry picked from commit b1ed92c7af01a9ac7e122a541ce1bdb9be0524c4)

2 files changed, 14 insertions, 15 deletions

diff --git a/nova/objects/instance.py b/nova/objects/instance.py
index 920d55c745..90dd3f7612 100644
--- a/nova/objects/instance.py
+++ b/nova/objects/instance.py
@@ -1235,18 +1235,24 @@ class InstanceList(base.ObjectListBase, base.NovaObject):
             db_inst_list = db.instance_get_all_by_filters(
                 context, filters, sort_key, sort_dir, limit=limit,
                 marker=marker, columns_to_join=_expected_cols(expected_attrs))
-        return _make_instance_list(context, cls(), db_inst_list,
-                                   expected_attrs)
+        return db_inst_list
 
     @base.remotable_classmethod
     def get_by_filters(cls, context, filters,
                        sort_key='created_at', sort_dir='desc', limit=None,
                        marker=None, expected_attrs=None, use_slave=False,
                        sort_keys=None, sort_dirs=None):
-        return cls._get_by_filters_impl(
+        db_inst_list = cls._get_by_filters_impl(
             context, filters, sort_key=sort_key, sort_dir=sort_dir,
             limit=limit, marker=marker, expected_attrs=expected_attrs,
             use_slave=use_slave, sort_keys=sort_keys, sort_dirs=sort_dirs)
+        # NOTE(melwitt): _make_instance_list could result in joined objects'
+        # (from expected_attrs) _from_db_object methods being called during
+        # Instance._from_db_object, each of which might choose to perform
+        # database writes. So, we call this outside of _get_by_filters_impl to
+        # avoid being nested inside a 'reader' database transaction context.
+        return _make_instance_list(context, cls(), db_inst_list,
+                                   expected_attrs)
 
     @staticmethod
     @db.select_db_reader_mode
diff --git a/nova/tests/functional/regressions/test_bug_1746509.py b/nova/tests/functional/regressions/test_bug_1746509.py
index a69161cab7..e5e86f2660 100644
--- a/nova/tests/functional/regressions/test_bug_1746509.py
+++ b/nova/tests/functional/regressions/test_bug_1746509.py
@@ -10,8 +10,6 @@
 # License for the specific language governing permissions and limitations
 # under the License.
 
-import six
-
 import nova.context
 from nova import db
 from nova import objects
@@ -57,13 +55,8 @@ class InstanceListWithServicesTestCase(test.TestCase):
                                 host='fake-host')
         inst.create()
 
-        # TODO(melwitt): Remove these asserts when the bug is fixed.
-        ex = self.assertRaises(TypeError, objects.InstanceList.get_by_filters,
-                               self.context, {}, expected_attrs=['services'])
-        self.assertIn("Can't upgrade a READER transaction to a WRITER "
-                      "mid-transaction", six.text_type(ex))
-
-        # TODO(melwitt): Uncomment this assert when the bug is fixed.
-        # insts = objects.InstanceList.get_by_filters(
-        #     self.context, {}, expected_attrs=['services'])
-        # self.assertEqual(1, len(insts))
+        insts = objects.InstanceList.get_by_filters(
+            self.context, {}, expected_attrs=['services'])
+        self.assertEqual(1, len(insts))
+        self.assertEqual(1, len(insts[0].services))
+        self.assertIsNotNone(insts[0].services[0].uuid)