From 9e0db83b810da7390bf6d9264039b900977f2d6e Mon Sep 17 00:00:00 2001 From: =?utf8?q?Stephan=20M=C3=BCller?= Date: Thu, 10 Sep 2020 14:28:47 +0200 Subject: [PATCH] mgr/dashboard: Speed improvements of pool details MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit Now all variables that are shown in a listing that trigger a more complex render cycle will only be updated if pool properties have changed and not only the time series data, which isn't shown anymore, as it can be seen graphically enhanced in the pool listing. The pool detail component now uses the `onPush` change detection strategy, to only call `ngOnChanges` if one of the input variables have changed. The function that only updates variables if they have changed is now available through a helper class in order to provide this useful functionality through out the dashboard. Fixes: https://tracker.ceph.com/issues/46375 Signed-off-by: Stephan Müller --- .../pool-details/pool-details.component.html | 4 +- .../pool-details.component.spec.ts | 152 +++++++++++++----- .../pool-details/pool-details.component.ts | 32 ++-- .../pool-list/pool-list.component.spec.ts | 29 ++-- .../pool/pool-list/pool-list.component.ts | 4 +- .../shared/classes/cd-helper.class.spec.ts | 66 ++++++++ .../src/app/shared/classes/cd-helper.class.ts | 19 +++ .../frontend/src/testing/unit-test-helper.ts | 13 ++ 8 files changed, 240 insertions(+), 79 deletions(-) create mode 100644 src/pybind/mgr/dashboard/frontend/src/app/shared/classes/cd-helper.class.spec.ts create mode 100644 src/pybind/mgr/dashboard/frontend/src/app/shared/classes/cd-helper.class.ts diff --git a/src/pybind/mgr/dashboard/frontend/src/app/ceph/pool/pool-details/pool-details.component.html b/src/pybind/mgr/dashboard/frontend/src/app/ceph/pool/pool-details/pool-details.component.html index c2c6513888e7..1b0cd563cc8a 100644 --- a/src/pybind/mgr/dashboard/frontend/src/app/ceph/pool/pool-details/pool-details.component.html +++ b/src/pybind/mgr/dashboard/frontend/src/app/ceph/pool/pool-details/pool-details.component.html @@ -9,7 +9,7 @@ i18n>Details @@ -19,7 +19,7 @@ Performance Details - diff --git a/src/pybind/mgr/dashboard/frontend/src/app/ceph/pool/pool-details/pool-details.component.spec.ts b/src/pybind/mgr/dashboard/frontend/src/app/ceph/pool/pool-details/pool-details.component.spec.ts index 188f65d7905f..511257b7f855 100644 --- a/src/pybind/mgr/dashboard/frontend/src/app/ceph/pool/pool-details/pool-details.component.spec.ts +++ b/src/pybind/mgr/dashboard/frontend/src/app/ceph/pool/pool-details/pool-details.component.spec.ts @@ -1,11 +1,12 @@ import { HttpClientTestingModule } from '@angular/common/http/testing'; +import { ChangeDetectorRef } from '@angular/core'; import { ComponentFixture, TestBed } from '@angular/core/testing'; import { BrowserAnimationsModule } from '@angular/platform-browser/animations'; import { RouterTestingModule } from '@angular/router/testing'; import { NgbNavModule } from '@ng-bootstrap/ng-bootstrap'; -import { configureTestBed, TabHelper } from '../../../../testing/unit-test-helper'; +import { configureTestBed, Mocks, TabHelper } from '../../../../testing/unit-test-helper'; import { Permissions } from '../../../shared/models/permissions'; import { SharedModule } from '../../../shared/shared.module'; import { RbdConfigurationListComponent } from '../../block/rbd-configuration-list/rbd-configuration-list.component'; @@ -15,6 +16,23 @@ describe('PoolDetailsComponent', () => { let poolDetailsComponent: PoolDetailsComponent; let fixture: ComponentFixture; + // Needed because of ChangeDetectionStrategy.OnPush + // https://github.com/angular/angular/issues/12313#issuecomment-444623173 + let changeDetector: ChangeDetectorRef; + const detectChanges = () => { + poolDetailsComponent.ngOnChanges(); + changeDetector.detectChanges(); // won't call ngOnChanges on it's own but updates fixture + }; + + const updatePoolSelection = (selection: any) => { + poolDetailsComponent.selection = selection; + detectChanges(); + }; + + const currentPoolUpdate = () => { + updatePoolSelection(poolDetailsComponent.selection); + }; + configureTestBed({ imports: [ BrowserAnimationsModule, @@ -28,12 +46,15 @@ describe('PoolDetailsComponent', () => { beforeEach(() => { fixture = TestBed.createComponent(PoolDetailsComponent); + // Needed because of ChangeDetectionStrategy.OnPush + // https://github.com/angular/angular/issues/12313#issuecomment-444623173 + changeDetector = fixture.componentRef.injector.get(ChangeDetectorRef); poolDetailsComponent = fixture.componentInstance; poolDetailsComponent.selection = undefined; poolDetailsComponent.permissions = new Permissions({ grafana: ['read'] }); - fixture.detectChanges(); + updatePoolSelection({ tiers: [0], pool: 0, pool_name: 'micro_pool' }); }); it('should create', () => { @@ -41,66 +62,109 @@ describe('PoolDetailsComponent', () => { }); describe('Pool details tabset', () => { - beforeEach(() => { - poolDetailsComponent.selection = { - tiers: [0], - pool: 0 - }; - }); - it('should recognize a tabset child', () => { - fixture.detectChanges(); + detectChanges(); const ngbNav = TabHelper.getNgbNav(fixture); expect(ngbNav).toBeDefined(); }); - it('should show "Cache Tiers Details" tab if selected pool has "tiers"', () => { - fixture.detectChanges(); - const tabsItem = TabHelper.getNgbNavItems(fixture); - const tabsText = TabHelper.getTextContents(fixture); - expect(tabsItem.length).toBe(3); - expect(tabsText[2]).toBe('Cache Tiers Details'); - expect(tabsItem[0].active).toBeTruthy(); - }); - - it('should not show "Cache Tiers Details" tab if selected pool has no "tiers"', () => { - poolDetailsComponent.selection = { - tiers: [] - }; - fixture.detectChanges(); + it('should not change the tabs active status when selection is the same as before', () => { const tabs = TabHelper.getNgbNavItems(fixture); - expect(tabs.length).toEqual(2); expect(tabs[0].active).toBeTruthy(); - }); - - it('current active status of tabs should not change when selection is the same as previous selection', () => { - fixture.detectChanges(); - const tabs = TabHelper.getNgbNavItems(fixture); + currentPoolUpdate(); expect(tabs[0].active).toBeTruthy(); const ngbNav = TabHelper.getNgbNav(fixture); ngbNav.select(tabs[1].id); - - fixture.detectChanges(); + expect(tabs[1].active).toBeTruthy(); + currentPoolUpdate(); expect(tabs[1].active).toBeTruthy(); }); - it('returns pool details correctly', () => { - const pool = { prop1: 1, cdIsBinary: true, prop2: 2, cdExecuting: true, prop3: 3 }; + it('should filter out cdExecuting, cdIsBinary and all stats', () => { + updatePoolSelection({ + prop1: 1, + cdIsBinary: true, + prop2: 2, + cdExecuting: true, + prop3: 3, + stats: { anyStat: 3, otherStat: [1, 2, 3] } + }); const expectedPool = { prop1: 1, prop2: 2, prop3: 3 }; - - expect(poolDetailsComponent.filterNonPoolData(pool)).toEqual(expectedPool); + expect(poolDetailsComponent.poolDetails).toEqual(expectedPool); }); - it('pool data filtering is called', () => { - const filterNonPoolDataSpy = spyOn( - poolDetailsComponent, - 'filterNonPoolData' - ).and.callThrough(); - - fixture.detectChanges(); + describe('Updates of shown data', () => { + const expectedChange = ( + expected: { + selectedPoolConfiguration?: object; + poolDetails?: object; + }, + newSelection: object, + doesNotEqualOld = true + ) => { + const getData = () => { + const data = {}; + Object.keys(expected).forEach((key) => (data[key] = poolDetailsComponent[key])); + return data; + }; + const oldData = getData(); + updatePoolSelection(newSelection); + const newData = getData(); + if (doesNotEqualOld) { + expect(expected).not.toEqual(oldData); + } else { + expect(expected).toEqual(oldData); + } + expect(expected).toEqual(newData); + }; - expect(filterNonPoolDataSpy).toHaveBeenCalled(); + it('should update shown data on change', () => { + expectedChange( + { + poolDetails: { + pg_num: 256, + pg_num_target: 256, + pg_placement_num: 256, + pg_placement_num_target: 256, + pool: 2, + pool_name: 'somePool', + type: 'replicated' + } + }, + Mocks.getPool('somePool', 2) + ); + }); + + it('should not update shown data if no detail has changed on pool refresh', () => { + expectedChange( + { + poolDetails: { + pool: 0, + pool_name: 'micro_pool', + tiers: [0] + } + }, + poolDetailsComponent.selection, + false + ); + }); + + it('should show "Cache Tiers Details" tab if selected pool has "tiers"', () => { + const tabsItem = TabHelper.getNgbNavItems(fixture); + const tabsText = TabHelper.getTextContents(fixture); + expect(poolDetailsComponent.selection['tiers'].length).toBe(1); + expect(tabsItem.length).toBe(3); + expect(tabsText[2]).toBe('Cache Tiers Details'); + expect(tabsItem[0].active).toBeTruthy(); + }); + + it('should not show "Cache Tiers Details" tab if selected pool has no "tiers"', () => { + updatePoolSelection({ tiers: [] }); + const tabs = TabHelper.getNgbNavItems(fixture); + expect(tabs.length).toEqual(2); + expect(tabs[0].active).toBeTruthy(); + }); }); }); }); diff --git a/src/pybind/mgr/dashboard/frontend/src/app/ceph/pool/pool-details/pool-details.component.ts b/src/pybind/mgr/dashboard/frontend/src/app/ceph/pool/pool-details/pool-details.component.ts index 8c9278216d8e..584c3022e0dc 100644 --- a/src/pybind/mgr/dashboard/frontend/src/app/ceph/pool/pool-details/pool-details.component.ts +++ b/src/pybind/mgr/dashboard/frontend/src/app/ceph/pool/pool-details/pool-details.component.ts @@ -1,8 +1,9 @@ -import { Component, Input, OnChanges } from '@angular/core'; +import { ChangeDetectionStrategy, Component, Input, OnChanges } from '@angular/core'; import _ from 'lodash'; import { PoolService } from '../../../shared/api/pool.service'; +import { CdHelperClass } from '../../../shared/classes/cd-helper.class'; import { CdTableColumn } from '../../../shared/models/cd-table-column'; import { RbdConfigurationEntry } from '../../../shared/models/configuration'; import { Permissions } from '../../../shared/models/permissions'; @@ -10,17 +11,23 @@ import { Permissions } from '../../../shared/models/permissions'; @Component({ selector: 'cd-pool-details', templateUrl: './pool-details.component.html', - styleUrls: ['./pool-details.component.scss'] + styleUrls: ['./pool-details.component.scss'], + changeDetection: ChangeDetectionStrategy.OnPush }) export class PoolDetailsComponent implements OnChanges { - cacheTierColumns: Array = []; - @Input() - selection: any; + cacheTiers: any[]; @Input() permissions: Permissions; @Input() - cacheTiers: any[]; + selection: any; + + cacheTierColumns: Array = []; + // 'stats' won't be shown as the pure stat numbers won't tell the user much, + // if they are not converted or used in a chart (like the ones available in the pool listing) + omittedPoolAttributes = ['cdExecuting', 'cdIsBinary', 'stats']; + + poolDetails: object; selectedPoolConfiguration: RbdConfigurationEntry[]; constructor(private poolService: PoolService) { @@ -60,13 +67,14 @@ export class PoolDetailsComponent implements OnChanges { ngOnChanges() { if (this.selection) { - this.poolService.getConfiguration(this.selection.pool_name).subscribe((poolConf) => { - this.selectedPoolConfiguration = poolConf; + this.poolService + .getConfiguration(this.selection.pool_name) + .subscribe((poolConf: RbdConfigurationEntry[]) => { + CdHelperClass.updateChanged(this, { selectedPoolConfiguration: poolConf }); + }); + CdHelperClass.updateChanged(this, { + poolDetails: _.omit(this.selection, this.omittedPoolAttributes) }); } } - - filterNonPoolData(pool: object): object { - return _.omit(pool, ['cdExecuting', 'cdIsBinary']); - } } diff --git a/src/pybind/mgr/dashboard/frontend/src/app/ceph/pool/pool-list/pool-list.component.spec.ts b/src/pybind/mgr/dashboard/frontend/src/app/ceph/pool/pool-list/pool-list.component.spec.ts index e4c216f68a33..f661a7d9b0ab 100644 --- a/src/pybind/mgr/dashboard/frontend/src/app/ceph/pool/pool-list/pool-list.component.spec.ts +++ b/src/pybind/mgr/dashboard/frontend/src/app/ceph/pool/pool-list/pool-list.component.spec.ts @@ -8,7 +8,7 @@ import _ from 'lodash'; import { ToastrModule } from 'ngx-toastr'; import { of } from 'rxjs'; -import { configureTestBed, expectItemTasks } from '../../../../testing/unit-test-helper'; +import { configureTestBed, expectItemTasks, Mocks } from '../../../../testing/unit-test-helper'; import { ConfigurationService } from '../../../shared/api/configuration.service'; import { PoolService } from '../../../shared/api/pool.service'; import { CriticalConfirmationModalComponent } from '../../../shared/components/critical-confirmation-modal/critical-confirmation-modal.component'; @@ -29,18 +29,8 @@ describe('PoolListComponent', () => { let fixture: ComponentFixture; let poolService: PoolService; - const createPool = (name: string, id: number): Pool => { - return _.merge(new Pool(name), { - pool: id, - pg_num: 256, - pg_placement_num: 256, - pg_num_target: 256, - pg_placement_num_target: 256 - }); - }; - const getPoolList = (): Pool[] => { - return [createPool('a', 0), createPool('b', 1), createPool('c', 2)]; + return [Mocks.getPool('a', 0), Mocks.getPool('b', 1), Mocks.getPool('c', 2)]; }; configureTestBed({ @@ -141,13 +131,15 @@ describe('PoolListComponent', () => { describe('pool deletion', () => { let taskWrapper: TaskWrapperService; + let modalRef: any; const setSelectedPool = (poolName: string) => (component.selection.selected = [{ pool_name: poolName }]); const callDeletion = () => { component.deletePoolModal(); - const deletion: CriticalConfirmationModalComponent = component.modalRef.componentInstance; + expect(modalRef).toBeTruthy(); + const deletion: CriticalConfirmationModalComponent = modalRef && modalRef.componentInstance; deletion.submitActionObservable(); }; @@ -168,9 +160,10 @@ describe('PoolListComponent', () => { beforeEach(() => { spyOn(TestBed.inject(ModalService), 'show').and.callFake((deletionClass, initialState) => { - return { + modalRef = { componentInstance: Object.assign(new deletionClass(), initialState) }; + return modalRef; }); spyOn(poolService, 'delete').and.stub(); taskWrapper = TestBed.inject(TaskWrapperService); @@ -299,7 +292,7 @@ describe('PoolListComponent', () => { const getPoolData = (o: object) => [ _.merge( - _.merge(createPool('a', 0), { + _.merge(Mocks.getPool('a', 0), { cdIsBinary: true, pg_status: '', stats: { @@ -319,7 +312,7 @@ describe('PoolListComponent', () => { ]; beforeEach(() => { - pool = createPool('a', 0); + pool = Mocks.getPool('a', 0); }); it('transforms pools data correctly', () => { @@ -451,7 +444,7 @@ describe('PoolListComponent', () => { it('should select correct existing cache tier', () => { setSelectionTiers([0]); - expect(component.cacheTiers).toEqual([createPool('a', 0)]); + expect(component.cacheTiers).toEqual([Mocks.getPool('a', 0)]); }); it('should not select cache tier if id is invalid', () => { @@ -468,7 +461,7 @@ describe('PoolListComponent', () => { setSelectionTiers([0, 1, 2]); expect(component.cacheTiers).toEqual(getPoolList()); setSelectionTiers([0]); - expect(component.cacheTiers).toEqual([createPool('a', 0)]); + expect(component.cacheTiers).toEqual([Mocks.getPool('a', 0)]); setSelectionTiers([]); expect(component.cacheTiers).toEqual([]); }); diff --git a/src/pybind/mgr/dashboard/frontend/src/app/ceph/pool/pool-list/pool-list.component.ts b/src/pybind/mgr/dashboard/frontend/src/app/ceph/pool/pool-list/pool-list.component.ts index 97340f1452ea..e6a639f29282 100644 --- a/src/pybind/mgr/dashboard/frontend/src/app/ceph/pool/pool-list/pool-list.component.ts +++ b/src/pybind/mgr/dashboard/frontend/src/app/ceph/pool/pool-list/pool-list.component.ts @@ -1,6 +1,5 @@ import { Component, OnInit, TemplateRef, ViewChild } from '@angular/core'; -import { NgbModalRef } from '@ng-bootstrap/ng-bootstrap'; import _ from 'lodash'; import { ConfigurationService } from '../../../shared/api/configuration.service'; @@ -52,7 +51,6 @@ export class PoolListComponent extends ListWithDetails implements OnInit { pools: Pool[]; columns: CdTableColumn[]; selection = new CdTableSelection(); - modalRef: NgbModalRef; executingTasks: ExecutingTask[] = []; permissions: Permissions; tableActions: CdTableAction[]; @@ -222,7 +220,7 @@ export class PoolListComponent extends ListWithDetails implements OnInit { deletePoolModal() { const name = this.selection.first().pool_name; - this.modalRef = this.modalService.show(CriticalConfirmationModalComponent, { + this.modalService.show(CriticalConfirmationModalComponent, { itemDescription: 'Pool', itemNames: [name], submitActionObservable: () => diff --git a/src/pybind/mgr/dashboard/frontend/src/app/shared/classes/cd-helper.class.spec.ts b/src/pybind/mgr/dashboard/frontend/src/app/shared/classes/cd-helper.class.spec.ts new file mode 100644 index 000000000000..a5a28650d084 --- /dev/null +++ b/src/pybind/mgr/dashboard/frontend/src/app/shared/classes/cd-helper.class.spec.ts @@ -0,0 +1,66 @@ +import { CdHelperClass } from './cd-helper.class'; + +class MockClass { + n = 42; + o = { + x: 'something', + y: [1, 2, 3], + z: true + }; + b: boolean; +} + +describe('CdHelperClass', () => { + describe('updateChanged', () => { + let old: MockClass; + let used: MockClass; + let structure = { + n: 42, + o: { + x: 'something', + y: [1, 2, 3], + z: true + } + } as any; + + beforeEach(() => { + old = new MockClass(); + used = new MockClass(); + structure = { + n: 42, + o: { + x: 'something', + y: [1, 2, 3], + z: true + } + }; + }); + + it('should not update anything', () => { + CdHelperClass.updateChanged(used, structure); + expect(used).toEqual(old); + }); + + it('should only change n', () => { + CdHelperClass.updateChanged(used, { n: 17 }); + expect(used.n).not.toEqual(old.n); + expect(used.n).toBe(17); + }); + + it('should update o on change of o.y', () => { + CdHelperClass.updateChanged(used, structure); + structure.o.y.push(4); + expect(used.o.y).toEqual(old.o.y); + CdHelperClass.updateChanged(used, structure); + expect(used.o.y).toEqual([1, 2, 3, 4]); + }); + + it('should change b, o and n', () => { + structure.o.x.toUpperCase(); + structure.n++; + structure.b = true; + CdHelperClass.updateChanged(used, structure); + expect(used).toEqual(structure); + }); + }); +}); diff --git a/src/pybind/mgr/dashboard/frontend/src/app/shared/classes/cd-helper.class.ts b/src/pybind/mgr/dashboard/frontend/src/app/shared/classes/cd-helper.class.ts new file mode 100644 index 000000000000..7a31d34583a4 --- /dev/null +++ b/src/pybind/mgr/dashboard/frontend/src/app/shared/classes/cd-helper.class.ts @@ -0,0 +1,19 @@ +import * as _ from 'lodash'; + +export class CdHelperClass { + /** + * Simple way to only update variables if they have really changed and not just the reference + * + * @param componentThis - In order to update the variables if necessary + * @param change - The variable name (attribute of the object) is followed by the current value + * it would update even if it equals + */ + static updateChanged(componentThis: any, change: { [publicVarName: string]: any }) { + Object.keys(change).forEach((publicVarName) => { + const data = change[publicVarName]; + if (!_.isEqual(data, componentThis[publicVarName])) { + componentThis[publicVarName] = data; + } + }); + } +} diff --git a/src/pybind/mgr/dashboard/frontend/src/testing/unit-test-helper.ts b/src/pybind/mgr/dashboard/frontend/src/testing/unit-test-helper.ts index c31d70eadd8c..9e8442da1858 100644 --- a/src/pybind/mgr/dashboard/frontend/src/testing/unit-test-helper.ts +++ b/src/pybind/mgr/dashboard/frontend/src/testing/unit-test-helper.ts @@ -5,10 +5,12 @@ import { By } from '@angular/platform-browser'; import { BrowserDynamicTestingModule } from '@angular/platform-browser-dynamic/testing'; import { NgbModal, NgbNav, NgbNavItem } from '@ng-bootstrap/ng-bootstrap'; +import _ from 'lodash'; import { configureTestSuite } from 'ng-bullet'; import { of } from 'rxjs'; import { InventoryDevice } from '../app/ceph/cluster/inventory/inventory-devices/inventory-device.model'; +import { Pool } from '../app/ceph/pool/pool'; import { OrchestratorService } from '../app/shared/api/orchestrator.service'; import { TableActionsComponent } from '../app/shared/datatable/table-actions/table-actions.component'; import { Icons } from '../app/shared/enum/icons.enum'; @@ -383,6 +385,17 @@ export class Mocks { return { name, type, type_id, id, children, device_class }; } + static getPool = (name: string, id: number): Pool => { + return _.merge(new Pool(name), { + pool: id, + type: 'replicated', + pg_num: 256, + pg_placement_num: 256, + pg_num_target: 256, + pg_placement_num_target: 256 + }); + }; + /** * Create the following test crush map: * > default -- 2.47.3