From ed076e623458467c9dc4f2a9f49f3abb8466d562 Mon Sep 17 00:00:00 2001 From: Timur Sufiev Date: Fri, 14 Aug 2015 20:20:45 +0300 Subject: [PATCH] Do not make unnecessary calls to Nova from FIPs and Volumes tabs If none of the floating IPs listed within Horizon are attached to an instance, do not request a list of Nova instances (because we have no use for it). Apply the same behavior for Volumes tab (both Admin and Project). This patch both saves us from making unnecessary and potentially expensive call to Nova and lays the ground for a more selective request of instance details from Nova. The latter will be possible once Nova supports filtering instance details by >1 instance_id. For reference see https://blueprints.launchpad.net/nova/+spec/get-multi-servers-filter Change-Id: Ie7563b9e03b286b4cf51507463213162af3383b1 Partial-Bug: #1442310 --- .../dashboards/admin/volumes/tabs.py | 4 ++- .../dashboards/admin/volumes/tests.py | 36 +++++++++++++++---- .../project/access_and_security/tabs.py | 25 +++++++------ .../project/access_and_security/tests.py | 15 +++++--- .../dashboards/project/volumes/tabs.py | 25 ++++++++++--- .../dashboards/project/volumes/test.py | 27 +++++++++++--- 6 files changed, 100 insertions(+), 32 deletions(-) diff --git a/openstack_dashboard/dashboards/admin/volumes/tabs.py b/openstack_dashboard/dashboards/admin/volumes/tabs.py index b822eed060..ab285264aa 100644 --- a/openstack_dashboard/dashboards/admin/volumes/tabs.py +++ b/openstack_dashboard/dashboards/admin/volumes/tabs.py @@ -40,7 +40,9 @@ class VolumeTab(volumes_tabs.PagedTableMixin, tabs.TableTab, def get_volumes_data(self): volumes = self._get_volumes(search_opts={'all_tenants': True}) - instances = self._get_instances(search_opts={'all_tenants': True}) + attached_instance_ids = self._get_attached_instance_ids(volumes) + instances = self._get_instances(search_opts={'all_tenants': True}, + instance_ids=attached_instance_ids) volume_ids_with_snapshots = self._get_volumes_ids_with_snapshots( search_opts={'all_tenants': True}) self._set_volume_attributes( diff --git a/openstack_dashboard/dashboards/admin/volumes/tests.py b/openstack_dashboard/dashboards/admin/volumes/tests.py index ee71c56eb3..1b24a9c361 100644 --- a/openstack_dashboard/dashboards/admin/volumes/tests.py +++ b/openstack_dashboard/dashboards/admin/volumes/tests.py @@ -12,6 +12,8 @@ # License for the specific language governing permissions and limitations # under the License. +import copy + from django.conf import settings from django.core.urlresolvers import reverse from django import http @@ -37,16 +39,22 @@ class VolumeTests(test.BaseAdminViewTests): cinder: ('volume_list_paged', 'volume_snapshot_list'), keystone: ('tenant_list',)}) - def test_index(self): + def _test_index(self, instanceless_volumes=False): + volumes = self.cinder_volumes.list() + if instanceless_volumes: + for volume in volumes: + volume.attachments = [] + cinder.volume_list_paged(IsA(http.HttpRequest), sort_dir="desc", marker=None, paginate=True, search_opts={'all_tenants': True})\ - .AndReturn([self.cinder_volumes.list(), False, False]) + .AndReturn([volumes, False, False]) cinder.volume_snapshot_list(IsA(http.HttpRequest), search_opts={ 'all_tenants': True}).AndReturn([]) - api.nova.server_list(IsA(http.HttpRequest), search_opts={ - 'all_tenants': True}) \ - .AndReturn([self.servers.list(), False]) + if not instanceless_volumes: + api.nova.server_list(IsA(http.HttpRequest), search_opts={ + 'all_tenants': True}) \ + .AndReturn([self.servers.list(), False]) keystone.tenant_list(IsA(http.HttpRequest)) \ .AndReturn([self.tenants.list(), False]) @@ -57,6 +65,12 @@ class VolumeTests(test.BaseAdminViewTests): volumes = res.context['volumes_table'].data self.assertItemsEqual(volumes, self.cinder_volumes.list()) + def test_index_without_attachments(self): + self._test_index(instanceless_volumes=True) + + def test_index_with_attachments(self): + self._test_index(instanceless_volumes=False) + @test.create_stubs({api.nova: ('server_list',), cinder: ('volume_list_paged',), keystone: ('tenant_list',)}) @@ -82,10 +96,18 @@ class VolumeTests(test.BaseAdminViewTests): self.mox.UnsetStubs() return res + def ensure_attachments_exist(self, volumes): + volumes = copy.copy(volumes) + for volume in volumes: + if not volume.attachments: + volume.attachments.append({ + "id": "1", "server_id": '1', "device": "/dev/hda"}) + return volumes + @override_settings(API_RESULT_PAGE_SIZE=2) def test_index_paginated(self): size = settings.API_RESULT_PAGE_SIZE - mox_volumes = self.cinder_volumes.list() + mox_volumes = self.ensure_attachments_exist(self.cinder_volumes.list()) # get first page expected_volumes = mox_volumes[:size] @@ -121,7 +143,7 @@ class VolumeTests(test.BaseAdminViewTests): @override_settings(API_RESULT_PAGE_SIZE=2) def test_index_paginated_prev(self): size = settings.API_RESULT_PAGE_SIZE - mox_volumes = self.cinder_volumes.list() + mox_volumes = self.ensure_attachments_exist(self.cinder_volumes.list()) # prev from some page expected_volumes = mox_volumes[size:2 * size] diff --git a/openstack_dashboard/dashboards/project/access_and_security/tabs.py b/openstack_dashboard/dashboards/project/access_and_security/tabs.py index 1cfe9a8f7a..76fb8ddcf1 100644 --- a/openstack_dashboard/dashboards/project/access_and_security/tabs.py +++ b/openstack_dashboard/dashboards/project/access_and_security/tabs.py @@ -104,18 +104,23 @@ class FloatingIPsTab(tabs.TableTab): _('Unable to retrieve floating IP pools.')) pool_dict = dict([(obj.id, obj.name) for obj in floating_ip_pools]) - instances = [] - try: - instances, has_more = nova.server_list(self.request) - except Exception: - exceptions.handle(self.request, - _('Unable to retrieve instance list.')) + attached_instance_ids = [ip.instance_id for ip in floating_ips + if ip.instance_id is not None] + if attached_instance_ids: + instances = [] + try: + # TODO(tsufiev): we should pass attached_instance_ids to + # nova.server_list as soon as Nova API allows for this + instances, has_more = nova.server_list(self.request) + except Exception: + exceptions.handle(self.request, + _('Unable to retrieve instance list.')) - instances_dict = dict([(obj.id, obj.name) for obj in instances]) + instances_dict = dict([(obj.id, obj.name) for obj in instances]) - for ip in floating_ips: - ip.instance_name = instances_dict.get(ip.instance_id) - ip.pool_name = pool_dict.get(ip.pool, ip.pool) + for ip in floating_ips: + ip.instance_name = instances_dict.get(ip.instance_id) + ip.pool_name = pool_dict.get(ip.pool, ip.pool) return floating_ips diff --git a/openstack_dashboard/dashboards/project/access_and_security/tests.py b/openstack_dashboard/dashboards/project/access_and_security/tests.py index a90fe4cb48..e7dc307493 100644 --- a/openstack_dashboard/dashboards/project/access_and_security/tests.py +++ b/openstack_dashboard/dashboards/project/access_and_security/tests.py @@ -44,16 +44,20 @@ class AccessAndSecurityTests(test.TestCase): 'server_list',), api.base: ('is_service_enabled',), quotas: ('tenant_quota_usages',)}) - def _test_index(self, ec2_enabled): + def _test_index(self, ec2_enabled=True, instanceless_ips=False): keypairs = self.keypairs.list() sec_groups = self.security_groups.list() floating_ips = self.floating_ips.list() + if instanceless_ips: + for fip in floating_ips: + fip.instance_id = None quota_data = self.quota_usages.first() quota_data['security_groups']['available'] = 10 - api.nova.server_list( - IsA(http.HttpRequest)) \ - .AndReturn([self.servers.list(), False]) + if not instanceless_ips: + api.nova.server_list( + IsA(http.HttpRequest)) \ + .AndReturn([self.servers.list(), False]) api.nova.keypair_list( IsA(http.HttpRequest)) \ .AndReturn(keypairs) @@ -118,6 +122,9 @@ class AccessAndSecurityTests(test.TestCase): def test_index_with_ec2_disabled(self): self._test_index(ec2_enabled=False) + def test_index_with_instanceless_fips(self): + self._test_index(instanceless_ips=True) + @test.create_stubs({api.network: ('floating_ip_target_list', 'tenant_floating_ip_list',)}) def test_association(self): diff --git a/openstack_dashboard/dashboards/project/volumes/tabs.py b/openstack_dashboard/dashboards/project/volumes/tabs.py index 10737aef87..a6412db0e0 100644 --- a/openstack_dashboard/dashboards/project/volumes/tabs.py +++ b/openstack_dashboard/dashboards/project/volumes/tabs.py @@ -50,8 +50,12 @@ class VolumeTableMixIn(object): _('Unable to retrieve volume list.')) return [] - def _get_instances(self, search_opts=None): + def _get_instances(self, search_opts=None, instance_ids=None): + if not instance_ids: + return [] try: + # TODO(tsufiev): we should pass attached_instance_ids to + # nova.server_list as soon as Nova API allows for this instances, has_more = api.nova.server_list(self.request, search_opts=search_opts) return instances @@ -75,6 +79,15 @@ class VolumeTableMixIn(object): return volume_ids + def _get_attached_instance_ids(self, volumes): + attached_instance_ids = [] + for volume in volumes: + for att in volume.attachments: + server_id = att.get('server_id', None) + if server_id is not None: + attached_instance_ids.append(server_id) + return attached_instance_ids + # set attachment string and if volume has snapshots def _set_volume_attributes(self, volumes, @@ -85,9 +98,10 @@ class VolumeTableMixIn(object): if volume_ids_with_snapshots: if volume.id in volume_ids_with_snapshots: setattr(volume, 'has_snapshot', True) - for att in volume.attachments: - server_id = att.get('server_id', None) - att['instance'] = instances.get(server_id, None) + if instances: + for att in volume.attachments: + server_id = att.get('server_id', None) + att['instance'] = instances.get(server_id, None) class PagedTableMixin(object): @@ -123,7 +137,8 @@ class VolumeTab(PagedTableMixin, tabs.TableTab, VolumeTableMixIn): def get_volumes_data(self): volumes = self._get_volumes() - instances = self._get_instances() + attached_instance_ids = self._get_attached_instance_ids(volumes) + instances = self._get_instances(instance_ids=attached_instance_ids) volume_ids_with_snapshots = self._get_volumes_ids_with_snapshots() self._set_volume_attributes( volumes, instances, volume_ids_with_snapshots) diff --git a/openstack_dashboard/dashboards/project/volumes/test.py b/openstack_dashboard/dashboards/project/volumes/test.py index f9b27fef35..4838ca5887 100644 --- a/openstack_dashboard/dashboards/project/volumes/test.py +++ b/openstack_dashboard/dashboards/project/volumes/test.py @@ -12,6 +12,8 @@ # License for the specific language governing permissions and limitations # under the License. +import copy + from django.conf import settings from django.core.urlresolvers import reverse from django import http @@ -44,10 +46,13 @@ class VolumeAndSnapshotsAndBackupsTests(test.TestCase): 'volume_backup_list_paged', ), api.nova: ('server_list',)}) - def _test_index(self, backup_supported=True): + def _test_index(self, backup_supported=True, instanceless_volumes=False): vol_backups = self.cinder_volume_backups.list() vol_snaps = self.cinder_volume_snapshots.list() volumes = self.cinder_volumes.list() + if instanceless_volumes: + for volume in volumes: + volume.attachments = [] api.cinder.volume_backup_supported(IsA(http.HttpRequest)).\ MultipleTimes().AndReturn(backup_supported) @@ -55,8 +60,9 @@ class VolumeAndSnapshotsAndBackupsTests(test.TestCase): IsA(http.HttpRequest), marker=None, search_opts=None, sort_dir='desc', paginate=True).\ AndReturn([volumes, False, False]) - api.nova.server_list(IsA(http.HttpRequest), search_opts=None).\ - AndReturn([self.servers.list(), False]) + if not instanceless_volumes: + api.nova.server_list(IsA(http.HttpRequest), search_opts=None).\ + AndReturn([self.servers.list(), False]) api.cinder.volume_snapshot_list(IsA(http.HttpRequest)).\ AndReturn(vol_snaps) @@ -93,6 +99,9 @@ class VolumeAndSnapshotsAndBackupsTests(test.TestCase): def test_index_backup_not_supported(self): self._test_index(backup_supported=False) + def test_index_no_volume_attachments(self): + self._test_index(instanceless_volumes=True) + @test.create_stubs({api.cinder: ('tenant_absolute_limits', 'volume_list_paged', 'volume_backup_supported', @@ -121,9 +130,17 @@ class VolumeAndSnapshotsAndBackupsTests(test.TestCase): self.mox.UnsetStubs() return res + def ensure_attachments_exist(self, volumes): + volumes = copy.copy(volumes) + for volume in volumes: + if not volume.attachments: + volume.attachments.append({ + "id": "1", "server_id": '1', "device": "/dev/hda"}) + return volumes + @override_settings(API_RESULT_PAGE_SIZE=2) def test_index_paginated(self): - mox_volumes = self.cinder_volumes.list() + mox_volumes = self.ensure_attachments_exist(self.cinder_volumes.list()) size = settings.API_RESULT_PAGE_SIZE # get first page @@ -159,7 +176,7 @@ class VolumeAndSnapshotsAndBackupsTests(test.TestCase): @override_settings(API_RESULT_PAGE_SIZE=2) def test_index_paginated_prev_page(self): - mox_volumes = self.cinder_volumes.list() + mox_volumes = self.ensure_attachments_exist(self.cinder_volumes.list()) size = settings.API_RESULT_PAGE_SIZE # prev from some page