From dde6fba200008ecf0eca2defef7361d2edd654ac Mon Sep 17 00:00:00 2001 From: Michal Dulko Date: Tue, 15 Sep 2015 15:52:57 +0200 Subject: [PATCH] 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 --- cinder/api/contrib/hosts.py | 5 +++- cinder/api/contrib/services.py | 4 ++- cinder/cmd/manage.py | 5 +++- cinder/tests/unit/api/contrib/test_hosts.py | 12 +++++++-- .../tests/unit/api/contrib/test_services.py | 26 +++++++++++++++++-- cinder/tests/unit/test_cmd.py | 25 +++++++++++++----- 6 files changed, 63 insertions(+), 14 deletions(-) diff --git a/cinder/api/contrib/hosts.py b/cinder/api/contrib/hosts.py index df6a6e2ffe3..00ce08ae5d3 100644 --- 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 f07402ff185..ccb75f5ef10 100644 --- 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 05834b89c21..be918fdc86b 100644 --- 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 524dff57830..4c91351257e 100644 --- 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 df664ae06aa..5cd9c1ebb6e 100644 --- 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 e30ff0b2af8..6abf79c3cba 100644 --- 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"