From: Nizamudeen A Date: Sun, 25 Apr 2021 08:47:07 +0000 (+0530) Subject: mgr/dashboard: RGW buckets async validator performance enhancement X-Git-Tag: v15.2.14~48^2~1 X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=96786592ec719c453a01b663aa1f2dbd59f97fe9;p=ceph.git mgr/dashboard: RGW buckets async validator performance enhancement The rgw bucket creation form has the Name field which have an async validator. The validator calls all the bucket name and check if the entered name is unique or not. This happens on every keystroke. So if 100 or more buckets are there, then the async validation can be real slow and causes misvalidations in different fields. I changed the validation logic and did some cleanups to improve the performance of the async validation. Fixes: https://tracker.ceph.com/issues/50514 Signed-off-by: Nizamudeen A (cherry picked from commit 005327c4e12fef8d7054894d9df021c0b3c53e19) Conflicts: src/pybind/mgr/dashboard/frontend/src/app/ceph/rgw/rgw-bucket-form/rgw-bucket-form.component.ts - Solved some import conflicts. Used the I18N import and removed the forkJoin import src/pybind/mgr/dashboard/frontend/src/app/shared/api/rgw-bucket.service.spec.ts - Dont need ${RgwHelper.DAEMON_QUERY_PARAM} src/pybind/mgr/dashboard/frontend/src/app/shared/api/rgw-bucket.service.ts - Removed enumerate function --- diff --git a/src/pybind/mgr/dashboard/frontend/src/app/ceph/rgw/rgw-bucket-form/rgw-bucket-form.component.spec.ts b/src/pybind/mgr/dashboard/frontend/src/app/ceph/rgw/rgw-bucket-form/rgw-bucket-form.component.spec.ts index 9b4b3ef67824d..62292337e4444 100644 --- a/src/pybind/mgr/dashboard/frontend/src/app/ceph/rgw/rgw-bucket-form/rgw-bucket-form.component.spec.ts +++ b/src/pybind/mgr/dashboard/frontend/src/app/ceph/rgw/rgw-bucket-form/rgw-bucket-form.component.spec.ts @@ -1,12 +1,12 @@ import { HttpClientTestingModule } from '@angular/common/http/testing'; -import { ComponentFixture, TestBed } from '@angular/core/testing'; -import { FormControl, ReactiveFormsModule } from '@angular/forms'; +import { ComponentFixture, fakeAsync, TestBed, tick } from '@angular/core/testing'; +import { ReactiveFormsModule } from '@angular/forms'; import { Router } from '@angular/router'; import { RouterTestingModule } from '@angular/router/testing'; import * as _ from 'lodash'; import { ToastrModule } from 'ngx-toastr'; -import { of as observableOf } from 'rxjs'; +import { of as observableOf, throwError } from 'rxjs'; import { configureTestBed, FormHelper, i18nProviders } from '../../../../testing/unit-test-helper'; import { RgwBucketService } from '../../../shared/api/rgw-bucket.service'; @@ -53,93 +53,75 @@ describe('RgwBucketFormComponent', () => { describe('bucketNameValidator', () => { const testValidator = (name: string, valid: boolean) => { - const validatorFn = component.bucketNameValidator(); - const ctrl = new FormControl(name); - ctrl.markAsDirty(); - const validatorPromise = validatorFn(ctrl); - expect(validatorPromise instanceof Promise).toBeTruthy(); - if (validatorPromise instanceof Promise) { - validatorPromise.then((resp) => { - if (valid) { - expect(resp).toBe(null); - } else { - expect(resp instanceof Object).toBeTruthy(); - expect(resp.bucketNameInvalid).toBeTruthy(); - } - }); + rgwBucketServiceGetSpy.and.returnValue(throwError('foo')); + formHelper.setValue('bid', name, true); + tick(500); + if (valid) { + formHelper.expectValid('bid'); + } else { + formHelper.expectError('bid', 'bucketNameInvalid'); } }; - it('should validate empty name', () => { - testValidator('', true); - }); + it('should validate empty name', fakeAsync(() => { + formHelper.expectErrorChange('bid', '', 'required', true); + })); - it('bucket names cannot be formatted as IP address', () => { + it('bucket names cannot be formatted as IP address', fakeAsync(() => { testValidator('172.10.4.51', false); - }); + })); - it('bucket name must be >= 3 characters long (1/2)', () => { + it('bucket name must be >= 3 characters long (1/2)', fakeAsync(() => { testValidator('ab', false); - }); + })); - it('bucket name must be >= 3 characters long (2/2)', () => { + it('bucket name must be >= 3 characters long (2/2)', fakeAsync(() => { testValidator('abc', true); - }); + })); - it('bucket name must be <= than 63 characters long (1/2)', () => { + it('bucket name must be <= than 63 characters long (1/2)', fakeAsync(() => { testValidator(_.repeat('a', 64), false); - }); + })); - it('bucket name must be <= than 63 characters long (2/2)', () => { + it('bucket name must be <= than 63 characters long (2/2)', fakeAsync(() => { testValidator(_.repeat('a', 63), true); - }); + })); - it('bucket names must not contain uppercase characters or underscores (1/2)', () => { + it('bucket names must not contain uppercase characters or underscores (1/2)', fakeAsync(() => { testValidator('iAmInvalid', false); - }); + })); - it('bucket names must not contain uppercase characters or underscores (2/2)', () => { + it('bucket names must not contain uppercase characters or underscores (2/2)', fakeAsync(() => { testValidator('i_am_invalid', false); - }); + })); - it('bucket names with invalid labels (1/3)', () => { + it('bucket names with invalid labels (1/3)', fakeAsync(() => { testValidator('abc.1def.Ghi2', false); - }); + })); - it('bucket names with invalid labels (2/3)', () => { - testValidator('abc.1-xy', false); - }); + it('bucket names with invalid labels (2/3)', fakeAsync(() => { + testValidator('abc.1_xy', false); + })); - it('bucket names with invalid labels (3/3)', () => { + it('bucket names with invalid labels (3/3)', fakeAsync(() => { testValidator('abc.*def', false); - }); + })); - it('bucket names must be a series of one or more labels and can contain lowercase letters, numbers, and hyphens (1/3)', () => { + it('bucket names must be a series of one or more labels and can contain lowercase letters, numbers, and hyphens (1/3)', fakeAsync(() => { testValidator('xyz.abc', true); - }); + })); - it('bucket names must be a series of one or more labels and can contain lowercase letters, numbers, and hyphens (2/3)', () => { + it('bucket names must be a series of one or more labels and can contain lowercase letters, numbers, and hyphens (2/3)', fakeAsync(() => { testValidator('abc.1-def', true); - }); + })); - it('bucket names must be a series of one or more labels and can contain lowercase letters, numbers, and hyphens (3/3)', () => { + it('bucket names must be a series of one or more labels and can contain lowercase letters, numbers, and hyphens (3/3)', fakeAsync(() => { testValidator('abc.ghi2', true); - }); + })); - it('bucket names must be unique', () => { - spyOn(rgwBucketService, 'enumerate').and.returnValue(observableOf(['abcd'])); - const validatorFn = component.bucketNameValidator(); - const ctrl = new FormControl('abcd'); - ctrl.markAsDirty(); - const validatorPromise = validatorFn(ctrl); - expect(validatorPromise instanceof Promise).toBeTruthy(); - if (validatorPromise instanceof Promise) { - validatorPromise.then((resp) => { - expect(resp instanceof Object).toBeTruthy(); - expect(resp.bucketNameExists).toBeTruthy(); - }); - } - }); + it('bucket names must be unique', fakeAsync(() => { + testValidator('bucket-name-is-unique', true); + })); it('should get zonegroup and placement targets', () => { const payload: Record = { diff --git a/src/pybind/mgr/dashboard/frontend/src/app/ceph/rgw/rgw-bucket-form/rgw-bucket-form.component.ts b/src/pybind/mgr/dashboard/frontend/src/app/ceph/rgw/rgw-bucket-form/rgw-bucket-form.component.ts index 86a3805235484..81e7519e96d69 100644 --- a/src/pybind/mgr/dashboard/frontend/src/app/ceph/rgw/rgw-bucket-form/rgw-bucket-form.component.ts +++ b/src/pybind/mgr/dashboard/frontend/src/app/ceph/rgw/rgw-bucket-form/rgw-bucket-form.component.ts @@ -4,6 +4,8 @@ import { ActivatedRoute, Router } from '@angular/router'; import { I18n } from '@ngx-translate/i18n-polyfill'; import * as _ from 'lodash'; +import { Observable, of as observableOf, timer as observableTimer } from 'rxjs'; +import { map, switchMapTo } from 'rxjs/operators'; import { RgwBucketService } from '../../../shared/api/rgw-bucket.service'; import { RgwSiteService } from '../../../shared/api/rgw-site.service'; @@ -224,59 +226,67 @@ export class RgwBucketFormComponent implements OnInit { * start and end with a lowercase letter or a number. */ bucketNameValidator(): AsyncValidatorFn { - const rgwBucketService = this.rgwBucketService; - return (control: AbstractControl): Promise => { - return new Promise((resolve) => { - // Exit immediately if user has not interacted with the control yet - // or the control value is empty. - if (control.pristine || control.value === '') { - resolve(null); - return; + return (control: AbstractControl): Observable => { + // Exit immediately if user has not interacted with the control yet + // or the control value is empty. + if (control.pristine || control.value === '') { + return observableOf(null); + } + const constraints = []; + // - Bucket names cannot be formatted as IP address. + constraints.push(() => { + const ipv4Rgx = /^((25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.){3}(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)$/i; + const ipv6Rgx = /^(?:[a-f0-9]{1,4}:){7}[a-f0-9]{1,4}$/i; + const name = this.bucketForm.get('bid').value; + let notAnIP = true; + if (ipv4Rgx.test(name) || ipv6Rgx.test(name)) { + notAnIP = false; } - const constraints = []; - // - Bucket names cannot be formatted as IP address. - constraints.push((name: AbstractControl) => { - const validatorFn = CdValidators.ip(); - return !validatorFn(name); - }); - // - Bucket names can be between 3 and 63 characters long. - constraints.push((name: string) => _.inRange(name.length, 3, 64)); - // - Bucket names must not contain uppercase characters or underscores. - // - Bucket names must start with a lowercase letter or number. - // - Bucket names must be a series of one or more labels. Adjacent - // labels are separated by a single period (.). Bucket names can - // contain lowercase letters, numbers, and hyphens. Each label must - // start and end with a lowercase letter or a number. - constraints.push((name: string) => { - const labels = _.split(name, '.'); - return _.every(labels, (label) => { - // Bucket names must not contain uppercase characters or underscores. - if (label !== _.toLower(label) || label.includes('_')) { - return false; - } - // Bucket names can contain lowercase letters, numbers, and hyphens. - if (!/[0-9a-z-]/.test(label)) { - return false; - } - // Each label must start and end with a lowercase letter or a number. - return _.every([0, label.length], (index) => { - return /[a-z]/.test(label[index]) || _.isInteger(_.parseInt(label[index])); - }); + return notAnIP; + }); + // - Bucket names can be between 3 and 63 characters long. + constraints.push((name: string) => _.inRange(name.length, 3, 64)); + // - Bucket names must not contain uppercase characters or underscores. + // - Bucket names must start with a lowercase letter or number. + // - Bucket names must be a series of one or more labels. Adjacent + // labels are separated by a single period (.). Bucket names can + // contain lowercase letters, numbers, and hyphens. Each label must + // start and end with a lowercase letter or a number. + constraints.push((name: string) => { + const labels = _.split(name, '.'); + return _.every(labels, (label) => { + // Bucket names must not contain uppercase characters or underscores. + if (label !== _.toLower(label) || label.includes('_')) { + return false; + } + // Bucket names can contain lowercase letters, numbers, and hyphens. + if (!/[0-9a-z-]/.test(label)) { + return false; + } + // Each label must start and end with a lowercase letter or a number. + return _.every([0, label.length], (index) => { + return /[a-z]/.test(label[index]) || _.isInteger(_.parseInt(label[index])); }); }); - if (!_.every(constraints, (func: Function) => func(control.value))) { - resolve({ bucketNameInvalid: true }); - return; - } - // - Bucket names must be unique. - rgwBucketService.exists(control.value).subscribe((resp: boolean) => { + }); + if (!_.every(constraints, (func: Function) => func(control.value))) { + return observableTimer().pipe( + map(() => { + return { bucketNameInvalid: true }; + }) + ); + } + // - Bucket names must be unique. + return observableTimer().pipe( + switchMapTo(this.rgwBucketService.exists.call(this.rgwBucketService, control.value)), + map((resp: boolean) => { if (!resp) { - resolve(null); + return null; } else { - resolve({ bucketNameExists: true }); + return { bucketNameExists: true }; } - }); - }); + }) + ); }; } diff --git a/src/pybind/mgr/dashboard/frontend/src/app/ceph/rgw/rgw-user-form/rgw-user-form.component.spec.ts b/src/pybind/mgr/dashboard/frontend/src/app/ceph/rgw/rgw-user-form/rgw-user-form.component.spec.ts index d9b112f76623d..70d819e4bb4e9 100644 --- a/src/pybind/mgr/dashboard/frontend/src/app/ceph/rgw/rgw-user-form/rgw-user-form.component.spec.ts +++ b/src/pybind/mgr/dashboard/frontend/src/app/ceph/rgw/rgw-user-form/rgw-user-form.component.spec.ts @@ -162,14 +162,14 @@ describe('RgwUserFormComponent', () => { it('should validate that username is valid', fakeAsync(() => { spyOn(rgwUserService, 'get').and.returnValue(throwError('foo')); formHelper.setValue('user_id', 'ab', true); - tick(500); + tick(); formHelper.expectValid('user_id'); })); it('should validate that username is invalid', fakeAsync(() => { spyOn(rgwUserService, 'get').and.returnValue(observableOf({})); formHelper.setValue('user_id', 'abc', true); - tick(500); + tick(); formHelper.expectError('user_id', 'notUnique'); })); }); diff --git a/src/pybind/mgr/dashboard/frontend/src/app/shared/api/rgw-bucket.service.spec.ts b/src/pybind/mgr/dashboard/frontend/src/app/shared/api/rgw-bucket.service.spec.ts index 978aca1fc4fcc..eefabb66d5476 100644 --- a/src/pybind/mgr/dashboard/frontend/src/app/shared/api/rgw-bucket.service.spec.ts +++ b/src/pybind/mgr/dashboard/frontend/src/app/shared/api/rgw-bucket.service.spec.ts @@ -75,7 +75,7 @@ describe('RgwBucketService', () => { service.exists('foo').subscribe((resp) => { result = resp; }); - const req = httpTesting.expectOne('api/rgw/bucket'); + const req = httpTesting.expectOne('api/rgw/bucket/foo'); expect(req.request.method).toBe('GET'); req.flush(['foo', 'bar']); expect(result).toBe(true); diff --git a/src/pybind/mgr/dashboard/frontend/src/app/shared/api/rgw-bucket.service.ts b/src/pybind/mgr/dashboard/frontend/src/app/shared/api/rgw-bucket.service.ts index b73bff0dddc59..5e12cb3046f53 100644 --- a/src/pybind/mgr/dashboard/frontend/src/app/shared/api/rgw-bucket.service.ts +++ b/src/pybind/mgr/dashboard/frontend/src/app/shared/api/rgw-bucket.service.ts @@ -3,7 +3,7 @@ import { Injectable } from '@angular/core'; import * as _ from 'lodash'; import { of as observableOf } from 'rxjs'; -import { mergeMap } from 'rxjs/operators'; +import { catchError, mapTo } from 'rxjs/operators'; import { cdEncode } from '../decorators/cd-encode'; import { ApiModule } from './api.module'; @@ -27,14 +27,6 @@ export class RgwBucketService { return this.http.get(this.url, { params: params }); } - /** - * Get the list of bucket names. - * @return {Observable} - */ - enumerate() { - return this.http.get(this.url); - } - get(bucket: string) { return this.http.get(`${this.url}/${bucket}`); } @@ -102,10 +94,13 @@ export class RgwBucketService { * @return {Observable} */ exists(bucket: string) { - return this.enumerate().pipe( - mergeMap((resp: string[]) => { - const index = _.indexOf(resp, bucket); - return observableOf(-1 !== index); + return this.get(bucket).pipe( + mapTo(true), + catchError((error: Event) => { + if (_.isFunction(error.preventDefault)) { + error.preventDefault(); + } + return observableOf(false); }) ); } diff --git a/src/pybind/mgr/dashboard/frontend/src/app/shared/forms/cd-validators.ts b/src/pybind/mgr/dashboard/frontend/src/app/shared/forms/cd-validators.ts index 0d302bb901357..dba458b2b5e5b 100644 --- a/src/pybind/mgr/dashboard/frontend/src/app/shared/forms/cd-validators.ts +++ b/src/pybind/mgr/dashboard/frontend/src/app/shared/forms/cd-validators.ts @@ -349,29 +349,26 @@ export class CdValidators { serviceFn: existsServiceFn, serviceFnThis: any = null, usernameFn?: Function, - uidfield = false, - dueTime = 500 + uidField = false ): AsyncValidatorFn { - let uname: string; + let uName: string; return (control: AbstractControl): Observable => { // Exit immediately if user has not interacted with the control yet // or the control value is empty. if (control.pristine || isEmptyInputValue(control.value)) { return observableOf(null); } - uname = control.value; + uName = control.value; if (_.isFunction(usernameFn) && usernameFn() !== null && usernameFn() !== '') { - if (uidfield) { - uname = `${control.value}$${usernameFn()}`; + if (uidField) { + uName = `${control.value}$${usernameFn()}`; } else { - uname = `${usernameFn()}$${control.value}`; + uName = `${usernameFn()}$${control.value}`; } } - // Forgot previous requests if a new one arrives within the specified - // delay time. - return observableTimer(dueTime).pipe( - switchMapTo(serviceFn.call(serviceFnThis, uname)), + return observableTimer().pipe( + switchMapTo(serviceFn.call(serviceFnThis, uName)), map((resp: boolean) => { if (!resp) { return null;