mgr/dashboard: Improve Change Detection on RBD Snapshot

We are now using OnPush for the RbdSnapshotList component.

RbdSnapshotActionsModel is now only created once.

Some data is only updated when it changes.

Fixes: https://tracker.ceph.com/issues/47685
Signed-off-by: Tiago Melo <tmelo@suse.com>

diff --git a/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/rbd-snapshot-form/rbd-snapshot-form-modal.component.ts b/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/rbd-snapshot-form/rbd-snapshot-form-modal.component.ts
index b908a7ca35fab135da3e689094fb252439256992..cb744d6056fc3e60b8011632a5245cb4e1866bfb 100644 (file)
--- a/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/rbd-snapshot-form/rbd-snapshot-form-modal.component.ts
+++ b/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/rbd-snapshot-form/rbd-snapshot-form-modal.component.ts
@@ -1,4 +1,4 @@
-import { Component, OnInit } from '@angular/core';
+import { Component } from '@angular/core';
 import { FormControl, Validators } from '@angular/forms';
 
 import { NgbActiveModal } from '@ng-bootstrap/ng-bootstrap';
@@ -17,7 +17,7 @@ import { TaskManagerService } from '../../../shared/services/task-manager.servic
   templateUrl: './rbd-snapshot-form-modal.component.html',
   styleUrls: ['./rbd-snapshot-form-modal.component.scss']
 })
-export class RbdSnapshotFormModalComponent implements OnInit {
+export class RbdSnapshotFormModalComponent {
   poolName: string;
   namespace: string;
   imageName: string;
@@ -29,7 +29,7 @@ export class RbdSnapshotFormModalComponent implements OnInit {
   action: string;
   resource: string;
 
-  public onSubmit: Subject<string>;
+  public onSubmit: Subject<string> = new Subject();
 
   constructor(
     public activeModal: NgbActiveModal,
@@ -51,10 +51,6 @@ export class RbdSnapshotFormModalComponent implements OnInit {
     });
   }
 
-  ngOnInit() {
-    this.onSubmit = new Subject();
-  }
-
   setSnapName(snapName: string) {
     this.snapName = snapName;
     this.snapshotForm.get('snapshotName').setValue(snapName);

diff --git a/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/rbd-snapshot-list/rbd-snapshot-actions.model.ts b/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/rbd-snapshot-list/rbd-snapshot-actions.model.ts
index 2b22da09aab65b3821e8bb1f86f77f943f65e9d6..0bf61061e40e1ecd60076aac12ee2def9866244b 100644 (file)
--- a/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/rbd-snapshot-list/rbd-snapshot-actions.model.ts
+++ b/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/rbd-snapshot-list/rbd-snapshot-actions.model.ts
@@ -18,7 +18,11 @@ export class RbdSnapshotActionsModel {
 
   cloneFormatVersion = 1;
 
-  constructor(actionLabels: ActionLabelsI18n, featuresName: string[], rbdService: RbdService) {
+  constructor(
+    actionLabels: ActionLabelsI18n,
+    public featuresName: string[],
+    rbdService: RbdService
+  ) {
     rbdService.cloneFormatVersion().subscribe((version: number) => {
       this.cloneFormatVersion = version;
     });
@@ -50,7 +54,8 @@ export class RbdSnapshotActionsModel {
     this.clone = {
       permission: 'create',
       canBePrimary: (selection: CdTableSelection) => selection.hasSingleSelection,
-      disable: (selection: CdTableSelection) => this.getCloneDisableDesc(selection, featuresName),
+      disable: (selection: CdTableSelection) =>
+        this.getCloneDisableDesc(selection, this.featuresName),
       icon: Icons.clone,
       name: actionLabels.CLONE
     };

diff --git a/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/rbd-snapshot-list/rbd-snapshot-list.component.spec.ts b/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/rbd-snapshot-list/rbd-snapshot-list.component.spec.ts
index 44f4130183f0c7966cc607511960a74266d65575..fec0995ca1fb0e18b77f11ac2e7b24122c1e236a 100644 (file)
--- a/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/rbd-snapshot-list/rbd-snapshot-list.component.spec.ts
+++ b/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/rbd-snapshot-list/rbd-snapshot-list.component.spec.ts
@@ -111,7 +111,8 @@ describe('RbdSnapshotListComponent', () => {
         notificationService,
         null,
         null,
-        actionLabelsI18n
+        actionLabelsI18n,
+        null
       );
       spyOn(rbdService, 'deleteSnapshot').and.returnValue(observableThrowError({ status: 500 }));
       spyOn(notificationService, 'notifyTask').and.stub();
@@ -227,6 +228,7 @@ describe('RbdSnapshotListComponent', () => {
   });
 
   it('should test all TableActions combinations', () => {
+    component.ngOnInit();
     const permissionHelper: PermissionHelper = new PermissionHelper(component.permission);
     const tableActions: TableActionsComponent = permissionHelper.setPermissionsAndGetActions(
       component.tableActions

diff --git a/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/rbd-snapshot-list/rbd-snapshot-list.component.ts b/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/rbd-snapshot-list/rbd-snapshot-list.component.ts
index d14136c7c888de2ba5758a919b1f99b4d9cb8a82..26542b55321dd88fee57e1950de430da16811137 100644 (file)
--- a/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/rbd-snapshot-list/rbd-snapshot-list.component.ts
+++ b/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/rbd-snapshot-list/rbd-snapshot-list.component.ts
@@ -1,10 +1,20 @@
-import { Component, Input, OnChanges, OnInit, TemplateRef, ViewChild } from '@angular/core';
+import {
+  ChangeDetectionStrategy,
+  ChangeDetectorRef,
+  Component,
+  Input,
+  OnChanges,
+  OnInit,
+  TemplateRef,
+  ViewChild
+} from '@angular/core';
 
 import { NgbModalRef } from '@ng-bootstrap/ng-bootstrap';
 import moment from 'moment';
 import { of } from 'rxjs';
 
 import { RbdService } from '../../../shared/api/rbd.service';
+import { CdHelperClass } from '../../../shared/classes/cd-helper.class';
 import { ConfirmationModalComponent } from '../../../shared/components/confirmation-modal/confirmation-modal.component';
 import { CriticalConfirmationModalComponent } from '../../../shared/components/critical-confirmation-modal/critical-confirmation-modal.component';
 import { ActionLabelsI18n } from '../../../shared/constants/app.constants';
@@ -33,7 +43,8 @@ import { RbdSnapshotModel } from './rbd-snapshot.model';
   selector: 'cd-rbd-snapshot-list',
   templateUrl: './rbd-snapshot-list.component.html',
   styleUrls: ['./rbd-snapshot-list.component.scss'],
-  providers: [TaskListService]
+  providers: [TaskListService],
+  changeDetection: ChangeDetectionStrategy.OnPush
 })
 export class RbdSnapshotListComponent implements OnInit, OnChanges {
   @Input()
@@ -54,6 +65,8 @@ export class RbdSnapshotListComponent implements OnInit, OnChanges {
   permission: Permission;
   selection = new CdTableSelection();
   tableActions: CdTableAction[];
+  rbdTableActions: RbdSnapshotActionsModel;
+  imageSpec: ImageSpec;
 
   data: RbdSnapshotModel[];
 
@@ -79,7 +92,8 @@ export class RbdSnapshotListComponent implements OnInit, OnChanges {
     private notificationService: NotificationService,
     private summaryService: SummaryService,
     private taskListService: TaskListService,
-    private actionLabels: ActionLabelsI18n
+    private actionLabels: ActionLabelsI18n,
+    private cdr: ChangeDetectorRef
   ) {
     this.permission = this.authStorageService.getPermissions().rbdImage;
   }
@@ -125,28 +139,26 @@ export class RbdSnapshotListComponent implements OnInit, OnChanges {
         pipe: this.cdDatePipe
       }
     ];
-  }
-
-  ngOnChanges() {
-    const imageSpec = new ImageSpec(this.poolName, this.namespace, this.rbdName);
 
-    const actions = new RbdSnapshotActionsModel(
+    this.imageSpec = new ImageSpec(this.poolName, this.namespace, this.rbdName);
+    this.rbdTableActions = new RbdSnapshotActionsModel(
       this.actionLabels,
       this.featuresName,
       this.rbdService
     );
-    actions.create.click = () => this.openCreateSnapshotModal();
-    actions.rename.click = () => this.openEditSnapshotModal();
-    actions.protect.click = () => this.toggleProtection();
-    actions.unprotect.click = () => this.toggleProtection();
+    this.rbdTableActions.create.click = () => this.openCreateSnapshotModal();
+    this.rbdTableActions.rename.click = () => this.openEditSnapshotModal();
+    this.rbdTableActions.protect.click = () => this.toggleProtection();
+    this.rbdTableActions.unprotect.click = () => this.toggleProtection();
     const getImageUri = () =>
       this.selection.first() &&
-      `${imageSpec.toStringEncoded()}/${encodeURIComponent(this.selection.first().name)}`;
-    actions.clone.routerLink = () => `/block/rbd/clone/${getImageUri()}`;
-    actions.copy.routerLink = () => `/block/rbd/copy/${getImageUri()}`;
-    actions.rollback.click = () => this.rollbackModal();
-    actions.deleteSnap.click = () => this.deleteSnapshotModal();
-    this.tableActions = actions.ordering;
+      `${this.imageSpec.toStringEncoded()}/${encodeURIComponent(this.selection.first().name)}`;
+    this.rbdTableActions.clone.routerLink = () => `/block/rbd/clone/${getImageUri()}`;
+    this.rbdTableActions.copy.routerLink = () => `/block/rbd/copy/${getImageUri()}`;
+    this.rbdTableActions.rollback.click = () => this.rollbackModal();
+    this.rbdTableActions.deleteSnap.click = () => this.deleteSnapshotModal();
+
+    this.tableActions = this.rbdTableActions.ordering;
 
     const itemFilter = (entry: any, task: Task) => {
       return entry.name === task.metadata['snapshot_name'];
@@ -156,21 +168,47 @@ export class RbdSnapshotListComponent implements OnInit, OnChanges {
       return (
         ['rbd/snap/create', 'rbd/snap/delete', 'rbd/snap/edit', 'rbd/snap/rollback'].includes(
           task.name
-        ) && imageSpec.toString() === task.metadata['image_spec']
+        ) && this.imageSpec.toString() === task.metadata['image_spec']
       );
     };
 
     this.taskListService.init(
       () => of(this.snapshots),
       null,
-      (items) => (this.data = items),
-      () => (this.data = this.snapshots),
+      (items) => {
+        const hasChanges = CdHelperClass.updateChanged(this, {
+          data: items
+        });
+        if (hasChanges) {
+          this.cdr.detectChanges();
+          this.data = [...this.data];
+        }
+      },
+      () => {
+        const hasChanges = CdHelperClass.updateChanged(this, {
+          data: this.snapshots
+        });
+        if (hasChanges) {
+          this.cdr.detectChanges();
+          this.data = [...this.data];
+        }
+      },
       taskFilter,
       itemFilter,
       this.builders
     );
   }
 
+  ngOnChanges() {
+    if (this.columns) {
+      this.imageSpec = new ImageSpec(this.poolName, this.namespace, this.rbdName);
+      if (this.rbdTableActions) {
+        this.rbdTableActions.featuresName = this.featuresName;
+      }
+      this.taskListService.fetch();
+    }
+  }
+
   private openSnapshotModal(taskName: string, snapName: string = null) {
     this.modalRef = this.modalService.show(RbdSnapshotFormModalComponent);
     this.modalRef.componentInstance.poolName = this.poolName;
@@ -188,12 +226,10 @@ export class RbdSnapshotListComponent implements OnInit, OnChanges {
       const executingTask = new ExecutingTask();
       executingTask.name = taskName;
       executingTask.metadata = {
-        image_name: this.rbdName,
-        pool_name: this.poolName,
+        image_spec: this.imageSpec.toString(),
         snapshot_name: snapshotName
       };
       this.summaryService.addRunningTask(executingTask);
-      this.ngOnChanges();
     });
   }
 
@@ -223,7 +259,6 @@ export class RbdSnapshotListComponent implements OnInit, OnChanges {
         executingTask.name = finishedTask.name;
         executingTask.metadata = finishedTask.metadata;
         this.summaryService.addRunningTask(executingTask);
-        this.ngOnChanges();
         this.taskManagerService.subscribe(
           finishedTask.name,
           finishedTask.metadata,
@@ -250,7 +285,6 @@ export class RbdSnapshotListComponent implements OnInit, OnChanges {
         executingTask.metadata = finishedTask.metadata;
         this.summaryService.addRunningTask(executingTask);
         this.modalRef.close();
-        this.ngOnChanges();
         this.taskManagerService.subscribe(
           executingTask.name,
           executingTask.metadata,

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
index 7a31d34583a4557eeb02cf1586a404b672437b47..92e955ee4bd64636503404fd8a47a1a36a599d52 100644 (file)
--- 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
@@ -9,11 +9,16 @@ export class CdHelperClass {
    *                 it would update even if it equals
    */
   static updateChanged(componentThis: any, change: { [publicVarName: string]: any }) {
+    let hasChanges = false;
+
     Object.keys(change).forEach((publicVarName) => {
       const data = change[publicVarName];
       if (!_.isEqual(data, componentThis[publicVarName])) {
         componentThis[publicVarName] = data;
+        hasChanges = true;
       }
     });
+
+    return hasChanges;
   }
 }