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

[IMPROVEMENT] Support custom options for network filesystems for backup #6608

Closed
derekbit opened this issue Aug 29, 2023 · 8 comments
Closed
Assignees
Labels
area/backup-store Remote backup store related backport/1.5.2 kind/improvement Request for improvement of existing function priority/0 Must be fixed in this release (managed by PO) require/auto-e2e-test Require adding/updating auto e2e test cases if they can be automated require/doc Require updating the longhorn.io documentation
Milestone

Comments

@derekbit
Copy link
Member

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

Longhorn supports network filesystems such as NFS 4+ and SMB/CIFS as backup stores. The hardcoded options for the network filesystems cannot be satisfied in different environments or scenarios.
The ticket aims to support custom options for the network filesystems.

Describe the solution you'd like

Describe alternatives you've considered

Additional context

@derekbit derekbit added require/auto-e2e-test Require adding/updating auto e2e test cases if they can be automated require/doc Require updating the longhorn.io documentation kind/improvement Request for improvement of existing function area/backup-store Remote backup store related labels Aug 29, 2023
@innobead innobead added this to the v1.7.0 milestone Aug 29, 2023
@innobead innobead added the priority/0 Must be fixed in this release (managed by PO) label Aug 29, 2023
@derekbit
Copy link
Member Author

derekbit commented Sep 14, 2023

@innobead
Copy link
Member

Moving to 1.6 after discussing with @james-munson

@james-munson
Copy link
Contributor

Initial Tests:

  1. With old image, add query data nfsOptions=soft,timeo=450,retrans=3 to end of NFS URL.
    Should be unaffected and mount with defaults nfsvrs=4.2,soft,timeo=300,retry=2 (Check with nfsstat -m)

    Adding to setting via UI gives error "failed to set the setting backup-store with invalid value ..." because it contains a ",".
    That happens in longhorn-manager/datastore/longhorn.go function ValidateSetting(),
    calls longhorn-manager/types/setting.go, which is too coarse in its check. It calls url.Parse(), so it could look just at the path, not the query.

  2. With old image, add query data nfsOptions=soft&nfsOptions=timeo=450&nfsOptions=retrans=3 to end of NFS URL.
    Should be unaffected and mount with defaults nfsvrs=4.2,soft,timeo=300,retry=2 (Check with nfsstat -m)

    Adding to setting via UI gives no error. Creating a backup doesn't complete, and manager pod goes into crash loop.

    If no backup is made, this is logged by a manager:

[longhorn-manager-rxk7h] time="2023-09-29T20:08:24Z" level=error msg="Failed to get backup volume config metadata from backup target" backupVolume=testvol1 controller=longhorn-backup-volume error="error getting config metadata nfs://longhorn-test-nfs-svc.default:/opt/backupstore?nfsOptions=soft&nfsOptions=timeo=450&nfsOptions=retrans=3?volume=testvol1: failed to execute: /var/lib/longhorn/engine-binaries/longhornio-longhorn-engine-master-head/longhorn [backup head nfs://longhorn-test-nfs-svc.default:/opt/backupstore?nfsOptions=soft&nfsOptions=timeo=450&nfsOptions=retrans=3?volume=testvol1], output invalid volume name parsed, got \n, stderr, time=\"2023-09-29T20:08:24Z\" level=warning msg=\"Trying reading mount point /var/lib/longhorn-backupstore-mounts/longhorn-test-nfs-svc_default/opt/backupstore to make sure it is healthy\" pkg=nfs\ntime=\"2023-09-29T20:08:24Z\" level=info msg=\"Loaded driver for nfs://longhorn-test-nfs-svc.default:/opt/backupstore\" pkg=nfs\ntime=\"2023-09-29T20:08:24Z\" level=error msg=\"invalid volume name parsed, got \"\n, error exit status 1" node=jbm-test-01-pool2-68473cb5-xmztn

That appears to be confusing the logic that adds queries for lookup of existing backups. It doesn't strip existing values before adding "?volume=..." and two "?" characters in the Query field are not legal.

@james-munson
Copy link
Contributor

james-munson commented Oct 11, 2023

This requires changes to
backupstore: Implementation.
longhorn-engine: Vendor file changes from backupstore.
longhorn-manager: Tweak to validation of backupstore target URL setting and vendor files.

It will also need changes to docs, on the settings topic.

@longhorn-io-github-bot
Copy link

longhorn-io-github-bot commented Oct 11, 2023

Pre Ready-For-Testing Checklist

  • Where is the reproduce steps/test steps documented?
    The reproduce steps/test steps are:
    In settings, with NFS backup target, add mount options to the backup target URL in the form:

?nfsOptions=soft,timeo=330,retrans=4

and save that. When the backup target is next mounted, the options given will be used for the nfs mount command for the target.
Note that if the target is already mounted, it will not be unmounted and re-mounted, so the change may not take effect immediately. It may be necessary to manually unmount the target to see them applied right away.

  • Is there a workaround for the issue? If so, where is it documented?
    None

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

  • Does the PR include deployment change (YAML/Chart)? If so, where are the PRs for both YAML file and Chart?
    N/A

  • Have the backend code been merged (Manager, Engine, Instance Manager, BackupStore etc) (including backport-needed/*)?
    The PR is at
    NFS backup target mount options聽backupstore#149
    Update vendor files for backupstore.聽longhorn-engine#937
    Tweak validation of backuptarget URL.聽longhorn-manager#2216
    Update vendor files for backupstore.聽longhorn-manager#2244

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

  • If labeled: require/LEP Has the Longhorn Enhancement Proposal PR submitted?
    N/A

  • If labeled: area/ui Has the UI issue filed or ready to be merged (including backport-needed/*)?
    N/A

  • If labeled: require/doc Has the necessary document PR submitted or merged (including backport-needed/*)?
    The documentation issue/PR is at
    6608 - Docs changes for backup target mount options.聽website#800

  • 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
    The automation test case PR is at
    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/*)?
    N/A

  • If labeled: require/manual-test-plan Has the manual test plan been documented?
    N/A

  • If the fix introduces the code for backward compatibility Has a separate issue been filed with the label release/obsolete-compatibility?
    N/A

@roger-ryao
Copy link

roger-ryao commented Nov 17, 2023

Verified on master-head 20231117

The test steps

#6608 (comment)

Result Passed

  • 1. Set up the backuptarget as either nfs://longhorn-test-nfs-svc.default:/opt/backupstore?nfsOptions=soft,timeo=330,retrans=4 or nfs://longhorn-test-nfs-svc.default:/opt/backupstore?nfsOptions=hard,sync. With these configurations, we can successfully perform volume backup and restoration.
  • 2. require/doc: While we have the require/doc label with this issue, there isn't a related PR.

@james-munson
Copy link
Contributor

Docs PR at longhorn/website#800. Thanks.

@roger-ryao
Copy link

Verified on master-head 20231204

The test steps

#6608 (comment)

Result Passed

  • 1. Set up the backuptarget as either
    nfs://longhorn-test-nfs-svc.default:/opt/backupstore?nfsOptions=soft,timeo=330,retrans=4 or
    nfs://longhorn-test-nfs-svc.default:/opt/backupstore?nfsOptions=hard,sync or
    cifs://longhorn-test-cifs-svc.default/backupstore?cifsOptions=rsize=65536,wsize=65536,soft.
    With these configurations, we can successfully perform volume backup and restoration.

  • 2. require/doc:

Ref

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/backup-store Remote backup store related backport/1.5.2 kind/improvement Request for improvement of existing function priority/0 Must be fixed in this release (managed by PO) require/auto-e2e-test Require adding/updating auto e2e test cases if they can be automated require/doc Require updating the longhorn.io documentation
Projects
None yet
Development

No branches or pull requests

5 participants