mgr/dashboard: RGW buckets async validator performance enhancement and name constraints #41023

nizamial09 · 2021-04-26T06:51:28Z

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.

BEFORE

bucketname-error.mp4

AFTER

bucketname_fix.mp4

Also explains the rgw bucket name constrains for each bucket name validation
errors.

Fixes: https://tracker.ceph.com/issues/50514
Fixes: https://tracker.ceph.com/issues/50516
Signed-off-by: Nizamudeen A nia@redhat.com

Checklist

References tracker ticket
Updates documentation if necessary
Includes tests for new functionality or reproducer for bug

Show available Jenkins commands

jenkins retest this please
jenkins test classic perf
jenkins test crimson perf
jenkins test signed
jenkins test make check
jenkins test make check arm64
jenkins test submodules
jenkins test dashboard
jenkins test api
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox

nizamial09 · 2021-04-26T06:52:20Z

jenkins test dashboard

nizamial09 · 2021-04-26T07:00:45Z

jenkins test dashboard

aaSharma14

Just a suggestion.

...ybind/mgr/dashboard/frontend/src/app/ceph/rgw/rgw-bucket-form/rgw-bucket-form.component.html

epuertat · 2021-05-05T15:12:48Z

src/pybind/mgr/dashboard/frontend/src/app/ceph/rgw/rgw-bucket-form/rgw-bucket-form.component.ts

+        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;


Aren't there other places in our code where IPv4/6 needs be validated so we make this reusable? Or couldn't we replace part of this validation logic with some external validator library (I did a quick check but no luck)?

I think we should add tests for this

const tests = [ '1.1.1.01', '001.1.1.01', ] 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; for(let t in tests) { const b = ipv4Rgx.test(tests[t]) console.log(b) }

output:

true true

I don't think anyone will type that but who knows

@epuertat There's a CdValidators.ip() we are using, but I was not succesfull in making it work over here. It was actually used here previously but its actually incorrectly validating the fields (even if we enter IP address it doesn't give error). I checked the external libraries but none for the IP address.

@pereman2 The test for the IPaddress is being covered here: https://github.com/ceph/ceph/pull/41023/files#diff-fc6d2da6d3b05b9128d9d1832bea8b55aa9c4c53c918802b19e6b915cba6415dR72

I think this will verify the IPs validity. Is that okay?
Also thank you for the idea of putting multiple IP address. Didn't thought of that until I saw the example from you.

Nice, thank you!

epuertat · 2021-05-05T15:23:14Z

...ybind/mgr/dashboard/frontend/src/app/ceph/rgw/rgw-bucket-form/rgw-bucket-form.component.html

+            <span class="invalid-feedback"
+                  *ngIf="bucketForm.showError('bid', frm, 'containsUpperCase')"
+                  i18n>Bucket names must not contain uppercase characters or underscores.</span>
+            <span class="invalid-feedback"
+                  *ngIf="bucketForm.showError('bid', frm, 'lowerCaseOrNumber')"
+                  i18n>Each label must start and end with a lowercase letter or a number.</span>
+            <span class="invalid-feedback"
+                  *ngIf="bucketForm.showError('bid', frm, 'ipAddress')"
+                  i18n>Bucket names cannot be formatted as IP address.</span>
+            <span class="invalid-feedback"
+                  *ngIf="bucketForm.showError('bid', frm, 'shouldBeInRange')"
+                  i18n>Bucket names can be between 3 and 63 characters long.</span>


Is there any way to reduce the amount of boilerplate code here? Perhas directly return the error message from the validator (or is that an antipatter... I'm not too familiar with this part of Angular)?

I tried and I think I may need to modify showError function for that or something else. But with the current implementations I wasn't able to return error message. If I could do that I can remove these boiler plates from almost all of the form pages. Do you want me to explore that possibility along with this PR? @epuertat (if not I'll raise one tracker and explore it separately)

nizamial09 · 2021-05-10T20:16:59Z

jenkins retest this

nizamial09 · 2021-05-10T20:17:12Z

jenkins retest this please

nizamial09 · 2021-05-11T08:10:15Z

jenkins test dashboard

src/pybind/mgr/dashboard/frontend/src/app/shared/forms/cd-validators.ts

src/pybind/mgr/dashboard/frontend/src/app/ceph/rgw/rgw-bucket-form/rgw-bucket-form.component.ts

...nd/mgr/dashboard/frontend/src/app/ceph/rgw/rgw-bucket-form/rgw-bucket-form.component.spec.ts

nizamial09 · 2021-05-11T14:43:31Z

@alfonsomthd @avanthakkar Took care of that weird behaviour. and yeah dashes are allowed but not at the end.

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 <nia@redhat.com>

avanthakkar

Great work @nizamial09, LGTM!

avanthakkar · 2021-05-11T16:33:55Z

jenkins test dashboard

avanthakkar · 2021-05-11T16:34:52Z

@nizamial09 make-check failure:

[lint:tslint  ] Linting "ceph-dashboard"...
[lint:tslint  ] /home/jenkins-build/build/workspace/ceph-pull-requests/src/pybind/mgr/dashboard/frontend/src/app/ceph/rgw/rgw-bucket-form/rgw-bucket-form.component.ts:294:42
[lint:tslint  ] ERROR: 294:42  whitespace  missing whitespace
[lint:tslint  ] ERROR: 294:43  whitespace  missing whitespace

Explain the rgw bucket name constrains for each bucket name validation errors. Fixes: https://tracker.ceph.com/issues/50516 Signed-off-by: Nizamudeen A <nia@redhat.com>

epuertat

LGTM @nizamial09 ! Feel free to file trackers for the issues you detected while fixing this one. I still feel that understanding/debugging/extending this part of the code is not exactly straight-forward :/

nizamial09 · 2021-05-11T17:59:09Z

jenkins test dashboard

nizamial09 · 2021-05-11T18:05:59Z

@epuertat I created one tracker for it: https://tracker.ceph.com/issues/50762 I'll look for some alternates to return these errors.

alfonsomthd · 2021-05-12T07:19:18Z

...nd/mgr/dashboard/frontend/src/app/ceph/rgw/rgw-bucket-form/rgw-bucket-form.component.spec.ts

-      testValidator('172.10.4.51', false);
-    });
+    it('bucket names cannot be formatted as IP address', fakeAsync(() => {
+      const testIPs = ['1.1.1.01', '001.1.1.01', '127.0.0.1'];


You can also add an IPv6 like 2001:0db8:85a3:0000:0000:8a2e:0370:7334

Sorry @alfonsomthd I didn't had a chance to take a look at this in the morning.

epuertat · 2021-05-12T07:49:04Z

@epuertat I created one tracker for it: tracker.ceph.com/issues/50762 I'll look for some alternates to return these errors.

Thanks @nizamial09! I liked what I read in this link that @avanthakkar shared.

nizamial09 requested a review from a team April 26, 2021 06:51

nizamial09 requested a review from a team as a code owner April 26, 2021 06:51

nizamial09 requested review from avanthakkar and callithea and removed request for a team April 26, 2021 06:51

github-actions bot added dashboard pybind labels Apr 26, 2021

nizamial09 added the bug-fix label Apr 26, 2021

nizamial09 requested review from aaSharma14, alfonsomthd and epuertat April 26, 2021 06:52

nizamial09 changed the title ~~mgr/dashboard: RGW buckets async validator performance enhancement~~ mgr/dashboard: RGW buckets async validator performance enhancement Apr 26, 2021

nizamial09 added this to In progress in Dashboard via automation Apr 26, 2021

nizamial09 added the needs-review label Apr 26, 2021

nizamial09 changed the title ~~mgr/dashboard: RGW buckets async validator performance enhancement~~ mgr/dashboard: RGW buckets async validator performance enhancement and name constraints Apr 28, 2021

aaSharma14 reviewed May 3, 2021

View reviewed changes

...ybind/mgr/dashboard/frontend/src/app/ceph/rgw/rgw-bucket-form/rgw-bucket-form.component.html Outdated Show resolved Hide resolved

alfonsomthd moved this from In progress to Review in progress in Dashboard May 3, 2021

epuertat reviewed May 5, 2021

View reviewed changes

nizamial09 force-pushed the bucket-name-async-validator-performance branch from 9c0408f to f180057 Compare May 10, 2021 16:52

nizamial09 requested review from pereman2 and aaSharma14 May 10, 2021 17:00

nizamial09 requested a review from epuertat May 10, 2021 20:20

nizamial09 force-pushed the bucket-name-async-validator-performance branch from f180057 to 8eb20c8 Compare May 11, 2021 08:12

avanthakkar reviewed May 11, 2021

View reviewed changes

...nd/mgr/dashboard/frontend/src/app/ceph/rgw/rgw-bucket-form/rgw-bucket-form.component.spec.ts Outdated Show resolved Hide resolved

nizamial09 force-pushed the bucket-name-async-validator-performance branch from b894d82 to 14d593d Compare May 11, 2021 14:53

nizamial09 requested a review from a team as a code owner May 11, 2021 14:53

github-actions bot added the orchestrator label May 11, 2021

nizamial09 force-pushed the bucket-name-async-validator-performance branch from 14d593d to 9b0c723 Compare May 11, 2021 14:54

nizamial09 removed the orchestrator label May 11, 2021

nizamial09 requested a review from avanthakkar May 11, 2021 14:54

nizamial09 force-pushed the bucket-name-async-validator-performance branch 3 times, most recently from c2d57f7 to 45fce2e Compare May 11, 2021 15:34

avanthakkar approved these changes May 11, 2021

View reviewed changes

Dashboard automation moved this from Review in progress to Reviewer approved May 11, 2021

mgr/dashboard: Detailed error messages in rgw bucket name validation

ab04e53

Explain the rgw bucket name constrains for each bucket name validation errors. Fixes: https://tracker.ceph.com/issues/50516 Signed-off-by: Nizamudeen A <nia@redhat.com>

nizamial09 force-pushed the bucket-name-async-validator-performance branch from 45fce2e to ab04e53 Compare May 11, 2021 17:19

epuertat approved these changes May 11, 2021

View reviewed changes

nizamial09 moved this from Reviewer approved to Ready-to-merge in Dashboard May 12, 2021

alfonsomthd added the skip-teuthology For PRs whose changes do not have an effect on QA runs/changes are not being tested in Teuthology label May 12, 2021

Dashboard automation moved this from Ready-to-merge to Reviewer approved May 12, 2021

alfonsomthd approved these changes May 12, 2021

View reviewed changes

epuertat merged commit a0495c8 into ceph:master May 12, 2021

Dashboard automation moved this from Reviewer approved to Done May 12, 2021

epuertat deleted the bucket-name-async-validator-performance branch May 12, 2021 08:26

nizamial09 mentioned this pull request May 12, 2021

pacific: mgr/dashboard: RGW buckets async validator performance enhancement and name constraints #41296

Merged

nizamial09 mentioned this pull request Jun 30, 2021

octopus: mgr/dashboard: RGW buckets async validator performance enhancement and name constraints #42123

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mgr/dashboard: RGW buckets async validator performance enhancement and name constraints #41023

mgr/dashboard: RGW buckets async validator performance enhancement and name constraints #41023

nizamial09 commented Apr 26, 2021 •

edited by epuertat

nizamial09 commented Apr 26, 2021

nizamial09 commented Apr 26, 2021

aaSharma14 left a comment

epuertat May 5, 2021

pereman2 May 6, 2021 •

edited

nizamial09 May 10, 2021

nizamial09 May 10, 2021

pereman2 May 11, 2021

epuertat May 5, 2021

nizamial09 May 10, 2021

nizamial09 commented May 10, 2021

nizamial09 commented May 10, 2021

nizamial09 commented May 11, 2021

nizamial09 commented May 11, 2021 •

edited

avanthakkar left a comment

avanthakkar commented May 11, 2021

avanthakkar commented May 11, 2021

epuertat left a comment

nizamial09 commented May 11, 2021

nizamial09 commented May 11, 2021

alfonsomthd May 12, 2021

nizamial09 May 12, 2021

epuertat commented May 12, 2021

		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;

mgr/dashboard: RGW buckets async validator performance enhancement and name constraints #41023

mgr/dashboard: RGW buckets async validator performance enhancement and name constraints #41023

Conversation

nizamial09 commented Apr 26, 2021 • edited by epuertat

Checklist

nizamial09 commented Apr 26, 2021

nizamial09 commented Apr 26, 2021

aaSharma14 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pereman2 May 6, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nizamial09 commented May 10, 2021

nizamial09 commented May 10, 2021

nizamial09 commented May 11, 2021

nizamial09 commented May 11, 2021 • edited

avanthakkar left a comment

Choose a reason for hiding this comment

avanthakkar commented May 11, 2021

avanthakkar commented May 11, 2021

epuertat left a comment

Choose a reason for hiding this comment

nizamial09 commented May 11, 2021

nizamial09 commented May 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epuertat commented May 12, 2021

nizamial09 commented Apr 26, 2021 •

edited by epuertat

pereman2 May 6, 2021 •

edited

nizamial09 commented May 11, 2021 •

edited