Merge "Fix compute service set handling for 2.53+"

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,
+                        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,
+                args = (service_id, force_down) if requires_service_id else (
-                              force_down=force_down)
+                    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)

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))

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 <https://storyboard.openstack.org/#!/story/2005349>`_]