Fix service version pinning on start

When a service starts (c-sch, c-bak, c-vol) they set the versions for
the RPC client and serializer using existing Service entries in the DB,
but they don't consider themselves when determining the minimum version.

By not considering themselves we have 2 problematic scenarios:

1- When a service is first started it will pin to None version since
there are no Services in the DB 2- When a service is upgraded and
restarted it will pin to the old version since the entry in the DB has
not been updated yet.

The problem is that Service.__init__ creates the service manager, which
in turn creates required RPC API instances, and it is at this point that
the pinning happens without the presence of this service DB entry.

This patch fixes this by moving the creation of the DB entry from the
Service.start method to the Service.__init__ method that gets called
from Service.create method.

There was an alternative solution, but it was more intrusive since it
required changes not only to Service.start but also to all managers
(since each one create different instances RPC APIs.

Closes-Bug: #1569299
backport-potential: mitaka
Change-Id: I21f8e81d9923f44b22cc43c9c173bc560a93c631

cinder/service.py

@@ -129,6 +129,22 @@ class Service(service.Service):
         if CONF.profiler.enabled:
             manager_class = profiler.trace_cls("rpc")(manager_class)
 
+        # NOTE(geguileo): We need to create the Service DB entry before we
+        # create the manager, otherwise capped versions for serializer and rpc
+        # client would used existing DB entries not including us, which could
+        # result in us using None (if it's the first time the service is run)
+        # or an old version (if this is a normal upgrade of a single service).
+        ctxt = context.get_admin_context()
+        try:
+            service_ref = objects.Service.get_by_args(ctxt, host, binary)
+            service_ref.rpc_current_version = manager_class.RPC_API_VERSION
+            obj_version = objects_base.OBJ_VERSIONS.get_current()
+            service_ref.object_current_version = obj_version
+            service_ref.save()
+            self.service_id = service_ref.id
+        except exception.NotFound:
+            self._create_service_ref(ctxt, manager_class.RPC_API_VERSION)
+
         self.manager = manager_class(host=self.host,
                                      service_name=service_name,
                                      *args, **kwargs)
@@ -148,17 +164,6 @@ class Service(service.Service):
                  {'topic': self.topic, 'version_string': version_string})
         self.model_disconnected = False
         self.manager.init_host()
-        ctxt = context.get_admin_context()
-        try:
-            service_ref = objects.Service.get_by_args(
-                ctxt, self.host, self.binary)
-            service_ref.rpc_current_version = self.manager.RPC_API_VERSION
-            obj_version = objects_base.OBJ_VERSIONS.get_current()
-            service_ref.object_current_version = obj_version
-            service_ref.save()
-            self.service_id = service_ref.id
-        except exception.NotFound:
-            self._create_service_ref(ctxt)
 
         LOG.debug("Creating RPC server for service %s", self.topic)
 
@@ -207,7 +212,7 @@ class Service(service.Service):
                      'new_down_time': new_down_time})
                 CONF.set_override('service_down_time', new_down_time)
 
-    def _create_service_ref(self, context):
+    def _create_service_ref(self, context, rpc_version=None):
         zone = CONF.storage_availability_zone
         kwargs = {
             'host': self.host,
@@ -215,7 +220,7 @@ class Service(service.Service):
             'topic': self.topic,
             'report_count': 0,
             'availability_zone': zone,
-            'rpc_current_version': self.manager.RPC_API_VERSION,
+            'rpc_current_version': rpc_version or self.manager.RPC_API_VERSION,
             'object_current_version': objects_base.OBJ_VERSIONS.get_current(),
         }
         service_ref = objects.Service(context=context, **kwargs)

cinder/tests/unit/test_service.py

@@ -136,7 +136,12 @@ class ServiceTestCase(test.TestCase):
                                      binary=self.binary,
                                      topic=self.topic)
 
-        self.assertTrue(app)
+        self.assertIsNotNone(app)
+
+        # Check that we have the service ID
+        self.assertTrue(hasattr(app, 'service_id'))
+        # Check that the entry has been really created in the DB
+        objects.Service.get_by_id(context.get_admin_context(), app.service_id)
 
     def test_report_state_newly_disconnected(self):
         service_ref = {'host': self.host,