Refactor HostAPI.service_update

Change If32bca070185937ef83f689b7163d965a89ec10a is going to
reflect a compute service's disabled status with the
COMPUTE_STATUS_DISABLED trait in placement. This change
does some prep work so that a common method in the HostAPI
(service_update) can be used to abstract that for pre-2.53
and post-2.53 service update APIs.

In the pre-2.53 case, the Service is retrieved using the
host and binary which requires looking up the HostMapping.
In the post-2.53 case, we already have the Service object and
just need to save the changes.

Part of blueprint pre-filter-disabled-computes

Change-Id: Id916ccd21542c90bf80caec246dfdba72a9c58b8

Author: mriedem

nova/api/openstack/compute/services.py

@@ -204,7 +204,8 @@ def _update(self, context, host, binary, payload):
             raise webob.exc.HTTPBadRequest(explanation=msg)
 
         try:
-            self.host_api.service_update(context, host, binary, payload)
+            self.host_api.service_update_by_host_and_binary(
+                context, host, binary, payload)
         except (exception.HostBinaryNotFound,
                 exception.HostMappingNotFound) as exc:
             raise webob.exc.HTTPNotFound(explanation=exc.format_message())
@@ -403,7 +404,7 @@ def update(self, req, id, body):
             raise webob.exc.HTTPBadRequest(explanation=msg)
 
         # Now save our updates to the service record in the database.
-        service.save()
+        self.host_api.service_update(context, service)
 
         # Return the full service record details.
         additional_fields = ['forced_down']

nova/compute/api.py

@@ -5090,22 +5090,44 @@ def service_get_by_compute_host(self, context, host_name):
         """Get service entry for the given compute hostname."""
         return objects.Service.get_by_compute_host(context, host_name)
 
-    def _service_update(self, context, host_name, binary, params_to_update):
-        """Performs the actual service update operation."""
-        service = objects.Service.get_by_args(context, host_name, binary)
-        service.update(params_to_update)
+    def service_update(self, context, service):
+        """Performs the actual service update operation.
+
+        :param context: nova auth RequestContext
+        :param service: nova.objects.Service object with changes already
+            set on the object
+        """
         service.save()
+        # TODO(mriedem): Reflect COMPUTE_STATUS_DISABLED trait changes to the
+        # associated compute node resource providers if the service's disabled
+        # status changed.
         return service
 
     @target_host_cell
-    def service_update(self, context, host_name, binary, params_to_update):
+    def service_update_by_host_and_binary(self, context, host_name, binary,
+                                          params_to_update):
         """Enable / Disable a service.
 
+        Determines the cell that the service is in using the HostMapping.
+
         For compute services, this stops new builds and migrations going to
         the host.
+
+        See also ``service_update``.
+
+        :param context: nova auth RequestContext
+        :param host_name: hostname of the service
+        :param binary: service binary (really only supports "nova-compute")
+        :param params_to_update: dict of changes to make to the Service object
+        :raises: HostMappingNotFound if the host is not mapped to a cell
+        :raises: HostBinaryNotFound if a services table record is not found
+            with the given host_name and binary
         """
-        return self._service_update(context, host_name, binary,
-                                    params_to_update)
+        # TODO(mriedem): Service.get_by_args is deprecated; we should use
+        # get_by_compute_host here (remember to update the "raises" docstring).
+        service = objects.Service.get_by_args(context, host_name, binary)
+        service.update(params_to_update)
+        return self.service_update(context, service)
 
     def _service_delete(self, context, service_id):
         """Performs the actual Service deletion operation."""

nova/tests/unit/compute/test_host_api.py

@@ -320,7 +320,7 @@ def _do_test(mock_service_get_by_compute_host):
 
         _do_test()
 
-    def test_service_update(self):
+    def test_service_update_by_host_and_binary(self):
         host_name = 'fake-host'
         binary = 'nova-compute'
         params_to_update = dict(disabled=True)
@@ -333,7 +333,7 @@ def _do_test(mock_service_update, mock_service_get_by_host_and_binary):
             mock_service_get_by_host_and_binary.return_value = expected_result
             mock_service_update.return_value = expected_result
 
-            result = self.host_api.service_update(
+            result = self.host_api.service_update_by_host_and_binary(
                 self.ctxt, host_name, binary, params_to_update)
             self._compare_obj(result, expected_result)