From f50b3555773a1559e29c75ac48857b50cea8dfe5 Mon Sep 17 00:00:00 2001 From: wangxiyuan Date: Fri, 23 Jun 2017 10:41:08 +0800 Subject: [PATCH] Fix resource count for os-host show os-host show API should only return the resouce count on the specified host. The logic in Cinder which counts the project's resouce on all host is wrong. Change-Id: I96cd285a82b44af8818514692818e739443dcc45 Closes-bug: #1699936 --- cinder/api/contrib/hosts.py | 26 ++++---- cinder/db/api.py | 10 +-- cinder/db/sqlalchemy/api.py | 34 +++++++---- cinder/objects/snapshot.py | 5 +- cinder/tests/unit/api/contrib/test_hosts.py | 61 +++++++++++++++++++ cinder/tests/unit/objects/test_snapshot.py | 3 +- cinder/tests/unit/test_db_api.py | 37 +++++++++++ ...-host-show-incorrect-fg8698gu7y6r7d15.yaml | 5 ++ 8 files changed, 148 insertions(+), 33 deletions(-) create mode 100644 releasenotes/notes/bug-1699936-fix-host-show-incorrect-fg8698gu7y6r7d15.yaml diff --git a/cinder/api/contrib/hosts.py b/cinder/api/contrib/hosts.py index eca9e570994..0d5bf6630b2 100644 --- a/cinder/api/contrib/hosts.py +++ b/cinder/api/contrib/hosts.py @@ -135,12 +135,12 @@ class HostController(wsgi.Controller): def show(self, req, id): """Shows the volume usage info given by hosts. - :param context: security context - :param host: hostname - :returns: expected to use HostShowTemplate. + :param req: security context + :param id: hostname + :returns: dict -- the host resources dictionary. ex.:: - {'host': {'resource':D},..} + {'host': [{'resource': D},..]} D: {'host': 'hostname','project': 'admin', 'volume_count': 1, 'total_volume_gb': 2048} """ @@ -154,33 +154,33 @@ class HostController(wsgi.Controller): host_ref = objects.Service.get_by_host_and_topic( context, host, constants.VOLUME_TOPIC) - # Getting total available/used resource - # TODO(jdg): Add summary info for Snapshots + # Getting total available/used resource on a host. volume_refs = db.volume_get_all_by_host(context, host_ref.host) - (count, sum) = db.volume_data_get_for_host(context, - host_ref.host) + (count, vol_sum) = db.volume_data_get_for_host(context, + host_ref.host) snap_count_total = 0 snap_sum_total = 0 resources = [{'resource': {'host': host, 'project': '(total)', 'volume_count': str(count), - 'total_volume_gb': str(sum), + 'total_volume_gb': str(vol_sum), 'snapshot_count': str(snap_count_total), 'total_snapshot_gb': str(snap_sum_total)}}] project_ids = [v['project_id'] for v in volume_refs] project_ids = list(set(project_ids)) for project_id in project_ids: - (count, sum) = db.volume_data_get_for_project(context, project_id) + (count, vol_sum) = db.volume_data_get_for_project( + context, project_id, host=host_ref.host) (snap_count, snap_sum) = ( - objects.Snapshot.snapshot_data_get_for_project(context, - project_id)) + objects.Snapshot.snapshot_data_get_for_project( + context, project_id, host=host_ref.host)) resources.append( {'resource': {'host': host, 'project': project_id, 'volume_count': str(count), - 'total_volume_gb': str(sum), + 'total_volume_gb': str(vol_sum), 'snapshot_count': str(snap_count), 'total_snapshot_gb': str(snap_sum)}}) snap_count_total += int(snap_count) diff --git a/cinder/db/api.py b/cinder/db/api.py index 4297fab57b7..b71832029ab 100644 --- a/cinder/db/api.py +++ b/cinder/db/api.py @@ -253,9 +253,9 @@ def volume_data_get_for_host(context, host, count_only=False): count_only) -def volume_data_get_for_project(context, project_id): +def volume_data_get_for_project(context, project_id, host=None): """Get (volume_count, gigabytes) for project.""" - return IMPL.volume_data_get_for_project(context, project_id) + return IMPL.volume_data_get_for_project(context, project_id, host=host) def volume_destroy(context, volume_id): @@ -501,11 +501,13 @@ def snapshot_update(context, snapshot_id, values): return IMPL.snapshot_update(context, snapshot_id, values) -def snapshot_data_get_for_project(context, project_id, volume_type_id=None): +def snapshot_data_get_for_project(context, project_id, volume_type_id=None, + host=None): """Get count and gigabytes used for snapshots for specified project.""" return IMPL.snapshot_data_get_for_project(context, project_id, - volume_type_id) + volume_type_id, + host=host) def snapshot_get_all_active_by_window(context, begin, end=None, diff --git a/cinder/db/sqlalchemy/api.py b/cinder/db/sqlalchemy/api.py index 6cb2d32b6df..06b67c2bb2b 100644 --- a/cinder/db/sqlalchemy/api.py +++ b/cinder/db/sqlalchemy/api.py @@ -1592,13 +1592,15 @@ def volume_data_get_for_host(context, host, count_only=False): @require_admin_context def _volume_data_get_for_project(context, project_id, volume_type_id=None, - session=None): + session=None, host=None): query = model_query(context, func.count(models.Volume.id), func.sum(models.Volume.size), read_deleted="no", session=session).\ filter_by(project_id=project_id) + if host: + query = query.filter(_filter_host(models.Volume.host, host)) if volume_type_id: query = query.filter_by(volume_type_id=volume_type_id) @@ -1629,8 +1631,10 @@ def _backup_data_get_for_project(context, project_id, volume_type_id=None, @require_admin_context -def volume_data_get_for_project(context, project_id, volume_type_id=None): - return _volume_data_get_for_project(context, project_id, volume_type_id) +def volume_data_get_for_project(context, project_id, + volume_type_id=None, host=None): + return _volume_data_get_for_project(context, project_id, + volume_type_id, host=host) @require_admin_context @@ -3225,27 +3229,31 @@ def snapshot_get_all_by_project(context, project_id, filters=None, marker=None, @require_context def _snapshot_data_get_for_project(context, project_id, volume_type_id=None, - session=None): + session=None, host=None): authorize_project_context(context, project_id) query = model_query(context, func.count(models.Snapshot.id), func.sum(models.Snapshot.volume_size), read_deleted="no", - session=session).\ - filter_by(project_id=project_id) - - if volume_type_id: - query = query.join('volume').filter_by(volume_type_id=volume_type_id) - - result = query.first() + session=session) + if volume_type_id or host: + query = query.join('volume') + if volume_type_id: + query = query.filter( + models.Volume.volume_type_id == volume_type_id) + if host: + query = query.filter(_filter_host(models.Volume.host, host)) + result = query.filter(models.Snapshot.project_id == project_id).first() # NOTE(vish): convert None to 0 return (result[0] or 0, result[1] or 0) @require_context -def snapshot_data_get_for_project(context, project_id, volume_type_id=None): - return _snapshot_data_get_for_project(context, project_id, volume_type_id) +def snapshot_data_get_for_project(context, project_id, + volume_type_id=None, host=None): + return _snapshot_data_get_for_project(context, project_id, volume_type_id, + host=host) @require_context diff --git a/cinder/objects/snapshot.py b/cinder/objects/snapshot.py index 4233b60ddcb..b7190775164 100644 --- a/cinder/objects/snapshot.py +++ b/cinder/objects/snapshot.py @@ -273,9 +273,10 @@ class Snapshot(cleanable.CinderCleanableObject, base.CinderObject, @classmethod def snapshot_data_get_for_project(cls, context, project_id, - volume_type_id=None): + volume_type_id=None, host=None): return db.snapshot_data_get_for_project(context, project_id, - volume_type_id) + volume_type_id, + host=host) @staticmethod def _is_cleanable(status, obj_version): diff --git a/cinder/tests/unit/api/contrib/test_hosts.py b/cinder/tests/unit/api/contrib/test_hosts.py index ae8db60ea73..2dee75f79c4 100644 --- a/cinder/tests/unit/api/contrib/test_hosts.py +++ b/cinder/tests/unit/api/contrib/test_hosts.py @@ -14,6 +14,7 @@ # under the License. import datetime +import mock import iso8601 from oslo_utils import timeutils @@ -22,7 +23,10 @@ import webob.exc from cinder.api.contrib import hosts as os_hosts from cinder import context from cinder import exception +from cinder.objects import service from cinder import test +from cinder.tests.unit import fake_constants +from cinder.tests.unit import utils as test_utils created_time = datetime.datetime(2012, 11, 14, 1, 20, 41, 95099) @@ -150,6 +154,63 @@ class HostTestCase(test.TestCase): 'bogus_host_name', body={'disabled': 0}) + @mock.patch.object(service.Service, 'get_by_host_and_topic') + def test_show_host(self, mock_get_host): + host = 'test_host' + test_service = service.Service(id=1, host=host, + binary='cinder-volume', + topic='cinder-volume') + mock_get_host.return_value = test_service + + ctxt1 = context.RequestContext(project_id=fake_constants.PROJECT_ID, + is_admin=True) + ctxt2 = context.RequestContext(project_id=fake_constants.PROJECT2_ID, + is_admin=True) + # Create two volumes with different project. + volume1 = test_utils.create_volume(ctxt1, + host=host, size=1) + test_utils.create_volume(ctxt2, host=host, size=1) + # This volume is not on the same host. It should not be counted. + test_utils.create_volume(ctxt2, host='fake_host', size=1) + test_utils.create_snapshot(ctxt1, volume_id=volume1.id) + + resp = self.controller.show(self.req, host) + + host_resp = resp['host'] + # There are 3 resource list: total, project1, project2 + self.assertEqual(3, len(host_resp)) + expected = [ + { + "resource": { + "volume_count": "2", + "total_volume_gb": "2", + "host": "test_host", + "total_snapshot_gb": "1", + "project": "(total)", + "snapshot_count": "1"} + }, + { + "resource": { + "volume_count": "1", + "total_volume_gb": "1", + "host": "test_host", + "project": fake_constants.PROJECT2_ID, + "total_snapshot_gb": "0", + "snapshot_count": "0"} + }, + { + "resource": { + "volume_count": "1", + "total_volume_gb": "1", + "host": "test_host", + "total_snapshot_gb": "1", + "project": fake_constants.PROJECT_ID, + "snapshot_count": "1"} + } + ] + self.assertListEqual(expected, sorted( + host_resp, key=lambda h: h['resource']['project'])) + def test_show_forbidden(self): self.req.environ['cinder.context'].is_admin = False dest = 'dummydest' diff --git a/cinder/tests/unit/objects/test_snapshot.py b/cinder/tests/unit/objects/test_snapshot.py index c0d462221d7..5639b38bae0 100644 --- a/cinder/tests/unit/objects/test_snapshot.py +++ b/cinder/tests/unit/objects/test_snapshot.py @@ -201,7 +201,8 @@ class TestSnapshot(test_objects.BaseObjectsTestCase): volume_type_id) snapshot_data_get.assert_called_once_with(self.context, self.project_id, - volume_type_id) + volume_type_id, + host=None) @mock.patch('cinder.db.sqlalchemy.api.snapshot_get') def test_refresh(self, snapshot_get): diff --git a/cinder/tests/unit/test_db_api.py b/cinder/tests/unit/test_db_api.py index bd23b0ae8db..35fe374ff68 100644 --- a/cinder/tests/unit/test_db_api.py +++ b/cinder/tests/unit/test_db_api.py @@ -549,6 +549,22 @@ class DBAPIVolumeTestCase(BaseTest): db.volume_data_get_for_project( self.ctxt, 'p%d' % i)) + def test_volume_data_get_for_project_with_host(self): + + db.volume_create(self.ctxt, {'project_id': fake.PROJECT_ID, + 'size': 100, + 'host': 'host1'}) + db.volume_create(self.ctxt, {'project_id': fake.PROJECT2_ID, + 'size': 200, + 'host': 'host1'}) + db.volume_create(self.ctxt, {'project_id': fake.PROJECT2_ID, + 'size': 300, + 'host': 'host2'}) + resp = db.volume_data_get_for_project(self.ctxt, + fake.PROJECT2_ID, + host='host2') + self.assertEqual((1, 300), resp) + def test_volume_detached_from_instance(self): volume = db.volume_create(self.ctxt, {}) instance_uuid = 'aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa' @@ -1820,6 +1836,27 @@ class DBAPISnapshotTestCase(BaseTest): {'fake_key': 'fake'}), ignored_keys='volume') + def test_snapshot_get_all_by_project_with_host(self): + db.volume_create(self.ctxt, {'id': 1, 'host': 'host1', 'size': 1, + 'project_id': fake.PROJECT_ID}) + db.volume_create(self.ctxt, {'id': 2, 'host': 'host1', 'size': 2, + 'project_id': fake.PROJECT2_ID}) + db.volume_create(self.ctxt, {'id': 3, 'host': 'host2', 'size': 3, + 'project_id': fake.PROJECT2_ID}) + db.snapshot_create(self.ctxt, {'id': 1, 'volume_id': 1, + 'project_id': fake.PROJECT_ID, + 'volume_size': 1}) + db.snapshot_create(self.ctxt, {'id': 2, 'volume_id': 2, + 'project_id': fake.PROJECT2_ID, + 'volume_size': 2}) + db.snapshot_create(self.ctxt, {'id': 3, 'volume_id': 3, + 'project_id': fake.PROJECT2_ID, + 'volume_size': 3}) + resp = db.snapshot_data_get_for_project(self.ctxt, + fake.PROJECT2_ID, + host='host2') + self.assertEqual((1, 3), resp) + def test_snapshot_metadata_get(self): metadata = {'a': 'b', 'c': 'd'} db.volume_create(self.ctxt, {'id': 1}) diff --git a/releasenotes/notes/bug-1699936-fix-host-show-incorrect-fg8698gu7y6r7d15.yaml b/releasenotes/notes/bug-1699936-fix-host-show-incorrect-fg8698gu7y6r7d15.yaml new file mode 100644 index 00000000000..167c1015dbc --- /dev/null +++ b/releasenotes/notes/bug-1699936-fix-host-show-incorrect-fg8698gu7y6r7d15.yaml @@ -0,0 +1,5 @@ +--- +fixes: + - | + Now the ``os-host show`` API will count project's + resource correctly.