From 81d1e80d25a3b82ccb260d371108e2e92ba2a5cb Mon Sep 17 00:00:00 2001 From: =?utf8?q?Alfonso=20Mart=C3=ADnez?= Date: Tue, 7 Apr 2020 18:07:54 +0200 Subject: [PATCH] mgr/dashboard: fix errors related to frontend service subscriptions. MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit - Created TimerService: for getting a scheduled observable that checks if there are any observers subscribed before sending a request. Now FeatureTogglesService uses it internally (solving a hidden bug in it). - SummaryService & RbdMirroringService: instead of having window.setInterval/setTimeout tasks launched in constructor, now they have a method for polling (and subscribing to it). These methods are called in the appropriate component so we unsubscribe properly. - Fixed: some subscriptions were not being unsubscribed properly. - RbdFormComponent: little refactoring and fixed (also improved) unit tests. Fixes: https://tracker.ceph.com/issues/44228 Signed-off-by: Alfonso Martínez --- .../iscsi-target-form.component.spec.ts | 1 - .../mirroring/overview/overview.component.ts | 24 ++-- .../block/rbd-form/rbd-form.component.spec.ts | 52 ++++++--- .../ceph/block/rbd-form/rbd-form.component.ts | 8 +- .../rbd-trash-purge-modal.component.spec.ts | 1 - .../nfs/nfs-form/nfs-form.component.spec.ts | 1 - .../nfs/nfs-list/nfs-list.component.spec.ts | 7 +- .../workbench-layout.component.ts | 25 ++++- .../navigation/navigation.component.ts | 33 ++++-- .../notifications/notifications.component.ts | 26 +++-- .../shared/api/rbd-mirroring.service.spec.ts | 54 +++++---- .../app/shared/api/rbd-mirroring.service.ts | 39 +++---- .../notifications-sidebar.component.ts | 104 +++++++++--------- .../services/feature-toggles.service.ts | 14 +-- .../shared/services/summary.service.spec.ts | 23 ++-- .../app/shared/services/summary.service.ts | 42 ++++--- .../services/task-manager.service.spec.ts | 1 + .../shared/services/task-manager.service.ts | 6 +- .../app/shared/services/timer.service.spec.ts | 68 ++++++++++++ .../src/app/shared/services/timer.service.ts | 27 +++++ 20 files changed, 358 insertions(+), 198 deletions(-) create mode 100644 src/pybind/mgr/dashboard/frontend/src/app/shared/services/timer.service.spec.ts create mode 100644 src/pybind/mgr/dashboard/frontend/src/app/shared/services/timer.service.ts diff --git a/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/iscsi-target-form/iscsi-target-form.component.spec.ts b/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/iscsi-target-form/iscsi-target-form.component.spec.ts index 47c61ad120b..7e02dee2912 100644 --- a/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/iscsi-target-form/iscsi-target-form.component.spec.ts +++ b/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/iscsi-target-form/iscsi-target-form.component.spec.ts @@ -166,7 +166,6 @@ describe('IscsiTargetFormComponent', () => { httpTesting.expectOne('ui-api/iscsi/settings').flush(SETTINGS); httpTesting.expectOne('ui-api/iscsi/portals').flush(PORTALS); httpTesting.expectOne('ui-api/iscsi/version').flush(VERSION); - httpTesting.expectOne('api/summary').flush({}); httpTesting.expectOne('api/block/image').flush(RBD_LIST); httpTesting.expectOne('api/iscsi/target').flush(LIST_TARGET); httpTesting.verify(); diff --git a/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/mirroring/overview/overview.component.ts b/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/mirroring/overview/overview.component.ts index 6a3c7ef210e..682ed68c8bc 100644 --- a/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/mirroring/overview/overview.component.ts +++ b/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/mirroring/overview/overview.component.ts @@ -25,14 +25,11 @@ export class OverviewComponent implements OnInit, OnDestroy { permission: Permission; tableActions: CdTableAction[]; selection = new CdTableSelection(); - - subs: Subscription; - modalRef: BsModalRef; - peersExist = true; siteName: any; status: ViewCacheStatus; + private subs = new Subscription(); constructor( private authStorageService: AuthStorageService, @@ -68,15 +65,18 @@ export class OverviewComponent implements OnInit, OnDestroy { } ngOnInit() { - this.subs = this.rbdMirroringService.subscribeSummary((data: any) => { - if (!data) { - return; - } - this.status = data.content_data.status; - this.siteName = data.site_name; + this.subs.add(this.rbdMirroringService.startPolling()); + this.subs.add( + this.rbdMirroringService.subscribeSummary((data: any) => { + if (!data) { + return; + } + this.status = data.content_data.status; + this.siteName = data.site_name; - this.peersExist = !!data.content_data.pools.find((o: Pool) => o['peer_uuids'].length > 0); - }); + this.peersExist = !!data.content_data.pools.find((o: Pool) => o['peer_uuids'].length > 0); + }) + ); } ngOnDestroy(): void { diff --git a/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/rbd-form/rbd-form.component.spec.ts b/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/rbd-form/rbd-form.component.spec.ts index 77330e648d6..5556a847a6b 100644 --- a/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/rbd-form/rbd-form.component.spec.ts +++ b/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/rbd-form/rbd-form.component.spec.ts @@ -1,5 +1,5 @@ import { HttpClientTestingModule } from '@angular/common/http/testing'; -import { ComponentFixture, discardPeriodicTasks, fakeAsync, TestBed } from '@angular/core/testing'; +import { ComponentFixture, fakeAsync, TestBed, tick } from '@angular/core/testing'; import { ReactiveFormsModule } from '@angular/forms'; import { ActivatedRoute, Router } from '@angular/router'; import { RouterTestingModule } from '@angular/router/testing'; @@ -8,7 +8,7 @@ import { TooltipModule } from 'ngx-bootstrap/tooltip'; import { ToastrModule } from 'ngx-toastr'; import { By } from '@angular/platform-browser'; -import { of } from 'rxjs'; +import { NEVER, of } from 'rxjs'; import { delay } from 'rxjs/operators'; import { ActivatedRouteStub } from '../../../../testing/activated-route-stub'; @@ -65,14 +65,18 @@ describe('RbdFormComponent', () => { let cloneAction: jasmine.Spy; let copyAction: jasmine.Spy; let rbdServiceGetSpy: jasmine.Spy; + let routerNavigate: jasmine.Spy; + + const DELAY = 100; beforeEach(() => { - createAction = spyOn(component, 'createAction').and.stub(); - editAction = spyOn(component, 'editAction').and.stub(); - cloneAction = spyOn(component, 'cloneAction').and.stub(); - copyAction = spyOn(component, 'copyAction').and.stub(); + createAction = spyOn(component, 'createAction').and.returnValue(of(null)); + editAction = spyOn(component, 'editAction'); + editAction.and.returnValue(of(null)); + cloneAction = spyOn(component, 'cloneAction').and.returnValue(of(null)); + copyAction = spyOn(component, 'copyAction').and.returnValue(of(null)); spyOn(component, 'setResponse').and.stub(); - spyOn(TestBed.get(Router), 'navigate').and.stub(); + routerNavigate = spyOn(TestBed.get(Router), 'navigate').and.stub(); rbdServiceGetSpy = spyOn(TestBed.get(RbdService), 'get'); rbdServiceGetSpy.and.returnValue(of({ pool_name: 'foo', pool_image: 'bar' })); component.mode = undefined; @@ -86,12 +90,28 @@ describe('RbdFormComponent', () => { expect(editAction).toHaveBeenCalledTimes(0); expect(cloneAction).toHaveBeenCalledTimes(0); expect(copyAction).toHaveBeenCalledTimes(0); + expect(routerNavigate).toHaveBeenCalledTimes(1); + }); + + it('should unsubscribe right after image data is received', () => { + component.mode = RbdFormMode.editing; + rbdServiceGetSpy.and.returnValue(of({ pool_name: 'foo', pool_image: 'bar' })); + editAction.and.returnValue(NEVER); + component.ngOnInit(); + component.submit(); + + expect(component['rbdImage'].observers.length).toEqual(0); + expect(createAction).toHaveBeenCalledTimes(0); + expect(editAction).toHaveBeenCalledTimes(1); + expect(cloneAction).toHaveBeenCalledTimes(0); + expect(copyAction).toHaveBeenCalledTimes(0); + expect(routerNavigate).toHaveBeenCalledTimes(0); }); it('should not edit image if no image data is received', fakeAsync(() => { component.mode = RbdFormMode.editing; rbdServiceGetSpy.and.returnValue( - of({ pool_name: 'foo', pool_image: 'bar' }).pipe(delay(100)) + of({ pool_name: 'foo', pool_image: 'bar' }).pipe(delay(DELAY)) ); component.ngOnInit(); component.submit(); @@ -100,8 +120,9 @@ describe('RbdFormComponent', () => { expect(editAction).toHaveBeenCalledTimes(0); expect(cloneAction).toHaveBeenCalledTimes(0); expect(copyAction).toHaveBeenCalledTimes(0); + expect(routerNavigate).toHaveBeenCalledTimes(0); - discardPeriodicTasks(); + tick(DELAY); })); it('should edit image after image data is received', () => { @@ -113,12 +134,13 @@ describe('RbdFormComponent', () => { expect(editAction).toHaveBeenCalledTimes(1); expect(cloneAction).toHaveBeenCalledTimes(0); expect(copyAction).toHaveBeenCalledTimes(0); + expect(routerNavigate).toHaveBeenCalledTimes(1); }); it('should not clone image if no image data is received', fakeAsync(() => { component.mode = RbdFormMode.cloning; rbdServiceGetSpy.and.returnValue( - of({ pool_name: 'foo', pool_image: 'bar' }).pipe(delay(100)) + of({ pool_name: 'foo', pool_image: 'bar' }).pipe(delay(DELAY)) ); component.ngOnInit(); component.submit(); @@ -127,8 +149,9 @@ describe('RbdFormComponent', () => { expect(editAction).toHaveBeenCalledTimes(0); expect(cloneAction).toHaveBeenCalledTimes(0); expect(copyAction).toHaveBeenCalledTimes(0); + expect(routerNavigate).toHaveBeenCalledTimes(0); - discardPeriodicTasks(); + tick(DELAY); })); it('should clone image after image data is received', () => { @@ -140,12 +163,13 @@ describe('RbdFormComponent', () => { expect(editAction).toHaveBeenCalledTimes(0); expect(cloneAction).toHaveBeenCalledTimes(1); expect(copyAction).toHaveBeenCalledTimes(0); + expect(routerNavigate).toHaveBeenCalledTimes(1); }); it('should not copy image if no image data is received', fakeAsync(() => { component.mode = RbdFormMode.copying; rbdServiceGetSpy.and.returnValue( - of({ pool_name: 'foo', pool_image: 'bar' }).pipe(delay(100)) + of({ pool_name: 'foo', pool_image: 'bar' }).pipe(delay(DELAY)) ); component.ngOnInit(); component.submit(); @@ -154,8 +178,9 @@ describe('RbdFormComponent', () => { expect(editAction).toHaveBeenCalledTimes(0); expect(cloneAction).toHaveBeenCalledTimes(0); expect(copyAction).toHaveBeenCalledTimes(0); + expect(routerNavigate).toHaveBeenCalledTimes(0); - discardPeriodicTasks(); + tick(DELAY); })); it('should copy image after image data is received', () => { @@ -167,6 +192,7 @@ describe('RbdFormComponent', () => { expect(editAction).toHaveBeenCalledTimes(0); expect(cloneAction).toHaveBeenCalledTimes(0); expect(copyAction).toHaveBeenCalledTimes(1); + expect(routerNavigate).toHaveBeenCalledTimes(1); }); }); diff --git a/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/rbd-form/rbd-form.component.ts b/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/rbd-form/rbd-form.component.ts index 235baeb5d87..7820ad7d8ee 100644 --- a/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/rbd-form/rbd-form.component.ts +++ b/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/rbd-form/rbd-form.component.ts @@ -5,8 +5,8 @@ import { ActivatedRoute, Router } from '@angular/router'; import { I18n } from '@ngx-translate/i18n-polyfill'; import * as _ from 'lodash'; -import { AsyncSubject, forkJoin, Observable } from 'rxjs'; -import { switchMap } from 'rxjs/operators'; +import { forkJoin, Observable, ReplaySubject } from 'rxjs'; +import { first, switchMap } from 'rxjs/operators'; import { PoolService } from '../../../shared/api/pool.service'; import { RbdService } from '../../../shared/api/rbd.service'; @@ -89,7 +89,7 @@ export class RbdFormComponent implements OnInit { ]; action: string; resource: string; - private rbdImage = new AsyncSubject(); + private rbdImage = new ReplaySubject(1); icons = Icons; @@ -698,9 +698,9 @@ export class RbdFormComponent implements OnInit { if (!this.mode) { this.rbdImage.next('create'); } - this.rbdImage.complete(); this.rbdImage .pipe( + first(), switchMap(() => { if (this.mode === this.rbdFormMode.editing) { return this.editAction(); diff --git a/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/rbd-trash-purge-modal/rbd-trash-purge-modal.component.spec.ts b/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/rbd-trash-purge-modal/rbd-trash-purge-modal.component.spec.ts index 0202b080a2e..db80c710714 100644 --- a/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/rbd-trash-purge-modal/rbd-trash-purge-modal.component.spec.ts +++ b/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/rbd-trash-purge-modal/rbd-trash-purge-modal.component.spec.ts @@ -66,7 +66,6 @@ describe('RbdTrashPurgeModalComponent', () => { it('should call ngOnInit without pool permissions', () => { component.poolPermission = new Permission([]); component.ngOnInit(); - httpTesting.expectOne('api/summary'); httpTesting.verify(); }); diff --git a/src/pybind/mgr/dashboard/frontend/src/app/ceph/nfs/nfs-form/nfs-form.component.spec.ts b/src/pybind/mgr/dashboard/frontend/src/app/ceph/nfs/nfs-form/nfs-form.component.spec.ts index 2b138eba7bf..4b5ec0c9099 100644 --- a/src/pybind/mgr/dashboard/frontend/src/app/ceph/nfs/nfs-form/nfs-form.component.spec.ts +++ b/src/pybind/mgr/dashboard/frontend/src/app/ceph/nfs/nfs-form/nfs-form.component.spec.ts @@ -60,7 +60,6 @@ describe('NfsFormComponent', () => { activatedRoute = TestBed.get(ActivatedRoute); fixture.detectChanges(); - httpTesting.expectOne('api/summary').flush([]); httpTesting.expectOne('api/nfs-ganesha/daemon').flush([ { daemon_id: 'node1', cluster_id: 'cluster1' }, { daemon_id: 'node2', cluster_id: 'cluster1' }, diff --git a/src/pybind/mgr/dashboard/frontend/src/app/ceph/nfs/nfs-list/nfs-list.component.spec.ts b/src/pybind/mgr/dashboard/frontend/src/app/ceph/nfs/nfs-list/nfs-list.component.spec.ts index 43d6c7c155f..36039aa4d97 100644 --- a/src/pybind/mgr/dashboard/frontend/src/app/ceph/nfs/nfs-list/nfs-list.component.spec.ts +++ b/src/pybind/mgr/dashboard/frontend/src/app/ceph/nfs/nfs-list/nfs-list.component.spec.ts @@ -5,7 +5,7 @@ import { RouterTestingModule } from '@angular/router/testing'; import { TabsModule } from 'ngx-bootstrap/tabs'; import { ToastrModule } from 'ngx-toastr'; -import { BehaviorSubject, of } from 'rxjs'; +import { of } from 'rxjs'; import { configureTestBed, @@ -55,10 +55,6 @@ describe('NfsListComponent', () => { summaryService = TestBed.get(SummaryService); nfsService = TestBed.get(NfsService); httpTesting = TestBed.get(HttpTestingController); - - // this is needed because summaryService isn't being reset after each test. - summaryService['summaryDataSource'] = new BehaviorSubject(null); - summaryService['summaryData$'] = summaryService['summaryDataSource'].asObservable(); }); it('should create', () => { @@ -70,7 +66,6 @@ describe('NfsListComponent', () => { fixture.detectChanges(); spyOn(nfsService, 'list').and.callThrough(); httpTesting.expectOne('api/nfs-ganesha/daemon').flush([]); - httpTesting.expectOne('api/summary'); }); afterEach(() => { diff --git a/src/pybind/mgr/dashboard/frontend/src/app/core/layouts/workbench-layout/workbench-layout.component.ts b/src/pybind/mgr/dashboard/frontend/src/app/core/layouts/workbench-layout/workbench-layout.component.ts index 64d3f4c38ed..66589c9609c 100644 --- a/src/pybind/mgr/dashboard/frontend/src/app/core/layouts/workbench-layout/workbench-layout.component.ts +++ b/src/pybind/mgr/dashboard/frontend/src/app/core/layouts/workbench-layout/workbench-layout.component.ts @@ -1,9 +1,11 @@ -import { Component } from '@angular/core'; +import { Component, OnDestroy, OnInit } from '@angular/core'; import { Router } from '@angular/router'; import { TooltipConfig } from 'ngx-bootstrap/tooltip'; +import { Subscription } from 'rxjs'; -import { NotificationService } from '../../../shared/services/notification.service'; +import { SummaryService } from '../../../shared/services/summary.service'; +import { TaskManagerService } from '../../../shared/services/task-manager.service'; @Component({ selector: 'cd-workbench-layout', @@ -19,8 +21,23 @@ import { NotificationService } from '../../../shared/services/notification.servi } ] }) -export class WorkbenchLayoutComponent { - constructor(private router: Router, public notificationService: NotificationService) {} +export class WorkbenchLayoutComponent implements OnInit, OnDestroy { + private subs = new Subscription(); + + constructor( + private router: Router, + private summaryService: SummaryService, + private taskManagerService: TaskManagerService + ) {} + + ngOnInit() { + this.subs.add(this.summaryService.startPolling()); + this.subs.add(this.taskManagerService.init(this.summaryService)); + } + + ngOnDestroy() { + this.subs.unsubscribe(); + } isDashboardPage() { return this.router.url === '/dashboard'; diff --git a/src/pybind/mgr/dashboard/frontend/src/app/core/navigation/navigation/navigation.component.ts b/src/pybind/mgr/dashboard/frontend/src/app/core/navigation/navigation/navigation.component.ts index 30bb8f18cf2..b8310fb7351 100644 --- a/src/pybind/mgr/dashboard/frontend/src/app/core/navigation/navigation/navigation.component.ts +++ b/src/pybind/mgr/dashboard/frontend/src/app/core/navigation/navigation/navigation.component.ts @@ -1,4 +1,6 @@ -import { Component, HostBinding, OnInit } from '@angular/core'; +import { Component, HostBinding, OnDestroy, OnInit } from '@angular/core'; + +import { Subscription } from 'rxjs'; import { Icons } from '../../../shared/enum/icons.enum'; import { Permissions } from '../../../shared/models/permissions'; @@ -14,7 +16,7 @@ import { SummaryService } from '../../../shared/services/summary.service'; templateUrl: './navigation.component.html', styleUrls: ['./navigation.component.scss'] }) -export class NavigationComponent implements OnInit { +export class NavigationComponent implements OnInit, OnDestroy { @HostBinding('class.isPwdDisplayed') isPwdDisplayed = false; permissions: Permissions; @@ -29,6 +31,7 @@ export class NavigationComponent implements OnInit { simplebar = { autoHide: false }; + private subs = new Subscription(); constructor( private authStorageService: AuthStorageService, @@ -40,15 +43,23 @@ export class NavigationComponent implements OnInit { } ngOnInit() { - this.summaryService.subscribe((data: any) => { - if (!data) { - return; - } - this.summaryData = data; - }); - this.authStorageService.isPwdDisplayed$.subscribe((isDisplayed) => { - this.isPwdDisplayed = isDisplayed; - }); + this.subs.add( + this.summaryService.subscribe((data: any) => { + if (!data) { + return; + } + this.summaryData = data; + }) + ); + this.subs.add( + this.authStorageService.isPwdDisplayed$.subscribe((isDisplayed) => { + this.isPwdDisplayed = isDisplayed; + }) + ); + } + + ngOnDestroy(): void { + this.subs.unsubscribe(); } blockHealthColor() { diff --git a/src/pybind/mgr/dashboard/frontend/src/app/core/navigation/notifications/notifications.component.ts b/src/pybind/mgr/dashboard/frontend/src/app/core/navigation/notifications/notifications.component.ts index ec615cc4c1a..aaedd546eff 100644 --- a/src/pybind/mgr/dashboard/frontend/src/app/core/navigation/notifications/notifications.component.ts +++ b/src/pybind/mgr/dashboard/frontend/src/app/core/navigation/notifications/notifications.component.ts @@ -1,6 +1,6 @@ -import { Component, OnInit } from '@angular/core'; +import { Component, OnDestroy, OnInit } from '@angular/core'; -import * as _ from 'lodash'; +import { Subscription } from 'rxjs'; import { Icons } from '../../../shared/enum/icons.enum'; import { NotificationService } from '../../../shared/services/notification.service'; @@ -11,10 +11,10 @@ import { SummaryService } from '../../../shared/services/summary.service'; templateUrl: './notifications.component.html', styleUrls: ['./notifications.component.scss'] }) -export class NotificationsComponent implements OnInit { +export class NotificationsComponent implements OnInit, OnDestroy { icons = Icons; - hasRunningTasks = false; + private subs = new Subscription(); constructor( public notificationService: NotificationService, @@ -22,12 +22,18 @@ export class NotificationsComponent implements OnInit { ) {} ngOnInit() { - this.summaryService.subscribe((data: any) => { - if (!data) { - return; - } - this.hasRunningTasks = data.executing_tasks.length > 0; - }); + this.subs.add( + this.summaryService.subscribe((data: any) => { + if (!data) { + return; + } + this.hasRunningTasks = data.executing_tasks.length > 0; + }) + ); + } + + ngOnDestroy(): void { + this.subs.unsubscribe(); } toggleSidebar() { diff --git a/src/pybind/mgr/dashboard/frontend/src/app/shared/api/rbd-mirroring.service.spec.ts b/src/pybind/mgr/dashboard/frontend/src/app/shared/api/rbd-mirroring.service.spec.ts index 5bd9abad2e2..37be86346dd 100644 --- a/src/pybind/mgr/dashboard/frontend/src/app/shared/api/rbd-mirroring.service.spec.ts +++ b/src/pybind/mgr/dashboard/frontend/src/app/shared/api/rbd-mirroring.service.spec.ts @@ -1,4 +1,9 @@ -import { HttpClientTestingModule, HttpTestingController } from '@angular/common/http/testing'; +import { HttpRequest } from '@angular/common/http'; +import { + HttpClientTestingModule, + HttpTestingController, + TestRequest +} from '@angular/common/http/testing'; import { fakeAsync, TestBed, tick } from '@angular/core/testing'; import { configureTestBed } from '../../../testing/unit-test-helper'; @@ -7,6 +12,8 @@ import { RbdMirroringService } from './rbd-mirroring.service'; describe('RbdMirroringService', () => { let service: RbdMirroringService; let httpTesting: HttpTestingController; + let getMirroringSummaryCalls: () => TestRequest[]; + let flushCalls: (call: TestRequest) => void; const summary: Record = { status: 0, @@ -31,10 +38,16 @@ describe('RbdMirroringService', () => { beforeEach(() => { service = TestBed.get(RbdMirroringService); httpTesting = TestBed.get(HttpTestingController); - - const req = httpTesting.expectOne('api/block/mirroring/summary'); - expect(req.request.method).toBe('GET'); - req.flush(summary); + getMirroringSummaryCalls = () => { + return httpTesting.match((request: HttpRequest) => { + return request.url.match(/api\/block\/mirroring\/summary/) && request.method === 'GET'; + }); + }; + flushCalls = (call: TestRequest) => { + if (!call.cancelled) { + call.flush(summary); + } + }; }); afterEach(() => { @@ -46,28 +59,31 @@ describe('RbdMirroringService', () => { }); it('should periodically poll summary', fakeAsync(() => { + const subs = service.startPolling(); + tick(); const calledWith: any[] = []; - service.subscribeSummary((data) => { + service.subscribeSummary((data: any) => { calledWith.push(data); }); - service.refreshAndSchedule(); - tick(30000); - // In order to not trigger setTimeout again, - // which would raise 'Error: 1 timer(s) still in the queue.' - spyOn(service, 'refreshAndSchedule').and.callFake(() => true); - tick(30000); - - const calls = httpTesting.match((request) => { - return request.url.match(/api\/block\/mirroring\/summary/) && request.method === 'GET'; - }); + tick(service.REFRESH_INTERVAL * 2); + const calls = getMirroringSummaryCalls(); - expect(calls.length).toEqual(2); - calls.forEach((call) => call.flush(summary)); + expect(calls.length).toEqual(3); + calls.forEach((call: TestRequest) => flushCalls(call)); + expect(calledWith).toEqual([null, summary]); - expect(calledWith).toEqual([summary, summary, summary]); + subs.unsubscribe(); })); it('should get current summary', () => { + service.refresh(); + const calledWith: any[] = []; + service.subscribeSummary((data: any) => { + calledWith.push(data); + }); + const calls = getMirroringSummaryCalls(); + calls.forEach((call: TestRequest) => flushCalls(call)); + expect(service.getCurrentSummary()).toEqual(summary); }); diff --git a/src/pybind/mgr/dashboard/frontend/src/app/shared/api/rbd-mirroring.service.ts b/src/pybind/mgr/dashboard/frontend/src/app/shared/api/rbd-mirroring.service.ts index d32b09487e8..c1f069323be 100644 --- a/src/pybind/mgr/dashboard/frontend/src/app/shared/api/rbd-mirroring.service.ts +++ b/src/pybind/mgr/dashboard/frontend/src/app/shared/api/rbd-mirroring.service.ts @@ -1,9 +1,10 @@ import { HttpClient } from '@angular/common/http'; -import { Injectable, NgZone } from '@angular/core'; +import { Injectable } from '@angular/core'; -import { BehaviorSubject, Subscription } from 'rxjs'; +import { BehaviorSubject, Observable, Subscription } from 'rxjs'; import { cdEncode, cdEncodeNot } from '../decorators/cd-encode'; +import { TimerService } from '../services/timer.service'; import { ApiModule } from './api.module'; @cdEncode @@ -11,32 +12,32 @@ import { ApiModule } from './api.module'; providedIn: ApiModule }) export class RbdMirroringService { + readonly REFRESH_INTERVAL = 30000; // Observable sources private summaryDataSource = new BehaviorSubject(null); - // Observable streams summaryData$ = this.summaryDataSource.asObservable(); - constructor(private http: HttpClient, private ngZone: NgZone) { - this.refreshAndSchedule(); + constructor(private http: HttpClient, private timerService: TimerService) {} + + startPolling(): Subscription { + return this.timerService + .get(() => this.retrieveSummaryObservable(), this.REFRESH_INTERVAL) + .subscribe(this.retrieveSummaryObserver()); } - refresh() { - this.http.get('api/block/mirroring/summary').subscribe((data) => { - this.summaryDataSource.next(data); - }); + refresh(): Subscription { + return this.retrieveSummaryObservable().subscribe(this.retrieveSummaryObserver()); } - refreshAndSchedule() { - this.refresh(); + private retrieveSummaryObservable(): Observable { + return this.http.get('api/block/mirroring/summary'); + } - this.ngZone.runOutsideAngular(() => { - setTimeout(() => { - this.ngZone.run(() => { - this.refreshAndSchedule(); - }); - }, 30000); - }); + private retrieveSummaryObserver(): (data: any) => void { + return (data: any) => { + this.summaryDataSource.next(data); + }; } /** @@ -48,7 +49,7 @@ export class RbdMirroringService { /** * Subscribes to the summaryData, - * which is updated once every 30 seconds or when a new task is created. + * which is updated periodically or when a new task is created. */ subscribeSummary(next: (summary: any) => void, error?: (error: any) => void): Subscription { return this.summaryData$.subscribe(next, error); diff --git a/src/pybind/mgr/dashboard/frontend/src/app/shared/components/notifications-sidebar/notifications-sidebar.component.ts b/src/pybind/mgr/dashboard/frontend/src/app/shared/components/notifications-sidebar/notifications-sidebar.component.ts index 6af8bc04ac6..0fdde25b6dc 100644 --- a/src/pybind/mgr/dashboard/frontend/src/app/shared/components/notifications-sidebar/notifications-sidebar.component.ts +++ b/src/pybind/mgr/dashboard/frontend/src/app/shared/components/notifications-sidebar/notifications-sidebar.component.ts @@ -13,16 +13,16 @@ import * as _ from 'lodash'; import * as moment from 'moment'; import { Subscription } from 'rxjs'; -import { ExecutingTask } from '../../../shared/models/executing-task'; -import { SummaryService } from '../../../shared/services/summary.service'; -import { TaskMessageService } from '../../../shared/services/task-message.service'; import { Icons } from '../../enum/icons.enum'; import { CdNotification } from '../../models/cd-notification'; +import { ExecutingTask } from '../../models/executing-task'; import { FinishedTask } from '../../models/finished-task'; import { AuthStorageService } from '../../services/auth-storage.service'; import { NotificationService } from '../../services/notification.service'; import { PrometheusAlertService } from '../../services/prometheus-alert.service'; import { PrometheusNotificationService } from '../../services/prometheus-notification.service'; +import { SummaryService } from '../../services/summary.service'; +import { TaskMessageService } from '../../services/task-message.service'; @Component({ selector: 'cd-notifications-sidebar', @@ -39,8 +39,7 @@ export class NotificationsSidebarComponent implements OnInit, OnDestroy { executingTasks: ExecutingTask[] = []; - private sidebarSubscription: Subscription; - private notificationDataSubscription: Subscription; + private subs = new Subscription(); icons = Icons; @@ -68,12 +67,7 @@ export class NotificationsSidebarComponent implements OnInit, OnDestroy { ngOnDestroy() { window.clearInterval(this.interval); window.clearTimeout(this.timeout); - if (this.sidebarSubscription) { - this.sidebarSubscription.unsubscribe(); - } - if (this.notificationDataSubscription) { - this.notificationDataSubscription.unsubscribe(); - } + this.subs.unsubscribe(); } ngOnInit() { @@ -91,55 +85,59 @@ export class NotificationsSidebarComponent implements OnInit, OnDestroy { }); } - this.notificationDataSubscription = this.notificationService.data$.subscribe( - (notifications: CdNotification[]) => { + this.subs.add( + this.notificationService.data$.subscribe((notifications: CdNotification[]) => { this.notifications = _.orderBy(notifications, ['timestamp'], ['desc']); this.cdRef.detectChanges(); - } + }) ); - this.sidebarSubscription = this.notificationService.sidebarSubject.subscribe((forceClose) => { - if (forceClose) { - this.isSidebarOpened = false; - } else { - this.isSidebarOpened = !this.isSidebarOpened; - } + this.subs.add( + this.notificationService.sidebarSubject.subscribe((forceClose) => { + if (forceClose) { + this.isSidebarOpened = false; + } else { + this.isSidebarOpened = !this.isSidebarOpened; + } + + window.clearTimeout(this.timeout); + this.timeout = window.setTimeout(() => { + this.cdRef.detectChanges(); + }, 0); + }) + ); - window.clearTimeout(this.timeout); - this.timeout = window.setTimeout(() => { - this.cdRef.detectChanges(); - }, 0); - }); - - this.summaryService.subscribe((data: any) => { - if (!data) { - return; - } - this._handleTasks(data.executing_tasks); - - this.mutex.acquire().then((release) => { - _.filter( - data.finished_tasks, - (task: FinishedTask) => !this.last_task || moment(task.end_time).isAfter(this.last_task) - ).forEach((task) => { - const config = this.notificationService.finishedTaskToNotification(task, task.success); - const notification = new CdNotification(config); - notification.timestamp = task.end_time; - notification.duration = task.duration; - - if (!this.last_task || moment(task.end_time).isAfter(this.last_task)) { - this.last_task = task.end_time; - window.localStorage.setItem('last_task', this.last_task); - } - - this.notificationService.save(notification); - }); + this.subs.add( + this.summaryService.subscribe((data: any) => { + if (!data) { + return; + } + this._handleTasks(data.executing_tasks); + + this.mutex.acquire().then((release) => { + _.filter( + data.finished_tasks, + (task: FinishedTask) => !this.last_task || moment(task.end_time).isAfter(this.last_task) + ).forEach((task) => { + const config = this.notificationService.finishedTaskToNotification(task, task.success); + const notification = new CdNotification(config); + notification.timestamp = task.end_time; + notification.duration = task.duration; + + if (!this.last_task || moment(task.end_time).isAfter(this.last_task)) { + this.last_task = task.end_time; + window.localStorage.setItem('last_task', this.last_task); + } + + this.notificationService.save(notification); + }); - this.cdRef.detectChanges(); + this.cdRef.detectChanges(); - release(); - }); - }); + release(); + }); + }) + ); } _handleTasks(executingTasks: ExecutingTask[]) { diff --git a/src/pybind/mgr/dashboard/frontend/src/app/shared/services/feature-toggles.service.ts b/src/pybind/mgr/dashboard/frontend/src/app/shared/services/feature-toggles.service.ts index d2817319d03..bb7f2a0d614 100644 --- a/src/pybind/mgr/dashboard/frontend/src/app/shared/services/feature-toggles.service.ts +++ b/src/pybind/mgr/dashboard/frontend/src/app/shared/services/feature-toggles.service.ts @@ -1,10 +1,9 @@ import { HttpClient } from '@angular/common/http'; import { Injectable } from '@angular/core'; -import { Observable, timer } from 'rxjs'; -import { observeOn, shareReplay, switchMap } from 'rxjs/operators'; +import { Observable } from 'rxjs'; -import { NgZoneSchedulerService } from './ngzone-scheduler.service'; +import { TimerService } from './timer.service'; export class FeatureTogglesMap { rbd = true; @@ -25,11 +24,10 @@ export class FeatureTogglesService { readonly REFRESH_INTERVAL: number = 30000; private featureToggleMap$: FeatureTogglesMap$; - constructor(private http: HttpClient, protected ngZone: NgZoneSchedulerService) { - this.featureToggleMap$ = timer(0, this.REFRESH_INTERVAL, ngZone.leave).pipe( - switchMap(() => this.http.get(this.API_URL)), - shareReplay(1), - observeOn(ngZone.enter) + constructor(private http: HttpClient, private timerService: TimerService) { + this.featureToggleMap$ = this.timerService.get( + () => this.http.get(this.API_URL), + this.REFRESH_INTERVAL ); } diff --git a/src/pybind/mgr/dashboard/frontend/src/app/shared/services/summary.service.spec.ts b/src/pybind/mgr/dashboard/frontend/src/app/shared/services/summary.service.spec.ts index 9741084b2a8..100beefd2fb 100644 --- a/src/pybind/mgr/dashboard/frontend/src/app/shared/services/summary.service.spec.ts +++ b/src/pybind/mgr/dashboard/frontend/src/app/shared/services/summary.service.spec.ts @@ -2,7 +2,7 @@ import { HttpClient } from '@angular/common/http'; import { fakeAsync, TestBed, tick } from '@angular/core/testing'; import { RouterTestingModule } from '@angular/router/testing'; -import { of as observableOf, Subscriber } from 'rxjs'; +import { of as observableOf, Subscriber, Subscription } from 'rxjs'; import { configureTestBed } from '../../../testing/unit-test-helper'; import { ExecutingTask } from '../models/executing-task'; @@ -12,6 +12,7 @@ import { SummaryService } from './summary.service'; describe('SummaryService', () => { let summaryService: SummaryService; let authStorageService: AuthStorageService; + let subs: Subscription; const summary: Record = { executing_tasks: [], @@ -47,20 +48,22 @@ describe('SummaryService', () => { }); it('should call refresh', fakeAsync(() => { - summaryService.enablePolling(); authStorageService.set('foobar', undefined, undefined); const calledWith: any[] = []; - summaryService.subscribe((data) => { - calledWith.push(data); - }); + subs = new Subscription(); + subs.add(summaryService.startPolling()); + tick(); + subs.add( + summaryService.subscribe((data) => { + calledWith.push(data); + }) + ); expect(calledWith).toEqual([summary]); - summaryService.refresh(); + subs.add(summaryService.refresh()); expect(calledWith).toEqual([summary, summary]); - tick(10000); + tick(summaryService.REFRESH_INTERVAL * 2); expect(calledWith.length).toEqual(4); - // In order to not trigger setInterval again, - // which would raise 'Error: 1 timer(s) still in the queue.' - window.clearInterval(summaryService.polling); + subs.unsubscribe(); })); describe('Should test methods after first refresh', () => { diff --git a/src/pybind/mgr/dashboard/frontend/src/app/shared/services/summary.service.ts b/src/pybind/mgr/dashboard/frontend/src/app/shared/services/summary.service.ts index dc199b3f2e7..26038419214 100644 --- a/src/pybind/mgr/dashboard/frontend/src/app/shared/services/summary.service.ts +++ b/src/pybind/mgr/dashboard/frontend/src/app/shared/services/summary.service.ts @@ -1,46 +1,42 @@ import { HttpClient } from '@angular/common/http'; -import { Injectable, NgZone } from '@angular/core'; -import { Router } from '@angular/router'; +import { Injectable } from '@angular/core'; import * as _ from 'lodash'; -import { BehaviorSubject, Subscription } from 'rxjs'; +import { BehaviorSubject, Observable, Subscription } from 'rxjs'; import { ExecutingTask } from '../models/executing-task'; +import { TimerService } from './timer.service'; @Injectable({ providedIn: 'root' }) export class SummaryService { + readonly REFRESH_INTERVAL = 5000; // Observable sources private summaryDataSource = new BehaviorSubject(null); - // Observable streams summaryData$ = this.summaryDataSource.asObservable(); - polling: number; + constructor(private http: HttpClient, private timerService: TimerService) {} - constructor(private http: HttpClient, private router: Router, private ngZone: NgZone) { - this.enablePolling(); + startPolling(): Subscription { + return this.timerService + .get(() => this.retrieveSummaryObservable(), this.REFRESH_INTERVAL) + .subscribe(this.retrieveSummaryObserver()); } - enablePolling() { - this.refresh(); + refresh(): Subscription { + return this.retrieveSummaryObservable().subscribe(this.retrieveSummaryObserver()); + } - this.ngZone.runOutsideAngular(() => { - this.polling = window.setInterval(() => { - this.ngZone.run(() => { - this.refresh(); - }); - }, 5000); - }); + private retrieveSummaryObservable(): Observable { + return this.http.get('api/summary'); } - refresh() { - if (!_.includes(['/login', '/login-change-password'], this.router.url)) { - this.http.get('api/summary').subscribe((data) => { - this.summaryDataSource.next(data); - }); - } + private retrieveSummaryObserver(): (data: any) => void { + return (data: Object) => { + this.summaryDataSource.next(data); + }; } /** @@ -52,7 +48,7 @@ export class SummaryService { /** * Subscribes to the summaryData, - * which is updated once every 5 seconds or when a new task is created. + * which is updated periodically or when a new task is created. */ subscribe(next: (summary: any) => void, error?: (error: any) => void): Subscription { return this.summaryData$.subscribe(next, error); diff --git a/src/pybind/mgr/dashboard/frontend/src/app/shared/services/task-manager.service.spec.ts b/src/pybind/mgr/dashboard/frontend/src/app/shared/services/task-manager.service.spec.ts index 235d001f12e..6c797d50633 100644 --- a/src/pybind/mgr/dashboard/frontend/src/app/shared/services/task-manager.service.spec.ts +++ b/src/pybind/mgr/dashboard/frontend/src/app/shared/services/task-manager.service.spec.ts @@ -57,6 +57,7 @@ describe('TaskManagerService', () => { expect(taskManagerService.subscriptions.length).toBe(1); summaryService.refresh(); tick(); + taskManagerService.init(summaryService); expect(called).toEqual(true); expect(taskManagerService.subscriptions).toEqual([]); })); diff --git a/src/pybind/mgr/dashboard/frontend/src/app/shared/services/task-manager.service.ts b/src/pybind/mgr/dashboard/frontend/src/app/shared/services/task-manager.service.ts index d23b5b49fb9..2ef8da4136c 100644 --- a/src/pybind/mgr/dashboard/frontend/src/app/shared/services/task-manager.service.ts +++ b/src/pybind/mgr/dashboard/frontend/src/app/shared/services/task-manager.service.ts @@ -25,8 +25,8 @@ class TaskSubscription { export class TaskManagerService { subscriptions: Array = []; - constructor(summaryService: SummaryService) { - summaryService.subscribe((data: any) => { + init(summaryService: SummaryService) { + return summaryService.subscribe((data: any) => { if (!data) { return; } @@ -51,7 +51,7 @@ export class TaskManagerService { this.subscriptions.push(new TaskSubscription(name, metadata, onTaskFinished)); } - _getTask(subscription: TaskSubscription, tasks: Array): Task { + private _getTask(subscription: TaskSubscription, tasks: Array): Task { for (const task of tasks) { if (task.name === subscription.name && _.isEqual(task.metadata, subscription.metadata)) { return task; diff --git a/src/pybind/mgr/dashboard/frontend/src/app/shared/services/timer.service.spec.ts b/src/pybind/mgr/dashboard/frontend/src/app/shared/services/timer.service.spec.ts new file mode 100644 index 00000000000..641fa717db6 --- /dev/null +++ b/src/pybind/mgr/dashboard/frontend/src/app/shared/services/timer.service.spec.ts @@ -0,0 +1,68 @@ +import { fakeAsync, TestBed, tick } from '@angular/core/testing'; + +import { of, Subscription } from 'rxjs'; + +import { configureTestBed } from '../../../testing/unit-test-helper'; +import { TimerService } from './timer.service'; + +describe('TimerService', () => { + let service: TimerService; + let subs: Subscription; + let receivedData: any[]; + const next = () => of(true); + const observer = (data: boolean) => { + receivedData.push(data); + }; + + configureTestBed({ + providers: [TimerService] + }); + + beforeEach(() => { + service = TestBed.get(TimerService); + receivedData = []; + }); + + it('should be created', () => { + expect(service).toBeTruthy(); + }); + + it('should not emit any value when no subscribers', fakeAsync(() => { + subs = service.get(next).subscribe(observer); + tick(service.DEFAULT_REFRESH_INTERVAL); + expect(receivedData.length).toEqual(2); + + subs.unsubscribe(); + + tick(service.DEFAULT_REFRESH_INTERVAL); + expect(receivedData.length).toEqual(2); + })); + + it('should emit value with no dueTime and no refresh interval', fakeAsync(() => { + subs = service.get(next, null, null).subscribe(observer); + tick(service.DEFAULT_REFRESH_INTERVAL); + expect(receivedData.length).toEqual(1); + expect(receivedData).toEqual([true]); + + subs.unsubscribe(); + })); + + it('should emit expected values when refresh interval + no dueTime', fakeAsync(() => { + subs = service.get(next).subscribe(observer); + tick(service.DEFAULT_REFRESH_INTERVAL * 2); + expect(receivedData.length).toEqual(3); + expect(receivedData).toEqual([true, true, true]); + + subs.unsubscribe(); + })); + + it('should emit expected values when dueTime equal to refresh interval', fakeAsync(() => { + const dueTime = 1000; + subs = service.get(next, service.DEFAULT_REFRESH_INTERVAL, dueTime).subscribe(observer); + tick(service.DEFAULT_REFRESH_INTERVAL * 2); + expect(receivedData.length).toEqual(2); + expect(receivedData).toEqual([true, true]); + + subs.unsubscribe(); + })); +}); diff --git a/src/pybind/mgr/dashboard/frontend/src/app/shared/services/timer.service.ts b/src/pybind/mgr/dashboard/frontend/src/app/shared/services/timer.service.ts new file mode 100644 index 00000000000..2ae2f4cdf5e --- /dev/null +++ b/src/pybind/mgr/dashboard/frontend/src/app/shared/services/timer.service.ts @@ -0,0 +1,27 @@ +import { Injectable } from '@angular/core'; + +import { Observable, timer } from 'rxjs'; +import { observeOn, shareReplay, switchMap } from 'rxjs/operators'; + +import { NgZoneSchedulerService } from './ngzone-scheduler.service'; + +@Injectable({ + providedIn: 'root' +}) +export class TimerService { + readonly DEFAULT_REFRESH_INTERVAL = 5000; + readonly DEFAULT_DUE_TIME = 0; + constructor(private ngZone: NgZoneSchedulerService) {} + + get( + next: () => Observable, + refreshInterval: number = this.DEFAULT_REFRESH_INTERVAL, + dueTime: number = this.DEFAULT_DUE_TIME + ): Observable { + return timer(dueTime, refreshInterval, this.ngZone.leave).pipe( + observeOn(this.ngZone.enter), + switchMap(next), + shareReplay({ refCount: true, bufferSize: 1 }) + ); + } +} -- 2.39.5