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: v17.1.0~1997^2~1 X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=005327c4e12fef8d7054894d9df021c0b3c53e19;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 --- 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 01f4626686a1..82e1a1e47f2b 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 _ from 'lodash'; import { ToastrModule } from 'ngx-toastr'; -import { of as observableOf } from 'rxjs'; +import { of as observableOf, throwError } from 'rxjs'; import { RgwBucketService } from '~/app/shared/api/rgw-bucket.service'; import { RgwSiteService } from '~/app/shared/api/rgw-site.service'; @@ -55,93 +55,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 1c51d701d12d..bde9390dcf31 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 @@ -3,7 +3,8 @@ import { AbstractControl, AsyncValidatorFn, ValidationErrors, Validators } from import { ActivatedRoute, Router } from '@angular/router'; import _ from 'lodash'; -import { forkJoin } from 'rxjs'; +import { forkJoin, Observable, of as observableOf, timer as observableTimer } from 'rxjs'; +import { map, switchMapTo } from 'rxjs/operators'; import { RgwBucketService } from '~/app/shared/api/rgw-bucket.service'; import { RgwSiteService } from '~/app/shared/api/rgw-site.service'; @@ -242,59 +243,67 @@ export class RgwBucketFormComponent extends CdForm 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 6bb86dd4cda8..15665d53bb95 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 3f2bae5f48a1..32c88b8c9475 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 @@ -80,7 +80,7 @@ describe('RgwBucketService', () => { service.exists('foo').subscribe((resp) => { result = resp; }); - const req = httpTesting.expectOne(`api/rgw/bucket?${RgwHelper.DAEMON_QUERY_PARAM}`); + const req = httpTesting.expectOne(`api/rgw/bucket/foo?${RgwHelper.DAEMON_QUERY_PARAM}`); 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 1d622518bba9..03fe5ca017dc 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 _ from 'lodash'; import { of as observableOf } from 'rxjs'; -import { mergeMap } from 'rxjs/operators'; +import { catchError, mapTo } from 'rxjs/operators'; import { RgwDaemonService } from '~/app/shared/api/rgw-daemon.service'; import { cdEncode } from '~/app/shared/decorators/cd-encode'; @@ -28,16 +28,6 @@ export class RgwBucketService { }); } - /** - * Get the list of bucket names. - * @return Observable - */ - enumerate() { - return this.rgwDaemonService.request((params: HttpParams) => { - return this.http.get(this.url, { params: params }); - }); - } - get(bucket: string) { return this.rgwDaemonService.request((params: HttpParams) => { return this.http.get(`${this.url}/${bucket}`, { params: params }); @@ -112,10 +102,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 1537c1500045..b72b8f18bdcf 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 @@ -348,29 +348,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;