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.

Sign up for GitHub

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

Jump to bottom

Add a note about best practice for choice of a backup store. #780

Merged
merged 1 commit into from
Nov 30, 2023
Merged

Add a note about best practice for choice of a backup store. #780

merged 1 commit into from
Nov 30, 2023

Conversation

james-munson
Copy link
Contributor

@james-munson james-munson commented Sep 22, 2023 •
edited
Loading

Proposed addition to address longhorn/longhorn#6773.
When this is acceptable, I will duplicate it to all current releases.

@james-munson james-munson requested a review from a team as a code owner September 22, 2023 21:47
@james-munson james-munson mentioned this pull request Oct 11, 2023
Closed
@james-munson james-munson requested a review from innobead October 25, 2023 23:28
jillian-maroket
jillian-maroket reviewed Oct 31, 2023
View reviewed changes
Copy link
Contributor

@jillian-maroket jillian-maroket left a comment

Choose a reason for hiding this comment

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

Let me know if the changes work.

content/docs/1.6.0/best-practices.md Outdated Show resolved Hide resolved
@netlify Netlify
Copy link

netlify bot commented Oct 31, 2023 •
edited
Loading

✅ Deploy Preview for longhornio ready!

Name Link
🔨 Latest commit ccb8862
🔍 Latest deploy log https://app.netlify.com/sites/longhornio/deploys/6567cc4708f80a00086a8041
😎 Deploy Preview https://deploy-preview-780--longhornio.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

jillian-maroket
jillian-maroket previously approved these changes Nov 1, 2023
View reviewed changes
Copy link
Contributor

@jillian-maroket jillian-maroket 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!

ejweber
ejweber previously approved these changes Nov 1, 2023
View reviewed changes
Copy link
Contributor

@ejweber ejweber left a comment

Choose a reason for hiding this comment

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

LGTM as is.

My soft preference would be for some context around this (especially since I'm not 100% sure myself why we're making the recommendation. I'm assuming it's primarily because our ability to get NFS to "just work" is limited, but we generally have no issues with S3?

@james-munson james-munson dismissed stale reviews from ejweber and jillian-maroket via bf3c71c November 2, 2023 17:18
@james-munson james-munson force-pushed the 6773-backup-best-practice branch from 83c1182 to bf3c71c Compare November 2, 2023 17:18
@james-munson
Copy link
Contributor Author

Agreed, @ejweber, I concede that NFS and CIFS stores are more likely to have issues with mounting and general reliability. I'm not sure whether we wanted to stress it to that degree, so I just left it as is.
I duplicated the text for all 1.4.x and 1.5.x releases as well as 1.6. That's the only change in the re-pushed branch, so if we're comfortable with the text, it can be committed.

ejweber
ejweber previously approved these changes Nov 2, 2023
View reviewed changes
@james-munson james-munson force-pushed the 6773-backup-best-practice branch from bf3c71c to 1d374d0 Compare November 2, 2023 21:08
jillian-maroket
jillian-maroket previously approved these changes Nov 3, 2023
View reviewed changes
derekbit
derekbit reviewed Nov 17, 2023
View reviewed changes
content/docs/1.4.0/best-practices.md Outdated
@@ -94,7 +94,7 @@ If you're using `ext4` as the filesystem of the volume, we recommend adding a li

## Volume Maintenance

We highly recommend using the built-in backup feature of Longhorn.
Using Longhorn's built-in backup feature is highly recommended. You can save backups to an object store (such as S3 and Azure), an NFS server, or an SMB or CIFS server. Saving to an object store is preferable.
Copy link
Member

@derekbit derekbit Nov 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Saving to an object store is preferable.

It would be nice if you could comment on the advantages of using an object store.

Adding the statement to https://longhorn.io/docs/1.5.2/snapshots-and-backups/backup-and-restore/ is appreciated.

@james-munson james-munson dismissed stale reviews from jillian-maroket and ejweber via ed5a8fb November 21, 2023 19:37
@james-munson james-munson force-pushed the 6773-backup-best-practice branch from 1d374d0 to ed5a8fb Compare November 21, 2023 19:37
jillian-maroket
jillian-maroket reviewed Nov 22, 2023
View reviewed changes
Copy link
Contributor

@jillian-maroket jillian-maroket left a comment

Choose a reason for hiding this comment

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

Looks good overall. I just made a few minor changes.

content/docs/1.4.0/best-practices.md Outdated Show resolved Hide resolved
@james-munson james-munson force-pushed the 6773-backup-best-practice branch from 1737a48 to 8a46593 Compare November 27, 2023 21:17
PhanLe1010
PhanLe1010 previously approved these changes Nov 28, 2023
View reviewed changes
Copy link
Contributor

@PhanLe1010 PhanLe1010 left a comment

Choose a reason for hiding this comment

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

LGTM

innobead
innobead reviewed Nov 28, 2023
View reviewed changes
Copy link
Member

@innobead innobead left a comment

Choose a reason for hiding this comment

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

LGTM, but please resolve @derekbit feedback first before merging this PR. Thanks.

@james-munson james-munson force-pushed the 6773-backup-best-practice branch from 8a46593 to fae898a Compare November 28, 2023 17:54
@james-munson
Copy link
Contributor Author

Done, and all the commits have been squashed. This is ready for merge.

@james-munson
Add a note about best practice for choice of a backup store.
ccb8862 
Propagate to all 1.[456].x docs

Signed-off-by: James Munson <[email protected]>
@james-munson james-munson force-pushed the 6773-backup-best-practice branch from fae898a to ccb8862 Compare November 29, 2023 23:41
innobead
innobead approved these changes Nov 30, 2023
View reviewed changes
Copy link
Member

@innobead innobead left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This is best practice exactly.

However we can briefly explain why hosted dependent solutions (NFS/CIFS) is not best.

@innobead innobead merged commit 26b6dc1 into longhorn:master Nov 30, 2023
6 checks passed
@james-munson james-munson deleted the 6773-backup-best-practice branch November 30, 2023 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@derekbit derekbit derekbit left review comments

@jillian-maroket jillian-maroket jillian-maroket approved these changes

@innobead innobead innobead approved these changes

@PhanLe1010 PhanLe1010 PhanLe1010 left review comments

@ejweber ejweber Awaiting requested review from ejweber

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@james-munson @innobead @derekbit @PhanLe1010 @ejweber @jillian-maroket