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 `_]