From 6cfecade9e2ee86bbb7d95c3c401c9e4c70f6a96 Mon Sep 17 00:00:00 2001 From: Gorka Eguileor Date: Mon, 22 Feb 2016 20:00:11 +0100 Subject: [PATCH] Refactor sqlalchemy service methods This patch refactors the sqlalchemy DB methods related to the service to consolidate the methods into the minimum number that is actually needed, to reduce the number of DB calls per operation, and to reduce the data retrieved from the DB server. For method consolidation the patch removes `service_get_by_host_and_topic`, `service_get_by_args`, `service_get_all_by_topic`, and `service_get_all_by_binary`, and includes the functionality provided by those methods in `service_get` and `service_get_all` methods. To reduce the number of DB calls we won't retrieve the service from the DB when deleting or updating a service. And to reduce the data retrieved from the DB we filter UP nodes in the DB instead of locally. This patch is related to the efforts to support HA A/A in c-vol nodes. Job distribution patches are dependent on this one. Specs: https://review.openstack.org/327283 Implements: blueprint cinder-volume-active-active-support Change-Id: I5c453dcb5c38301721c3017ba8e782c0fdf850e6 --- cinder/db/api.py | 42 ++--- cinder/db/sqlalchemy/api.py | 152 ++++++++---------- cinder/db/sqlalchemy/models.py | 8 +- cinder/objects/service.py | 14 +- cinder/test.py | 7 + .../unit/api/contrib/test_admin_actions.py | 7 +- cinder/tests/unit/api/contrib/test_backups.py | 139 ++++++++-------- cinder/tests/unit/api/contrib/test_hosts.py | 11 +- .../tests/unit/api/contrib/test_services.py | 47 +++--- .../unit/api/contrib/test_snapshot_manage.py | 10 +- .../unit/api/contrib/test_volume_manage.py | 16 +- cinder/tests/unit/api/v1/stubs.py | 2 +- .../tests/unit/api/v1/test_volume_metadata.py | 6 +- cinder/tests/unit/api/v1/test_volumes.py | 5 +- cinder/tests/unit/api/v2/stubs.py | 4 + .../tests/unit/api/v2/test_volume_metadata.py | 5 +- cinder/tests/unit/api/v2/test_volumes.py | 5 +- cinder/tests/unit/objects/test_service.py | 60 +++---- .../test_allocated_capacity_weigher.py | 10 +- .../unit/scheduler/test_capacity_weigher.py | 10 +- .../unit/scheduler/test_filter_scheduler.py | 44 ++--- .../tests/unit/scheduler/test_host_manager.py | 40 ++--- .../scheduler/test_volume_number_weigher.py | 10 +- cinder/tests/unit/test_cmd.py | 9 +- cinder/tests/unit/test_db_api.py | 104 +++++------- cinder/tests/unit/test_service.py | 84 ++++------ cinder/tests/unit/test_volume.py | 145 +++++++---------- 27 files changed, 459 insertions(+), 537 deletions(-) diff --git a/cinder/db/api.py b/cinder/db/api.py index 7ee9b905a31..234269a67b6 100644 --- a/cinder/db/api.py +++ b/cinder/db/api.py @@ -99,34 +99,27 @@ def service_destroy(context, service_id): return IMPL.service_destroy(context, service_id) -def service_get(context, service_id): - """Get a service or raise if it does not exist.""" - return IMPL.service_get(context, service_id) +def service_get(context, service_id=None, **filters): + """Get a service that matches the criteria. + + A possible filter is is_up=True and it will filter nodes that are down. + + :param service_id: Id of the service. + :param filters: Filters for the query in the form of key/value. + + :raise ServiceNotFound: If service doesn't exist. + """ + return IMPL.service_get(context, service_id, **filters) -def service_get_by_host_and_topic(context, host, topic): - """Get a service by host it's on and topic it listens to.""" - return IMPL.service_get_by_host_and_topic(context, host, topic) +def service_get_all(context, **filters): + """Get all services that match the criteria. + A possible filter is is_up=True and it will filter nodes that are down. -def service_get_all(context, filters=None): - """Get all services.""" - return IMPL.service_get_all(context, filters) - - -def service_get_all_by_topic(context, topic, disabled=None): - """Get all services for a given topic.""" - return IMPL.service_get_all_by_topic(context, topic, disabled=disabled) - - -def service_get_all_by_binary(context, binary, disabled=None): - """Get all services for a given binary.""" - return IMPL.service_get_all_by_binary(context, binary, disabled) - - -def service_get_by_args(context, host, binary): - """Get the state of a service by node name and binary.""" - return IMPL.service_get_by_args(context, host, binary) + :param filters: Filters for the query in the form of key/value arguments. + """ + return IMPL.service_get_all(context, **filters) def service_create(context, values): @@ -138,7 +131,6 @@ def service_update(context, service_id, values): """Set the given properties on an service and update it. Raises NotFound if service does not exist. - """ return IMPL.service_update(context, service_id, values) diff --git a/cinder/db/sqlalchemy/api.py b/cinder/db/sqlalchemy/api.py index dce988c47a7..bd82ae6850f 100644 --- a/cinder/db/sqlalchemy/api.py +++ b/cinder/db/sqlalchemy/api.py @@ -362,105 +362,84 @@ QUOTA_SYNC_FUNCTIONS = { ################### -@require_admin_context -def service_destroy(context, service_id): - session = get_session() - with session.begin(): - service_ref = _service_get(context, service_id, session=session) - service_ref.delete(session=session) +def _clean_filters(filters): + return {k: v for k, v in filters.items() if v is not None} -@require_admin_context -def _service_get(context, service_id, session=None): - result = model_query( - context, - models.Service, - session=session).\ - filter_by(id=service_id).\ - first() - if not result: - raise exception.ServiceNotFound(service_id=service_id) - - return result - - -@require_admin_context -def service_get(context, service_id): - return _service_get(context, service_id) - - -@require_admin_context -def service_get_all(context, filters=None): +def _service_query(context, session=None, read_deleted='no', host=None, + is_up=None, **filters): + filters = _clean_filters(filters) if filters and not is_valid_model_filters(models.Service, filters): - return [] + return None - query = model_query(context, models.Service) + query = model_query(context, models.Service, session=session, + read_deleted=read_deleted) + + # Host is a particular case of filter, because we must retrieve not only + # exact matches (single backend configuration), but also match hosts that + # have the backend defined (multi backend configuration). + if host: + host_attr = models.Service.host + # Mysql is not doing case sensitive filtering, so we force it + if CONF.database.connection.startswith('mysql:'): + conditions = or_(host_attr == func.binary(host), + host_attr.op('LIKE BINARY')(host + '@%')) + else: + conditions = or_(host_attr == host, + host_attr.op('LIKE')(host + '@%')) + query = query.filter(conditions) if filters: - try: - host = filters.pop('host') - host_attr = models.Service.host - conditions = or_(host_attr == - host, host_attr.op('LIKE')(host + '@%')) - query = query.filter(conditions) - except KeyError: - pass - query = query.filter_by(**filters) - return query.all() + if is_up is not None: + date_limit = (timeutils.utcnow() - + dt.timedelta(seconds=CONF.service_down_time)) + svc = models.Service + filter_ = or_( + and_(svc.created_at.isnot(None), svc.created_at >= date_limit), + and_(svc.updated_at.isnot(None), svc.updated_at >= date_limit)) + query = query.filter(filter_ == is_up) + + return query @require_admin_context -def service_get_all_by_topic(context, topic, disabled=None): - query = model_query( - context, models.Service, read_deleted="no").\ - filter_by(topic=topic) - - if disabled is not None: - query = query.filter_by(disabled=disabled) - - return query.all() +def service_destroy(context, service_id): + query = _service_query(context, id=service_id) + if not query.update(models.Service.delete_values()): + raise exception.ServiceNotFound(service_id=service_id) @require_admin_context -def service_get_all_by_binary(context, binary, disabled=None): - query = model_query( - context, models.Service, read_deleted="no").filter_by(binary=binary) +def service_get(context, service_id=None, **filters): + """Get a service that matches the criteria. - if disabled is not None: - query = query.filter_by(disabled=disabled) + A possible filter is is_up=True and it will filter nodes that are down. - return query.all() + :param service_id: Id of the service. + :param filters: Filters for the query in the form of key/value. + :raise ServiceNotFound: If service doesn't exist. + """ + query = _service_query(context, id=service_id, **filters) + service = None if not query else query.first() + if not service: + serv_id = service_id or filters.get('topic') or filters.get('binary') + raise exception.ServiceNotFound(service_id=serv_id, + host=filters.get('host')) + return service @require_admin_context -def service_get_by_host_and_topic(context, host, topic): - result = model_query( - context, models.Service, read_deleted="no").\ - filter_by(disabled=False).\ - filter_by(host=host).\ - filter_by(topic=topic).\ - first() - if not result: - raise exception.ServiceNotFound(service_id=topic, - host=host) - return result +def service_get_all(context, **filters): + """Get all services that match the criteria. + A possible filter is is_up=True and it will filter nodes that are down. -@require_admin_context -def service_get_by_args(context, host, binary): - results = model_query(context, models.Service).\ - filter_by(host=host).\ - filter_by(binary=binary).\ - all() - - for result in results: - if host == result['host']: - return result - - raise exception.ServiceNotFound(service_id=binary, - host=host) + :param filters: Filters for the query in the form of key/value. + """ + query = _service_query(context, **filters) + return [] if not query else query.all() @require_admin_context @@ -479,14 +458,15 @@ def service_create(context, values): @require_admin_context @_retry_on_deadlock def service_update(context, service_id, values): - session = get_session() - with session.begin(): - service_ref = _service_get(context, service_id, session=session) - if ('disabled' in values): - service_ref['modified_at'] = timeutils.utcnow() - service_ref['updated_at'] = literal_column('updated_at') - service_ref.update(values) - return service_ref + if 'disabled' in values: + values = values.copy() + values['modified_at'] = values.get('modified_at', timeutils.utcnow()) + values['updated_at'] = values.get('updated_at', + literal_column('updated_at')) + query = _service_query(context, id=service_id) + result = query.update(values) + if not result: + raise exception.ServiceNotFound(service_id=service_id) ################### diff --git a/cinder/db/sqlalchemy/models.py b/cinder/db/sqlalchemy/models.py index d7879cf980d..8edfcd2e770 100644 --- a/cinder/db/sqlalchemy/models.py +++ b/cinder/db/sqlalchemy/models.py @@ -45,10 +45,14 @@ class CinderBase(models.TimestampMixin, deleted = Column(Boolean, default=False) metadata = None + @staticmethod + def delete_values(): + return {'deleted': True, + 'deleted_at': timeutils.utcnow()} + def delete(self, session): """Delete this object.""" - self.deleted = True - self.deleted_at = timeutils.utcnow() + self.update(self.delete_values()) self.save(session=session) diff --git a/cinder/objects/service.py b/cinder/objects/service.py index d64f21be697..c1e7dce466c 100644 --- a/cinder/objects/service.py +++ b/cinder/objects/service.py @@ -70,12 +70,13 @@ class Service(base.CinderPersistentObject, base.CinderObject, @classmethod def get_by_host_and_topic(cls, context, host, topic): - db_service = db.service_get_by_host_and_topic(context, host, topic) + db_service = db.service_get(context, disabled=False, host=host, + topic=topic) return cls._from_db_object(context, cls(context), db_service) @classmethod def get_by_args(cls, context, host, binary_key): - db_service = db.service_get_by_args(context, host, binary_key) + db_service = db.service_get(context, host=host, binary=binary_key) return cls._from_db_object(context, cls(context), db_service) def create(self): @@ -139,20 +140,19 @@ class ServiceList(base.ObjectListBase, base.CinderObject): @classmethod def get_all(cls, context, filters=None): - services = db.service_get_all(context, filters) + services = db.service_get_all(context, **(filters or {})) return base.obj_make_list(context, cls(context), objects.Service, services) @classmethod def get_all_by_topic(cls, context, topic, disabled=None): - services = db.service_get_all_by_topic(context, topic, - disabled=disabled) + services = db.service_get_all(context, topic=topic, disabled=disabled) return base.obj_make_list(context, cls(context), objects.Service, services) @classmethod def get_all_by_binary(cls, context, binary, disabled=None): - services = db.service_get_all_by_binary(context, binary, - disabled=disabled) + services = db.service_get_all(context, binary=binary, + disabled=disabled) return base.obj_make_list(context, cls(context), objects.Service, services) diff --git a/cinder/test.py b/cinder/test.py index b1ce45c4b92..a17c792cdde 100644 --- a/cinder/test.py +++ b/cinder/test.py @@ -321,6 +321,13 @@ class TestCase(testtools.TestCase): self.addCleanup(patcher.stop) return new_attr + def patch(self, path, *args, **kwargs): + """Use python mock to mock a path with automatic cleanup.""" + patcher = mock.patch(path, *args, **kwargs) + result = patcher.start() + self.addCleanup(patcher.stop) + return result + # Useful assertions def assertDictMatch(self, d1, d2, approx_equal=False, tolerance=0.001): """Assert two dicts are equivalent. diff --git a/cinder/tests/unit/api/contrib/test_admin_actions.py b/cinder/tests/unit/api/contrib/test_admin_actions.py index 680668b1750..409a740cd60 100644 --- a/cinder/tests/unit/api/contrib/test_admin_actions.py +++ b/cinder/tests/unit/api/contrib/test_admin_actions.py @@ -659,11 +659,12 @@ class AdminActionsTest(BaseAdminTest): vac.validate_update, {'status': 'creating'}) - @mock.patch('cinder.db.service_get_all_by_topic') + @mock.patch('cinder.backup.rpcapi.BackupAPI.delete_backup', mock.Mock()) + @mock.patch('cinder.db.service_get_all') @mock.patch('cinder.backup.api.API._check_support_to_force_delete') def _force_delete_backup_util(self, test_status, mock_check_support, - _mock_service_get_all_by_topic): - _mock_service_get_all_by_topic.return_value = [ + mock_service_get_all): + mock_service_get_all.return_value = [ {'availability_zone': "az1", 'host': 'testhost', 'disabled': 0, 'updated_at': timeutils.utcnow()}] # admin context diff --git a/cinder/tests/unit/api/contrib/test_backups.py b/cinder/tests/unit/api/contrib/test_backups.py index 17fc97d7d58..e4a02d82b60 100644 --- a/cinder/tests/unit/api/contrib/test_backups.py +++ b/cinder/tests/unit/api/contrib/test_backups.py @@ -57,6 +57,8 @@ class BackupsAPITestCase(test.TestCase): self.user_context = context.RequestContext( fake.USER_ID, fake.PROJECT_ID, auth_token=True) self.controller = backups.BackupsController() + self.patch('cinder.objects.service.Service._get_minimum_version', + return_value=None) @staticmethod def _create_backup(volume_id=fake.VOLUME_ID, @@ -462,12 +464,12 @@ class BackupsAPITestCase(test.TestCase): fake_auth_context=self.user_context)) self.assertEqual(400, res.status_int) - @mock.patch('cinder.db.service_get_all_by_topic') + @mock.patch('cinder.db.service_get_all') @mock.patch( 'cinder.api.openstack.wsgi.Controller.validate_name_and_description') def test_create_backup_json(self, mock_validate, - _mock_service_get_all_by_topic): - _mock_service_get_all_by_topic.return_value = [ + _mock_service_get_all): + _mock_service_get_all.return_value = [ {'availability_zone': 'fake_az', 'host': 'testhost', 'disabled': 0, 'updated_at': timeutils.utcnow()}] @@ -491,15 +493,17 @@ class BackupsAPITestCase(test.TestCase): self.assertEqual(202, res.status_int) self.assertIn('id', res_dict['backup']) - self.assertTrue(_mock_service_get_all_by_topic.called) + _mock_service_get_all.assert_called_once_with(mock.ANY, + disabled=False, + topic='cinder-backup') self.assertTrue(mock_validate.called) db.volume_destroy(context.get_admin_context(), volume_id) - @mock.patch('cinder.db.service_get_all_by_topic') + @mock.patch('cinder.db.service_get_all') def test_create_backup_inuse_no_force(self, - _mock_service_get_all_by_topic): - _mock_service_get_all_by_topic.return_value = [ + _mock_service_get_all): + _mock_service_get_all.return_value = [ {'availability_zone': 'fake_az', 'host': 'testhost', 'disabled': 0, 'updated_at': timeutils.utcnow()}] @@ -528,9 +532,9 @@ class BackupsAPITestCase(test.TestCase): db.volume_destroy(context.get_admin_context(), volume_id) - @mock.patch('cinder.db.service_get_all_by_topic') - def test_create_backup_inuse_force(self, _mock_service_get_all_by_topic): - _mock_service_get_all_by_topic.return_value = [ + @mock.patch('cinder.db.service_get_all') + def test_create_backup_inuse_force(self, _mock_service_get_all): + _mock_service_get_all.return_value = [ {'availability_zone': 'fake_az', 'host': 'testhost', 'disabled': 0, 'updated_at': timeutils.utcnow()}] @@ -557,17 +561,19 @@ class BackupsAPITestCase(test.TestCase): self.assertEqual(202, res.status_int) self.assertIn('id', res_dict['backup']) - self.assertTrue(_mock_service_get_all_by_topic.called) + _mock_service_get_all.assert_called_once_with(mock.ANY, + disabled=False, + topic='cinder-backup') db.backup_destroy(context.get_admin_context(), backup_id) db.volume_destroy(context.get_admin_context(), volume_id) - @mock.patch('cinder.db.service_get_all_by_topic') + @mock.patch('cinder.db.service_get_all') @mock.patch( 'cinder.api.openstack.wsgi.Controller.validate_name_and_description') def test_create_backup_snapshot_json(self, mock_validate, - _mock_service_get_all_by_topic): - _mock_service_get_all_by_topic.return_value = [ + _mock_service_get_all): + _mock_service_get_all.return_value = [ {'availability_zone': 'fake_az', 'host': 'testhost', 'disabled': 0, 'updated_at': timeutils.utcnow()}] @@ -591,7 +597,9 @@ class BackupsAPITestCase(test.TestCase): res_dict = jsonutils.loads(res.body) self.assertEqual(202, res.status_int) self.assertIn('id', res_dict['backup']) - self.assertTrue(_mock_service_get_all_by_topic.called) + _mock_service_get_all.assert_called_once_with(mock.ANY, + disabled=False, + topic='cinder-backup') self.assertTrue(mock_validate.called) db.volume_destroy(context.get_admin_context(), volume_id) @@ -705,14 +713,14 @@ class BackupsAPITestCase(test.TestCase): req, body) - @mock.patch('cinder.db.service_get_all_by_topic') + @mock.patch('cinder.db.service_get_all') @mock.patch( 'cinder.api.openstack.wsgi.Controller.validate_name_and_description') @ddt.data(False, True) def test_create_backup_delta(self, backup_from_snapshot, mock_validate, - _mock_service_get_all_by_topic): - _mock_service_get_all_by_topic.return_value = [ + _mock_service_get_all): + _mock_service_get_all.return_value = [ {'availability_zone': 'fake_az', 'host': 'testhost', 'disabled': 0, 'updated_at': timeutils.utcnow()}] @@ -746,7 +754,9 @@ class BackupsAPITestCase(test.TestCase): self.assertEqual(202, res.status_int) self.assertIn('id', res_dict['backup']) - self.assertTrue(_mock_service_get_all_by_topic.called) + _mock_service_get_all.assert_called_once_with(mock.ANY, + disabled=False, + topic='cinder-backup') self.assertTrue(mock_validate.called) db.backup_destroy(context.get_admin_context(), backup_id) @@ -754,10 +764,10 @@ class BackupsAPITestCase(test.TestCase): snapshot.destroy() db.volume_destroy(context.get_admin_context(), volume_id) - @mock.patch('cinder.db.service_get_all_by_topic') + @mock.patch('cinder.db.service_get_all') def test_create_incremental_backup_invalid_status( - self, _mock_service_get_all_by_topic): - _mock_service_get_all_by_topic.return_value = [ + self, _mock_service_get_all): + _mock_service_get_all.return_value = [ {'availability_zone': 'fake_az', 'host': 'testhost', 'disabled': 0, 'updated_at': timeutils.utcnow()}] @@ -869,12 +879,12 @@ class BackupsAPITestCase(test.TestCase): self.assertEqual(400, res.status_int) self.assertEqual(400, res_dict['badRequest']['code']) - @mock.patch('cinder.db.service_get_all_by_topic') + @mock.patch('cinder.db.service_get_all') def test_create_backup_WithOUT_enabled_backup_service( self, - _mock_service_get_all_by_topic): + _mock_service_get_all): # need an enabled backup service available - _mock_service_get_all_by_topic.return_value = [] + _mock_service_get_all.return_value = [] volume_id = utils.create_volume(self.context, size=2).id req = webob.Request.blank('/v2/%s/backups' % fake.PROJECT_ID) @@ -901,10 +911,10 @@ class BackupsAPITestCase(test.TestCase): volume = self.volume_api.get(context.get_admin_context(), volume_id) self.assertEqual('available', volume['status']) - @mock.patch('cinder.db.service_get_all_by_topic') + @mock.patch('cinder.db.service_get_all') def test_create_incremental_backup_invalid_no_full( - self, _mock_service_get_all_by_topic): - _mock_service_get_all_by_topic.return_value = [ + self, _mock_service_get_all): + _mock_service_get_all.return_value = [ {'availability_zone': 'fake_az', 'host': 'testhost', 'disabled': 0, 'updated_at': timeutils.utcnow()}] @@ -934,8 +944,8 @@ class BackupsAPITestCase(test.TestCase): db.volume_destroy(context.get_admin_context(), volume_id) - @mock.patch('cinder.db.service_get_all_by_topic') - def test_is_backup_service_enabled(self, _mock_service_get_all_by_topic): + @mock.patch('cinder.db.service_get_all') + def test_is_backup_service_enabled(self, _mock_service_get_all): testhost = 'test_host' alt_host = 'strange_host' @@ -960,12 +970,12 @@ class BackupsAPITestCase(test.TestCase): 'disabled': 0, 'updated_at': timeutils.utcnow()}] # Setup mock to run through the following service cases - _mock_service_get_all_by_topic.side_effect = [empty_service, - host_not_match, - az_not_match, - disabled_service, - dead_service, - multi_services] + _mock_service_get_all.side_effect = [empty_service, + host_not_match, + az_not_match, + disabled_service, + dead_service, + multi_services] volume_id = utils.create_volume(self.context, size=2, host=testhost).id @@ -1006,10 +1016,9 @@ class BackupsAPITestCase(test.TestCase): volume['availability_zone'], testhost)) - @mock.patch('cinder.db.service_get_all_by_topic') - def test_get_available_backup_service(self, - _mock_service_get_all_by_topic): - _mock_service_get_all_by_topic.return_value = [ + @mock.patch('cinder.db.service_get_all') + def test_get_available_backup_service(self, _mock_service_get_all): + _mock_service_get_all.return_value = [ {'availability_zone': 'az1', 'host': 'testhost1', 'disabled': 0, 'updated_at': timeutils.utcnow()}, {'availability_zone': 'az2', 'host': 'testhost2', @@ -1026,10 +1035,10 @@ class BackupsAPITestCase(test.TestCase): 'testhost4', 'az1') self.assertEqual('testhost1', actual_host) - @mock.patch('cinder.db.service_get_all_by_topic') + @mock.patch('cinder.db.service_get_all') def test_get_available_backup_service_with_same_host( - self, _mock_service_get_all_by_topic): - _mock_service_get_all_by_topic.return_value = [ + self, _mock_service_get_all): + _mock_service_get_all.return_value = [ {'availability_zone': 'az1', 'host': 'testhost1', 'disabled': 0, 'updated_at': timeutils.utcnow()}, {'availability_zone': 'az2', 'host': 'testhost2', @@ -1045,10 +1054,9 @@ class BackupsAPITestCase(test.TestCase): self.backup_api._get_available_backup_service_host, 'testhost4', 'az1') - @mock.patch('cinder.db.service_get_all_by_topic') - def test_delete_backup_available( - self, _mock_service_get_all_by_topic): - _mock_service_get_all_by_topic.return_value = [ + @mock.patch('cinder.db.service_get_all') + def test_delete_backup_available(self, _mock_service_get_all): + _mock_service_get_all.return_value = [ {'availability_zone': 'az1', 'host': 'testhost', 'disabled': 0, 'updated_at': timeutils.utcnow()}] backup_id = self._create_backup(status=fields.BackupStatus.AVAILABLE) @@ -1065,10 +1073,10 @@ class BackupsAPITestCase(test.TestCase): db.backup_destroy(context.get_admin_context(), backup_id) - @mock.patch('cinder.db.service_get_all_by_topic') + @mock.patch('cinder.db.service_get_all') def test_delete_delta_backup(self, - _mock_service_get_all_by_topic): - _mock_service_get_all_by_topic.return_value = [ + _mock_service_get_all): + _mock_service_get_all.return_value = [ {'availability_zone': 'az1', 'host': 'testhost', 'disabled': 0, 'updated_at': timeutils.utcnow()}] backup_id = self._create_backup(status=fields.BackupStatus.AVAILABLE) @@ -1088,10 +1096,10 @@ class BackupsAPITestCase(test.TestCase): db.backup_destroy(context.get_admin_context(), delta_id) db.backup_destroy(context.get_admin_context(), backup_id) - @mock.patch('cinder.db.service_get_all_by_topic') + @mock.patch('cinder.db.service_get_all') def test_delete_backup_error(self, - _mock_service_get_all_by_topic): - _mock_service_get_all_by_topic.return_value = [ + _mock_service_get_all): + _mock_service_get_all.return_value = [ {'availability_zone': 'az1', 'host': 'testhost', 'disabled': 0, 'updated_at': timeutils.utcnow()}] backup_id = self._create_backup(status=fields.BackupStatus.ERROR) @@ -1141,10 +1149,10 @@ class BackupsAPITestCase(test.TestCase): db.backup_destroy(context.get_admin_context(), backup_id) - @mock.patch('cinder.db.service_get_all_by_topic') + @mock.patch('cinder.db.service_get_all') def test_delete_backup_with_InvalidBackup2(self, - _mock_service_get_all_by_topic): - _mock_service_get_all_by_topic.return_value = [ + _mock_service_get_all): + _mock_service_get_all.return_value = [ {'availability_zone': 'az1', 'host': 'testhost', 'disabled': 0, 'updated_at': timeutils.utcnow()}] volume_id = utils.create_volume(self.context, size=5).id @@ -1170,10 +1178,10 @@ class BackupsAPITestCase(test.TestCase): db.backup_destroy(context.get_admin_context(), delta_backup_id) db.backup_destroy(context.get_admin_context(), backup_id) - @mock.patch('cinder.db.service_get_all_by_topic') + @mock.patch('cinder.db.service_get_all') def test_delete_backup_service_down(self, - _mock_service_get_all_by_topic): - _mock_service_get_all_by_topic.return_value = [ + _mock_service_get_all): + _mock_service_get_all.return_value = [ {'availability_zone': 'az1', 'host': 'testhost', 'disabled': 0, 'updated_at': '1775-04-19 05:00:00'}] backup_id = self._create_backup(status='available') @@ -1256,18 +1264,17 @@ class BackupsAPITestCase(test.TestCase): self.assertEqual("Missing required element 'restore' in request body.", res_dict['badRequest']['message']) - @mock.patch('cinder.db.service_get_all_by_topic') + @mock.patch('cinder.db.service_get_all') @mock.patch('cinder.volume.api.API.create') def test_restore_backup_volume_id_unspecified( - self, _mock_volume_api_create, - _mock_service_get_all_by_topic): + self, _mock_volume_api_create, _mock_service_get_all): # intercept volume creation to ensure created volume # has status of available def fake_volume_api_create(context, size, name, description): volume_id = utils.create_volume(self.context, size=size).id return db.volume_get(context, volume_id) - _mock_service_get_all_by_topic.return_value = [ + _mock_service_get_all.return_value = [ {'availability_zone': 'az1', 'host': 'testhost', 'disabled': 0, 'updated_at': timeutils.utcnow()}] _mock_volume_api_create.side_effect = fake_volume_api_create @@ -1288,11 +1295,11 @@ class BackupsAPITestCase(test.TestCase): self.assertEqual(202, res.status_int) self.assertEqual(backup_id, res_dict['restore']['backup_id']) - @mock.patch('cinder.db.service_get_all_by_topic') + @mock.patch('cinder.db.service_get_all') @mock.patch('cinder.volume.api.API.create') def test_restore_backup_name_specified(self, _mock_volume_api_create, - _mock_service_get_all_by_topic): + _mock_service_get_all): # Intercept volume creation to ensure created volume # has status of available def fake_volume_api_create(context, size, name, description): @@ -1301,7 +1308,7 @@ class BackupsAPITestCase(test.TestCase): return db.volume_get(context, volume_id) _mock_volume_api_create.side_effect = fake_volume_api_create - _mock_service_get_all_by_topic.return_value = [ + _mock_service_get_all.return_value = [ {'availability_zone': 'az1', 'host': 'testhost', 'disabled': 0, 'updated_at': timeutils.utcnow()}] diff --git a/cinder/tests/unit/api/contrib/test_hosts.py b/cinder/tests/unit/api/contrib/test_hosts.py index f482a172d17..19fbf3b6856 100644 --- a/cinder/tests/unit/api/contrib/test_hosts.py +++ b/cinder/tests/unit/api/contrib/test_hosts.py @@ -21,7 +21,6 @@ import webob.exc from cinder.api.contrib import hosts as os_hosts from cinder import context -from cinder import db from cinder import test @@ -69,10 +68,6 @@ def stub_utcnow(with_timezone=False): return datetime.datetime(2013, 7, 3, 0, 0, 2, tzinfo=tzinfo) -def stub_service_get_all(context, filters=None): - return SERVICE_LIST - - class FakeRequest(object): environ = {'cinder.context': context.get_admin_context()} GET = {} @@ -90,9 +85,9 @@ class HostTestCase(test.TestCase): super(HostTestCase, self).setUp() self.controller = os_hosts.HostController() self.req = FakeRequest() - self.stubs.Set(db, 'service_get_all', - stub_service_get_all) - self.stubs.Set(timeutils, 'utcnow', stub_utcnow) + self.patch('cinder.db.service_get_all', autospec=True, + return_value=SERVICE_LIST) + self.mock_object(timeutils, 'utcnow', stub_utcnow) def _test_host_update(self, host, key, val, expected_value): body = {key: val} diff --git a/cinder/tests/unit/api/contrib/test_services.py b/cinder/tests/unit/api/contrib/test_services.py index 9c330060a15..1e77d5325d3 100644 --- a/cinder/tests/unit/api/contrib/test_services.py +++ b/cinder/tests/unit/api/contrib/test_services.py @@ -17,15 +17,13 @@ import datetime from iso8601 import iso8601 -from oslo_utils import timeutils +import mock import webob.exc from cinder.api.contrib import services from cinder.api import extensions from cinder import context -from cinder import db from cinder import exception -from cinder import policy from cinder import test from cinder.tests.unit.api import fakes from cinder.tests.unit import fake_constants as fake @@ -132,21 +130,24 @@ class FakeRequestWithHostBinary(object): GET = {"host": "host1", "binary": "cinder-volume"} -def fake_service_get_all(context, filters=None): - filters = filters or {} - host = filters.get('host') - binary = filters.get('binary') - return [s for s in fake_services_list - if (not host or s['host'] == host or - s['host'].startswith(host + '@')) - and (not binary or s['binary'] == binary)] - - -def fake_service_get_by_host_binary(context, host, binary): +def fake_service_get_all(context, **filters): + result = [] + host = filters.pop('host', None) for service in fake_services_list: - if service['host'] == host and service['binary'] == binary: - return service - return None + if (host and service['host'] != host and + not service['host'].startswith(host + '@')): + continue + + if all(v is None or service.get(k) == v for k, v in filters.items()): + result.append(service) + return result + + +def fake_service_get(context, service_id=None, **filters): + result = fake_service_get_all(context, id=service_id, **filters) + if not result: + raise exception.ServiceNotFound(service_id=service_id) + return result[0] def fake_service_get_by_id(value): @@ -174,18 +175,16 @@ def fake_utcnow(with_timezone=False): return datetime.datetime(2012, 10, 29, 13, 42, 11, tzinfo=tzinfo) +@mock.patch('cinder.db.service_get_all', fake_service_get_all) +@mock.patch('cinder.db.service_get', fake_service_get) +@mock.patch('oslo_utils.timeutils.utcnow', fake_utcnow) +@mock.patch('cinder.db.sqlalchemy.api.service_update', fake_service_update) +@mock.patch('cinder.policy.enforce', fake_policy_enforce) class ServicesTest(test.TestCase): def setUp(self): super(ServicesTest, self).setUp() - self.stubs.Set(db, "service_get_all", fake_service_get_all) - self.stubs.Set(timeutils, "utcnow", fake_utcnow) - self.stubs.Set(db, "service_get_by_args", - fake_service_get_by_host_binary) - self.stubs.Set(db, "service_update", fake_service_update) - self.stubs.Set(policy, "enforce", fake_policy_enforce) - self.context = context.get_admin_context() self.ext_mgr = extensions.ExtensionManager() self.ext_mgr.extensions = {} diff --git a/cinder/tests/unit/api/contrib/test_snapshot_manage.py b/cinder/tests/unit/api/contrib/test_snapshot_manage.py index 0bdd8bd447d..59d84ffec66 100644 --- a/cinder/tests/unit/api/contrib/test_snapshot_manage.py +++ b/cinder/tests/unit/api/contrib/test_snapshot_manage.py @@ -109,7 +109,7 @@ class SnapshotManageTest(test.TestCase): @mock.patch('cinder.volume.rpcapi.VolumeAPI.manage_existing_snapshot') @mock.patch('cinder.volume.api.API.create_snapshot_in_db') - @mock.patch('cinder.db.service_get_by_args') + @mock.patch('cinder.db.service_get') def test_manage_snapshot_ok(self, mock_db, mock_create_snapshot, mock_rpcapi): """Test successful manage volume execution. @@ -126,11 +126,9 @@ class SnapshotManageTest(test.TestCase): res = self._get_resp_post(body) self.assertEqual(202, res.status_int, res) - # Check the db.service_get_by_host_and_topic was called with correct - # arguments. - self.assertEqual(1, mock_db.call_count) - args = mock_db.call_args[0] - self.assertEqual('fake_host', args[1]) + # Check the db.service_get was called with correct arguments. + mock_db.assert_called_once_with( + mock.ANY, host='fake_host', binary='cinder-volume') # Check the create_snapshot_in_db was called with correct arguments. self.assertEqual(1, mock_create_snapshot.call_count) diff --git a/cinder/tests/unit/api/contrib/test_volume_manage.py b/cinder/tests/unit/api/contrib/test_volume_manage.py index 22f67595d29..6d475aae291 100644 --- a/cinder/tests/unit/api/contrib/test_volume_manage.py +++ b/cinder/tests/unit/api/contrib/test_volume_manage.py @@ -40,13 +40,13 @@ def app(): return mapper -def db_service_get_by_host_and_topic(context, host, topic): - """Replacement for db.service_get_by_host_and_topic. +def service_get_by_host_and_topic(context, host, topic): + """Replacement for Service.service_get_by_host_and_topic. - We stub the db.service_get_by_host_and_topic method to return something - for a specific host, and raise an exception for anything else. We don't - use the returned data (the code under test just use the call to check for - existence of a host, so the content returned doesn't matter. + We mock the Service.service_get_by_host_and_topic method to return + something for a specific host, and raise an exception for anything else. + We don't use the returned data (the code under test just use the call to + check for existence of a host, so the content returned doesn't matter. """ if host == 'host_ok': return {} @@ -126,8 +126,8 @@ def api_get_manageable_volumes(*args, **kwargs): return vols -@mock.patch('cinder.db.service_get_by_host_and_topic', - db_service_get_by_host_and_topic) +@mock.patch('cinder.objects.service.Service.get_by_host_and_topic', + service_get_by_host_and_topic) @mock.patch('cinder.volume.volume_types.get_volume_type_by_name', vt_get_volume_type_by_name) @mock.patch('cinder.volume.volume_types.get_volume_type', diff --git a/cinder/tests/unit/api/v1/stubs.py b/cinder/tests/unit/api/v1/stubs.py index 80a4db480cd..628d3f1368d 100644 --- a/cinder/tests/unit/api/v1/stubs.py +++ b/cinder/tests/unit/api/v1/stubs.py @@ -190,7 +190,7 @@ def stub_snapshot_update(self, context, *args, **param): pass -def stub_service_get_all_by_topic(context, topic, disabled=None): +def stub_service_get_all(context, **filters): return [{'availability_zone': "zone1:host1", "disabled": 0}] diff --git a/cinder/tests/unit/api/v1/test_volume_metadata.py b/cinder/tests/unit/api/v1/test_volume_metadata.py index 31fcc32a42b..bf049b10031 100644 --- a/cinder/tests/unit/api/v1/test_volume_metadata.py +++ b/cinder/tests/unit/api/v1/test_volume_metadata.py @@ -145,8 +145,10 @@ class volumeMetaDataTest(test.TestCase): self.stubs.Set(volume.api.API, 'get', stubs.stub_volume_get) self.stubs.Set(cinder.db, 'volume_metadata_get', return_volume_metadata) - self.stubs.Set(cinder.db, 'service_get_all_by_topic', - stubs.stub_service_get_all_by_topic) + self.patch( + 'cinder.db.service_get_all', autospec=True, + return_value=stubs.stub_service_get_all(None)) + self.ext_mgr = extensions.ExtensionManager() self.ext_mgr.extensions = {} self.volume_controller = volumes.VolumeController(self.ext_mgr) diff --git a/cinder/tests/unit/api/v1/test_volumes.py b/cinder/tests/unit/api/v1/test_volumes.py index f941759d78a..9b86723a15f 100644 --- a/cinder/tests/unit/api/v1/test_volumes.py +++ b/cinder/tests/unit/api/v1/test_volumes.py @@ -48,8 +48,9 @@ class VolumeApiTest(test.TestCase): self.controller = volumes.VolumeController(self.ext_mgr) self.stubs.Set(db, 'volume_get_all', stubs.stub_volume_get_all) - self.stubs.Set(db, 'service_get_all_by_topic', - stubs.stub_service_get_all_by_topic) + self.patch( + 'cinder.db.service_get_all', autospec=True, + return_value=stubs.stub_service_get_all(None)) self.stubs.Set(volume_api.API, 'delete', stubs.stub_volume_delete) def test_volume_create(self): diff --git a/cinder/tests/unit/api/v2/stubs.py b/cinder/tests/unit/api/v2/stubs.py index daa90ab2576..dd19f7d8322 100644 --- a/cinder/tests/unit/api/v2/stubs.py +++ b/cinder/tests/unit/api/v2/stubs.py @@ -220,6 +220,10 @@ def stub_snapshot_update(self, context, *args, **param): pass +def stub_service_get_all(*args, **kwargs): + return [{'availability_zone': "zone1:host1", "disabled": 0}] + + def stub_service_get_all_by_topic(context, topic, disabled=None): return [{'availability_zone': "zone1:host1", "disabled": 0}] diff --git a/cinder/tests/unit/api/v2/test_volume_metadata.py b/cinder/tests/unit/api/v2/test_volume_metadata.py index 80e9c85d346..a8feb1bb860 100644 --- a/cinder/tests/unit/api/v2/test_volume_metadata.py +++ b/cinder/tests/unit/api/v2/test_volume_metadata.py @@ -132,8 +132,9 @@ class volumeMetaDataTest(test.TestCase): self.stubs.Set(volume.api.API, 'get', get_volume) self.stubs.Set(db, 'volume_metadata_get', return_volume_metadata) - self.stubs.Set(db, 'service_get_all_by_topic', - stubs.stub_service_get_all_by_topic) + self.patch( + 'cinder.db.service_get_all', autospec=True, + return_value=stubs.stub_service_get_all_by_topic(None, None)) self.stubs.Set(self.volume_api, 'update_volume_metadata', fake_update_volume_metadata) diff --git a/cinder/tests/unit/api/v2/test_volumes.py b/cinder/tests/unit/api/v2/test_volumes.py index 903c16ed826..acdd242053d 100644 --- a/cinder/tests/unit/api/v2/test_volumes.py +++ b/cinder/tests/unit/api/v2/test_volumes.py @@ -59,8 +59,9 @@ class VolumeApiTest(test.TestCase): self.stubs.Set(db, 'volume_get_all', stubs.stub_volume_get_all) self.stubs.Set(volume_api.API, 'delete', stubs.stub_volume_delete) - self.stubs.Set(db, 'service_get_all_by_topic', - stubs.stub_service_get_all_by_topic) + self.patch( + 'cinder.db.service_get_all', autospec=True, + return_value=stubs.stub_service_get_all_by_topic(None, None)) self.maxDiff = None self.ctxt = context.RequestContext(fake.USER_ID, fake.PROJECT_ID, True) diff --git a/cinder/tests/unit/objects/test_service.py b/cinder/tests/unit/objects/test_service.py index 57586d8d1a9..e0e97ca9f5e 100644 --- a/cinder/tests/unit/objects/test_service.py +++ b/cinder/tests/unit/objects/test_service.py @@ -30,25 +30,25 @@ class TestService(test_objects.BaseObjectsTestCase): self._compare(self, db_service, service) service_get.assert_called_once_with(self.context, 1) - @mock.patch('cinder.db.service_get_by_host_and_topic') - def test_get_by_host_and_topic(self, service_get_by_host_and_topic): + @mock.patch('cinder.db.service_get') + def test_get_by_host_and_topic(self, service_get): db_service = fake_service.fake_db_service() - service_get_by_host_and_topic.return_value = db_service + service_get.return_value = db_service service = objects.Service.get_by_host_and_topic( self.context, 'fake-host', 'fake-topic') self._compare(self, db_service, service) - service_get_by_host_and_topic.assert_called_once_with( - self.context, 'fake-host', 'fake-topic') + service_get.assert_called_once_with( + self.context, disabled=False, host='fake-host', topic='fake-topic') - @mock.patch('cinder.db.service_get_by_args') - def test_get_by_args(self, service_get_by_args): + @mock.patch('cinder.db.service_get') + def test_get_by_args(self, service_get): db_service = fake_service.fake_db_service() - service_get_by_args.return_value = db_service + service_get.return_value = db_service service = objects.Service.get_by_args( self.context, 'fake-host', 'fake-key') self._compare(self, db_service, service) - service_get_by_args.assert_called_once_with( - self.context, 'fake-host', 'fake-key') + service_get.assert_called_once_with( + self.context, host='fake-host', binary='fake-key') @mock.patch('cinder.db.service_create') def test_create(self, service_create): @@ -102,21 +102,21 @@ class TestService(test_objects.BaseObjectsTestCase): call_bool, mock.call(self.context, 123)]) - @mock.patch('cinder.db.service_get_all_by_binary') + @mock.patch('cinder.db.service_get_all') def _test_get_minimum_version(self, services_update, expected, - service_get_all_by_binary): + service_get_all): services = [fake_service.fake_db_service(**s) for s in services_update] - service_get_all_by_binary.return_value = services + service_get_all.return_value = services min_rpc = objects.Service.get_minimum_rpc_version(self.context, 'foo') self.assertEqual(expected[0], min_rpc) min_obj = objects.Service.get_minimum_obj_version(self.context, 'foo') self.assertEqual(expected[1], min_obj) - service_get_all_by_binary.assert_has_calls( - [mock.call(self.context, 'foo', disabled=None)] * 2) + service_get_all.assert_has_calls( + [mock.call(self.context, binary='foo', disabled=None)] * 2) - @mock.patch('cinder.db.service_get_all_by_binary') - def test_get_minimum_version(self, service_get_all_by_binary): + @mock.patch('cinder.db.service_get_all') + def test_get_minimum_version(self, service_get_all): services_update = [ {'rpc_current_version': '1.0', 'object_current_version': '1.3'}, {'rpc_current_version': '1.1', 'object_current_version': '1.2'}, @@ -125,8 +125,8 @@ class TestService(test_objects.BaseObjectsTestCase): expected = ('1.0', '1.2') self._test_get_minimum_version(services_update, expected) - @mock.patch('cinder.db.service_get_all_by_binary') - def test_get_minimum_version_liberty(self, service_get_all_by_binary): + @mock.patch('cinder.db.service_get_all') + def test_get_minimum_version_liberty(self, service_get_all): services_update = [ {'rpc_current_version': '1.0', 'object_current_version': '1.3'}, {'rpc_current_version': '1.1', 'object_current_version': None}, @@ -144,30 +144,30 @@ class TestServiceList(test_objects.BaseObjectsTestCase): filters = {'host': 'host', 'binary': 'foo', 'disabled': False} services = objects.ServiceList.get_all(self.context, filters) - service_get_all.assert_called_once_with(self.context, filters) + service_get_all.assert_called_once_with(self.context, **filters) self.assertEqual(1, len(services)) TestService._compare(self, db_service, services[0]) - @mock.patch('cinder.db.service_get_all_by_topic') - def test_get_all_by_topic(self, service_get_all_by_topic): + @mock.patch('cinder.db.service_get_all') + def test_get_all_by_topic(self, service_get_all): db_service = fake_service.fake_db_service() - service_get_all_by_topic.return_value = [db_service] + service_get_all.return_value = [db_service] services = objects.ServiceList.get_all_by_topic( self.context, 'foo', 'bar') - service_get_all_by_topic.assert_called_once_with( - self.context, 'foo', disabled='bar') + service_get_all.assert_called_once_with( + self.context, topic='foo', disabled='bar') self.assertEqual(1, len(services)) TestService._compare(self, db_service, services[0]) - @mock.patch('cinder.db.service_get_all_by_binary') - def test_get_all_by_binary(self, service_get_all_by_binary): + @mock.patch('cinder.db.service_get_all') + def test_get_all_by_binary(self, service_get_all): db_service = fake_service.fake_db_service() - service_get_all_by_binary.return_value = [db_service] + service_get_all.return_value = [db_service] services = objects.ServiceList.get_all_by_binary( self.context, 'foo', 'bar') - service_get_all_by_binary.assert_called_once_with( - self.context, 'foo', disabled='bar') + service_get_all.assert_called_once_with( + self.context, binary='foo', disabled='bar') self.assertEqual(1, len(services)) TestService._compare(self, db_service, services[0]) diff --git a/cinder/tests/unit/scheduler/test_allocated_capacity_weigher.py b/cinder/tests/unit/scheduler/test_allocated_capacity_weigher.py index f01c9e8bb5f..e1ca2451f8b 100644 --- a/cinder/tests/unit/scheduler/test_allocated_capacity_weigher.py +++ b/cinder/tests/unit/scheduler/test_allocated_capacity_weigher.py @@ -43,14 +43,14 @@ class AllocatedCapacityWeigherTestCase(test.TestCase): [weights.capacity.AllocatedCapacityWeigher], hosts, weight_properties)[0] - @mock.patch('cinder.db.sqlalchemy.api.service_get_all_by_topic') - def _get_all_hosts(self, _mock_service_get_all_by_topic, disabled=False): + @mock.patch('cinder.db.sqlalchemy.api.service_get_all') + def _get_all_hosts(self, _mock_service_get_all, disabled=False): ctxt = context.get_admin_context() - fakes.mock_host_manager_db_calls(_mock_service_get_all_by_topic, + fakes.mock_host_manager_db_calls(_mock_service_get_all, disabled=disabled) host_states = self.host_manager.get_all_host_states(ctxt) - _mock_service_get_all_by_topic.assert_called_once_with( - ctxt, CONF.volume_topic, disabled=disabled) + _mock_service_get_all.assert_called_once_with( + ctxt, topic=CONF.volume_topic, disabled=disabled) return host_states def test_default_of_spreading_first(self): diff --git a/cinder/tests/unit/scheduler/test_capacity_weigher.py b/cinder/tests/unit/scheduler/test_capacity_weigher.py index fb6826a655e..c762abb03e5 100644 --- a/cinder/tests/unit/scheduler/test_capacity_weigher.py +++ b/cinder/tests/unit/scheduler/test_capacity_weigher.py @@ -43,14 +43,14 @@ class CapacityWeigherTestCase(test.TestCase): hosts, weight_properties) - @mock.patch('cinder.db.sqlalchemy.api.service_get_all_by_topic') - def _get_all_hosts(self, _mock_service_get_all_by_topic, disabled=False): + @mock.patch('cinder.db.sqlalchemy.api.service_get_all') + def _get_all_hosts(self, _mock_service_get_all, disabled=False): ctxt = context.get_admin_context() - fakes.mock_host_manager_db_calls(_mock_service_get_all_by_topic, + fakes.mock_host_manager_db_calls(_mock_service_get_all, disabled=disabled) host_states = self.host_manager.get_all_host_states(ctxt) - _mock_service_get_all_by_topic.assert_called_once_with( - ctxt, CONF.volume_topic, disabled=disabled) + _mock_service_get_all.assert_called_once_with( + ctxt, topic=CONF.volume_topic, disabled=disabled) return host_states # If thin_provisioning_support = False, use the following formula: diff --git a/cinder/tests/unit/scheduler/test_filter_scheduler.py b/cinder/tests/unit/scheduler/test_filter_scheduler.py index c1e3cd3df92..b1c896d0925 100644 --- a/cinder/tests/unit/scheduler/test_filter_scheduler.py +++ b/cinder/tests/unit/scheduler/test_filter_scheduler.py @@ -50,16 +50,16 @@ class FilterSchedulerTestCase(test_scheduler.SchedulerTestCase): sched.schedule_create_consistencygroup, fake_context, 'faki-id1', request_spec_list, {}) - @mock.patch('cinder.db.service_get_all_by_topic') + @mock.patch('cinder.db.service_get_all') def test_schedule_consistencygroup(self, - _mock_service_get_all_by_topic): + _mock_service_get_all): # Make sure _schedule_group() can find host successfully. sched = fakes.FakeFilterScheduler() sched.host_manager = fakes.FakeHostManager() fake_context = context.RequestContext('user', 'project', is_admin=True) - fakes.mock_host_manager_db_calls(_mock_service_get_all_by_topic) + fakes.mock_host_manager_db_calls(_mock_service_get_all) specs = {'capabilities:consistencygroup_support': ' True'} request_spec = {'volume_properties': {'project_id': 1, @@ -75,12 +75,12 @@ class FilterSchedulerTestCase(test_scheduler.SchedulerTestCase): request_spec_list, {}) self.assertIsNotNone(weighed_host.obj) - self.assertTrue(_mock_service_get_all_by_topic.called) + self.assertTrue(_mock_service_get_all.called) - @mock.patch('cinder.db.service_get_all_by_topic') + @mock.patch('cinder.db.service_get_all') def test_schedule_consistencygroup_no_cg_support_in_extra_specs( self, - _mock_service_get_all_by_topic): + _mock_service_get_all): # Make sure _schedule_group() can find host successfully even # when consistencygroup_support is not specified in volume type's # extra specs @@ -89,7 +89,7 @@ class FilterSchedulerTestCase(test_scheduler.SchedulerTestCase): fake_context = context.RequestContext('user', 'project', is_admin=True) - fakes.mock_host_manager_db_calls(_mock_service_get_all_by_topic) + fakes.mock_host_manager_db_calls(_mock_service_get_all) request_spec = {'volume_properties': {'project_id': 1, 'size': 0}, @@ -104,7 +104,7 @@ class FilterSchedulerTestCase(test_scheduler.SchedulerTestCase): request_spec_list, {}) self.assertIsNotNone(weighed_host.obj) - self.assertTrue(_mock_service_get_all_by_topic.called) + self.assertTrue(_mock_service_get_all.called) def test_create_volume_no_hosts(self): # Ensure empty hosts/child_zones result in NoValidHosts exception. @@ -174,8 +174,8 @@ class FilterSchedulerTestCase(test_scheduler.SchedulerTestCase): fake_context, request_spec, {}) self.assertTrue(self.was_admin) - @mock.patch('cinder.db.service_get_all_by_topic') - def test_schedule_happy_day(self, _mock_service_get_all_by_topic): + @mock.patch('cinder.db.service_get_all') + def test_schedule_happy_day(self, _mock_service_get_all): # Make sure there's nothing glaringly wrong with _schedule() # by doing a happy day pass through. sched = fakes.FakeFilterScheduler() @@ -183,16 +183,16 @@ class FilterSchedulerTestCase(test_scheduler.SchedulerTestCase): fake_context = context.RequestContext('user', 'project', is_admin=True) - fakes.mock_host_manager_db_calls(_mock_service_get_all_by_topic) + fakes.mock_host_manager_db_calls(_mock_service_get_all) request_spec = {'volume_type': {'name': 'LVM_iSCSI'}, 'volume_properties': {'project_id': 1, 'size': 1}} weighed_host = sched._schedule(fake_context, request_spec, {}) self.assertIsNotNone(weighed_host.obj) - self.assertTrue(_mock_service_get_all_by_topic.called) + self.assertTrue(_mock_service_get_all.called) - @mock.patch('cinder.db.service_get_all_by_topic') + @mock.patch('cinder.db.service_get_all') def test_create_volume_clear_host_different_with_cg(self, _mock_service_get_all): # Ensure we clear those hosts whose backend is not same as @@ -208,7 +208,7 @@ class FilterSchedulerTestCase(test_scheduler.SchedulerTestCase): weighed_host = sched._schedule(fake_context, request_spec, {}) self.assertIsNone(weighed_host) - @mock.patch('cinder.db.service_get_all_by_topic') + @mock.patch('cinder.db.service_get_all') def test_create_volume_host_same_as_cg(self, _mock_service_get_all): # Ensure we don't clear the host whose backend is same as # consistencygroup's backend. @@ -338,7 +338,7 @@ class FilterSchedulerTestCase(test_scheduler.SchedulerTestCase): return (sched, fake_context) - @mock.patch('cinder.db.service_get_all_by_topic') + @mock.patch('cinder.db.service_get_all') def test_host_passes_filters_happy_day(self, _mock_service_get_topic): """Do a successful pass through of with host_passes_filters().""" sched, ctx = self._host_passes_filters_setup( @@ -352,7 +352,7 @@ class FilterSchedulerTestCase(test_scheduler.SchedulerTestCase): self.assertEqual('host1', utils.extract_host(ret_host.host)) self.assertTrue(_mock_service_get_topic.called) - @mock.patch('cinder.db.service_get_all_by_topic') + @mock.patch('cinder.db.service_get_all') def test_host_passes_filters_default_pool_happy_day( self, _mock_service_get_topic): """Do a successful pass through of with host_passes_filters().""" @@ -367,7 +367,7 @@ class FilterSchedulerTestCase(test_scheduler.SchedulerTestCase): self.assertEqual('host5', utils.extract_host(ret_host.host)) self.assertTrue(_mock_service_get_topic.called) - @mock.patch('cinder.db.service_get_all_by_topic') + @mock.patch('cinder.db.service_get_all') def test_host_passes_filters_no_capacity(self, _mock_service_get_topic): """Fail the host due to insufficient capacity.""" sched, ctx = self._host_passes_filters_setup( @@ -381,7 +381,7 @@ class FilterSchedulerTestCase(test_scheduler.SchedulerTestCase): ctx, 'host1#lvm1', request_spec, {}) self.assertTrue(_mock_service_get_topic.called) - @mock.patch('cinder.db.service_get_all_by_topic') + @mock.patch('cinder.db.service_get_all') def test_retype_policy_never_migrate_pass(self, _mock_service_get_topic): # Retype should pass if current host passes filters and # policy=never. host4 doesn't have enough space to hold an additional @@ -401,7 +401,7 @@ class FilterSchedulerTestCase(test_scheduler.SchedulerTestCase): migration_policy='never') self.assertEqual('host4', utils.extract_host(host_state.host)) - @mock.patch('cinder.db.service_get_all_by_topic') + @mock.patch('cinder.db.service_get_all') def test_retype_with_pool_policy_never_migrate_pass( self, _mock_service_get_topic): # Retype should pass if current host passes filters and @@ -422,7 +422,7 @@ class FilterSchedulerTestCase(test_scheduler.SchedulerTestCase): migration_policy='never') self.assertEqual('host3#lvm3', host_state.host) - @mock.patch('cinder.db.service_get_all_by_topic') + @mock.patch('cinder.db.service_get_all') def test_retype_policy_never_migrate_fail(self, _mock_service_get_topic): # Retype should fail if current host doesn't pass filters and # policy=never. @@ -439,7 +439,7 @@ class FilterSchedulerTestCase(test_scheduler.SchedulerTestCase): request_spec, filter_properties={}, migration_policy='never') - @mock.patch('cinder.db.service_get_all_by_topic') + @mock.patch('cinder.db.service_get_all') def test_retype_policy_demand_migrate_pass(self, _mock_service_get_topic): # Retype should pass if current host fails filters but another host # is suitable when policy=on-demand. @@ -457,7 +457,7 @@ class FilterSchedulerTestCase(test_scheduler.SchedulerTestCase): migration_policy='on-demand') self.assertEqual('host1', utils.extract_host(host_state.host)) - @mock.patch('cinder.db.service_get_all_by_topic') + @mock.patch('cinder.db.service_get_all') def test_retype_policy_demand_migrate_fail(self, _mock_service_get_topic): # Retype should fail if current host doesn't pass filters and # no other suitable candidates exist even if policy=on-demand. diff --git a/cinder/tests/unit/scheduler/test_host_manager.py b/cinder/tests/unit/scheduler/test_host_manager.py index f1f99e8e29c..9c45d7839b7 100644 --- a/cinder/tests/unit/scheduler/test_host_manager.py +++ b/cinder/tests/unit/scheduler/test_host_manager.py @@ -121,8 +121,8 @@ class HostManagerTestCase(test.TestCase): self.assertDictMatch(expected, service_states) @mock.patch('cinder.utils.service_is_up') - @mock.patch('cinder.db.service_get_all_by_topic') - def test_has_all_capabilities(self, _mock_service_get_all_by_topic, + @mock.patch('cinder.db.service_get_all') + def test_has_all_capabilities(self, _mock_service_get_all, _mock_service_is_up): _mock_service_is_up.return_value = True services = [ @@ -133,8 +133,8 @@ class HostManagerTestCase(test.TestCase): dict(id=3, host='host3', topic='volume', disabled=False, availability_zone='zone1', updated_at=timeutils.utcnow()), ] - _mock_service_get_all_by_topic.return_value = services - # Create host_manager again to let db.service_get_all_by_topic mock run + _mock_service_get_all.return_value = services + # Create host_manager again to let db.service_get_all mock run self.host_manager = host_manager.HostManager() self.assertFalse(self.host_manager.has_all_capabilities()) @@ -153,12 +153,12 @@ class HostManagerTestCase(test.TestCase): host3_volume_capabs) self.assertTrue(self.host_manager.has_all_capabilities()) - @mock.patch('cinder.db.service_get_all_by_topic') + @mock.patch('cinder.db.service_get_all') @mock.patch('cinder.utils.service_is_up') @mock.patch('oslo_utils.timeutils.utcnow') def test_update_and_get_pools(self, _mock_utcnow, _mock_service_is_up, - _mock_service_get_all_by_topic): + _mock_service_get_all): """Test interaction between update and get_pools This test verifies that each time that get_pools is called it gets the @@ -182,7 +182,7 @@ class HostManagerTestCase(test.TestCase): timestamp=None, reserved_percentage=0), } - _mock_service_get_all_by_topic.return_value = services + _mock_service_get_all.return_value = services _mock_service_is_up.return_value = True _mock_warning = mock.Mock() host_manager.LOG.warn = _mock_warning @@ -206,10 +206,10 @@ class HostManagerTestCase(test.TestCase): self.assertEqual(1, len(res)) self.assertEqual(dates[2], res[0]['capabilities']['timestamp']) - @mock.patch('cinder.db.service_get_all_by_topic') + @mock.patch('cinder.db.service_get_all') @mock.patch('cinder.utils.service_is_up') def test_get_all_host_states(self, _mock_service_is_up, - _mock_service_get_all_by_topic): + _mock_service_get_all): context = 'fake_context' topic = CONF.volume_topic @@ -257,16 +257,16 @@ class HostManagerTestCase(test.TestCase): # First test: service_is_up is always True, host5 is disabled, # host4 has no capabilities self.host_manager.service_states = service_states - _mock_service_get_all_by_topic.return_value = services + _mock_service_get_all.return_value = services _mock_service_is_up.return_value = True _mock_warning = mock.Mock() host_manager.LOG.warning = _mock_warning # Get all states self.host_manager.get_all_host_states(context) - _mock_service_get_all_by_topic.assert_called_with(context, - topic, - disabled=False) + _mock_service_get_all.assert_called_with(context, + disabled=False, + topic=topic) expected = [] for service in service_objs: expected.append(mock.call(service)) @@ -284,14 +284,14 @@ class HostManagerTestCase(test.TestCase): # Second test: Now service_is_up returns False for host3 _mock_service_is_up.reset_mock() _mock_service_is_up.side_effect = [True, True, False, True] - _mock_service_get_all_by_topic.reset_mock() + _mock_service_get_all.reset_mock() _mock_warning.reset_mock() # Get all states, make sure host 3 is reported as down self.host_manager.get_all_host_states(context) - _mock_service_get_all_by_topic.assert_called_with(context, - topic, - disabled=False) + _mock_service_get_all.assert_called_with(context, + disabled=False, + topic=topic) self.assertEqual(expected, _mock_service_is_up.call_args_list) self.assertGreater(_mock_warning.call_count, 0) @@ -306,10 +306,10 @@ class HostManagerTestCase(test.TestCase): test_service.TestService._compare(self, volume_node, host_state_map[host].service) - @mock.patch('cinder.db.service_get_all_by_topic') + @mock.patch('cinder.db.service_get_all') @mock.patch('cinder.utils.service_is_up') def test_get_pools(self, _mock_service_is_up, - _mock_service_get_all_by_topic): + _mock_service_get_all): context = 'fake_context' services = [ @@ -336,7 +336,7 @@ class HostManagerTestCase(test.TestCase): provisioned_capacity_gb=9300), } - _mock_service_get_all_by_topic.return_value = services + _mock_service_get_all.return_value = services _mock_service_is_up.return_value = True _mock_warning = mock.Mock() host_manager.LOG.warn = _mock_warning diff --git a/cinder/tests/unit/scheduler/test_volume_number_weigher.py b/cinder/tests/unit/scheduler/test_volume_number_weigher.py index 5512470c7fe..9982cdcde9d 100644 --- a/cinder/tests/unit/scheduler/test_volume_number_weigher.py +++ b/cinder/tests/unit/scheduler/test_volume_number_weigher.py @@ -69,14 +69,14 @@ class VolumeNumberWeigherTestCase(test.TestCase): hosts, weight_properties)[0] - @mock.patch('cinder.db.sqlalchemy.api.service_get_all_by_topic') - def _get_all_hosts(self, _mock_service_get_all_by_topic, disabled=False): + @mock.patch('cinder.db.sqlalchemy.api.service_get_all') + def _get_all_hosts(self, _mock_service_get_all, disabled=False): ctxt = context.get_admin_context() - fakes.mock_host_manager_db_calls(_mock_service_get_all_by_topic, + fakes.mock_host_manager_db_calls(_mock_service_get_all, disabled=disabled) host_states = self.host_manager.get_all_host_states(ctxt) - _mock_service_get_all_by_topic.assert_called_once_with( - ctxt, CONF.volume_topic, disabled=disabled) + _mock_service_get_all.assert_called_once_with( + ctxt, topic=CONF.volume_topic, disabled=disabled) return host_states def test_volume_number_weight_multiplier1(self): diff --git a/cinder/tests/unit/test_cmd.py b/cinder/tests/unit/test_cmd.py index 7de6375dd11..13fd5feb307 100644 --- a/cinder/tests/unit/test_cmd.py +++ b/cinder/tests/unit/test_cmd.py @@ -439,7 +439,7 @@ class TestCinderManageCmd(test.TestCase): host_cmds.list() get_admin_context.assert_called_once_with() - service_get_all.assert_called_once_with(mock.sentinel.ctxt, None) + service_get_all.assert_called_once_with(mock.sentinel.ctxt) self.assertEqual(expected_out, fake_out.getvalue()) @mock.patch('cinder.db.service_get_all') @@ -462,7 +462,7 @@ class TestCinderManageCmd(test.TestCase): host_cmds.list(zone='fake-az1') get_admin_context.assert_called_once_with() - service_get_all.assert_called_once_with(mock.sentinel.ctxt, None) + service_get_all.assert_called_once_with(mock.sentinel.ctxt) self.assertEqual(expected_out, fake_out.getvalue()) @mock.patch('cinder.objects.base.CinderObjectSerializer') @@ -747,7 +747,7 @@ class TestCinderManageCmd(test.TestCase): self.assertEqual(expected_out, fake_out.getvalue()) get_admin_context.assert_called_with() - service_get_all.assert_called_with(ctxt, None) + service_get_all.assert_called_with(ctxt) def test_service_commands_list(self): service = {'binary': 'cinder-binary', @@ -858,8 +858,7 @@ class TestCinderManageCmd(test.TestCase): self.assertEqual(2, exit) @mock.patch('cinder.db.service_destroy') - @mock.patch('cinder.db.service_get_by_args', - return_value = {'id': '12'}) + @mock.patch('cinder.db.service_get', return_value = {'id': '12'}) def test_remove_service_success(self, mock_get_by_args, mock_service_destroy): service_commands = cinder_manage.ServiceCommands() diff --git a/cinder/tests/unit/test_db_api.py b/cinder/tests/unit/test_db_api.py index 7e60bcad1bf..d2132699904 100644 --- a/cinder/tests/unit/test_db_api.py +++ b/cinder/tests/unit/test_db_api.py @@ -18,6 +18,7 @@ import datetime import enum import mock +from oslo_config import cfg from oslo_utils import uuidutils import six @@ -33,6 +34,7 @@ from cinder import test from cinder.tests.unit import fake_constants as fake from cinder.tests.unit import utils +CONF = cfg.CONF THREE = 3 THREE_HUNDREDS = 300 ONE_HUNDREDS = 100 @@ -127,7 +129,12 @@ class DBAPIServiceTestCase(BaseTest): def _create_service(self, values): v = self._get_base_values() v.update(values) - return db.service_create(self.ctxt, v) + service = db.service_create(self.ctxt, v) + # We need to read the contents from the DB if we have set updated_at + # or created_at fields + if 'updated_at' in values or 'created_at' in values: + service = db.service_get(self.ctxt, service.id) + return service def test_service_create(self): service = self._create_service({}) @@ -142,8 +149,9 @@ class DBAPIServiceTestCase(BaseTest): db.service_destroy(self.ctxt, service1['id']) self.assertRaises(exception.ServiceNotFound, db.service_get, self.ctxt, service1['id']) - self._assertEqualObjects(db.service_get(self.ctxt, service2['id']), - service2) + self._assertEqualObjects( + service2, + db.service_get(self.ctxt, service2['id'])) def test_service_update(self): service = self._create_service({}) @@ -175,33 +183,35 @@ class DBAPIServiceTestCase(BaseTest): def test_service_get_by_host_and_topic(self): service1 = self._create_service({'host': 'host1', 'topic': 'topic1'}) - real_service1 = db.service_get_by_host_and_topic(self.ctxt, - host='host1', - topic='topic1') + real_service1 = db.service_get(self.ctxt, host='host1', topic='topic1') self._assertEqualObjects(service1, real_service1) def test_service_get_all(self): + expired = (datetime.datetime.utcnow() + - datetime.timedelta(seconds=CONF.service_down_time + 1)) values = [ - {'host': 'host1', 'binary': 'b1'}, + {'host': 'host1', 'binary': 'b1', 'created_at': expired}, {'host': 'host1@ceph', 'binary': 'b2'}, {'host': 'host2', 'binary': 'b2'}, - {'disabled': True} + {'disabled': True, 'created_at': expired, 'updated_at': expired} ] services = [self._create_service(vals) for vals in values] - disabled_services = [services[-1]] + disabled_services = services[-1:] non_disabled_services = services[:-1] + up_services = services[1:3] + down_services = [services[0], services[3]] expected = services[:2] expected_bin = services[1:3] compares = [ - (services, db.service_get_all(self.ctxt, {})), (services, db.service_get_all(self.ctxt)), - (expected, db.service_get_all(self.ctxt, {'host': 'host1'})), - (expected_bin, db.service_get_all(self.ctxt, {'binary': 'b2'})), - (disabled_services, db.service_get_all(self.ctxt, - {'disabled': True})), + (expected, db.service_get_all(self.ctxt, host='host1')), + (expected_bin, db.service_get_all(self.ctxt, binary='b2')), + (disabled_services, db.service_get_all(self.ctxt, disabled=True)), (non_disabled_services, db.service_get_all(self.ctxt, - {'disabled': False})), + disabled=False)), + (up_services, db.service_get_all(self.ctxt, is_up=True)), + (down_services, db.service_get_all(self.ctxt, is_up=False)), ] for comp in compares: self._assertEqualListsOfObjects(*comp) @@ -215,7 +225,7 @@ class DBAPIServiceTestCase(BaseTest): ] services = [self._create_service(vals) for vals in values] expected = services[:3] - real = db.service_get_all_by_topic(self.ctxt, 't1') + real = db.service_get_all(self.ctxt, topic='t1') self._assertEqualListsOfObjects(expected, real) def test_service_get_all_by_binary(self): @@ -227,7 +237,7 @@ class DBAPIServiceTestCase(BaseTest): ] services = [self._create_service(vals) for vals in values] expected = services[:3] - real = db.service_get_all_by_binary(self.ctxt, 'b1') + real = db.service_get_all(self.ctxt, binary='b1') self._assertEqualListsOfObjects(expected, real) def test_service_get_by_args(self): @@ -237,58 +247,30 @@ class DBAPIServiceTestCase(BaseTest): ] services = [self._create_service(vals) for vals in values] - service1 = db.service_get_by_args(self.ctxt, 'host1', 'a') + service1 = db.service_get(self.ctxt, host='host1', binary='a') self._assertEqualObjects(services[0], service1) - service2 = db.service_get_by_args(self.ctxt, 'host2', 'b') + service2 = db.service_get(self.ctxt, host='host2', binary='b') self._assertEqualObjects(services[1], service2) def test_service_get_by_args_not_found_exception(self): self.assertRaises(exception.ServiceNotFound, - db.service_get_by_args, - self.ctxt, 'non-exists-host', 'a') + db.service_get, + self.ctxt, host='non-exists-host', binary='a') - @mock.patch('cinder.db.sqlalchemy.api.model_query') - def test_service_get_by_args_with_case_insensitive(self, model_query): - class case_insensitive_filter(object): - def __init__(self, records): - self.records = records + @mock.patch('sqlalchemy.orm.query.Query.filter_by') + def test_service_get_by_args_with_case_insensitive(self, filter_by): + CONF.set_default('connection', 'mysql://', 'database') + db.service_get(self.ctxt, host='host', binary='a') - def filter_by(self, **kwargs): - ret = mock.Mock() - ret.all = mock.Mock() - - results = [] - for record in self.records: - for key, value in kwargs.items(): - if record[key].lower() != value.lower(): - break - else: - results.append(record) - - ret.filter_by = case_insensitive_filter(results).filter_by - ret.all.return_value = results - return ret - - values = [ - {'host': 'host', 'binary': 'a'}, - {'host': 'HOST', 'binary': 'a'} - ] - services = [self._create_service(vals) for vals in values] - - query = mock.Mock() - query.filter_by = case_insensitive_filter(services).filter_by - model_query.return_value = query - - service1 = db.service_get_by_args(self.ctxt, 'host', 'a') - self._assertEqualObjects(services[0], service1) - - service2 = db.service_get_by_args(self.ctxt, 'HOST', 'a') - self._assertEqualObjects(services[1], service2) - - self.assertRaises(exception.ServiceNotFound, - db.service_get_by_args, - self.ctxt, 'Host', 'a') + self.assertNotEqual(0, filter_by.call_count) + self.assertEqual(1, filter_by.return_value.filter.call_count) + or_op = filter_by.return_value.filter.call_args[0][0].clauses[0] + self.assertIsInstance(or_op, + sqlalchemy_api.sql.elements.BinaryExpression) + binary_op = or_op.right + self.assertIsInstance(binary_op, sqlalchemy_api.sql.functions.Function) + self.assertEqual('binary', binary_op.name) class DBAPIVolumeTestCase(BaseTest): diff --git a/cinder/tests/unit/test_service.py b/cinder/tests/unit/test_service.py index c41bbf2c2a8..8ce0b306962 100644 --- a/cinder/tests/unit/test_service.py +++ b/cinder/tests/unit/test_service.py @@ -128,6 +128,12 @@ class ServiceTestCase(test.TestCase): self.host = 'foo' self.binary = 'cinder-fake' self.topic = 'fake' + self.service_ref = {'host': self.host, + 'binary': self.binary, + 'topic': self.topic, + 'report_count': 0, + 'availability_zone': 'nova', + 'id': 1} def test_create(self): # NOTE(vish): Create was moved out of mock replay to make sure that @@ -143,17 +149,13 @@ class ServiceTestCase(test.TestCase): # 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, - 'binary': self.binary, - 'topic': self.topic, - 'report_count': 0, - 'availability_zone': 'nova', - 'id': 1} + @mock.patch.object(objects.service.Service, 'get_by_args') + @mock.patch.object(objects.service.Service, 'get_by_id') + def test_report_state_newly_disconnected(self, get_by_id, get_by_args): + get_by_args.side_effect = exception.NotFound() + get_by_id.side_effect = db_exc.DBConnectionError() with mock.patch.object(objects.service, 'db') as mock_db: - mock_db.service_get_by_args.side_effect = exception.NotFound() - mock_db.service_create.return_value = service_ref - mock_db.service_get.side_effect = db_exc.DBConnectionError() + mock_db.service_create.return_value = self.service_ref serv = service.Service( self.host, @@ -166,17 +168,13 @@ class ServiceTestCase(test.TestCase): self.assertTrue(serv.model_disconnected) self.assertFalse(mock_db.service_update.called) - def test_report_state_disconnected_DBError(self): - service_ref = {'host': self.host, - 'binary': self.binary, - 'topic': self.topic, - 'report_count': 0, - 'availability_zone': 'nova', - 'id': 1} + @mock.patch.object(objects.service.Service, 'get_by_args') + @mock.patch.object(objects.service.Service, 'get_by_id') + def test_report_state_disconnected_DBError(self, get_by_id, get_by_args): + get_by_args.side_effect = exception.NotFound() + get_by_id.side_effect = db_exc.DBError() with mock.patch.object(objects.service, 'db') as mock_db: - mock_db.service_get_by_args.side_effect = exception.NotFound() - mock_db.service_create.return_value = service_ref - mock_db.service_get.side_effect = db_exc.DBError() + mock_db.service_create.return_value = self.service_ref serv = service.Service( self.host, @@ -189,41 +187,27 @@ class ServiceTestCase(test.TestCase): self.assertTrue(serv.model_disconnected) self.assertFalse(mock_db.service_update.called) - def test_report_state_newly_connected(self): - service_ref = {'host': self.host, - 'binary': self.binary, - 'topic': self.topic, - 'report_count': 0, - 'availability_zone': 'nova', - 'id': 1} - with mock.patch.object(objects.service, 'db') as mock_db,\ - mock.patch('cinder.db.sqlalchemy.api.get_by_id') as get_by_id: - mock_db.service_get_by_args.side_effect = exception.NotFound() - mock_db.service_create.return_value = service_ref - get_by_id.return_value = service_ref + @mock.patch('cinder.db.sqlalchemy.api.service_update') + @mock.patch('cinder.db.sqlalchemy.api.service_get') + def test_report_state_newly_connected(self, get_by_id, service_update): + get_by_id.return_value = self.service_ref - serv = service.Service( - self.host, - self.binary, - self.topic, - 'cinder.tests.unit.test_service.FakeManager' - ) - serv.start() - serv.model_disconnected = True - serv.report_state() + serv = service.Service( + self.host, + self.binary, + self.topic, + 'cinder.tests.unit.test_service.FakeManager' + ) + serv.start() + serv.model_disconnected = True + serv.report_state() - self.assertFalse(serv.model_disconnected) - self.assertTrue(mock_db.service_update.called) + self.assertFalse(serv.model_disconnected) + self.assertTrue(service_update.called) def test_report_state_manager_not_working(self): - service_ref = {'host': self.host, - 'binary': self.binary, - 'topic': self.topic, - 'report_count': 0, - 'availability_zone': 'nova', - 'id': 1} with mock.patch('cinder.db') as mock_db: - mock_db.service_get.return_value = service_ref + mock_db.service_get.return_value = self.service_ref serv = service.Service( self.host, diff --git a/cinder/tests/unit/test_volume.py b/cinder/tests/unit/test_volume.py index d1583eb0a4e..e003c262e9e 100644 --- a/cinder/tests/unit/test_volume.py +++ b/cinder/tests/unit/test_volume.py @@ -240,105 +240,71 @@ class BaseVolumeTestCase(test.TestCase): class AvailabilityZoneTestCase(BaseVolumeTestCase): + def setUp(self): + super(AvailabilityZoneTestCase, self).setUp() + self.get_all = self.patch( + 'cinder.db.service_get_all', autospec=True, + return_value = [{'availability_zone': 'a', 'disabled': False}]) + def test_list_availability_zones_cached(self): - volume_api = cinder.volume.api.API() - with mock.patch.object(volume_api.db, - 'service_get_all_by_topic') as get_all: - get_all.return_value = [ - { - 'availability_zone': 'a', - 'disabled': False, - }, - ] - azs = volume_api.list_availability_zones(enable_cache=True) - self.assertEqual([{"name": 'a', 'available': True}], list(azs)) - self.assertIsNotNone(volume_api.availability_zones_last_fetched) - self.assertTrue(get_all.called) - volume_api.list_availability_zones(enable_cache=True) - self.assertEqual(1, get_all.call_count) + azs = self.volume_api.list_availability_zones(enable_cache=True) + self.assertEqual([{"name": 'a', 'available': True}], list(azs)) + self.assertIsNotNone(self.volume_api.availability_zones_last_fetched) + self.assertTrue(self.get_all.called) + self.volume_api.list_availability_zones(enable_cache=True) + self.assertEqual(1, self.get_all.call_count) def test_list_availability_zones_no_cached(self): - volume_api = cinder.volume.api.API() - with mock.patch.object(volume_api.db, - 'service_get_all_by_topic') as get_all: - get_all.return_value = [ - { - 'availability_zone': 'a', - 'disabled': False, - }, - ] - azs = volume_api.list_availability_zones(enable_cache=False) - self.assertEqual([{"name": 'a', 'available': True}], list(azs)) - self.assertIsNone(volume_api.availability_zones_last_fetched) + azs = self.volume_api.list_availability_zones(enable_cache=False) + self.assertEqual([{"name": 'a', 'available': True}], list(azs)) + self.assertIsNone(self.volume_api.availability_zones_last_fetched) - with mock.patch.object(volume_api.db, - 'service_get_all_by_topic') as get_all: - get_all.return_value = [ - { - 'availability_zone': 'a', - 'disabled': True, - }, - ] - azs = volume_api.list_availability_zones(enable_cache=False) - self.assertEqual([{"name": 'a', 'available': False}], list(azs)) - self.assertIsNone(volume_api.availability_zones_last_fetched) + self.get_all.return_value[0]['disabled'] = True + azs = self.volume_api.list_availability_zones(enable_cache=False) + self.assertEqual([{"name": 'a', 'available': False}], list(azs)) + self.assertIsNone(self.volume_api.availability_zones_last_fetched) def test_list_availability_zones_refetched(self): timeutils.set_time_override() self.addCleanup(timeutils.clear_time_override) - volume_api = cinder.volume.api.API() - with mock.patch.object(volume_api.db, - 'service_get_all_by_topic') as get_all: - get_all.return_value = [ - { - 'availability_zone': 'a', - 'disabled': False, - }, - ] - azs = volume_api.list_availability_zones(enable_cache=True) - self.assertEqual([{"name": 'a', 'available': True}], list(azs)) - self.assertIsNotNone(volume_api.availability_zones_last_fetched) - last_fetched = volume_api.availability_zones_last_fetched - self.assertTrue(get_all.called) - volume_api.list_availability_zones(enable_cache=True) - self.assertEqual(1, get_all.call_count) + azs = self.volume_api.list_availability_zones(enable_cache=True) + self.assertEqual([{"name": 'a', 'available': True}], list(azs)) + self.assertIsNotNone(self.volume_api.availability_zones_last_fetched) + last_fetched = self.volume_api.availability_zones_last_fetched + self.assertTrue(self.get_all.called) + self.volume_api.list_availability_zones(enable_cache=True) + self.assertEqual(1, self.get_all.call_count) - # The default cache time is 3600, push past that... - timeutils.advance_time_seconds(3800) - get_all.return_value = [ - { - 'availability_zone': 'a', - 'disabled': False, - }, - { - 'availability_zone': 'b', - 'disabled': False, - }, - ] - azs = volume_api.list_availability_zones(enable_cache=True) - azs = sorted([n['name'] for n in azs]) - self.assertEqual(['a', 'b'], azs) - self.assertEqual(2, get_all.call_count) - self.assertGreater(volume_api.availability_zones_last_fetched, - last_fetched) + # The default cache time is 3600, push past that... + timeutils.advance_time_seconds(3800) + self.get_all.return_value = [ + { + 'availability_zone': 'a', + 'disabled': False, + }, + { + 'availability_zone': 'b', + 'disabled': False, + }, + ] + azs = self.volume_api.list_availability_zones(enable_cache=True) + azs = sorted([n['name'] for n in azs]) + self.assertEqual(['a', 'b'], azs) + self.assertEqual(2, self.get_all.call_count) + self.assertGreater(self.volume_api.availability_zones_last_fetched, + last_fetched) def test_list_availability_zones_enabled_service(self): - services = [ + def sort_func(obj): + return obj['name'] + + self.get_all.return_value = [ {'availability_zone': 'ping', 'disabled': 0}, {'availability_zone': 'ping', 'disabled': 1}, {'availability_zone': 'pong', 'disabled': 0}, {'availability_zone': 'pung', 'disabled': 1}, ] - def stub_service_get_all_by_topic(*args, **kwargs): - return services - - self.stubs.Set(db, 'service_get_all_by_topic', - stub_service_get_all_by_topic) - - def sort_func(obj): - return obj['name'] - volume_api = cinder.volume.api.API() azs = volume_api.list_availability_zones() azs = sorted(azs, key=sort_func) @@ -1493,8 +1459,7 @@ class VolumeTestCase(BaseVolumeTestCase): snapshot_obj = fake_snapshot.fake_snapshot_obj(self.context, **snapshot) - with mock.patch.object(db, - 'service_get_all_by_topic') as mock_get_service, \ + with mock.patch('cinder.db.service_get_all') as mock_get_service, \ mock.patch.object(volume_api, 'list_availability_zones') as mock_get_azs: mock_get_service.return_value = [{'host': 'foo'}] @@ -5356,7 +5321,7 @@ class ReplicationTestCase(BaseVolumeTestCase): @mock.patch.object(volume_rpcapi.VolumeAPI, 'failover_host') @mock.patch.object(cinder.db, 'conditional_update') - @mock.patch.object(cinder.db, 'service_get_by_args') + @mock.patch.object(cinder.db, 'service_get') def test_failover_host(self, mock_db_args, mock_db_update, mock_failover): """Test replication failover_host.""" @@ -5371,7 +5336,7 @@ class ReplicationTestCase(BaseVolumeTestCase): @mock.patch.object(volume_rpcapi.VolumeAPI, 'failover_host') @mock.patch.object(cinder.db, 'conditional_update') - @mock.patch.object(cinder.db, 'service_get_by_args') + @mock.patch.object(cinder.db, 'service_get') def test_failover_host_unexpected_status(self, mock_db_args, mock_db_update, mock_failover): @@ -5389,7 +5354,7 @@ class ReplicationTestCase(BaseVolumeTestCase): @mock.patch.object(volume_rpcapi.VolumeAPI, 'freeze_host') @mock.patch.object(cinder.db, 'conditional_update') - @mock.patch.object(cinder.db, 'service_get_by_args') + @mock.patch.object(cinder.db, 'service_get') def test_freeze_host(self, mock_db_args, mock_db_update, mock_freeze): """Test replication freeze_host.""" @@ -5404,7 +5369,7 @@ class ReplicationTestCase(BaseVolumeTestCase): @mock.patch.object(volume_rpcapi.VolumeAPI, 'freeze_host') @mock.patch.object(cinder.db, 'conditional_update') - @mock.patch.object(cinder.db, 'service_get_by_args') + @mock.patch.object(cinder.db, 'service_get') def test_freeze_host_unexpected_status(self, mock_db_args, mock_db_update, mock_freeze): @@ -5422,7 +5387,7 @@ class ReplicationTestCase(BaseVolumeTestCase): @mock.patch.object(volume_rpcapi.VolumeAPI, 'thaw_host') @mock.patch.object(cinder.db, 'conditional_update') - @mock.patch.object(cinder.db, 'service_get_by_args') + @mock.patch.object(cinder.db, 'service_get') def test_thaw_host(self, mock_db_args, mock_db_update, mock_thaw): """Test replication thaw_host.""" @@ -5438,7 +5403,7 @@ class ReplicationTestCase(BaseVolumeTestCase): @mock.patch.object(volume_rpcapi.VolumeAPI, 'thaw_host') @mock.patch.object(cinder.db, 'conditional_update') - @mock.patch.object(cinder.db, 'service_get_by_args') + @mock.patch.object(cinder.db, 'service_get') def test_thaw_host_unexpected_status(self, mock_db_args, mock_db_update, mock_thaw):