From f419f1a4b12cb6a26e7dcd0c5a91aba9b9e9cb32 Mon Sep 17 00:00:00 2001 From: Balazs Gibizer Date: Wed, 14 Oct 2020 18:35:42 +0200 Subject: [PATCH] Use _evacuate_server helper in func test We have a nice helper for calling evacuate and waiting for the result so let's replace the direct post api calls in func test with the helper. Change-Id: I87a10e6ad65ae493cd34c6c0e62d60c5d6f9d1d8 --- nova/tests/functional/integrated_helpers.py | 35 ++++++++-- .../tests/functional/libvirt/test_evacuate.py | 9 +-- .../test_compute_task.py | 4 +- .../regressions/test_bug_1669054.py | 11 +--- .../regressions/test_bug_1713783.py | 9 +-- .../regressions/test_bug_1764883.py | 9 +-- .../regressions/test_bug_1794996.py | 7 +- .../regressions/test_bug_1815153.py | 8 +-- .../regressions/test_bug_1823370.py | 8 +-- nova/tests/functional/test_nova_manage.py | 7 +- nova/tests/functional/test_server_group.py | 51 ++++++--------- nova/tests/functional/test_servers.py | 65 ++++++------------- nova/tests/functional/wsgi/test_services.py | 5 +- 13 files changed, 95 insertions(+), 133 deletions(-) diff --git a/nova/tests/functional/integrated_helpers.py b/nova/tests/functional/integrated_helpers.py index 2fb618cd2730..c5207e680150 100644 --- a/nova/tests/functional/integrated_helpers.py +++ b/nova/tests/functional/integrated_helpers.py @@ -94,6 +94,12 @@ class StubComputeRPCAPI(compute_rpcapi.ComputeAPI): return rpc.ClientRouter(default_client) +# placeholder used as a default parameter value to distinguish between the case +# when the parameter is specified by the caller with None from the case when it +# was not specified +NOT_SPECIFIED = object() + + class InstanceHelperMixin: def _wait_for_server_parameter( @@ -485,11 +491,32 @@ class InstanceHelperMixin: self.api.post_server_action(server['id'], {'unshelve': {}}) return self._wait_for_state_change(server, expected_state) - def _evacuate_server(self, server, host, expected_state='ACTIVE'): + def _evacuate_server( + self, server, extra_post_args=None, expected_host=None, + expected_state='ACTIVE', expected_task_state=NOT_SPECIFIED, + expected_migration_status='done'): """Evacuate a server.""" - self.api.post_server_action(server['id'], {'evacuate': {}}) - self._wait_for_server_parameter( - server, {'OS-EXT-SRV-ATTR:host': host, 'status': expected_state}) + api = getattr(self, 'admin_api', self.api) + + post = {'evacuate': {}} + if extra_post_args: + post['evacuate'].update(extra_post_args) + + expected_result = {'status': expected_state} + if expected_host: + expected_result['OS-EXT-SRV-ATTR:host'] = expected_host + if expected_task_state is not NOT_SPECIFIED: + expected_result['OS-EXT-STS:task_state'] = expected_task_state + + api.post_server_action(server['id'], post) + + # NOTE(gibi): The order of waiting for the migration and returning + # a fresh server from _wait_for_server_parameter is important as + # the compute manager sets status of the instance before sets the + # host and finally sets the migration status. So waiting for the + # migration first makes the returned server object more consistent. + self._wait_for_migration_status(server, [expected_migration_status]) + return self._wait_for_server_parameter(server, expected_result) class PlacementHelperMixin: diff --git a/nova/tests/functional/libvirt/test_evacuate.py b/nova/tests/functional/libvirt/test_evacuate.py index d9bcf0e21c23..dcf5af14f5f6 100644 --- a/nova/tests/functional/libvirt/test_evacuate.py +++ b/nova/tests/functional/libvirt/test_evacuate.py @@ -514,20 +514,17 @@ class _LibvirtEvacuateTest(integrated_helpers.InstanceHelperMixin): def _evacuate_with_failure(self, server, compute1): # Perform an evacuation during which we experience a failure on the # destination host - instance_uuid = server['id'] with mock.patch.object(compute1.driver, 'plug_vifs') as plug_vifs: plug_vifs.side_effect = test.TestingException - self.api.post_server_action(instance_uuid, - {'evacuate': {'host': 'compute1'}}) + server = self._evacuate_server( + server, {'host': 'compute1'}, expected_state='ERROR', + expected_task_state=None, expected_migration_status='failed') # Wait for the rebuild to start, then complete fake_notifier.wait_for_versioned_notifications( 'instance.rebuild.start') - self._wait_for_migration_status(server, ['failed']) - server = self._wait_for_server_parameter( - server, {'OS-EXT-STS:task_state': None}) # Meta-test plug_vifs.assert_called() diff --git a/nova/tests/functional/notification_sample_tests/test_compute_task.py b/nova/tests/functional/notification_sample_tests/test_compute_task.py index e7e7b13f9b41..20caa3b78d15 100644 --- a/nova/tests/functional/notification_sample_tests/test_compute_task.py +++ b/nova/tests/functional/notification_sample_tests/test_compute_task.py @@ -69,9 +69,9 @@ class TestComputeTaskNotificationSample( # NOTE(takashin): The rebuild action and the evacuate action shares # same code path. So the 'evacuate' action is used for this test. - post = {'evacuate': {}} + self._evacuate_server( + server, expected_state='ERROR', expected_migration_status='error') - self.admin_api.post_server_action(server['id'], post) self._wait_for_notification('compute_task.rebuild_server.error') # 0. instance.evacuate # 1. scheduler.select_destinations.start diff --git a/nova/tests/functional/regressions/test_bug_1669054.py b/nova/tests/functional/regressions/test_bug_1669054.py index 751466fd41d2..6180dbfbaab3 100644 --- a/nova/tests/functional/regressions/test_bug_1669054.py +++ b/nova/tests/functional/regressions/test_bug_1669054.py @@ -57,14 +57,9 @@ class ResizeEvacuateTestCase(integrated_helpers._IntegratedTestBase): host2.stop() self.api.force_down_service('host2', 'nova-compute', forced_down=True) # Now try to evacuate the server back to the original source compute. - req = {'evacuate': {'onSharedStorage': False}} - self.api.post_server_action(server['id'], req) - server = self._wait_for_state_change(server, 'ACTIVE') - # The evacuate flow in the compute manager is annoying in that it - # sets the instance status to ACTIVE before updating the host, so we - # have to wait for the migration record to be 'done' to avoid a race. - self._wait_for_migration_status(server, ['done']) - self.assertEqual(self.compute.host, server['OS-EXT-SRV-ATTR:host']) + server = self._evacuate_server( + server, {'onSharedStorage': 'False'}, + expected_host=self.compute.host, expected_migration_status='done') # Assert the RequestSpec.ignore_hosts field is not populated. reqspec = objects.RequestSpec.get_by_instance_uuid( diff --git a/nova/tests/functional/regressions/test_bug_1713783.py b/nova/tests/functional/regressions/test_bug_1713783.py index 521d4470794b..86e9ae919c55 100644 --- a/nova/tests/functional/regressions/test_bug_1713783.py +++ b/nova/tests/functional/regressions/test_bug_1713783.py @@ -96,14 +96,11 @@ class FailedEvacuateStateTests(test.TestCase, fake_notifier.reset() # Initiate evacuation - post = {'evacuate': {}} - self.api.post_server_action(server['id'], post) - + self._evacuate_server( + server, expected_state='ERROR', expected_host=self.hostname, + expected_migration_status='error') self._wait_for_notification_event_type('compute_task.rebuild_server') - server = self._wait_for_state_change(server, 'ERROR') - self.assertEqual(self.hostname, server['OS-EXT-SRV-ATTR:host']) - # Check migrations migrations = self.api.get_migrations() self.assertEqual(1, len(migrations)) diff --git a/nova/tests/functional/regressions/test_bug_1764883.py b/nova/tests/functional/regressions/test_bug_1764883.py index d8d97276e63f..431af81d8668 100644 --- a/nova/tests/functional/regressions/test_bug_1764883.py +++ b/nova/tests/functional/regressions/test_bug_1764883.py @@ -97,12 +97,9 @@ class TestEvacuationWithSourceReturningDuringRebuild( self.computes.get(self.source_compute).stop() self.api.force_down_service(self.source_compute, 'nova-compute', True) - # Start evacuating the instance from the source_host - self.api.post_server_action(server['id'], {'evacuate': {}}) - - # Wait for the instance to go into an ACTIVE state - self._wait_for_state_change(server, 'ACTIVE') - server = self.api.get_server(server['id']) + # Evacuate the instance from the source_host + server = self._evacuate_server( + server, expected_migration_status='done') host = server['OS-EXT-SRV-ATTR:host'] migrations = self.api.get_migrations() diff --git a/nova/tests/functional/regressions/test_bug_1794996.py b/nova/tests/functional/regressions/test_bug_1794996.py index ee0756e603f8..15ed5e06476c 100644 --- a/nova/tests/functional/regressions/test_bug_1794996.py +++ b/nova/tests/functional/regressions/test_bug_1794996.py @@ -52,12 +52,7 @@ class TestEvacuateDeleteServerRestartOriginalCompute( source_compute_id, {'forced_down': 'true'}) # evacuate the server - post = {'evacuate': {}} - self.api.post_server_action( - server['id'], post) - expected_params = {'OS-EXT-SRV-ATTR:host': dest_hostname, - 'status': 'ACTIVE'} - server = self._wait_for_server_parameter(server, expected_params) + server = self._evacuate_server(server, expected_host=dest_hostname) # Expect to have allocation and usages on both computes as the # source compute is still down diff --git a/nova/tests/functional/regressions/test_bug_1815153.py b/nova/tests/functional/regressions/test_bug_1815153.py index cadd20c8d878..5860187e7103 100644 --- a/nova/tests/functional/regressions/test_bug_1815153.py +++ b/nova/tests/functional/regressions/test_bug_1815153.py @@ -142,11 +142,9 @@ class NonPersistentFieldNotResetTest( # Its status becomes 'ACTIVE'. # If requested_destination is not reset, a status of the server # becomes 'ERROR' because the target host is down. - self.api.post_server_action( - server['id'], {'evacuate': {'host': target_host}}) - expected_params = {'OS-EXT-SRV-ATTR:host': original_host, - 'status': 'ERROR'} - server = self._wait_for_server_parameter(server, expected_params) + server = self._evacuate_server( + server, {'host': target_host}, expected_host=original_host, + expected_state='ERROR', expected_migration_status='error') # Make sure 'is_bfv' is set. reqspec = objects.RequestSpec.get_by_instance_uuid(self.ctxt, diff --git a/nova/tests/functional/regressions/test_bug_1823370.py b/nova/tests/functional/regressions/test_bug_1823370.py index 30aa88a183b4..5e69905f5f11 100644 --- a/nova/tests/functional/regressions/test_bug_1823370.py +++ b/nova/tests/functional/regressions/test_bug_1823370.py @@ -64,8 +64,6 @@ class MultiCellEvacuateTestCase(integrated_helpers._IntegratedTestBase): # Now evacuate the server which should send it to host3 since it is # in the same cell as host1, even though host2 in cell2 is weighed # higher than host3. - req = {'evacuate': {'onSharedStorage': False}} - self.api.post_server_action(server['id'], req) - self._wait_for_migration_status(server, ['done']) - server = self._wait_for_state_change(server, 'ACTIVE') - self.assertEqual('host3', server['OS-EXT-SRV-ATTR:host']) + self._evacuate_server( + server, {'onSharedStorage': 'False'}, expected_host='host3', + expected_migration_status='done') diff --git a/nova/tests/functional/test_nova_manage.py b/nova/tests/functional/test_nova_manage.py index 8c81ac1cf34f..17d103c30782 100644 --- a/nova/tests/functional/test_nova_manage.py +++ b/nova/tests/functional/test_nova_manage.py @@ -1713,11 +1713,8 @@ class TestNovaManagePlacementAudit( self.admin_api.put_service(source_service_id, {'forced_down': 'true'}) # evacuate the instance to the target - post = {'evacuate': {"host": dest_hostname}} - self.admin_api.post_server_action(server['id'], post) - self._wait_for_server_parameter(server, - {'OS-EXT-SRV-ATTR:host': dest_hostname, - 'status': 'ACTIVE'}) + self._evacuate_server( + server, {'host': dest_hostname}, expected_host=dest_hostname) # Now the instance is gone, we can delete the compute service self.admin_api.api_delete('/os-services/%s' % source_service_id) diff --git a/nova/tests/functional/test_server_group.py b/nova/tests/functional/test_server_group.py index f46bd633a749..f7f293c817b6 100644 --- a/nova/tests/functional/test_server_group.py +++ b/nova/tests/functional/test_server_group.py @@ -446,10 +446,9 @@ class ServerGroupTestV21(ServerGroupTestBase): # Start additional host to test evacuation self.start_service('compute', host='host3') - post = {'evacuate': {'onSharedStorage': False}} - self.admin_api.post_server_action(servers[1]['id'], post) - self._wait_for_migration_status(servers[1], ['done']) - evacuated_server = self._wait_for_state_change(servers[1], 'ACTIVE') + evacuated_server = self._evacuate_server( + servers[1], {'onSharedStorage': 'False'}, + expected_migration_status='done') # check that the server is evacuated to another host self.assertNotEqual(evacuated_server['OS-EXT-SRV-ATTR:host'], @@ -467,11 +466,9 @@ class ServerGroupTestV21(ServerGroupTestBase): # Set forced_down on the host to ensure nova considers the host down. self._set_forced_down(host, True) - post = {'evacuate': {'onSharedStorage': False}} - self.admin_api.post_server_action(servers[1]['id'], post) - self._wait_for_migration_status(servers[1], ['error']) - server_after_failed_evac = self._wait_for_state_change( - servers[1], 'ERROR') + server_after_failed_evac = self._evacuate_server( + servers[1], {'onSharedStorage': 'False'}, expected_state='ERROR', + expected_migration_status='error') # assert that after a failed evac the server active on the same host # as before @@ -487,11 +484,9 @@ class ServerGroupTestV21(ServerGroupTestBase): # Set forced_down on the host to ensure nova considers the host down. self._set_forced_down(host, True) - post = {'evacuate': {'onSharedStorage': False}} - self.admin_api.post_server_action(servers[1]['id'], post) - self._wait_for_migration_status(servers[1], ['error']) - server_after_failed_evac = self._wait_for_state_change( - servers[1], 'ERROR') + server_after_failed_evac = self._evacuate_server( + servers[1], {'onSharedStorage': 'False'}, expected_state='ERROR', + expected_migration_status='error') # assert that after a failed evac the server active on the same host # as before @@ -629,10 +624,8 @@ class ServerGroupTestV215(ServerGroupTestV21): # Start additional host to test evacuation compute3 = self.start_service('compute', host='host3') - post = {'evacuate': {}} - self.admin_api.post_server_action(servers[1]['id'], post) - self._wait_for_migration_status(servers[1], ['done']) - evacuated_server = self._wait_for_state_change(servers[1], 'ACTIVE') + evacuated_server = self._evacuate_server( + servers[1], expected_migration_status='done') # check that the server is evacuated self.assertNotEqual(evacuated_server['OS-EXT-SRV-ATTR:host'], @@ -652,11 +645,9 @@ class ServerGroupTestV215(ServerGroupTestV21): # Set forced_down on the host to ensure nova considers the host down. self._set_forced_down(host, True) - post = {'evacuate': {}} - self.admin_api.post_server_action(servers[1]['id'], post) - self._wait_for_migration_status(servers[1], ['error']) - server_after_failed_evac = self._wait_for_state_change( - servers[1], 'ERROR') + server_after_failed_evac = self._evacuate_server( + servers[1], expected_state='ERROR', + expected_migration_status='error') # assert that after a failed evac the server active on the same host # as before @@ -672,11 +663,9 @@ class ServerGroupTestV215(ServerGroupTestV21): # Set forced_down on the host to ensure nova considers the host down. self._set_forced_down(host, True) - post = {'evacuate': {}} - self.admin_api.post_server_action(servers[1]['id'], post) - self._wait_for_migration_status(servers[1], ['error']) - server_after_failed_evac = self._wait_for_state_change( - servers[1], 'ERROR') + server_after_failed_evac = self._evacuate_server( + servers[1], expected_state='ERROR', + expected_migration_status='error') # assert that after a failed evac the server active on the same host # as before @@ -814,10 +803,8 @@ class ServerGroupTestV215(ServerGroupTestV21): # Set forced_down on the host to ensure nova considers the host down. self._set_forced_down(host, True) - post = {'evacuate': {}} - self.admin_api.post_server_action(servers[1]['id'], post) - self._wait_for_migration_status(servers[1], ['done']) - evacuated_server = self._wait_for_state_change(servers[1], 'ACTIVE') + evacuated_server = self._evacuate_server( + servers[1], expected_migration_status='done') # Note(gibi): need to get the server again as the state of the instance # goes to ACTIVE first then the host of the instance changes to the diff --git a/nova/tests/functional/test_servers.py b/nova/tests/functional/test_servers.py index dd528640560e..c80df64ae2a1 100644 --- a/nova/tests/functional/test_servers.py +++ b/nova/tests/functional/test_servers.py @@ -2045,8 +2045,11 @@ class ServerMovingTests(integrated_helpers.ProviderUsageBaseTestCase): fake_notifier.reset() # Initiate evacuation - post = {'evacuate': {}} - self.api.post_server_action(server['id'], post) + # There is no other host to evacuate to so the rebuild should put the + # VM to ERROR state, but it should remain on source compute + server = self._evacuate_server( + server, expected_state='ERROR', expected_host=source_hostname, + expected_migration_status='error') # NOTE(elod.illes): Should be changed to non-polling solution when # patch https://review.opendev.org/#/c/482629/ gets merged: @@ -2056,13 +2059,6 @@ class ServerMovingTests(integrated_helpers.ProviderUsageBaseTestCase): self._run_periodics() - # There is no other host to evacuate to so the rebuild should put the - # VM to ERROR state, but it should remain on source compute - expected_params = {'OS-EXT-SRV-ATTR:host': source_hostname, - 'status': 'ERROR'} - server = self._wait_for_server_parameter(server, - expected_params) - # Check migrations migrations = self.api.get_migrations() self.assertEqual(1, len(migrations)) @@ -2140,13 +2136,7 @@ class ServerMovingTests(integrated_helpers.ProviderUsageBaseTestCase): source_compute_id, {'forced_down': 'true'}) # evacuate the server - post = {'evacuate': {}} - self.api.post_server_action( - server['id'], post) - expected_params = {'OS-EXT-SRV-ATTR:host': dest_hostname, - 'status': 'ACTIVE'} - server = self._wait_for_server_parameter(server, - expected_params) + server = self._evacuate_server(server, expected_host=dest_hostname) # Expect to have allocation and usages on both computes as the # source compute is still down @@ -2398,11 +2388,11 @@ class ServerMovingTests(integrated_helpers.ProviderUsageBaseTestCase): with mock.patch('nova.compute.claims.MoveClaim', fake_move_claim): # evacuate the server - self.api.post_server_action(server['id'], {'evacuate': {}}) # the migration will fail on the dest node and the instance will # stay ACTIVE and task_state will be set to None. - server = self._wait_for_server_parameter( - server, {'status': 'ACTIVE', 'OS-EXT-STS:task_state': None}) + server = self._evacuate_server( + server, expected_task_state=None, + expected_migration_status='failed') # Run the periodics to show those don't modify allocations. self._run_periodics() @@ -2474,10 +2464,11 @@ class ServerMovingTests(integrated_helpers.ProviderUsageBaseTestCase): with mock.patch.object( self.compute2.driver, 'rebuild', fake_rebuild): # evacuate the server - self.api.post_server_action(server['id'], {'evacuate': {}}) # the migration will fail on the dest node and the instance will # go into error state - server = self._wait_for_state_change(server, 'ERROR') + server = self._evacuate_server( + server, expected_state='ERROR', + expected_migration_status='failed') # Run the periodics to show those don't modify allocations. self._run_periodics() @@ -7162,14 +7153,8 @@ class ServerMoveWithPortResourceRequestTest( self.admin_api.put_service( self.compute1_service_id, {'forced_down': 'true'}) - req = {'evacuate': {}} - if host: - req['evacuate']['host'] = host - - self.api.post_server_action(server['id'], req) - self._wait_for_server_parameter(server, - {'OS-EXT-SRV-ATTR:host': 'host2', - 'status': 'ACTIVE'}) + self._evacuate_server( + server, {'host': host} if host else {}, expected_host='host2') self._check_allocation_during_evacuate( server, self.flavor_with_group_policy, self.compute1_rp_uuid, @@ -7212,16 +7197,11 @@ class ServerMoveWithPortResourceRequestTest( 'nova.compute.resource_tracker.ResourceTracker.rebuild_claim', side_effect=exception.ComputeResourcesUnavailable( reason='test evacuate failure')): - req = {'evacuate': {}} - self.api.post_server_action(server['id'], req) # Evacuate does not have reschedule loop so evacuate expected to # simply fail and the server remains on the source host - server = self._wait_for_server_parameter(server, - {'OS-EXT-SRV-ATTR:host': 'host1', - 'status': 'ACTIVE', - 'OS-EXT-STS:task_state': None}) - - self._wait_for_migration_status(server, ['failed']) + server = self._evacuate_server( + server, expected_host='host1', expected_task_state=None, + expected_migration_status='failed') # As evacuation failed the resource allocation should be untouched self._check_allocation( @@ -7267,12 +7247,9 @@ class ServerMoveWithPortResourceRequestTest( # The compute manager on host2 will raise from # update_pci_request_spec_with_allocated_interface_name - self.api.post_server_action(server['id'], {'evacuate': {}}) - server = self._wait_for_server_parameter(server, - {'OS-EXT-SRV-ATTR:host': 'host1', - 'OS-EXT-STS:task_state': None, - 'status': 'ERROR'}) - self._wait_for_migration_status(server, ['failed']) + server = self._evacuate_server( + server, expected_host='host1', expected_state='ERROR', + expected_task_state=None, expected_migration_status='failed') self.assertIn( 'does not have a properly formatted name', @@ -8179,7 +8156,7 @@ class AcceleratorServerOpsTest(AcceleratorServerBase): arqs = self.cyborg.fake_get_arqs_for_instance(self.server['id']) compute_to_stop, compute_to_evacuate = self._test_evacuate( self.server, self.NUM_HOSTS) - self._evacuate_server(self.server, compute_to_evacuate.host) + self._evacuate_server(self.server, {'host': compute_to_evacuate.host}) compute_to_stop.start() self.server = self.api.get_server(self.server['id']) arqs_new = self.cyborg.fake_get_arqs_for_instance(self.server['id']) diff --git a/nova/tests/functional/wsgi/test_services.py b/nova/tests/functional/wsgi/test_services.py index e5247bd47a26..f73a79b38961 100644 --- a/nova/tests/functional/wsgi/test_services.py +++ b/nova/tests/functional/wsgi/test_services.py @@ -143,12 +143,9 @@ class TestServicesAPI(integrated_helpers.ProviderUsageBaseTestCase): host1.stop() # Start another host and trigger the server evacuate to that host. self._start_compute('host2') - self.admin_api.post_server_action(server['id'], {'evacuate': {}}) # The host does not change until after the status is changed to ACTIVE # so wait for both parameters. - self._wait_for_server_parameter(server, { - 'status': 'ACTIVE', - 'OS-EXT-SRV-ATTR:host': 'host2'}) + self._evacuate_server(server, expected_host='host2') # Delete the compute service for host1 and check the related # placement resources for that host. self.admin_api.api_delete('/os-services/%s' % service['id'])