Fix online data migration

The online migration remove_temporary_admin_metadata_data_migration
was recently merged and broke gate for the kolla project.

There are 2 issues with the current code:
1. The db api doesn't return the total and updated values
2. We are issuing a limit and update query together which is
not allowed generating the following error:

sqlalchemy.exc.InvalidRequestError: Can't call Query.update()
or Query.delete() when limit() has been called

This patch fixes the issue by creating a select subquery to get
all the ID values with the limit as max_count. We then create
a new query for update and pass the select subquery as a filter.

We don't require a releasenote since the bug and fix are in the
same release.

Closes-Bug: 2052805
Change-Id: Ida994f767eecb094c177db15dfc80a0c0fe56447

cinder/db/api.py

@@ -1980,4 +1980,5 @@ def attachment_specs_update_or_create(context,
 
 # TODO: (D Release) remove method and this comment
 def remove_temporary_admin_metadata_data_migration(context, max_count):
-    IMPL.remove_temporary_admin_metadata_data_migration(context, max_count)
+    return IMPL.remove_temporary_admin_metadata_data_migration(
+        context, max_count)

cinder/db/sqlalchemy/api.py

@@ -8579,14 +8579,21 @@ def worker_destroy(context, **filters):
 ###############################
 
 
-# TODO: (A Release) remove method and this comment
+# TODO: (D Release) remove method and this comment
 @enginefacade.writer
 def remove_temporary_admin_metadata_data_migration(context, max_count):
+    admin_meta_table = models.VolumeAdminMetadata
     query = model_query(context,
-                        models.VolumeAdminMetadata).filter_by(key='temporary')
+                        admin_meta_table.id).filter_by(key='temporary')
     total = query.count()
-    updated = query.limit(max_count).update(
+    ids_query = query.limit(max_count).subquery()
-        models.VolumeAdminMetadata.delete_values())
+    update_args = {'synchronize_session': False}
+
+    # We cannot use limit with update or delete so create a new query
+    updated = model_query(context, admin_meta_table).\
+        filter(admin_meta_table.id.in_(ids_query)).\
+        update(admin_meta_table.delete_values(), **update_args)
+
     return total, updated
 
 

cinder/tests/unit/test_db_api.py

@@ -3903,7 +3903,8 @@ class DBAPIGroupTypeTestCase(BaseTest):
 
 
 class OnlineMigrationTestCase(BaseTest):
-    # TODO: (A Release) remove method and this comment
+
+    # TODO: (D Release) remove method and this comment
     @mock.patch.object(sqlalchemy_api,
                        'remove_temporary_admin_metadata_data_migration')
     def test_db_remove_temporary_admin_metadata_data_migration(self,
@@ -3913,30 +3914,35 @@ class OnlineMigrationTestCase(BaseTest):
         db.remove_temporary_admin_metadata_data_migration(*params)
         migration_mock.assert_called_once_with(*params)
 
-    # TODO: (A Release) remove method and this comment
+    # TODO: (D Release) remove method and this comment
     @mock.patch.object(sqlalchemy_api, 'models')
     @mock.patch.object(sqlalchemy_api, 'model_query')
     def test_remove_temporary_admin_metadata_data_migration_mocked(
             self, query_mock, models_mock):
         """Test method implementation."""
-        result = sqlalchemy_api.remove_temporary_admin_metadata_data_migration(
-            self.ctxt, mock.sentinel.max_count)
 
-        query_mock.assert_called_once_with(self.ctxt,
+        # Call DB API layer directly to test return values
-                                           models_mock.VolumeAdminMetadata)
+        total, updated = db.remove_temporary_admin_metadata_data_migration(
+            self.ctxt, mock.sentinel.max_count)
+        self.assertEqual(2, query_mock.call_count)
+        query_mock.assert_called_with(self.ctxt,
+                                      models_mock.VolumeAdminMetadata)
         filter_by = query_mock.return_value.filter_by
         filter_by.assert_called_once_with(key='temporary')
         query = filter_by.return_value
         query.count.assert_called_once_with()
         query.limit.assert_called_once_with(mock.sentinel.max_count)
+        subquery = query.limit.return_value.subquery
+        subquery.assert_called_once_with()
         del_vals_mock = models_mock.VolumeAdminMetadata.delete_values
         del_vals_mock.assert_called_once_with()
-
+        filter_subquery = query_mock.return_value.filter
-        update = query.limit.return_value.update
+        filter_subquery.assert_called_once_with(
-        update.assert_called_once_with(del_vals_mock.return_value)
+            models_mock.VolumeAdminMetadata.id.in_(filter_subquery))
-
+        update = filter_subquery.return_value.update
-        self.assertEqual((query.count.return_value, update.return_value),
+        update_args = {'synchronize_session': False}
-                         result)
+        update.assert_called_once_with(del_vals_mock.return_value,
+                                       **update_args)
 
     # TODO: (D Release) remove method and this comment
     def test_remove_temporary_admin_metadata_data_migration(self):
@@ -3958,10 +3964,10 @@ class OnlineMigrationTestCase(BaseTest):
             self.ctxt, display_name='admin_metadata', use_quota=False,
             admin_metadata=vol3_admin_meta)
 
-        # Should only  remove "temporary" admin metadata
+        # Call DB API layer directly to test return values
-        result = sqlalchemy_api.remove_temporary_admin_metadata_data_migration(
+        total, updated = db.remove_temporary_admin_metadata_data_migration(
             self.ctxt, 4)
-        self.assertEqual(1, result)
+        self.assertEqual(1, updated)
 
         vol1.refresh()
         self.assertEqual(({}, vol1_admin_meta),