]> git.apps.os.sepia.ceph.com Git - ceph.git/commitdiff
mgr/dashboard: ECP modal enhanced validation 34448/head
authorStephan Müller <smueller@suse.com>
Tue, 7 Apr 2020 14:08:40 +0000 (16:08 +0200)
committerStephan Müller <smueller@suse.com>
Tue, 28 Apr 2020 15:45:28 +0000 (17:45 +0200)
New validations with meaningful error messages:
* For all plugins
  * k + m <= OSDs that are available
  * k > 1
* For LRC
  * Display calculation factor for k (always)
  * Show error calculation for k is not met (also shows calculation)
  * Display number of groups for l (always)
  * Show error if calculation for l is not met (also shows calculation)
* For SHEC
  * m <= k
  * c <= m

Fixes: https://tracker.ceph.com/issues/44621
Signed-off-by: Stephan Müller <smueller@suse.com>
src/pybind/mgr/dashboard/frontend/src/app/ceph/pool/erasure-code-profile-form/erasure-code-profile-form-modal.component.html
src/pybind/mgr/dashboard/frontend/src/app/ceph/pool/erasure-code-profile-form/erasure-code-profile-form-modal.component.spec.ts
src/pybind/mgr/dashboard/frontend/src/app/ceph/pool/erasure-code-profile-form/erasure-code-profile-form-modal.component.ts

index 89b383e898781ec40e65e537b6b2b92a31364862..cbdeb36b1b4a475595fdd09c917d20f8d79f93c3 100644 (file)
@@ -61,7 +61,7 @@
         <div class="form-group row">
           <label for="k"
                  class="cd-col-form-label">
-            <span [ngClass]="{'required': requiredControls.includes('k')}"
+            <span class="required"
                   i18n>Data chunks (k)</span>
             <cd-helper [html]="tooltips.k">
             </cd-helper>
             <span class="invalid-feedback"
                   *ngIf="form.showError('k', frm, 'min')"
                   i18n>Must be equal to or greater than 2.</span>
+            <span class="invalid-feedback"
+                  *ngIf="form.showError('k', frm, 'max')"
+                  i18n>Chunks (k+m) have exceeded the available OSDs of {{deviceCount}}.</span>
+            <span class="invalid-feedback"
+                  *ngIf="form.showError('k', frm, 'unequal')"
+                  i18n>For an equal distribution k has to be a multiple of (k+m)/l.</span>
+            <span class="invalid-feedback"
+                  *ngIf="form.showError('k', frm, 'kLowerM')"
+                  i18n>K has to be equal to or greater than m in order to recover data correctly through c.</span>
+            <span *ngIf="plugin === 'lrc'"
+                  class="form-text text-muted"
+                  i18n>Distribution factor: {{lrcMultiK}}</span>
           </div>
         </div>
 
         <div class="form-group row">
           <label for="m"
                  class="cd-col-form-label">
-            <span [ngClass]="{'required': requiredControls.includes('m')}"
+            <span class="required"
                   i18n>Coding chunks (m)</span>
             <cd-helper [html]="tooltips.m">
             </cd-helper>
             <span class="invalid-feedback"
                   *ngIf="form.showError('m', frm, 'min')"
                   i18n>Must be equal to or greater than 1.</span>
+            <span class="invalid-feedback"
+                  *ngIf="form.showError('m', frm, 'max')"
+                  i18n>Chunks (k+m) have exceeded the available OSDs of {{deviceCount}}.</span>
           </div>
         </div>
 
              *ngIf="plugin === 'shec'">
           <label for="c"
                  class="cd-col-form-label">
-            <ng-container i18n>Durability estimator (c)</ng-container>
+            <span class="required"
+                  i18n>Durability estimator (c)</span>
             <cd-helper [html]="tooltips.plugins.shec.c">
             </cd-helper>
           </label>
             <span class="invalid-feedback"
                   *ngIf="form.showError('c', frm, 'min')"
                   i18n>Must be equal to or greater than 1.</span>
+            <span class="invalid-feedback"
+                  *ngIf="form.showError('c', frm, 'cGreaterM')"
+                  i18n>C has to be equal to or lower than m as m defines the amount of chunks that can be used.</span>
           </div>
         </div>
 
             <span class="invalid-feedback"
                   *ngIf="form.showError('l', frm, 'min')"
                   i18n>Must be equal to or greater than 1.</span>
+            <span class="invalid-feedback"
+                  *ngIf="form.showError('l', frm, 'unequal')"
+                  i18n>Can't split up chunks (k+m) correctly with the current locality.</span>
+            <span class="form-text text-muted"
+                  i18n>Locality groups: {{lrcGroups}}</span>
           </div>
         </div>
 
index 1886ebd730fea5cf3e967fc42a1925aad7196a7a..2628f1f69a42899de41031afac3256fbd7d7a52d 100644 (file)
@@ -128,7 +128,7 @@ describe('ErasureCodeProfileFormModalComponent', () => {
     });
 
     it('sets k to min error', () => {
-      formHelper.expectErrorChange('k', 0, 'min');
+      formHelper.expectErrorChange('k', 1, 'min');
     });
 
     it('sets m to min error', () => {
@@ -171,6 +171,14 @@ describe('ErasureCodeProfileFormModalComponent', () => {
       it(`should not show any other plugin specific form control`, () => {
         fixtureHelper.expectIdElementsVisible(['c', 'l', 'crushLocality'], false);
       });
+
+      it('should not allow "k" to be changed more than possible', () => {
+        formHelper.expectErrorChange('k', 10, 'max');
+      });
+
+      it('should not allow "m" to be changed more than possible', () => {
+        formHelper.expectErrorChange('m', 10, 'max');
+      });
     });
 
     describe(`for 'isa' plugin`, () => {
@@ -178,10 +186,9 @@ describe('ErasureCodeProfileFormModalComponent', () => {
         formHelper.setValue('plugin', 'isa');
       });
 
-      it(`does not require 'm' and 'k'`, () => {
-        formHelper.setValue('k', null);
-        formHelper.expectValidChange('k', null);
-        formHelper.expectValidChange('m', null);
+      it(`does require 'm' and 'k'`, () => {
+        formHelper.expectErrorChange('k', null, 'required');
+        formHelper.expectErrorChange('m', null, 'required');
       });
 
       it(`should show 'technique'`, () => {
@@ -192,16 +199,28 @@ describe('ErasureCodeProfileFormModalComponent', () => {
       it(`should not show any other plugin specific form control`, () => {
         fixtureHelper.expectIdElementsVisible(['c', 'l', 'crushLocality', 'packetSize'], false);
       });
+
+      it('should not allow "k" to be changed more than possible', () => {
+        formHelper.expectErrorChange('k', 10, 'max');
+      });
+
+      it('should not allow "m" to be changed more than possible', () => {
+        formHelper.expectErrorChange('m', 10, 'max');
+      });
     });
 
     describe(`for 'lrc' plugin`, () => {
       beforeEach(() => {
         formHelper.setValue('plugin', 'lrc');
+        formHelper.expectValid('k');
+        formHelper.expectValid('l');
+        formHelper.expectValid('m');
       });
 
       it(`requires 'm', 'l' and 'k'`, () => {
         formHelper.expectErrorChange('k', null, 'required');
         formHelper.expectErrorChange('m', null, 'required');
+        formHelper.expectErrorChange('l', null, 'required');
       });
 
       it(`should show 'l' and 'crushLocality'`, () => {
@@ -211,16 +230,118 @@ describe('ErasureCodeProfileFormModalComponent', () => {
       it(`should not show any other plugin specific form control`, () => {
         fixtureHelper.expectIdElementsVisible(['c', 'packetSize', 'technique'], false);
       });
+
+      it('should not allow "k" to be changed more than possible', () => {
+        formHelper.expectErrorChange('k', 10, 'max');
+      });
+
+      it('should not allow "m" to be changed more than possible', () => {
+        formHelper.expectErrorChange('m', 10, 'max');
+      });
+
+      it('should not allow "l" to be changed so that (k+m) is not a multiple of "l"', () => {
+        formHelper.expectErrorChange('l', 4, 'unequal');
+      });
+
+      it('should update validity of k and l on m change', () => {
+        formHelper.expectValidChange('m', 3);
+        formHelper.expectError('k', 'unequal');
+        formHelper.expectError('l', 'unequal');
+      });
+
+      describe('lrc calculation', () => {
+        const expectCorrectCalculation = (
+          k: number,
+          m: number,
+          l: number,
+          failedControl: string[] = []
+        ) => {
+          formHelper.setValue('k', k);
+          formHelper.setValue('m', m);
+          formHelper.setValue('l', l);
+          ['k', 'l'].forEach((name) => {
+            if (failedControl.includes(name)) {
+              formHelper.expectError(name, 'unequal');
+            } else {
+              formHelper.expectValid(name);
+            }
+          });
+        };
+
+        const tests = {
+          kFails: [
+            [2, 1, 1],
+            [2, 2, 1],
+            [3, 1, 1],
+            [3, 2, 1],
+            [3, 1, 2],
+            [3, 3, 1],
+            [3, 3, 3],
+            [4, 1, 1],
+            [4, 2, 1],
+            [4, 2, 2],
+            [4, 3, 1],
+            [4, 4, 1]
+          ],
+          lFails: [
+            [2, 1, 2],
+            [3, 2, 2],
+            [3, 1, 3],
+            [3, 2, 3],
+            [4, 1, 2],
+            [4, 3, 2],
+            [4, 3, 3],
+            [4, 1, 3],
+            [4, 4, 3],
+            [4, 1, 4],
+            [4, 2, 4],
+            [4, 3, 4]
+          ],
+          success: [
+            [2, 2, 2],
+            [2, 2, 4],
+            [3, 3, 2],
+            [3, 3, 6],
+            [4, 2, 3],
+            [4, 2, 6],
+            [4, 4, 2],
+            [4, 4, 8],
+            [4, 4, 4]
+          ]
+        };
+
+        it('tests all cases where k fails', () => {
+          tests.kFails.forEach((testCase) => {
+            expectCorrectCalculation(testCase[0], testCase[1], testCase[2], ['k']);
+          });
+        });
+
+        it('tests all cases where l fails', () => {
+          tests.lFails.forEach((testCase) => {
+            expectCorrectCalculation(testCase[0], testCase[1], testCase[2], ['k', 'l']);
+          });
+        });
+
+        it('tests all cases where everything is valid', () => {
+          tests.success.forEach((testCase) => {
+            expectCorrectCalculation(testCase[0], testCase[1], testCase[2]);
+          });
+        });
+      });
     });
 
     describe(`for 'shec' plugin`, () => {
       beforeEach(() => {
         formHelper.setValue('plugin', 'shec');
+        formHelper.expectValid('c');
+        formHelper.expectValid('m');
+        formHelper.expectValid('k');
       });
 
-      it(`does not require 'm' and 'k'`, () => {
-        formHelper.expectValidChange('k', null);
-        formHelper.expectValidChange('m', null);
+      it(`does require 'm', 'c' and 'k'`, () => {
+        formHelper.expectErrorChange('k', null, 'required');
+        formHelper.expectErrorChange('m', null, 'required');
+        formHelper.expectErrorChange('c', null, 'required');
       });
 
       it(`should show 'c'`, () => {
@@ -233,6 +354,26 @@ describe('ErasureCodeProfileFormModalComponent', () => {
           false
         );
       });
+
+      it('should make sure that k has to be equal or greater than m', () => {
+        formHelper.expectValidChange('k', 3);
+        formHelper.expectErrorChange('k', 2, 'kLowerM');
+      });
+
+      it('should make sure that c has to be equal or less than m', () => {
+        formHelper.expectValidChange('c', 3);
+        formHelper.expectErrorChange('c', 4, 'cGreaterM');
+      });
+
+      it('should update validity of k and c on m change', () => {
+        formHelper.expectValidChange('m', 5);
+        formHelper.expectError('k', 'kLowerM');
+        formHelper.expectValid('c');
+
+        formHelper.expectValidChange('m', 1);
+        formHelper.expectError('c', 'cGreaterM');
+        formHelper.expectValid('k');
+      });
     });
   });
 
index aed3493fc4d091aad343f6f4246c5f7c910866bb..7f5aa7aed763c6045a33350ea43d828c88f0f791 100644 (file)
@@ -40,6 +40,8 @@ export class ErasureCodeProfileFormModalComponent extends CrushNodeSelectionClas
   techniques: string[];
   action: string;
   resource: string;
+  lrcGroups: number;
+  lrcMultiK: number;
 
   constructor(
     private formBuilder: CdFormBuilder,
@@ -70,26 +72,115 @@ export class ErasureCodeProfileFormModalComponent extends CrushNodeSelectionClas
         ]
       ],
       plugin: [this.PLUGIN.JERASURE, [Validators.required]],
-      k: [1], // Will be replaced by plugin defaults
-      m: [1], // Will be replaced by plugin defaults
-      crushFailureDomain: ['host'],
-      crushRoot: ['default'], // default for all - is a list possible???
-      crushDeviceClass: [''], // set none to empty at submit - get list from configs?
-      directory: [''],
+      k: [
+        4, // Will be overwritten with plugin defaults
+        [
+          Validators.required,
+          Validators.min(2),
+          CdValidators.custom('max', () => this.baseValueValidation(true)),
+          CdValidators.custom('unequal', (v: number) => this.lrcDataValidation(v)),
+          CdValidators.custom('kLowerM', (v: number) => this.shecDataValidation(v))
+        ]
+      ],
+      m: [
+        2, // Will be overwritten with plugin defaults
+        [
+          Validators.required,
+          Validators.min(1),
+          CdValidators.custom('max', () => this.baseValueValidation())
+        ]
+      ],
+      crushFailureDomain: '', // Will be preselected
+      crushRoot: null, // Will be preselected
+      crushDeviceClass: '', // Will be preselected
+      directory: '',
       // Only for 'jerasure' and 'isa' use
-      technique: ['reed_sol_van'],
+      technique: 'reed_sol_van',
       // Only for 'jerasure' use
       packetSize: [2048, [Validators.min(1)]],
       // Only for 'lrc' use
-      l: [1, [Validators.required, Validators.min(1)]],
-      crushLocality: [''], // set to none at the end (same list as for failure domains)
+      l: [
+        3, // Will be overwritten with plugin defaults
+        [
+          Validators.required,
+          Validators.min(1),
+          CdValidators.custom('unequal', (v: number) => this.lrcLocalityValidation(v))
+        ]
+      ],
+      crushLocality: '', // set to none at the end (same list as for failure domains)
       // Only for 'shec' use
-      c: [1, [Validators.required, Validators.min(1)]]
+      c: [
+        2, // Will be overwritten with plugin defaults
+        [
+          Validators.required,
+          Validators.min(1),
+          CdValidators.custom('cGreaterM', (v: number) => this.shecDurabilityValidation(v))
+        ]
+      ]
     });
+    this.form.get('k').valueChanges.subscribe(() => this.updateValidityOnChange(['m', 'l']));
+    this.form.get('m').valueChanges.subscribe(() => this.updateValidityOnChange(['k', 'l', 'c']));
+    this.form.get('l').valueChanges.subscribe(() => this.updateValidityOnChange(['k', 'm']));
     this.form.get('plugin').valueChanges.subscribe((plugin) => this.onPluginChange(plugin));
   }
 
-  onPluginChange(plugin: string) {
+  private baseValueValidation(dataChunk: boolean = false): boolean {
+    return this.validValidation(() => {
+      return (
+        this.getKMSum() > this.deviceCount &&
+        this.form.getValue('k') > this.form.getValue('m') === dataChunk
+      );
+    });
+  }
+
+  private validValidation(fn: () => boolean, plugin?: string): boolean {
+    if (!this.form || plugin ? this.plugin !== plugin : false) {
+      return false;
+    }
+    return fn();
+  }
+
+  private getKMSum(): number {
+    return this.form.getValue('k') + this.form.getValue('m');
+  }
+
+  private lrcDataValidation(k: number): boolean {
+    return this.validValidation(() => {
+      const m = this.form.getValue('m');
+      const l = this.form.getValue('l');
+      const km = k + m;
+      this.lrcMultiK = k / (km / l);
+      return k % (km / l) !== 0;
+    }, 'lrc');
+  }
+
+  private shecDataValidation(k: number): boolean {
+    return this.validValidation(() => {
+      const m = this.form.getValue('m');
+      return m > k;
+    }, 'shec');
+  }
+
+  private lrcLocalityValidation(l: number) {
+    return this.validValidation(() => {
+      const value = this.getKMSum();
+      this.lrcGroups = l > 0 ? value / l : 0;
+      return l > 0 && value % l !== 0;
+    }, 'lrc');
+  }
+
+  private shecDurabilityValidation(c: number): boolean {
+    return this.validValidation(() => {
+      const m = this.form.getValue('m');
+      return c > m;
+    }, 'shec');
+  }
+
+  private updateValidityOnChange(names: string[]) {
+    names.forEach((name) => this.form.get(name).updateValueAndValidity({ emitEvent: false }));
+  }
+
+  private onPluginChange(plugin: string) {
     this.plugin = plugin;
     if (plugin === this.PLUGIN.JERASURE) {
       this.setJerasureDefaults();
@@ -100,27 +191,14 @@ export class ErasureCodeProfileFormModalComponent extends CrushNodeSelectionClas
     } else if (plugin === this.PLUGIN.SHEC) {
       this.setShecDefaults();
     }
-  }
-
-  private setNumberValidators(name: string, required: boolean) {
-    const validators = [Validators.min(1)];
-    if (required) {
-      validators.push(Validators.required);
-    }
-    this.form.get(name).setValidators(validators);
-  }
-
-  private setKMValidators(required: boolean) {
-    ['k', 'm'].forEach((name) => this.setNumberValidators(name, required));
+    this.updateValidityOnChange(['m']); // Triggers k, m, c and l
   }
 
   private setJerasureDefaults() {
-    this.requiredControls = ['k', 'm'];
     this.setDefaults({
       k: 4,
       m: 2
     });
-    this.setKMValidators(true);
     this.techniques = [
       'reed_sol_van',
       'reed_sol_r6_op',
@@ -133,9 +211,6 @@ export class ErasureCodeProfileFormModalComponent extends CrushNodeSelectionClas
   }
 
   private setLrcDefaults() {
-    this.requiredControls = ['k', 'm', 'l'];
-    this.setKMValidators(true);
-    this.setNumberValidators('l', true);
     this.setDefaults({
       k: 4,
       m: 2,
@@ -144,8 +219,11 @@ export class ErasureCodeProfileFormModalComponent extends CrushNodeSelectionClas
   }
 
   private setIsaDefaults() {
-    this.requiredControls = [];
-    this.setKMValidators(false);
+    /**
+     * Actually k and m are not required - but they will be set to the default values in case
+     * if they are not set, therefore it's fine to mark them as required in order to get
+     * strange values that weren't set.
+     */
     this.setDefaults({
       k: 7,
       m: 3
@@ -154,8 +232,11 @@ export class ErasureCodeProfileFormModalComponent extends CrushNodeSelectionClas
   }
 
   private setShecDefaults() {
-    this.requiredControls = [];
-    this.setKMValidators(false);
+    /**
+     * Actually k, c and m are not required - but they will be set to the default values in case
+     * if they are not set, therefore it's fine to mark them as required in order to get
+     * strange values that weren't set.
+     */
     this.setDefaults({
       k: 4,
       m: 3,
@@ -165,8 +246,22 @@ export class ErasureCodeProfileFormModalComponent extends CrushNodeSelectionClas
 
   private setDefaults(defaults: object) {
     Object.keys(defaults).forEach((controlName) => {
-      if (this.form.get(controlName).pristine) {
-        this.form.silentSet(controlName, defaults[controlName]);
+      const control = this.form.get(controlName);
+      const value = control.value;
+      let overwrite = control.pristine;
+      /**
+       * As k, m, c and l are now set touched and dirty on the beginning, plugin change will
+       * overwrite their values as we can't determine if the user has changed anything.
+       * k and m can have two default values where as l and c can only have one,
+       * so there is no need to overwrite them.
+       */
+      if ('k' === controlName) {
+        overwrite = [4, 7].includes(value);
+      } else if ('m' === controlName) {
+        overwrite = [2, 3].includes(value);
+      }
+      if (overwrite) {
+        this.form.get(controlName).setValue(defaults[controlName]);
       }
     });
   }
@@ -195,10 +290,24 @@ export class ErasureCodeProfileFormModalComponent extends CrushNodeSelectionClas
           this.plugins = plugins;
           this.names = names;
           this.form.silentSet('directory', directory);
+          this.preValidateNumericInputFields();
         }
       );
   }
 
+  /**
+   * This allows k, m, l and c to be validated instantly on change, before the
+   * fields got changed before by the user.
+   */
+  private preValidateNumericInputFields() {
+    const kml = ['k', 'm', 'l', 'c'].map((name) => this.form.get(name));
+    kml.forEach((control) => {
+      control.markAsTouched();
+      control.markAsDirty();
+    });
+    kml[1].updateValueAndValidity(); // Update validity of k, m, c and l
+  }
+
   onSubmit() {
     if (this.form.invalid) {
       this.form.setErrors({ cdSubmitButton: true });