From 55684eea6a04315ecb651630ee1a8139594fda96 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Stephan=20M=C3=BCller?= Date: Tue, 7 Apr 2020 16:08:40 +0200 Subject: [PATCH] mgr/dashboard: ECP modal enhanced validation MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit 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 (cherry picked from commit 5da7c0259735c56a722f7671e6c16d6a1746a5ba) --- ...ure-code-profile-form-modal.component.html | 30 ++- ...-code-profile-form-modal.component.spec.ts | 157 +++++++++++++++- ...asure-code-profile-form-modal.component.ts | 177 ++++++++++++++---- 3 files changed, 319 insertions(+), 45 deletions(-) diff --git a/src/pybind/mgr/dashboard/frontend/src/app/ceph/pool/erasure-code-profile-form/erasure-code-profile-form-modal.component.html b/src/pybind/mgr/dashboard/frontend/src/app/ceph/pool/erasure-code-profile-form/erasure-code-profile-form-modal.component.html index 89b383e898781..cbdeb36b1b4a4 100644 --- a/src/pybind/mgr/dashboard/frontend/src/app/ceph/pool/erasure-code-profile-form/erasure-code-profile-form-modal.component.html +++ b/src/pybind/mgr/dashboard/frontend/src/app/ceph/pool/erasure-code-profile-form/erasure-code-profile-form-modal.component.html @@ -61,7 +61,7 @@
@@ -111,7 +126,8 @@ *ngIf="plugin === 'shec'"> @@ -125,6 +141,9 @@ Must be equal to or greater than 1. + C has to be equal to or lower than m as m defines the amount of chunks that can be used. @@ -150,6 +169,11 @@ Must be equal to or greater than 1. + Can't split up chunks (k+m) correctly with the current locality. + Locality groups: {{lrcGroups}} diff --git a/src/pybind/mgr/dashboard/frontend/src/app/ceph/pool/erasure-code-profile-form/erasure-code-profile-form-modal.component.spec.ts b/src/pybind/mgr/dashboard/frontend/src/app/ceph/pool/erasure-code-profile-form/erasure-code-profile-form-modal.component.spec.ts index 1886ebd730fea..2628f1f69a428 100644 --- a/src/pybind/mgr/dashboard/frontend/src/app/ceph/pool/erasure-code-profile-form/erasure-code-profile-form-modal.component.spec.ts +++ b/src/pybind/mgr/dashboard/frontend/src/app/ceph/pool/erasure-code-profile-form/erasure-code-profile-form-modal.component.spec.ts @@ -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'); + }); }); }); diff --git a/src/pybind/mgr/dashboard/frontend/src/app/ceph/pool/erasure-code-profile-form/erasure-code-profile-form-modal.component.ts b/src/pybind/mgr/dashboard/frontend/src/app/ceph/pool/erasure-code-profile-form/erasure-code-profile-form-modal.component.ts index aed3493fc4d09..7f5aa7aed763c 100644 --- a/src/pybind/mgr/dashboard/frontend/src/app/ceph/pool/erasure-code-profile-form/erasure-code-profile-form-modal.component.ts +++ b/src/pybind/mgr/dashboard/frontend/src/app/ceph/pool/erasure-code-profile-form/erasure-code-profile-form-modal.component.ts @@ -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 }); -- 2.39.5