Skip to content

mgr/dashboard: disable NFSv3 support in dashboard#41081

Merged
epuertat merged 1 commit intoceph:masterfrom
votdev:issue_49718_disable_nfs_v3
May 5, 2021
Merged

mgr/dashboard: disable NFSv3 support in dashboard#41081
epuertat merged 1 commit intoceph:masterfrom
votdev:issue_49718_disable_nfs_v3

Conversation

@votdev
Copy link
Member

@votdev votdev commented Apr 29, 2021 •
edited
Loading

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 requested a review from asettle April 29, 2021 11:29
@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 force-pushed the issue_49718_disable_nfs_v3 branch from cff3bdc to 8d83180 Compare April 29, 2021 16:00
@epuertat
Copy link
Member

epuertat commented Apr 29, 2021

jenkins test dashboard

epuertat
epuertat approved these changes Apr 29, 2021
View reviewed changes
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

epuertat commented Apr 29, 2021

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

@votdev
mgr/dashboard: disable NFSv3 support in dashboard
864a362 
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 •
edited
Loading

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 commented May 4, 2021

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

I'm OK with it.

@alfonsomthd
Copy link
Contributor

alfonsomthd commented May 4, 2021 •
edited
Loading

@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
alfonsomthd
alfonsomthd requested changes May 4, 2021
View reviewed changes
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.

#41081 (comment)

@votdev
Copy link
Member Author

votdev commented May 4, 2021 •
edited
Loading

@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

alfonsomthd commented May 4, 2021

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

alfonsomthd commented May 4, 2021

@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.

@epuertat epuertat merged commit 21ab6c5 into ceph:master May 5, 2021
@votdev votdev deleted the issue_49718_disable_nfs_v3 branch May 6, 2021 10:59
This was referenced May 6, 2021
Merged
Merged
@mgfritch mgfritch mentioned this pull request Feb 21, 2024
Merged
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mgfritch mgfritch mgfritch approved these changes

@asettle asettle asettle approved these changes

@alfonsomthd alfonsomthd alfonsomthd approved these changes

@epuertat epuertat epuertat approved these changes

@varshar16 varshar16 Awaiting requested review from varshar16

Assignees

No one assigned

Labels

cleanup dashboard pybind

Projects

Ceph-Dashboard
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@votdev @epuertat @alfonsomthd @mgfritch @asettle