mgr/dashboard: Show error on creating service with duplicate service id #47261

aaSharma14 · 2022-07-25T11:09:55Z

Fixes: https://tracker.ceph.com/issues/56689
Signed-off-by: Aashish Sharma aasharma@redhat.com

Contribution Guidelines

To sign and title your commits, please refer to Submitting Patches to Ceph.
If you are submitting a fix for a stable branch (e.g. "pacific"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.

Checklist

Tracker (select at least one)
- References tracker ticket
- Very recent bug; references commit where it was introduced
- New feature (ticket optional)
- Doc update (no ticket needed)
- Code cleanup (no ticket needed)
Component impact
- Affects Dashboard, opened tracker ticket
- Affects Orchestrator, opened tracker ticket
- No impact that needs to be tracked
Documentation (select at least one)
- Updates relevant documentation
- No doc update is appropriate
Tests (select at least one)
- Includes unit test(s)
- Includes integration test(s)
- Includes bug reproducer
- No tests

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 dashboard cephadm
jenkins test api
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox
jenkins test windows

avanthakkar

@aaSharma14 Can you add a unit-test for checking the duplicate service id creation behavior?

avanthakkar · 2022-07-25T15:38:50Z

jenkins test make check

nizamial09

I created an nfs service with id test. then I tried to create an iscsi service with the same id test it failed with the validation.

IMO, we should not consider the service_id for the validation, instead we should focus on the service_name, and check for its occurence in the table?

You can see the nfs.test at the bottom of the list.

Also, would it make sense to introduce a service_name field in the form which autopopulates based on the above two inputs (service_type.service_id)?

epuertat

I'm curious, what happens if you try to create the service from the REST API alone? We should ensure that we build our UI with leveraging the REST API capabilities, rather than working around it. For example, a POST with an existing service_type + service_id should fail. Is that what currently happens?

For this purpose the UI Endpoints can be very useful (and I don't just mean for async validation, but also for pre-fetching data for validation purposes).

In order not to fetch a list with all the service types + ids (which could be large), what about of (async) fetching the list of services ids for the currently selected service type (so when the service type drop down is selected)?

.../mgr/dashboard/frontend/src/app/ceph/cluster/services/service-form/service-form.component.ts

...gr/dashboard/frontend/src/app/ceph/cluster/services/service-form/service-form.component.html

aaSharma14 · 2022-07-28T06:34:56Z

I'm curious, what happens if you try to create the service from the REST API alone? We should ensure that we build our UI with leveraging the REST API capabilities, rather than working around it. For example, a POST with an existing service_type + service_id should fail. Is that what currently happens?

For this purpose the UI Endpoints can be very useful (and I don't just mean for async validation, but also for pre-fetching data for validation purposes).

In order not to fetch a list with all the service types + ids (which could be large), what about of (async) fetching the list of services ids for the currently selected service type (so when the service type drop down is selected)?

Thanks @epuertat , I have updated the PR with some back-end changes as well. So as per the new implementation, I have created a new PUT endpoint for editing a service. So if a user tries to create a service with existing service_type + service_id from the POST endpoint, the service won't get updated but It won't give any error as well (as per the cephadm implementation, there is no error message returned when we add the --no-overwrite flag to the apply command) and if the user uses the PUT endpoint to edit the service, the service will get updated accordingly.

aaSharma14 · 2022-07-28T06:38:00Z

I created an nfs service with id test. then I tried to create an iscsi service with the same id test it failed with the validation.

IMO, we should not consider the service_id for the validation, instead we should focus on the service_name, and check for its occurence in the table?

You can see the nfs.test at the bottom of the list.

Also, would it make sense to introduce a service_name field in the form which autopopulates based on the above two inputs (service_type.service_id)?

Thanks @nizamial09 , I have updated the PR. Regarding adding a service name field to the form, I'l open an other PR for that by opening a related tracker.

avanthakkar · 2022-07-28T10:32:45Z

I created an nfs service with id test. then I tried to create an iscsi service with the same id test it failed with the validation.

IMO, we should not consider the service_id for the validation, instead we should focus on the service_name, and check for its occurence in the table?

You can see the nfs.test at the bottom of the list.

Also, would it make sense to introduce a service_name field in the form which autopopulates based on the above two inputs (service_type.service_id)?

I don't think we need a new service_name field here..bcz its anyways assigned as ${service_type}.${service_id} , thats how it done in cephadm, it doesnt ask for name as input as id & type are enough.

nizamial09 · 2022-07-29T04:33:19Z

I don't think we need a new service_name field here..bcz its anyways assigned as ${service_type}.${service_id} , thats how it done in cephadm, it doesnt ask for name as input as id & type are enough.

i wasn't going for an input field that user needs to input. I was asking for a read-only field which auto-populates based on the service_type and service_id, just to let the user know that this will be the service_name once you create it.

nizamial09

validation looks good to me! left some code reviews..

src/pybind/mgr/dashboard/controllers/service.py

.../mgr/dashboard/frontend/src/app/ceph/cluster/services/service-form/service-form.component.ts

src/pybind/mgr/dashboard/controllers/service.py

.../mgr/dashboard/frontend/src/app/ceph/cluster/services/service-form/service-form.component.ts

epuertat

Great work @aaSharma14 ! Left some ideas over there, but this is already really nice.

src/pybind/mgr/dashboard/controllers/service.py

epuertat · 2022-07-29T17:01:36Z

src/pybind/mgr/dashboard/frontend/src/app/shared/api/ceph-service.service.ts

+      {
+        service_name: serviceName,
+        service_spec: serviceSpec
+      },


I guess this would require a model or a new class definition?

Editing a service is pretty much similar to the service creation (both require service name and service spec as parameters), I don't think this would require a separate model to be introduced.

epuertat · 2022-07-29T17:16:59Z

src/pybind/mgr/dashboard/openapi.yaml

+      description: "\n        :param service_spec: The service specification as JSON.\n\
+        \        :param service_name: The service name, e.g. 'alertmanager'.\n   \
+        \     :return: None\n        "


We should definitely improve the code that takes docstrings and print those as heredoc blocks:

Suggested change

description: "\n :param service_spec: The service specification as JSON.\n\

\ :param service_name: The service name, e.g. 'alertmanager'.\n \

\ :return: None\n "

description: |

:param service_spec: The service specification as JSON.

:param service_name: The service name, e.g. 'alertmanager'.

:return: None

WIll create a tracker and do this imrpovement for the entire openapi.yaml file.

src/pybind/mgr/dashboard/services/orchestrator.py

aaSharma14 · 2022-08-01T10:58:27Z

jenkins test dashboard

Fixes: https://tracker.ceph.com/issues/56689 Signed-off-by: Aashish Sharma <aasharma@redhat.com>

aaSharma14 requested a review from a team as a code owner July 25, 2022 11:09

aaSharma14 requested review from bryanmontalvan and nizamial09 and removed request for a team July 25, 2022 11:09

github-actions bot added dashboard pybind labels Jul 25, 2022

aaSharma14 requested review from avanthakkar and epuertat July 25, 2022 11:10

aaSharma14 added the bug-fix label Jul 25, 2022

aaSharma14 added this to In progress in Dashboard via automation Jul 25, 2022

avanthakkar approved these changes Jul 25, 2022

View reviewed changes

Dashboard automation moved this from In progress to Reviewer approved Jul 25, 2022

avanthakkar reviewed Jul 25, 2022

View reviewed changes

nizamial09 requested changes Jul 26, 2022

View reviewed changes

Dashboard automation moved this from Reviewer approved to Review in progress Jul 26, 2022

epuertat reviewed Jul 26, 2022

View reviewed changes

.../mgr/dashboard/frontend/src/app/ceph/cluster/services/service-form/service-form.component.ts Outdated Show resolved Hide resolved

...gr/dashboard/frontend/src/app/ceph/cluster/services/service-form/service-form.component.html Outdated Show resolved Hide resolved

aaSharma14 force-pushed the fix-duplicate-service-creation branch from 841bc17 to 7e88117 Compare July 28, 2022 06:26

aaSharma14 requested a review from nizamial09 July 28, 2022 06:38

nizamial09 reviewed Jul 29, 2022

View reviewed changes

github-actions bot added the api-change label Jul 29, 2022

aaSharma14 force-pushed the fix-duplicate-service-creation branch from 5b44a29 to 910c669 Compare July 29, 2022 08:07

aaSharma14 requested a review from nizamial09 July 29, 2022 08:09

aaSharma14 force-pushed the fix-duplicate-service-creation branch from 910c669 to 28c0558 Compare July 29, 2022 09:06

epuertat approved these changes Jul 29, 2022

View reviewed changes

aaSharma14 force-pushed the fix-duplicate-service-creation branch from 28c0558 to 2212623 Compare August 1, 2022 05:55

aaSharma14 force-pushed the fix-duplicate-service-creation branch from 2212623 to 659569e Compare August 1, 2022 06:04

nizamial09 approved these changes Aug 1, 2022

View reviewed changes

Dashboard automation moved this from Review in progress to Reviewer approved Aug 1, 2022

aaSharma14 force-pushed the fix-duplicate-service-creation branch from a89470c to f9190ca Compare August 1, 2022 08:37

mgr/dashboard: Show error on creating service with duplicate service id

07cfd44

Fixes: https://tracker.ceph.com/issues/56689 Signed-off-by: Aashish Sharma <aasharma@redhat.com>

aaSharma14 force-pushed the fix-duplicate-service-creation branch from f9190ca to 07cfd44 Compare August 2, 2022 05:44

aaSharma14 assigned nizamial09 Aug 2, 2022

nizamial09 merged commit 7e12357 into ceph:main Aug 2, 2022
12 of 13 checks passed

Dashboard automation moved this from Reviewer approved to Done Aug 2, 2022

nizamial09 deleted the fix-duplicate-service-creation branch August 2, 2022 08:09

This was referenced Aug 2, 2022

quincy: mgr/dashboard: Show error on creating service with duplicate service id #47403

Merged

pacific: mgr/dashboard: Show error on creating service with duplicate service id #47404

Merged

mgr/dashboard: Show error on creating service with duplicate service id #47261

mgr/dashboard: Show error on creating service with duplicate service id #47261

aaSharma14 commented Jul 25, 2022

avanthakkar left a comment

avanthakkar commented Jul 25, 2022

nizamial09 left a comment •

edited

epuertat left a comment

aaSharma14 commented Jul 28, 2022

aaSharma14 commented Jul 28, 2022

avanthakkar commented Jul 28, 2022

nizamial09 commented Jul 29, 2022

nizamial09 left a comment

epuertat left a comment

epuertat Jul 29, 2022

aaSharma14 Aug 1, 2022

epuertat Jul 29, 2022

aaSharma14 Aug 1, 2022

aaSharma14 commented Aug 1, 2022

mgr/dashboard: Show error on creating service with duplicate service id #47261

mgr/dashboard: Show error on creating service with duplicate service id #47261

Conversation

aaSharma14 commented Jul 25, 2022

Contribution Guidelines

Checklist

avanthakkar left a comment

Choose a reason for hiding this comment

avanthakkar commented Jul 25, 2022

nizamial09 left a comment • edited

Choose a reason for hiding this comment

epuertat left a comment

Choose a reason for hiding this comment

aaSharma14 commented Jul 28, 2022

aaSharma14 commented Jul 28, 2022

avanthakkar commented Jul 28, 2022

nizamial09 commented Jul 29, 2022

nizamial09 left a comment

Choose a reason for hiding this comment

epuertat left a comment

Choose a reason for hiding this comment

epuertat Jul 29, 2022

Choose a reason for hiding this comment

aaSharma14 Aug 1, 2022

Choose a reason for hiding this comment

epuertat Jul 29, 2022

Choose a reason for hiding this comment

aaSharma14 Aug 1, 2022

Choose a reason for hiding this comment

aaSharma14 commented Aug 1, 2022

nizamial09 left a comment •

edited