From 0ae53f2bf0a753edfacf3ed8b45bcc40b69428d1 Mon Sep 17 00:00:00 2001 From: melanie witt Date: Fri, 19 Apr 2019 00:59:25 +0000 Subject: [PATCH] Revert "Fix target_cell usage for scatter_gather_cells" This reverts commit 3cc3cc453dc16e22365bea597c1be5bb0be57aeb. We had determined that this commit had not actually fixed any bug or issue with: TypeError: Can't upgrade a READER transaction to a WRITER mid-transaction and the real fix was change Id24dea7465bafc1f6f58c4a121c4ffb35b7a634a. This reverts the unnecessary commit in order to simplify the code and it's possible that targeting the context before spawning a thread will result in decreased cell cache lock contention once the threads are spawned. Related-Bug: #1722404 Conflicts: nova/context.py NOTE(melwitt): The conflict is due to change I861b223ee46b0f0a31f646a4b45f8a02410253cf which is new since the original commit had landed. Change-Id: I3890d14d8679b96cfebcd735f3b03ec808730b5c --- nova/context.py | 13 ++++++------- nova/tests/unit/test_context.py | 18 ++---------------- 2 files changed, 8 insertions(+), 23 deletions(-) diff --git a/nova/context.py b/nova/context.py index 685829a08137..95002f176350 100644 --- a/nova/context.py +++ b/nova/context.py @@ -435,11 +435,9 @@ def scatter_gather_cells(context, cell_mappings, timeout, fn, *args, **kwargs): queue = eventlet.queue.LightQueue() results = {} - def gather_result(cell_mapping, fn, context, *args, **kwargs): - cell_uuid = cell_mapping.uuid + def gather_result(cell_uuid, fn, *args, **kwargs): try: - with target_cell(context, cell_mapping) as cctxt: - result = fn(cctxt, *args, **kwargs) + result = fn(*args, **kwargs) except Exception as e: # Only log the exception traceback for non-nova exceptions. if not isinstance(e, exception.NovaException): @@ -449,9 +447,10 @@ def scatter_gather_cells(context, cell_mappings, timeout, fn, *args, **kwargs): queue.put((cell_uuid, result)) for cell_mapping in cell_mappings: - greenthreads.append((cell_mapping.uuid, - utils.spawn(gather_result, cell_mapping, - fn, context, *args, **kwargs))) + with target_cell(context, cell_mapping) as cctxt: + greenthreads.append((cell_mapping.uuid, + utils.spawn(gather_result, cell_mapping.uuid, + fn, cctxt, *args, **kwargs))) with eventlet.timeout.Timeout(timeout, exception.CellTimeout): try: diff --git a/nova/tests/unit/test_context.py b/nova/tests/unit/test_context.py index 4827757a40c6..c640477ab802 100644 --- a/nova/tests/unit/test_context.py +++ b/nova/tests/unit/test_context.py @@ -346,35 +346,21 @@ class ContextTestCase(test.NoDBTestCase): @mock.patch('nova.context.target_cell') @mock.patch('nova.objects.InstanceList.get_by_filters') def test_scatter_gather_cells(self, mock_get_inst, mock_target_cell): + self.useFixture(nova_fixtures.SpawnIsSynchronousFixture()) ctxt = context.get_context() mapping = objects.CellMapping(database_connection='fake://db', transport_url='fake://mq', uuid=uuids.cell) mappings = objects.CellMappingList(objects=[mapping]) - # Use a mock manager to assert call order across mocks. - manager = mock.Mock() - manager.attach_mock(mock_get_inst, 'get_inst') - manager.attach_mock(mock_target_cell, 'target_cell') - filters = {'deleted': False} context.scatter_gather_cells( ctxt, mappings, 60, objects.InstanceList.get_by_filters, filters, sort_dir='foo') - # NOTE(melwitt): This only works without the SpawnIsSynchronous fixture - # because when the spawn is treated as synchronous and the thread - # function is called immediately, it will occur inside the target_cell - # context manager scope when it wouldn't with a real spawn. - - # Assert that InstanceList.get_by_filters was called before the - # target_cell context manager exited. - get_inst_call = mock.call.get_inst( + mock_get_inst.assert_called_once_with( mock_target_cell.return_value.__enter__.return_value, filters, sort_dir='foo') - expected_calls = [get_inst_call, - mock.call.target_cell().__exit__(None, None, None)] - manager.assert_has_calls(expected_calls) @mock.patch('nova.context.LOG.warning') @mock.patch('eventlet.timeout.Timeout')