Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mgr/dashboard: disable NFSv3 support in dashboard #41081

Merged
merged 1 commit into from May 5, 2021

Conversation

votdev
Copy link
Member

@votdev votdev commented Apr 29, 2021

The v3 checkbox is disabled. The v3 checkbox is unchecked and v4 is checked by default. The form can not be submitted if both protocols are unchecked.

Fixes: https://tracker.ceph.com/issues/49718
Related to: #40154

According to #40154 (comment) there is a bigger refactoring ongoing, but the issue must be backported to Octopus, so a simple cleanup would be easier to backport.

Signed-off-by: Volker Theile vtheile@suse.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

@votdev votdev requested a review from a team as a code owner April 29, 2021 11:25
@votdev votdev added this to In progress in Dashboard via automation Apr 29, 2021
@votdev votdev requested a review from asettle April 29, 2021 11:29
@votdev votdev moved this from In progress to Review in progress in Dashboard Apr 29, 2021
@votdev votdev moved this from Review in progress to In progress in Dashboard Apr 29, 2021
@votdev votdev changed the title mgr/dashboard: remove NFSv3 support from dashboard [WIP] mgr/dashboard: remove NFSv3 support from dashboard Apr 29, 2021
@votdev votdev force-pushed the issue_49718_disable_nfs_v3 branch from 3e397f5 to cff3bdc Compare April 29, 2021 14:46
@votdev votdev changed the title [WIP] mgr/dashboard: remove NFSv3 support from dashboard mgr/dashboard: remove NFSv3 support from dashboard Apr 29, 2021
@votdev votdev moved this from In progress to Review in progress in Dashboard Apr 29, 2021
Dashboard automation moved this from Review in progress to Reviewer approved Apr 29, 2021
@votdev votdev force-pushed the issue_49718_disable_nfs_v3 branch from cff3bdc to 8d83180 Compare April 29, 2021 16:00
@epuertat
Copy link
Member

jenkins test dashboard

Copy link
Member

@epuertat epuertat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks @votdev !

@epuertat
Copy link
Member

@alfonsomthd as you've been recently taking care of NFS stuff, what do you think about this approach?

Fixes: https://tracker.ceph.com/issues/49718
Related to: ceph#40154

Signed-off-by: Volker Theile <vtheile@suse.com>
@votdev votdev force-pushed the issue_49718_disable_nfs_v3 branch from 8d83180 to 864a362 Compare May 3, 2021 15:13
@votdev votdev changed the title mgr/dashboard: remove NFSv3 support from dashboard mgr/dashboard: disable NFSv3 support in dashboard May 3, 2021
@votdev
Copy link
Member Author

votdev commented May 3, 2021

The last commit removes the disabled property from the v4 checkbox. This way it is still possible to edit existing setups without forcing the export to support v4.
New NFS exports can not select v3, and the form can't be submitted if v4 is not checked.
Additionally a new unit test has been added to ensure this behavior.

@batrick batrick requested a review from varshar16 May 3, 2021 16:08
@votdev
Copy link
Member Author

votdev commented May 3, 2021

jenkins test dashboard

@alfonsomthd
Copy link
Contributor

@alfonsomthd as you've been recently taking care of NFS stuff, what do you think about this approach?

I'm OK with it.

@votdev votdev moved this from Reviewer approved to Ready-to-merge in Dashboard May 4, 2021
@alfonsomthd
Copy link
Contributor

alfonsomthd commented May 4, 2021

@votdev I see that before checking NFSv4, NFSv3 is greyed out to indicate not selectable, but after unchecking andd seeing required red color, when you check again NFSv4 then NFSv3 visually appears as selectable (the text is not greyed out):
Step 1:
Screenshot from 2021-05-04 09-20-09
Step 2:
Screenshot from 2021-05-04 09-20-20

@alfonsomthd alfonsomthd self-requested a review May 4, 2021 07:38
Copy link
Contributor

@alfonsomthd alfonsomthd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dashboard automation moved this from Ready-to-merge to Review in progress May 4, 2021
@votdev
Copy link
Member Author

votdev commented May 4, 2021

@votdev I see that before checking NFSv4, NFSv3 is greyed out to indicate not selectable, but after unchecking andd seeing required red color, when you check again NFSv4 then NFSv3 visually appears as selectable (the text is not greyed out):
Step 1:
Screenshot from 2021-05-04 09-20-09
Step 2:
Screenshot from 2021-05-04 09-20-20

I can not confirm this behavior with Firefox 88.0 (creating a new export) and Chrome 90.0.4430.93 (editing an existing export; the second window in the video).

Peek 2021-05-04 10-14

@alfonsomthd
Copy link
Contributor

I can not confirm this behavior with Firefox 88.0 (creating a new export) and Chrome 90.0.4430.93 (editing an existing export; the second window in the video).

Ceph.mp4

BTW I see in your video that "Create NFS Export" button is placed before "Cancel" button but in master is currently on reverse...

@alfonsomthd
Copy link
Contributor

@votdev Maybe the issue is related to validation logic and not related to this PR.

@votdev
Copy link
Member Author

votdev commented May 4, 2021

@votdev Maybe the issue is related to validation logic and not related to this PR.

Indeed, that's caused by the validation logic. I think we should ignore this little visual issue, otherwise some more code changes are required, and i really want to keep such changes low to do not get unexpected side effects.

Dashboard automation moved this from Review in progress to Reviewer approved May 4, 2021
@votdev votdev moved this from Reviewer approved to Ready-to-merge in Dashboard May 5, 2021
@epuertat epuertat merged commit 21ab6c5 into ceph:master May 5, 2021
Dashboard automation moved this from Ready-to-merge to Done May 5, 2021
@votdev votdev deleted the issue_49718_disable_nfs_v3 branch May 6, 2021 10:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Dashboard
  
Done
5 participants