From 4bd53dc1090fda86f6ce25b76a079e250c9206d8 Mon Sep 17 00:00:00 2001 From: Matt Riedemann Date: Wed, 24 Jul 2019 14:39:07 -0400 Subject: [PATCH] Fix compute service set handling for 2.53+ With compute API microversion 2.53 there is a single PUT /os-services/{service_id} API which takes the service id as a UUID. Since the openstack compute service set command only takes --host and --service (binary) to identify the service, this change checks if 2.53 or greater is being used and if so, looks up the service by host and binary and calls the appropriate methods in novaclient. If the command cannot uniquely identify a compute service with the given host and binary, an error is raised. A future change could add an --id option to be used with 2.53+ to pass the service id (as UUID) directly to avoid the host/binary filtering. Change-Id: I868e0868e8eb17e7e34eef3d2d58dceedd29c2b0 Story: 2005349 Task: 30302 --- openstackclient/compute/v2/service.py | 63 +++++++++-- .../tests/unit/compute/v2/test_service.py | 104 +++++++++++++++++- ...ute-service-set-2.53-3d2db875154e633a.yaml | 6 + 3 files changed, 162 insertions(+), 11 deletions(-) create mode 100644 releasenotes/notes/story-2005349-compute-service-set-2.53-3d2db875154e633a.yaml diff --git a/openstackclient/compute/v2/service.py b/openstackclient/compute/v2/service.py index f59f85de26..80c0be7ecb 100644 --- a/openstackclient/compute/v2/service.py +++ b/openstackclient/compute/v2/service.py @@ -163,6 +163,33 @@ class SetService(command.Command): ) return parser + @staticmethod + def _find_service_by_host_and_binary(cs, host, binary): + """Utility method to find a compute service by host and binary + + :param host: the name of the compute service host + :param binary: the compute service binary, e.g. nova-compute + :returns: novaclient.v2.services.Service dict-like object + :raises: CommandError if no or multiple results were found + """ + services = cs.list(host=host, binary=binary) + # Did we find anything? + if not len(services): + msg = _('Compute service for host "%(host)s" and binary ' + '"%(binary)s" not found.') % { + 'host': host, 'binary': binary} + raise exceptions.CommandError(msg) + # Did we find more than one result? This should not happen but let's + # be safe. + if len(services) > 1: + # TODO(mriedem): If we have an --id option for 2.53+ then we can + # say to use that option to uniquely identify the service. + msg = _('Multiple compute services found for host "%(host)s" and ' + 'binary "%(binary)s". Unable to proceed.') % { + 'host': host, 'binary': binary} + raise exceptions.CommandError(msg) + return services[0] + def take_action(self, parsed_args): compute_client = self.app.client_manager.compute cs = compute_client.services @@ -173,6 +200,20 @@ class SetService(command.Command): "--disable specified.") raise exceptions.CommandError(msg) + # Starting with microversion 2.53, there is a single + # PUT /os-services/{service_id} API for updating nova-compute + # services. If 2.53+ is used we need to find the nova-compute + # service using the --host and --service (binary) values. + requires_service_id = ( + compute_client.api_version >= api_versions.APIVersion('2.53')) + service_id = None + if requires_service_id: + # TODO(mriedem): Add an --id option so users can pass the service + # id (as a uuid) directly rather than make us look it up using + # host/binary. + service_id = SetService._find_service_by_host_and_binary( + cs, parsed_args.host, parsed_args.service).id + result = 0 enabled = None try: @@ -183,14 +224,21 @@ class SetService(command.Command): if enabled is not None: if enabled: - cs.enable(parsed_args.host, parsed_args.service) + args = (service_id,) if requires_service_id else ( + parsed_args.host, parsed_args.service) + cs.enable(*args) else: if parsed_args.disable_reason: - cs.disable_log_reason(parsed_args.host, - parsed_args.service, - parsed_args.disable_reason) + args = (service_id, parsed_args.disable_reason) if \ + requires_service_id else ( + parsed_args.host, + parsed_args.service, + parsed_args.disable_reason) + cs.disable_log_reason(*args) else: - cs.disable(parsed_args.host, parsed_args.service) + args = (service_id,) if requires_service_id else ( + parsed_args.host, parsed_args.service) + cs.disable(*args) except Exception: status = "enabled" if enabled else "disabled" LOG.error("Failed to set service status to %s", status) @@ -208,8 +256,9 @@ class SetService(command.Command): 'required') raise exceptions.CommandError(msg) try: - cs.force_down(parsed_args.host, parsed_args.service, - force_down=force_down) + args = (service_id, force_down) if requires_service_id else ( + parsed_args.host, parsed_args.service, force_down) + cs.force_down(*args) except Exception: state = "down" if force_down else "up" LOG.error("Failed to set service state to %s", state) diff --git a/openstackclient/tests/unit/compute/v2/test_service.py b/openstackclient/tests/unit/compute/v2/test_service.py index bd29912341..0d663b2e67 100644 --- a/openstackclient/tests/unit/compute/v2/test_service.py +++ b/openstackclient/tests/unit/compute/v2/test_service.py @@ -17,6 +17,7 @@ import mock from mock import call from novaclient import api_versions from osc_lib import exceptions +import six from openstackclient.compute.v2 import service from openstackclient.tests.unit.compute.v2 import fakes as compute_fakes @@ -344,7 +345,7 @@ class TestServiceSet(TestService): '2.11') result = self.cmd.take_action(parsed_args) self.service_mock.force_down.assert_called_once_with( - self.service.host, self.service.binary, force_down=False) + self.service.host, self.service.binary, False) self.assertNotCalled(self.service_mock.enable) self.assertNotCalled(self.service_mock.disable) self.assertIsNone(result) @@ -365,7 +366,7 @@ class TestServiceSet(TestService): '2.11') result = self.cmd.take_action(parsed_args) self.service_mock.force_down.assert_called_once_with( - self.service.host, self.service.binary, force_down=True) + self.service.host, self.service.binary, True) self.assertNotCalled(self.service_mock.enable) self.assertNotCalled(self.service_mock.disable) self.assertIsNone(result) @@ -390,7 +391,7 @@ class TestServiceSet(TestService): self.service_mock.enable.assert_called_once_with( self.service.host, self.service.binary) self.service_mock.force_down.assert_called_once_with( - self.service.host, self.service.binary, force_down=True) + self.service.host, self.service.binary, True) self.assertIsNone(result) def test_service_set_enable_and_state_down_with_exception(self): @@ -415,4 +416,99 @@ class TestServiceSet(TestService): self.assertRaises(exceptions.CommandError, self.cmd.take_action, parsed_args) self.service_mock.force_down.assert_called_once_with( - self.service.host, self.service.binary, force_down=True) + self.service.host, self.service.binary, True) + + def test_service_set_2_53_disable_down(self): + # Tests disabling and forcing down a compute service with microversion + # 2.53 which requires looking up the service by host and binary. + arglist = [ + '--disable', + '--down', + self.service.host, + self.service.binary, + ] + verifylist = [ + ('disable', True), + ('down', True), + ('host', self.service.host), + ('service', self.service.binary), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + self.app.client_manager.compute.api_version = api_versions.APIVersion( + '2.53') + service_id = '339478d0-0b95-4a94-be63-d5be05dfeb1c' + self.service_mock.list.return_value = [mock.Mock(id=service_id)] + result = self.cmd.take_action(parsed_args) + self.service_mock.disable.assert_called_once_with(service_id) + self.service_mock.force_down.assert_called_once_with(service_id, True) + self.assertIsNone(result) + + def test_service_set_2_53_disable_reason(self): + # Tests disabling with reason a compute service with microversion + # 2.53 which requires looking up the service by host and binary. + reason = 'earthquake' + arglist = [ + '--disable', + '--disable-reason', reason, + self.service.host, + self.service.binary, + ] + verifylist = [ + ('disable', True), + ('disable_reason', reason), + ('host', self.service.host), + ('service', self.service.binary), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + self.app.client_manager.compute.api_version = api_versions.APIVersion( + '2.53') + service_id = '339478d0-0b95-4a94-be63-d5be05dfeb1c' + self.service_mock.list.return_value = [mock.Mock(id=service_id)] + result = self.cmd.take_action(parsed_args) + self.service_mock.disable_log_reason.assert_called_once_with( + service_id, reason) + self.assertIsNone(result) + + def test_service_set_2_53_enable_up(self): + # Tests enabling and bringing up a compute service with microversion + # 2.53 which requires looking up the service by host and binary. + arglist = [ + '--enable', + '--up', + self.service.host, + self.service.binary, + ] + verifylist = [ + ('enable', True), + ('up', True), + ('host', self.service.host), + ('service', self.service.binary), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + self.app.client_manager.compute.api_version = api_versions.APIVersion( + '2.53') + service_id = '339478d0-0b95-4a94-be63-d5be05dfeb1c' + self.service_mock.list.return_value = [mock.Mock(id=service_id)] + result = self.cmd.take_action(parsed_args) + self.service_mock.enable.assert_called_once_with(service_id) + self.service_mock.force_down.assert_called_once_with(service_id, False) + self.assertIsNone(result) + + def test_service_set_find_service_by_host_and_binary_no_results(self): + # Tests that no compute services are found by host and binary. + self.service_mock.list.return_value = [] + ex = self.assertRaises(exceptions.CommandError, + self.cmd._find_service_by_host_and_binary, + self.service_mock, 'fake-host', 'nova-compute') + self.assertIn('Compute service for host "fake-host" and binary ' + '"nova-compute" not found.', six.text_type(ex)) + + def test_service_set_find_service_by_host_and_binary_many_results(self): + # Tests that more than one compute service is found by host and binary. + self.service_mock.list.return_value = [mock.Mock(), mock.Mock()] + ex = self.assertRaises(exceptions.CommandError, + self.cmd._find_service_by_host_and_binary, + self.service_mock, 'fake-host', 'nova-compute') + self.assertIn('Multiple compute services found for host "fake-host" ' + 'and binary "nova-compute". Unable to proceed.', + six.text_type(ex)) diff --git a/releasenotes/notes/story-2005349-compute-service-set-2.53-3d2db875154e633a.yaml b/releasenotes/notes/story-2005349-compute-service-set-2.53-3d2db875154e633a.yaml new file mode 100644 index 0000000000..cff3196014 --- /dev/null +++ b/releasenotes/notes/story-2005349-compute-service-set-2.53-3d2db875154e633a.yaml @@ -0,0 +1,6 @@ +--- +fixes: + - | + The ``compute service set`` command now properly handles + ``--os-compute-api-version`` 2.53 and greater. + [Story `2005349 `_]