From 9adb2516685671b730f620a83ae503bc5ae9bef4 Mon Sep 17 00:00:00 2001
From: =?utf8?q?Stephan=20M=C3=BCller?= <smueller@suse.com>
Date: Wed, 18 Mar 2020 16:29:43 +0100
Subject: [PATCH] mgr/dashboard: Use of crush node class in crush rule modal
MIME-Version: 1.0
Content-Type: text/plain; charset=utf8
Content-Transfer-Encoding: 8bit

Fixes: https://tracker.ceph.com/issues/44621
Signed-off-by: Stephan Müller <smueller@suse.com>
---
 .../crush-rule-form-modal.component.html      |  18 +--
 .../crush-rule-form-modal.component.spec.ts   |   4 +-
 .../crush-rule-form-modal.component.ts        | 107 ++----------------
 .../pool-form/pool-form.component.spec.ts     |  37 ++++--
 4 files changed, 46 insertions(+), 120 deletions(-)

diff --git a/src/pybind/mgr/dashboard/frontend/src/app/ceph/pool/crush-rule-form-modal/crush-rule-form-modal.component.html b/src/pybind/mgr/dashboard/frontend/src/app/ceph/pool/crush-rule-form-modal/crush-rule-form-modal.component.html
index c2e99387ffe1f..073b6e40f0af9 100644
--- a/src/pybind/mgr/dashboard/frontend/src/app/ceph/pool/crush-rule-form-modal/crush-rule-form-modal.component.html
+++ b/src/pybind/mgr/dashboard/frontend/src/app/ceph/pool/crush-rule-form-modal/crush-rule-form-modal.component.html
@@ -9,11 +9,11 @@
       <div class="modal-body">
         <div class="form-group row">
           <label for="name"
-                 class="col-form-label col-sm-3">
+                 class="cd-col-form-label">
             <ng-container i18n>Name</ng-container>
             <span class="required"></span>
           </label>
-          <div class="col-sm-9">
+          <div class="cd-col-form-input">
             <input type="text"
                    id="name"
                    name="name"
@@ -36,13 +36,13 @@
         <!-- Root -->
         <div class="form-group row">
           <label for="root"
-                 class="col-form-label col-sm-3">
+                 class="cd-col-form-label">
             <ng-container i18n>Root</ng-container>
             <cd-helper [html]="tooltips.root">
             </cd-helper>
             <span class="required"></span>
           </label>
-          <div class="col-sm-9">
+          <div class="cd-col-form-input">
             <select class="form-control custom-select"
                     id="root"
                     name="root"
@@ -64,13 +64,13 @@
         <!-- Failure Domain Type -->
         <div class="form-group row">
           <label for="failure_domain"
-                 class="col-form-label col-sm-3">
+                 class="cd-col-form-label">
             <ng-container i18n>Failure domain type</ng-container>
             <cd-helper [html]="tooltips.failure_domain">
             </cd-helper>
             <span class="required"></span>
           </label>
-          <div class="col-sm-9">
+          <div class="cd-col-form-input">
             <select class="form-control custom-select"
                     id="failure_domain"
                     name="failure_domain"
@@ -78,7 +78,7 @@
               <option *ngIf="!failureDomains"
                       ngValue=""
                       i18n>Loading...</option>
-              <option *ngFor="let domain of failureDomainKeys()"
+              <option *ngFor="let domain of failureDomainKeys"
                       [ngValue]="domain">
                 {{ domain }} ( {{failureDomains[domain].length}} )
               </option>
@@ -92,12 +92,12 @@
         <!-- Class -->
         <div class="form-group row">
           <label for="device_class"
-                 class="col-form-label col-sm-3">
+                 class="cd-col-form-label">
             <ng-container i18n>Device class</ng-container>
             <cd-helper [html]="tooltips.device_class">
             </cd-helper>
           </label>
-          <div class="col-sm-9">
+          <div class="cd-col-form-input">
             <select class="form-control custom-select"
                     id="device_class"
                     name="device_class"
diff --git a/src/pybind/mgr/dashboard/frontend/src/app/ceph/pool/crush-rule-form-modal/crush-rule-form-modal.component.spec.ts b/src/pybind/mgr/dashboard/frontend/src/app/ceph/pool/crush-rule-form-modal/crush-rule-form-modal.component.spec.ts
index 84d0d266e2735..003ec29bef7b2 100644
--- a/src/pybind/mgr/dashboard/frontend/src/app/ceph/pool/crush-rule-form-modal/crush-rule-form-modal.component.spec.ts
+++ b/src/pybind/mgr/dashboard/frontend/src/app/ceph/pool/crush-rule-form-modal/crush-rule-form-modal.component.spec.ts
@@ -64,7 +64,7 @@ describe('CrushRuleFormComponent', () => {
     failureDomains: (nodes: CrushNode[], types: string[]) => {
       const expectation = {};
       types.forEach((type) => (expectation[type] = nodes.filter((node) => node.type === type)));
-      const keys = component.failureDomainKeys();
+      const keys = component.failureDomainKeys;
       expect(keys).toEqual(types);
       keys.forEach((key) => {
         expect(component.failureDomains[key].length).toBe(expectation[key].length);
@@ -156,7 +156,7 @@ describe('CrushRuleFormComponent', () => {
   it('calls listing to get rules on ngInit', () => {
     expect(crushRuleService.getInfo).toHaveBeenCalled();
     expect(component.names.length).toBe(2);
-    expect(component['nodes'].length).toBe(12);
+    expect(component.buckets.length).toBe(5);
   });
 
   describe('lists', () => {
diff --git a/src/pybind/mgr/dashboard/frontend/src/app/ceph/pool/crush-rule-form-modal/crush-rule-form-modal.component.ts b/src/pybind/mgr/dashboard/frontend/src/app/ceph/pool/crush-rule-form-modal/crush-rule-form-modal.component.ts
index 27f30fbedfa22..3e9edb699e364 100644
--- a/src/pybind/mgr/dashboard/frontend/src/app/ceph/pool/crush-rule-form-modal/crush-rule-form-modal.component.ts
+++ b/src/pybind/mgr/dashboard/frontend/src/app/ceph/pool/crush-rule-form-modal/crush-rule-form-modal.component.ts
@@ -6,6 +6,7 @@ import * as _ from 'lodash';
 import { BsModalRef } from 'ngx-bootstrap/modal';
 
 import { CrushRuleService } from '../../../shared/api/crush-rule.service';
+import { CrushNodeSelectionClass } from '../../../shared/classes/crush.node.selection.class';
 import { ActionLabelsI18n } from '../../../shared/constants/app.constants';
 import { CdFormBuilder } from '../../../shared/forms/cd-form-builder';
 import { CdFormGroup } from '../../../shared/forms/cd-form-group';
@@ -19,13 +20,10 @@ import { TaskWrapperService } from '../../../shared/services/task-wrapper.servic
   templateUrl: './crush-rule-form-modal.component.html',
   styleUrls: ['./crush-rule-form-modal.component.scss']
 })
-export class CrushRuleFormModalComponent implements OnInit {
+export class CrushRuleFormModalComponent extends CrushNodeSelectionClass implements OnInit {
   @Output()
   submitAction = new EventEmitter();
 
-  buckets: CrushNode[] = [];
-  failureDomains: { [type: string]: CrushNode[] } = {};
-  devices: string[] = [];
   tooltips = this.crushRuleService.formTooltips;
 
   form: CdFormGroup;
@@ -33,9 +31,6 @@ export class CrushRuleFormModalComponent implements OnInit {
   action: string;
   resource: string;
 
-  private nodes: CrushNode[] = [];
-  private easyNodes: { [id: number]: CrushNode } = {};
-
   constructor(
     private formBuilder: CdFormBuilder,
     public bsModalRef: BsModalRef,
@@ -44,6 +39,7 @@ export class CrushRuleFormModalComponent implements OnInit {
     private i18n: I18n,
     public actionLabels: ActionLabelsI18n
   ) {
+    super();
     this.action = this.actionLabels.CREATE;
     this.resource = this.i18n('Crush Rule');
     this.createForm();
@@ -76,101 +72,14 @@ export class CrushRuleFormModalComponent implements OnInit {
     this.crushRuleService
       .getInfo()
       .subscribe(({ names, nodes }: { names: string[]; nodes: CrushNode[] }) => {
-        this.nodes = nodes;
-        nodes.forEach((node) => {
-          this.easyNodes[node.id] = node;
-        });
-        this.buckets = _.sortBy(
-          nodes.filter((n) => n.children),
-          'name'
+        this.initCrushNodeSelection(
+          nodes,
+          this.form.get('root'),
+          this.form.get('failure_domain'),
+          this.form.get('device_class')
         );
         this.names = names;
-        this.preSelectRoot();
       });
-    this.form.get('root').valueChanges.subscribe((root: CrushNode) => this.updateRoot(root));
-    this.form
-      .get('failure_domain')
-      .valueChanges.subscribe((domain: string) => this.updateDevices(domain));
-  }
-
-  private preSelectRoot() {
-    const rootNode = this.nodes.find((node) => node.type === 'root');
-    this.form.silentSet('root', rootNode);
-    this.updateRoot(rootNode);
-  }
-
-  private updateRoot(rootNode: CrushNode) {
-    const nodes = this.getSubNodes(rootNode);
-    const domains = {};
-    nodes.forEach((node) => {
-      if (!domains[node.type]) {
-        domains[node.type] = [];
-      }
-      domains[node.type].push(node);
-    });
-    Object.keys(domains).forEach((type) => {
-      if (domains[type].length <= 1) {
-        delete domains[type];
-      }
-    });
-    this.failureDomains = domains;
-    this.updateFailureDomain();
-  }
-
-  private getSubNodes(node: CrushNode): CrushNode[] {
-    let subNodes = [node]; // Includes parent node
-    if (!node.children) {
-      return subNodes;
-    }
-    node.children.forEach((id) => {
-      const childNode = this.easyNodes[id];
-      subNodes = subNodes.concat(this.getSubNodes(childNode));
-    });
-    return subNodes;
-  }
-
-  private updateFailureDomain() {
-    let failureDomain = this.getIncludedCustomValue(
-      'failure_domain',
-      Object.keys(this.failureDomains)
-    );
-    if (failureDomain === '') {
-      failureDomain = this.setMostCommonDomain();
-    }
-    this.updateDevices(failureDomain);
-  }
-
-  private getIncludedCustomValue(controlName: string, includedIn: string[]) {
-    const control = this.form.get(controlName);
-    return control.dirty && includedIn.includes(control.value) ? control.value : '';
-  }
-
-  private setMostCommonDomain(): string {
-    let winner = { n: 0, type: '' };
-    Object.keys(this.failureDomains).forEach((type) => {
-      const n = this.failureDomains[type].length;
-      if (winner.n < n) {
-        winner = { n, type };
-      }
-    });
-    this.form.silentSet('failure_domain', winner.type);
-    return winner.type;
-  }
-
-  updateDevices(failureDomain: string) {
-    const subNodes = _.flatten(
-      this.failureDomains[failureDomain].map((node) => this.getSubNodes(node))
-    );
-    this.devices = _.uniq(subNodes.filter((n) => n.device_class).map((n) => n.device_class)).sort();
-    const device =
-      this.devices.length === 1
-        ? this.devices[0]
-        : this.getIncludedCustomValue('device_class', this.devices);
-    this.form.get('device_class').setValue(device);
-  }
-
-  failureDomainKeys(): string[] {
-    return Object.keys(this.failureDomains).sort();
   }
 
   onSubmit() {
diff --git a/src/pybind/mgr/dashboard/frontend/src/app/ceph/pool/pool-form/pool-form.component.spec.ts b/src/pybind/mgr/dashboard/frontend/src/app/ceph/pool/pool-form/pool-form.component.spec.ts
index 85c0fe6cd19c7..5f06047c1a46b 100644
--- a/src/pybind/mgr/dashboard/frontend/src/app/ceph/pool/pool-form/pool-form.component.spec.ts
+++ b/src/pybind/mgr/dashboard/frontend/src/app/ceph/pool/pool-form/pool-form.component.spec.ts
@@ -799,6 +799,21 @@ describe('PoolFormComponent', () => {
       fixture.detectChanges();
     });
 
+    it('should select the newly created rule', () => {
+      expect(form.getValue('crushRule').rule_name).toBe('rep1');
+      const name = 'awesomeRule';
+      spyOn(TestBed.get(BsModalService), 'show').and.callFake(() => {
+        return {
+          content: {
+            submitAction: of({ name })
+          }
+        };
+      });
+      infoReturn.crush_rules_replicated.push(createCrushRule({ id: 8, name }));
+      component.addCrushRule();
+      expect(form.getValue('crushRule').rule_name).toBe(name);
+    });
+
     it('should not show info per default', () => {
       fixtureHelper.expectElementVisible('#crushRule', true);
       fixtureHelper.expectElementVisible('#crush-info-block', false);
@@ -837,30 +852,32 @@ describe('PoolFormComponent', () => {
 
       const expectSuccessfulDeletion = (name: string) => {
         expect(crushRuleService.delete).toHaveBeenCalledWith(name);
-        expect(taskWrapper.wrapTaskAroundCall).toHaveBeenCalledWith({
-          task: {
-            name: 'crushRule/delete',
-            metadata: {
-              name: name
+        expect(taskWrapper.wrapTaskAroundCall).toHaveBeenCalledWith(
+          expect.objectContaining({
+            task: {
+              name: 'crushRule/delete',
+              metadata: {
+                name: name
+              }
             }
-          },
-          call: undefined // because of stub
-        });
+          })
+        );
       };
 
       beforeEach(() => {
         modalSpy = spyOn(TestBed.get(BsModalService), 'show').and.callFake(
-          (deletionClass, config) => {
+          (deletionClass: any, config: any) => {
             deletion = Object.assign(new deletionClass(), config.initialState);
             return {
               content: deletion
             };
           }
         );
-        deleteSpy = spyOn(crushRuleService, 'delete').and.callFake((name) => {
+        deleteSpy = spyOn(crushRuleService, 'delete').and.callFake((name: string) => {
           const rules = infoReturn.crush_rules_replicated;
           const index = _.findIndex(rules, (rule) => rule.rule_name === name);
           rules.splice(index, 1);
+          return of(undefined);
         });
         taskWrapper = TestBed.get(TaskWrapperService);
         spyOn(taskWrapper, 'wrapTaskAroundCall').and.callThrough();
-- 
2.39.5