Fix service version to update the DB

When new code is installed with a newer service version, the version in
the DB should get updated.

The fix here is to:
1) eliminate the special case for 'version' in save()
2) cause version to be saved on startup

Change-Id: I96fa9dabfb9b7a5f1703baf80534d8b104dab4e6
Closes-Bug: 1579839

nova/objects/service.py

@@ -291,11 +291,6 @@ def create(self):
     def save(self):
         updates = self.obj_get_changes()
         updates.pop('id', None)
-        if list(updates.keys()) == ['version']:
-            # NOTE(danms): Since we set/dirty version in init, don't
-            # do a save if that's all that has changed. This keeps the
-            # "save is a no-op if nothing has changed" behavior.
-            return
         self._check_minimum_version()
         db_service = db.service_update(self._context, self.id, updates)
         self._from_db_object(self._context, self, db_service)

nova/service.py

@@ -58,21 +58,12 @@ def _create_service_ref(this_service, context):
     return service
 
 
-def _update_service_ref(this_service, context):
-    service = objects.Service.get_by_host_and_binary(context,
-                                                     this_service.host,
-                                                     this_service.binary)
-    if not service:
-        LOG.error(_LE('Unable to find a service record to update for '
-                      '%(binary)s on %(host)s'),
-                  {'binary': this_service.binary,
-                   'host': this_service.host})
-        return
+def _update_service_ref(service):
     if service.version != service_obj.SERVICE_VERSION:
         LOG.info(_LI('Updating service version for %(binary)s on '
                      '%(host)s from %(old)i to %(new)i'),
-                 {'binary': this_service.binary,
-                  'host': this_service.host,
+                 {'binary': service.binary,
+                  'host': service.host,
                   'old': service.version,
                   'new': service_obj.SERVICE_VERSION})
         service.version = service_obj.SERVICE_VERSION
@@ -128,7 +119,10 @@ def start(self):
         ctxt = context.get_admin_context()
         self.service_ref = objects.Service.get_by_host_and_binary(
             ctxt, self.host, self.binary)
-        if not self.service_ref:
+        if self.service_ref:
+            _update_service_ref(self.service_ref)
+
+        else:
             try:
                 self.service_ref = _create_service_ref(self, ctxt)
             except (exception.ServiceTopicExists,
@@ -360,7 +354,9 @@ def start(self):
         ctxt = context.get_admin_context()
         service_ref = objects.Service.get_by_host_and_binary(ctxt, self.host,
                                                              self.binary)
-        if not service_ref:
+        if service_ref:
+            _update_service_ref(service_ref)
+        else:
             try:
                 service_ref = _create_service_ref(self, ctxt)
             except (exception.ServiceTopicExists,
@@ -369,7 +365,6 @@ def start(self):
                 # don't fail here.
                 service_ref = objects.Service.get_by_host_and_binary(
                     ctxt, self.host, self.binary)
-        _update_service_ref(service_ref, ctxt)
 
         if self.manager:
             self.manager.init_host()

nova/tests/unit/objects/test_service.py

@@ -447,22 +447,6 @@ def test_version_loaded_from_db(self):
         obj._from_db_object(self.ctxt, obj, fake_different_service)
         self.assertEqual(fake_version, obj.version)
 
-    def test_save_noop_with_only_version(self):
-        o = objects.Service(context=self.ctxt, id=fake_service['id'])
-        o.obj_reset_changes(['id'])
-        self.assertEqual(set(['version']), o.obj_what_changed())
-        with mock.patch('nova.db.service_update') as mock_update:
-            o.save()
-            self.assertFalse(mock_update.called)
-        o.host = 'foo'
-        with mock.patch('nova.db.service_update') as mock_update:
-            mock_update.return_value = fake_service
-            o.save()
-            mock_update.assert_called_once_with(
-                self.ctxt, fake_service['id'],
-                {'version': service.SERVICE_VERSION,
-                 'host': 'foo'})
-
 
 class TestServiceStatusNotification(test.TestCase):
     def setUp(self):

nova/tests/unit/test_service.py

@@ -157,6 +157,23 @@ def test_init_and_start_hooks(self):
                                'nova.tests.unit.test_service.FakeManager')
         serv.start()
 
+    @mock.patch('nova.objects.service.Service.get_by_host_and_binary')
+    def test_start_updates_version(self, mock_get_by_host_and_binary):
+        # test that the service version gets updated on services startup
+        service_obj = mock.Mock()
+        service_obj.binary = 'fake-binary'
+        service_obj.host = 'fake-host'
+        service_obj.version = -42
+        mock_get_by_host_and_binary.return_value = service_obj
+
+        serv = service.Service(self.host, self.binary, self.topic,
+                              'nova.tests.unit.test_service.FakeManager')
+        serv.start()
+
+        # test service version got updated and saved:
+        service_obj.save.assert_called_once()
+        self.assertEqual(objects.service.SERVICE_VERSION, service_obj.version)
+
     def _test_service_check_create_race(self, ex):
         self.manager_mock = self.mox.CreateMock(FakeManager)
         self.mox.StubOutWithMock(sys.modules[__name__], 'FakeManager',