Check for None on service's updated_at

We weren't checking if service's updated_at is None when passing it to
timeutils.normalize_time which caused an exception. This commit adds
the check to cinder-manage and cinder-api (services and hosts). Also
regression unit tests are added.

Change-Id: Ia6ee28dc2cd20cece1e21d07692f47e3858d707d
Closes-Bug: 1495938

diff --git a/cinder/api/contrib/hosts.py b/cinder/api/contrib/hosts.py
index df6a6e2ffe33537056971a85fce2657c8f8bfd54..00ce08ae5d3d973e72c5c33340b50e6c32e747cf 100644 (file)
--- a/cinder/api/contrib/hosts.py
+++ b/cinder/api/contrib/hosts.py
@@ -115,12 +115,15 @@ def _list_hosts(req, service=None):
             active = 'disabled'
         LOG.debug('status, active and update: %s, %s, %s',
                   status, active, host.updated_at)
+        updated_at = host.updated_at
+        if updated_at:
+            updated_at = timeutils.normalize_time(updated_at)
         hosts.append({'host_name': host.host,
                       'service': host.topic,
                       'zone': host.availability_zone,
                       'service-status': status,
                       'service-state': active,
-                      'last-update': timeutils.normalize_time(host.updated_at),
+                      'last-update': updated_at,
                       })
     if service:
         hosts = [host for host in hosts

diff --git a/cinder/api/contrib/services.py b/cinder/api/contrib/services.py
index f07402ff18506b8263cf8f8a55d96ccc17cfac8b..ccb75f5ef10dd18c23c1cd9fddae323ebaf55a18 100644 (file)
--- a/cinder/api/contrib/services.py
+++ b/cinder/api/contrib/services.py
@@ -118,10 +118,12 @@ class ServiceController(wsgi.Controller):
             active = 'enabled'
             if svc.disabled:
                 active = 'disabled'
+            if updated_at:
+                updated_at = timeutils.normalize_time(updated_at)
             ret_fields = {'binary': svc.binary, 'host': svc.host,
                           'zone': svc.availability_zone,
                           'status': active, 'state': art,
-                          'updated_at': timeutils.normalize_time(updated_at)}
+                          'updated_at': updated_at}
             if detailed:
                 ret_fields['disabled_reason'] = svc.disabled_reason
             svcs.append(ret_fields)

diff --git a/cinder/cmd/manage.py b/cinder/cmd/manage.py
index 05834b89c21dcb81da5c2d7cdd19240bf2432f08..be918fdc86b4f4c764e841111d5db2282cf6806a 100644 (file)
--- a/cinder/cmd/manage.py
+++ b/cinder/cmd/manage.py
@@ -451,9 +451,12 @@ class ServiceCommands(object):
             status = 'enabled'
             if svc.disabled:
                 status = 'disabled'
+            updated_at = svc.updated_at
+            if updated_at:
+                updated_at = timeutils.normalize_time(updated_at)
             print(print_format % (svc.binary, svc.host.partition('.')[0],
                                   svc.availability_zone, status, art,
-                                  timeutils.normalize_time(svc.updated_at)))
+                                  updated_at))
 
     @args('binary', type=str,
           help='Service to delete from the host.')

diff --git a/cinder/tests/unit/api/contrib/test_hosts.py b/cinder/tests/unit/api/contrib/test_hosts.py
index 524dff57830619237a4bd4c761850605340a1bb0..4c91351257e84ca75728c400b65dc249d333d81c 100644 (file)
--- a/cinder/tests/unit/api/contrib/test_hosts.py
+++ b/cinder/tests/unit/api/contrib/test_hosts.py
@@ -41,7 +41,11 @@ SERVICE_LIST = [
      'availability_zone': 'cinder'},
     {'created_at': created_time, 'updated_at': curr_time,
      'host': 'test.host.1', 'topic': 'cinder-volume', 'disabled': 0,
-     'availability_zone': 'cinder'}]
+     'availability_zone': 'cinder'},
+    {'created_at': created_time, 'updated_at': None,
+     'host': 'test.host.1', 'topic': 'cinder-volume', 'disabled': 0,
+     'availability_zone': 'cinder'},
+]
 
 LIST_RESPONSE = [{'service-status': 'available', 'service': 'cinder-volume',
                   'zone': 'cinder', 'service-state': 'enabled',
@@ -54,7 +58,11 @@ LIST_RESPONSE = [{'service-status': 'available', 'service': 'cinder-volume',
                   'host_name': 'test.host.1', 'last-update': curr_time},
                  {'service-status': 'available', 'service': 'cinder-volume',
                   'zone': 'cinder', 'service-state': 'enabled',
-                  'host_name': 'test.host.1', 'last-update': curr_time}]
+                  'host_name': 'test.host.1', 'last-update': curr_time},
+                 {'service-status': 'unavailable', 'service': 'cinder-volume',
+                  'zone': 'cinder', 'service-state': 'enabled',
+                  'host_name': 'test.host.1', 'last-update': None},
+                 ]
 
 
 def stub_utcnow(with_timezone=False):

diff --git a/cinder/tests/unit/api/contrib/test_services.py b/cinder/tests/unit/api/contrib/test_services.py
index df664ae06aa1bb09eb9c740a760fe1043597ea32..5cd9c1ebb6e03daf5df55532e006938f0027cec9 100644 (file)
--- a/cinder/tests/unit/api/contrib/test_services.py
+++ b/cinder/tests/unit/api/contrib/test_services.py
@@ -85,6 +85,15 @@ fake_services_list = [
      'created_at': datetime.datetime(2012, 9, 18, 2, 46, 28),
      'disabled_reason': '',
      'modified_at': datetime.datetime(2012, 9, 18, 8, 1, 38)},
+    {'binary': 'cinder-scheduler',
+     'host': 'host2',
+     'availability_zone': 'cinder',
+     'id': 6,
+     'disabled': False,
+     'updated_at': None,
+     'created_at': datetime.datetime(2012, 9, 18, 2, 46, 28),
+     'disabled_reason': '',
+     'modified_at': None},
 ]
 
 
@@ -212,7 +221,13 @@ class ServicesTest(test.TestCase):
                                   'zone': 'cinder',
                                   'status': 'enabled', 'state': 'down',
                                   'updated_at': datetime.datetime(
-                                      2012, 9, 18, 8, 3, 38)}]}
+                                      2012, 9, 18, 8, 3, 38)},
+                                 {'binary': 'cinder-scheduler',
+                                  'host': 'host2',
+                                  'zone': 'cinder',
+                                  'status': 'enabled', 'state': 'down',
+                                  'updated_at': None},
+                                 ]}
         self.assertEqual(response, res_dict)
 
     def test_services_detail(self):
@@ -260,7 +275,14 @@ class ServicesTest(test.TestCase):
                                   'status': 'enabled', 'state': 'down',
                                   'updated_at': datetime.datetime(
                                       2012, 9, 18, 8, 3, 38),
-                                  'disabled_reason': ''}]}
+                                  'disabled_reason': ''},
+                                 {'binary': 'cinder-scheduler',
+                                  'host': 'host2',
+                                  'zone': 'cinder',
+                                  'status': 'enabled', 'state': 'down',
+                                  'updated_at': None,
+                                  'disabled_reason': ''},
+                                 ]}
         self.assertEqual(response, res_dict)
 
     def test_services_list_with_host(self):

diff --git a/cinder/tests/unit/test_cmd.py b/cinder/tests/unit/test_cmd.py
index e30ff0b2af860e2d5babf37613fc93c603ccbde2..6abf79c3cba9e605a7346494c490dbdf0e0b98e4 100644 (file)
--- a/cinder/tests/unit/test_cmd.py
+++ b/cinder/tests/unit/test_cmd.py
@@ -621,15 +621,10 @@ class TestCinderManageCmd(test.TestCase):
     @mock.patch('cinder.utils.service_is_up')
     @mock.patch('cinder.db.service_get_all')
     @mock.patch('cinder.context.get_admin_context')
-    def test_service_commands_list(self, get_admin_context, service_get_all,
-                                   service_is_up):
+    def _test_service_commands_list(self, service, get_admin_context,
+                                    service_get_all, service_is_up):
         ctxt = context.RequestContext('fake-user', 'fake-project')
         get_admin_context.return_value = ctxt
-        service = {'binary': 'cinder-binary',
-                   'host': 'fake-host.fake-domain',
-                   'availability_zone': 'fake-zone',
-                   'updated_at': '2014-06-30 11:22:33',
-                   'disabled': False}
         service_get_all.return_value = [service]
         service_is_up.return_value = True
         with mock.patch('sys.stdout', new=six.StringIO()) as fake_out:
@@ -655,6 +650,22 @@ class TestCinderManageCmd(test.TestCase):
             get_admin_context.assert_called_with()
             service_get_all.assert_called_with(ctxt, None)
 
+    def test_service_commands_list(self):
+        service = {'binary': 'cinder-binary',
+                   'host': 'fake-host.fake-domain',
+                   'availability_zone': 'fake-zone',
+                   'updated_at': '2014-06-30 11:22:33',
+                   'disabled': False}
+        self._test_service_commands_list(service)
+
+    def test_service_commands_list_no_updated_at(self):
+        service = {'binary': 'cinder-binary',
+                   'host': 'fake-host.fake-domain',
+                   'availability_zone': 'fake-zone',
+                   'updated_at': None,
+                   'disabled': False}
+        self._test_service_commands_list(service)
+
     def test_get_arg_string(self):
         args1 = "foobar"
         args2 = "-foo bar"