Merge pull request #22906 from tspmelo/wip-improve-summary

mgr/dashboard: Improve SummaryService and TaskWrapperService

Reviewed-by: Ricardo Marques <rimarques@suse.com>
Reviewed-by: Stephan Müller <smueller@suse.com>

src/pybind/mgr/dashboard/frontend/src/app/ceph/block/rbd-list/rbd-list.component.ts

@@ -2,6 +2,7 @@ import { Component, OnDestroy, OnInit, TemplateRef, ViewChild } from '@angular/c
 
 import * as _ from 'lodash';
 import { BsModalRef, BsModalService } from 'ngx-bootstrap';
+import { Subscription } from 'rxjs';
 
 import { RbdService } from '../../../shared/api/rbd.service';
 import { ConfirmationModalComponent } from '../../../shared/components/confirmation-modal/confirmation-modal.component';
@@ -36,13 +37,12 @@ export class RbdListComponent implements OnInit, OnDestroy {
 
   permission: Permission;
   images: any;
-  executingTasks: ExecutingTask[] = [];
   columns: CdTableColumn[];
   retries: number;
   viewCacheStatusList: any[];
   selection = new CdTableSelection();
 
-  summaryDataSubscription = null;
+  summaryDataSubscription: Subscription;
 
   modalRef: BsModalRef;
 
@@ -53,7 +53,8 @@ export class RbdListComponent implements OnInit, OnDestroy {
     private dimlessPipe: DimlessPipe,
     private summaryService: SummaryService,
     private modalService: BsModalService,
-    private taskWrapper: TaskWrapperService) {
+    private taskWrapper: TaskWrapperService
+  ) {
     this.permission = this.authStorageService.getPermissions().rbdImage;
   }
 
@@ -113,18 +114,13 @@ export class RbdListComponent implements OnInit, OnDestroy {
       }
     ];
 
-    this.summaryService.get().subscribe((resp: any) => {
-      if (!resp) {
+    this.summaryDataSubscription = this.summaryService.subscribe((data: any) => {
+      if (!data) {
+        this.table.reset(); // Disable loading indicator.
+        this.viewCacheStatusList = [{ status: ViewCacheStatus.ValueException }];
         return;
       }
-      this.loadImages(resp.executing_tasks);
-      this.summaryDataSubscription = this.summaryService.summaryData$.subscribe((data: any) => {
-        this.loadImages(data.executing_tasks);
-      });
-    },
-    () => {
-      this.table.reset(); // Disable loading indicator.
-      this.viewCacheStatusList = [{ status: ViewCacheStatus.ValueException }];
+      this.loadImages(data.executing_tasks);
     });
   }
 
@@ -135,9 +131,6 @@ export class RbdListComponent implements OnInit, OnDestroy {
   }
 
   loadImages(executingTasks) {
-    if (executingTasks === null) {
-      executingTasks = this.executingTasks;
-    }
     this.rbdService.list().subscribe(
       (resp: any[]) => {
         let images = [];
@@ -150,7 +143,7 @@ export class RbdListComponent implements OnInit, OnDestroy {
           images = images.concat(pool.value);
         });
         const viewCacheStatusList = [];
-        _.forEach(viewCacheStatusMap, (value, key) => {
+        _.forEach(viewCacheStatusMap, (value: any, key) => {
           viewCacheStatusList.push({
             status: parseInt(key, 10),
             statusFor:
@@ -169,7 +162,6 @@ export class RbdListComponent implements OnInit, OnDestroy {
           );
         });
         this.images = this.merge(images, executingTasks);
-        this.executingTasks = executingTasks;
       },
       () => {
         this.table.reset(); // Disable loading indicator.
@@ -264,7 +256,6 @@ export class RbdListComponent implements OnInit, OnDestroy {
             pool_name: poolName,
             image_name: imageName
           }),
-          tasks: this.executingTasks,
           call: this.rbdService.delete(poolName, imageName)
         }),
       modalRef: this.modalRef
@@ -278,12 +269,10 @@ export class RbdListComponent implements OnInit, OnDestroy {
           pool_name: poolName,
           image_name: imageName
         }),
-        tasks: this.executingTasks,
         call: this.rbdService.flatten(poolName, imageName)
       })
       .subscribe(undefined, undefined, () => {
         this.modalRef.hide();
-        this.loadImages(null);
       });
   }
 

src/pybind/mgr/dashboard/frontend/src/app/core/navigation/about/about.component.spec.ts

@@ -2,20 +2,24 @@ import { HttpClientTestingModule } from '@angular/common/http/testing';
 import { ComponentFixture, TestBed } from '@angular/core/testing';
 
 import { BsModalRef } from 'ngx-bootstrap';
-import 'rxjs/add/observable/of';
-import { Observable } from 'rxjs/Observable';
+import { BehaviorSubject } from 'rxjs';
 
 import { configureTestBed } from '../../../../testing/unit-test-helper';
 import { SummaryService } from '../../../shared/services/summary.service';
 import { SharedModule } from '../../../shared/shared.module';
 import { AboutComponent } from './about.component';
 
-class SummaryServiceMock {
-  summaryData$ = Observable.of({
+export class SummaryServiceMock {
+  summaryDataSource = new BehaviorSubject({
     version:
       'ceph version 14.0.0-855-gb8193bb4cd ' +
       '(b8193bb4cda16ccc5b028c3e1df62bc72350a15d) nautilus (dev)'
   });
+  summaryData$ = this.summaryDataSource.asObservable();
+
+  subscribe(call) {
+    return this.summaryData$.subscribe(call);
+  }
 }
 
 describe('AboutComponent', () => {

src/pybind/mgr/dashboard/frontend/src/app/core/navigation/about/about.component.ts

@@ -19,7 +19,7 @@ export class AboutComponent implements OnInit, OnDestroy {
   constructor(public modalRef: BsModalRef, private summaryService: SummaryService) {}
 
   ngOnInit() {
-    this.subs = this.summaryService.summaryData$.subscribe((summary: any) => {
+    this.subs = this.summaryService.subscribe((summary: any) => {
       if (!summary) {
         return;
       }

src/pybind/mgr/dashboard/frontend/src/app/core/navigation/dashboard-help/dashboard-help.component.ts

@@ -22,13 +22,17 @@ export class DashboardHelpComponent implements OnInit {
   ) {}
 
   ngOnInit() {
-    const subs = this.summaryService.summaryData$.subscribe((summary: any) => {
+    const subs = this.summaryService.subscribe((summary: any) => {
       if (!summary) {
         return;
       }
+
       const releaseName = this.cephReleaseNamePipe.transform(summary.version);
       this.docsUrl = `http://docs.ceph.com/docs/${releaseName}/mgr/dashboard/`;
-      subs.unsubscribe();
+
+      setTimeout(() => {
+        subs.unsubscribe();
+      }, 0);
     });
   }
 

src/pybind/mgr/dashboard/frontend/src/app/core/navigation/navigation/navigation.component.ts

@@ -22,7 +22,7 @@ export class NavigationComponent implements OnInit {
   }
 
   ngOnInit() {
-    this.summaryService.summaryData$.subscribe((data: any) => {
+    this.summaryService.subscribe((data: any) => {
       if (!data) {
         return;
       }

src/pybind/mgr/dashboard/frontend/src/app/core/navigation/task-manager/task-manager.component.ts

@@ -22,7 +22,7 @@ export class TaskManagerComponent implements OnInit {
   ) {}
 
   ngOnInit() {
-    this.summaryService.summaryData$.subscribe((data: any) => {
+    this.summaryService.subscribe((data: any) => {
       if (!data) {
         return;
       }

src/pybind/mgr/dashboard/frontend/src/app/shared/services/summary.service.spec.ts

@@ -1,9 +1,10 @@
 import { HttpClient } from '@angular/common/http';
 import { fakeAsync, TestBed, tick } from '@angular/core/testing';
 
-import { of as observableOf } from 'rxjs';
+import { of as observableOf, Subscriber } from 'rxjs';
 
 import { configureTestBed } from '../../../testing/unit-test-helper';
+import { ExecutingTask } from '../models/executing-task';
 import { AuthStorageService } from './auth-storage.service';
 import { SummaryService } from './summary.service';
 
@@ -11,18 +12,19 @@ describe('SummaryService', () => {
   let summaryService: SummaryService;
   let authStorageService: AuthStorageService;
 
+  const summary = {
+    executing_tasks: [],
+    health_status: 'HEALTH_OK',
+    mgr_id: 'x',
+    rbd_mirroring: { errors: 0, warnings: 0 },
+    rbd_pools: [],
+    have_mon_connection: true,
+    finished_tasks: [],
+    filesystems: [{ id: 1, name: 'cephfs_a' }]
+  };
+
   const httpClientSpy = {
-    get: () =>
-      observableOf({
-        executing_tasks: [],
-        health_status: 'HEALTH_OK',
-        mgr_id: 'x',
-        rbd_mirroring: { errors: 0, warnings: 0 },
-        rbd_pools: [],
-        have_mon_connection: true,
-        finished_tasks: [],
-        filesystems: [{ id: 1, name: 'cephfs_a' }]
-      })
+    get: () => observableOf(summary)
   };
 
   configureTestBed({
@@ -48,7 +50,7 @@ describe('SummaryService', () => {
       authStorageService.set('foobar');
       let result = false;
       summaryService.refresh();
-      summaryService.summaryData$.subscribe((res) => {
+      summaryService.subscribe(() => {
         result = true;
       });
       tick(5000);
@@ -58,7 +60,38 @@ describe('SummaryService', () => {
     })
   );
 
-  it('should get summary', () => {
-    expect(summaryService.get()).toEqual(jasmine.any(Object));
+  describe('Should test methods after first refresh', () => {
+    beforeEach(() => {
+      authStorageService.set('foobar');
+      summaryService.refresh();
+    });
+
+    it('should call getCurrentSummary', () => {
+      expect(summaryService.getCurrentSummary ()).toEqual(summary);
+    });
+
+    it('should call subscribe', () => {
+      let result;
+      const subscriber = summaryService.subscribe((data) => {
+        result = data;
+      });
+      expect(subscriber).toEqual(jasmine.any(Subscriber));
+      expect(result).toEqual(summary);
+    });
+
+    it('should call addRunningTask', () => {
+      summaryService.addRunningTask(
+        new ExecutingTask('rbd/delete', {
+          pool_name: 'somePool',
+          image_name: 'someImage'
+        })
+      );
+      const result = summaryService.getCurrentSummary ();
+      expect(result.executing_tasks.length).toBe(1);
+      expect(result.executing_tasks[0]).toEqual({
+        metadata: { image_name: 'someImage', pool_name: 'somePool' },
+        name: 'rbd/delete'
+      });
+    });
   });
 });

src/pybind/mgr/dashboard/frontend/src/app/shared/services/summary.service.ts

@@ -1,8 +1,10 @@
 import { HttpClient } from '@angular/common/http';
 import { Injectable, NgZone } from '@angular/core';
 
-import { BehaviorSubject } from 'rxjs';
+import * as _ from 'lodash';
+import { BehaviorSubject, Subscription } from 'rxjs';
 
+import { ExecutingTask } from '../models/executing-task';
 import { AuthStorageService } from './auth-storage.service';
 import { ServicesModule } from './services.module';
 
@@ -26,7 +28,7 @@ export class SummaryService {
 
   refresh() {
     if (this.authStorageService.isLoggedIn()) {
-      this.http.get('api/summary').subscribe(data => {
+      this.http.get('api/summary').subscribe((data) => {
         this.summaryDataSource.next(data);
       });
     }
@@ -40,7 +42,48 @@ export class SummaryService {
     });
   }
 
-  get() {
-    return this.http.get('api/summary');
+  /**
+   * Returns the current value of summaryData
+   *
+   * @returns {object}
+   * @memberof SummaryService
+   */
+  getCurrentSummary () {
+    return this.summaryDataSource.getValue();
+  }
+
+  /**
+   * Subscribes to the summaryData,
+   * which is updated once every 5 seconds or when a new task is created.
+   *
+   * @param {(summary: any) => void} call
+   * @returns {Subscription}
+   * @memberof SummaryService
+   */
+  subscribe(call: (summary: any) => void): Subscription {
+    return this.summaryData$.subscribe(call);
+  }
+
+  /**
+   * Inserts a newly created task to the local list of executing tasks.
+   * After that, it will automatically push that new information
+   * to all subscribers.
+   *
+   * @param {ExecutingTask} task
+   * @memberof SummaryService
+   */
+  addRunningTask(task: ExecutingTask) {
+    const current = this.summaryDataSource.getValue();
+    if (!current) {
+      return;
+    }
+
+    if (_.isArray(current.executing_tasks)) {
+      current.executing_tasks.push(task);
+    } else {
+      current.executing_tasks = [task];
+    }
+
+    this.summaryDataSource.next(current);
   }
 }

src/pybind/mgr/dashboard/frontend/src/app/shared/services/task-manager.service.spec.ts

@@ -1,41 +1,50 @@
 import { fakeAsync, TestBed, tick } from '@angular/core/testing';
 
 import * as _ from 'lodash';
-import { Subject } from 'rxjs';
+import { BehaviorSubject } from 'rxjs';
 
 import { configureTestBed } from '../../../testing/unit-test-helper';
 import { SummaryService } from './summary.service';
 import { TaskManagerService } from './task-manager.service';
 
+const summary = {
+  executing_tasks: [],
+  health_status: 'HEALTH_OK',
+  mgr_id: 'x',
+  rbd_mirroring: { errors: 0, warnings: 0 },
+  rbd_pools: [],
+  have_mon_connection: true,
+  finished_tasks: [{ name: 'foo', metadata: {} }],
+  filesystems: [{ id: 1, name: 'cephfs_a' }]
+};
+
+export class SummaryServiceMock {
+  summaryDataSource = new BehaviorSubject(summary);
+  summaryData$ = this.summaryDataSource.asObservable();
+
+  refresh() {
+    this.summaryDataSource.next(summary);
+  }
+  subscribe(call) {
+    return this.summaryData$.subscribe(call);
+  }
+}
+
 describe('TaskManagerService', () => {
   let taskManagerService: TaskManagerService;
+  let summaryService: any;
   let called: boolean;
 
-  const summaryDataSource = new Subject();
-  const fakeService = {
-    summaryData$: summaryDataSource.asObservable()
-  };
-
-  const summary = {
-    executing_tasks: [],
-    health_status: 'HEALTH_OK',
-    mgr_id: 'x',
-    rbd_mirroring: { errors: 0, warnings: 0 },
-    rbd_pools: [],
-    have_mon_connection: true,
-    finished_tasks: [{ name: 'foo', metadata: {} }],
-    filesystems: [{ id: 1, name: 'cephfs_a' }]
-  };
-
   configureTestBed(
     {
-      providers: [TaskManagerService, { provide: SummaryService, useValue: fakeService }]
+      providers: [TaskManagerService, { provide: SummaryService, useClass: SummaryServiceMock }]
     },
     true
   );
 
   beforeEach(() => {
     taskManagerService = TestBed.get(TaskManagerService);
+    summaryService = TestBed.get(SummaryService);
     called = false;
     taskManagerService.subscribe('foo', {}, () => (called = true));
   });
@@ -48,7 +57,7 @@ describe('TaskManagerService', () => {
     'should subscribe and be notified when task is finished',
     fakeAsync(() => {
       expect(taskManagerService.subscriptions.length).toBe(1);
-      summaryDataSource.next(summary);
+      summaryService.refresh();
       tick();
       expect(called).toEqual(true);
       expect(taskManagerService.subscriptions).toEqual([]);
@@ -63,7 +72,7 @@ describe('TaskManagerService', () => {
         executing_tasks: [{ name: 'foo', metadata: {} }],
         finished_tasks: []
       });
-      summaryDataSource.next(summary);
+      summaryService.refresh();
       tick();
       expect(taskManagerService.subscriptions).toEqual(original_subscriptions);
     })

src/pybind/mgr/dashboard/frontend/src/app/shared/services/task-manager.service.ts

@@ -27,7 +27,7 @@ export class TaskManagerService {
   subscriptions: Array<TaskSubscription> = [];
 
   constructor(summaryService: SummaryService) {
-    summaryService.summaryData$.subscribe((data: any) => {
+    summaryService.subscribe((data: any) => {
       if (!data) {
         return;
       }

src/pybind/mgr/dashboard/frontend/src/app/shared/services/task-wrapper.service.spec.ts

@@ -5,10 +5,10 @@ import { ToastModule } from 'ng2-toastr';
 import { Observable } from 'rxjs/Observable';
 
 import { configureTestBed } from '../../../testing/unit-test-helper';
-import { ExecutingTask } from '../models/executing-task';
 import { FinishedTask } from '../models/finished-task';
 import { SharedModule } from '../shared.module';
 import { NotificationService } from './notification.service';
+import { SummaryService } from './summary.service';
 import { TaskManagerService } from './task-manager.service';
 import { TaskWrapperService } from './task-wrapper.service';
 
@@ -30,8 +30,8 @@ describe('TaskWrapperService', () => {
 
   describe('wrapTaskAroundCall', () => {
     let notify: NotificationService;
-    let tasks: ExecutingTask[];
     let passed: boolean;
+    let summaryService: SummaryService;
 
     const fakeCall = (status?) =>
       new Observable((observer) => {
@@ -45,31 +45,31 @@ describe('TaskWrapperService', () => {
     const callWrapTaskAroundCall = (status, name) => {
       return service.wrapTaskAroundCall({
         task: new FinishedTask(name, { sth: 'else' }),
-        call: fakeCall(status),
-        tasks: tasks
+        call: fakeCall(status)
       });
     };
 
     beforeEach(() => {
       passed = false;
-      tasks = [];
       notify = TestBed.get(NotificationService);
+      summaryService = TestBed.get(SummaryService);
       spyOn(notify, 'show');
       spyOn(service, '_handleExecutingTasks').and.callThrough();
+      spyOn(summaryService, 'addRunningTask').and.callThrough();
     });
 
     it('should simulate a synchronous task', () => {
       callWrapTaskAroundCall(200, 'sync').subscribe(null, null, () => (passed = true));
       expect(service._handleExecutingTasks).not.toHaveBeenCalled();
       expect(passed).toBeTruthy();
-      expect(tasks.length).toBe(0);
+      expect(summaryService.addRunningTask).not.toHaveBeenCalled();
     });
 
     it('should simulate a asynchronous task', () => {
       callWrapTaskAroundCall(202, 'async').subscribe(null, null, () => (passed = true));
       expect(service._handleExecutingTasks).toHaveBeenCalled();
       expect(passed).toBeTruthy();
-      expect(tasks.length).toBe(1);
+      expect(summaryService.addRunningTask).toHaveBeenCalledTimes(1);
     });
 
     it('should call notifyTask if asynchronous task would have been finished', () => {
@@ -86,7 +86,7 @@ describe('TaskWrapperService', () => {
       callWrapTaskAroundCall(null, 'async').subscribe(null, () => (passed = true), null);
       expect(service._handleExecutingTasks).not.toHaveBeenCalled();
       expect(passed).toBeTruthy();
-      expect(tasks.length).toBe(0);
+      expect(summaryService.addRunningTask).not.toHaveBeenCalled();
     });
   });
 });

src/pybind/mgr/dashboard/frontend/src/app/shared/services/task-wrapper.service.ts

@@ -8,6 +8,7 @@ import { ExecutingTask } from '../models/executing-task';
 import { FinishedTask } from '../models/finished-task';
 import { NotificationService } from './notification.service';
 import { ServicesModule } from './services.module';
+import { SummaryService } from './summary.service';
 import { TaskManagerMessageService } from './task-manager-message.service';
 import { TaskManagerService } from './task-manager.service';
 
@@ -17,25 +18,19 @@ import { TaskManagerService } from './task-manager.service';
 export class TaskWrapperService {
   constructor(
     private notificationService: NotificationService,
+    private summaryService: SummaryService,
     private taskManagerMessageService: TaskManagerMessageService,
     private taskManagerService: TaskManagerService
   ) {}
 
-  wrapTaskAroundCall({
-    task,
-    call,
-    tasks
-  }: {
-    task: FinishedTask;
-    call: Observable<any>;
-    tasks?: ExecutingTask[];
-  }) {
+  wrapTaskAroundCall({ task, call }: { task: FinishedTask; call: Observable<any> }) {
     return new Observable((observer: Subscriber<any>) => {
       call.subscribe(
         (resp) => {
           if (resp.status === 202) {
-            this._handleExecutingTasks(task, tasks);
+            this._handleExecutingTasks(task);
           } else {
+            this.summaryService.refresh();
             task.success = true;
             this.notificationService.notifyTask(task);
           }
@@ -53,16 +48,16 @@ export class TaskWrapperService {
     });
   }
 
-  _handleExecutingTasks(task: FinishedTask, tasks?: ExecutingTask[]) {
+  _handleExecutingTasks(task: FinishedTask) {
     this.notificationService.show(
       NotificationType.info,
       this.taskManagerMessageService.getRunningMessage(task),
       this.taskManagerMessageService.getDescription(task)
     );
+
     const executingTask = new ExecutingTask(task.name, task.metadata);
-    if (tasks) {
-      tasks.push(executingTask);
-    }
+    this.summaryService.addRunningTask(executingTask);
+
     this.taskManagerService.subscribe(
       executingTask.name,
       executingTask.metadata,