From: Stephan Müller Date: Thu, 10 Sep 2020 12:28:47 +0000 (+0200) Subject: mgr/dashboard: Speed improvements of pool details X-Git-Tag: v17.0.0~991^2 X-Git-Url: http://git.apps.os.sepia.ceph.com/?a=commitdiff_plain;h=9e0db83b810da7390bf6d9264039b900977f2d6e;p=ceph.git mgr/dashboard: Speed improvements of pool details 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 --- 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 c2c6513888e79..1b0cd563cc8aa 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 188f65d7905f3..511257b7f8556 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 8c9278216d8ea..584c3022e0dc8 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 e4c216f68a332..f661a7d9b0ab8 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 97340f1452eaf..e6a639f29282c 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 0000000000000..a5a28650d084c --- /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 0000000000000..7a31d34583a45 --- /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 c31d70eadd8c6..9e8442da1858f 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