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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FEATURE] RWX volume supports different NFS version (4.2) and mount options #7638

Closed
derekbit opened this issue Jan 12, 2024 · 12 comments
Closed
Assignees
Labels
area/volume-rwx Volume RWX related kind/feature Feature request, new feature require/auto-e2e-test Require adding/updating auto e2e test cases if they can be automated require/doc Require updating the longhorn.io documentation require/manual-test-plan Require adding/updating manual test cases if they can't be automated
Milestone

Comments

@derekbit
Copy link
Member

derekbit commented Jan 12, 2024

Is your feature request related to a problem? Please describe (馃憤 if you like this request)

Currently, RWX volume is based on NFS v4.1 protocol. Some new features such as sparse files, file pre-allocation, server-side clone and copy and application data block (ADB) are introduced in v4.2.
The feature request aims to support both v4.1 and 4.2 as well as customized mount options.

Describe the solution you'd like

Describe alternatives you've considered

Additional context

cc @james-munson

@derekbit derekbit added kind/feature Feature request, new feature require/auto-e2e-test Require adding/updating auto e2e test cases if they can be automated require/doc Require updating the longhorn.io documentation require/lep Require adding/updating enhancement proposal require/manual-test-plan Require adding/updating manual test cases if they can't be automated area/volume-rwx Volume RWX related labels Jan 12, 2024
@derekbit derekbit added this to the v1.7.0 milestone Jan 12, 2024
@james-munson
Copy link
Contributor

Varying the mount options is already an option, simply by using nfsOptions="vers=4.2,hard,etc..." in the parameters for the volume's storage class.

This seems as simple as changing the defaultOptions to use "vers=4.2" in csi/node_server.go".

Note that there is still the possibility of overriding with 4.1 in the nfsOptions if desired.

Note also that NFS backupstore, if not overridden in the settings, uses a default of nfs4.2 already, with fallback attempts at 4.1 and 4.0 if unsuccessful. But there is no such fallback path for RWX volume mounts.

@derekbit
Copy link
Member Author

I see. Can you help update the RWX doc https://longhorn.io/docs/1.6.0/advanced-resources/rwx-workloads/ for the mount options? Thank you.

@james-munson
Copy link
Contributor

james-munson commented Jan 17, 2024

Base ticket for the feature change to NFS mounts for RWX volumes is #6655

I'll look over the docs as well. I see some of that was addressed in longhorn/website#777

@james-munson
Copy link
Contributor

I created #7716 to cover the docs improvement, so this can be expedited on its own.

@longhorn-io-github-bot
Copy link

longhorn-io-github-bot commented Jan 17, 2024

Pre Ready-For-Testing Checklist

  • Where is the reproduce steps/test steps documented?
    The reproduce steps/test steps are at: n/a

  • Is there a workaround for the issue? If so, where is it documented?
    The workaround is at: n/a

  • Does the PR include the explanation for the fix or the feature? Yes

  • Does the PR include deployment change (YAML/Chart)? If so, where are the PRs for both YAML file and Chart?
    The PR for the YAML change is at: n/a
    The PR for the chart change is at: n/a

  • Have the backend code been merged (Manager, Engine, Instance Manager, BackupStore etc) (including backport-needed/*)?
    The PR is at: n/a

  • Which areas/issues this PR might have potential impacts on?
    Area: RWX volumes
    Issues

  • If labeled: require/LEP Has the Longhorn Enhancement Proposal PR submitted?
    The LEP PR is at: n/a, override for mount options already exists, this just changes the default NFS version.

  • If labeled: area/ui Has the UI issue filed or ready to be merged (including backport-needed/*)?
    The UI issue/PR is at: n/a

  • If labeled: require/doc Has the necessary document PR submitted or merged (including backport-needed/*)?
    The documentation issue/PR is at: Tracked by separate ticket https://github/longhorn/longhorn/issues/7716

  • If labeled: require/automation-e2e Has the end-to-end test plan been merged? Have QAs agreed on the automation test case? If only test case skeleton w/o implementation, have you created an implementation issue (including backport-needed/*)
    The automation skeleton PR is at: n/a
    The automation test case PR is at: n/a
    The issue of automation test case implementation is at (please create by the template)

  • If labeled: require/automation-engine Has the engine integration test been merged (including backport-needed/*)?
    The engine automation PR is at: n/a

  • If labeled: require/manual-test-plan Has the manual test plan been documented?
    The updated manual test plan is at

  • If the fix introduces the code for backward compatibility Has a separate issue been filed with the label release/obsolete-compatibility?
    The compatibility issue is filed at: n/a

@james-munson james-munson removed the require/lep Require adding/updating enhancement proposal label Jan 17, 2024
@james-munson
Copy link
Contributor

This is reasonable to backport. All current releases use nfs-ganesha that supports v4.2.

@innobead innobead modified the milestones: v1.7.0, v1.6.0 Jan 18, 2024
@derekbit
Copy link
Member Author

@james-munson
Can you test v4.2 by a different StorageClass as well in tests? Thank you.

https://github.com/longhorn/longhorn-tests/blob/master/manager/integration/tests/test_rwx.py

@james-munson
Copy link
Contributor

I do not understand what is being asked for here. Guessing that it is to add regression test cases with specific overrides.

@derekbit
Copy link
Member Author

I do not understand what is being asked for here. Guessing that it is to add regression test cases with specific overrides.

If you check https://github.com/longhorn/longhorn-tests/blob/master/manager/integration/tests/test_rwx.py, you can find it only test NFS v4.1. You can add test v4.2 as well by a StorageClass with different mount options.

@innobead
Copy link
Member

@james-munson Please do what @derekbit suggested. Need to have e2e tests in 4.2 as well.

@james-munson
Copy link
Contributor

james-munson commented Jan 19, 2024

In fact, it now tests 4.2 when run on 1.6.x, master, or any release for which 4.2 is the default. But I take your point that there should be some test cases that test nfsOptions overrides explicitly, for non-default NFS versions.

Assigning #7639 to myself.

@james-munson
Copy link
Contributor

james-munson commented Jan 22, 2024

This issue has been superseded by #7741 which reverts the default to vers=4.1. It can be closed when that one is committed.

@innobead innobead closed this as not planned Won't fix, can't repro, duplicate, stale Jan 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/volume-rwx Volume RWX related kind/feature Feature request, new feature require/auto-e2e-test Require adding/updating auto e2e test cases if they can be automated require/doc Require updating the longhorn.io documentation require/manual-test-plan Require adding/updating manual test cases if they can't be automated
Projects
None yet
Development

No branches or pull requests

4 participants