From 7efb2c6267417d27938d16712bf0704a2e5f25df Mon Sep 17 00:00:00 2001 From: Gorka Eguileor Date: Tue, 12 Apr 2016 13:50:55 +0200 Subject: [PATCH] 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 | 31 ++++++++++++++++++------------- cinder/tests/unit/test_service.py | 7 ++++++- 2 files changed, 24 insertions(+), 14 deletions(-) diff --git a/cinder/service.py b/cinder/service.py index e9ad71015fc..b57db2e97b8 100644 --- a/cinder/service.py +++ b/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) diff --git a/cinder/tests/unit/test_service.py b/cinder/tests/unit/test_service.py index 143c17c80c1..c41bbf2c2a8 100644 --- a/cinder/tests/unit/test_service.py +++ b/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,