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

KB: add the article for potential risk with fstrim #54

Merged
merged 1 commit into from
Mar 7, 2024
Merged

KB: add the article for potential risk with fstrim #54

merged 1 commit into from
Mar 7, 2024

Conversation

Vicente-Cheng
Copy link
Contributor

Related issue: harvester/harvester#4988

@netlify Netlify
Copy link

netlify bot commented Jan 31, 2024 •
edited
Loading

✅ Deploy Preview for harvester-home-preview ready!

Name Link
🔨 Latest commit 0547f12
🔍 Latest deploy log https://app.netlify.com/sites/harvester-home-preview/deploys/65e89f96d44bcf000890be28
😎 Deploy Preview https://deploy-preview-54--harvester-home-preview.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.

bk201
bk201 reviewed Jan 31, 2024
View reviewed changes
kb/2024-01-30/the_potential_risk_with_fstrim.md Outdated Show resolved Hide resolved
kb/2024-01-30/the_potential_risk_with_fstrim.md Outdated Show resolved Hide resolved
kb/2024-01-30/the_potential_risk_with_fstrim.md Outdated Show resolved Hide resolved
@Vicente-Cheng Vicente-Cheng force-pushed the add-fsfrim-article branch 2 times, most recently from 6b8420c to 9b644d0 Compare January 31, 2024 07:27
@Vicente-Cheng Vicente-Cheng requested a review from bk201 January 31, 2024 07:28
@Vicente-Cheng Vicente-Cheng mentioned this pull request Jan 31, 2024
Closed
ejweber
ejweber reviewed Jan 31, 2024
View reviewed changes
Copy link
Contributor

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

I see that a discussion about the discard mount option was removed from this PR. IMO, it is very important to include it. When I opened the original issues, we found that the Ubuntu VM images were defaulting to explicitly include the discard option, and that was the reason the user was having problems.

longhorn/longhorn#7103 (comment)

@Vicente-Cheng
Copy link
Contributor Author

I see that a discussion about the discard mount option was removed from this PR. IMO, it is very important to include it. When I opened the original issues, we found that the Ubuntu VM images were defaulting to explicitly include the discard option, and that was the reason the user was having problems.

longhorn/longhorn#7103 (comment)

Thanks @ejweber,

Yes, you are right! Thanks for the reminder.
We still need to take care of some root filesystems with the discard option.

To prevent fstrim, we need to remove this option and make it work by remounting or rebooting.

I updated the article, please help to recheck it!

jillian-maroket
jillian-maroket reviewed Feb 2, 2024
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.

@Vicente-Cheng Some parts were a little unclear to me so let me know if I misunderstood anything.

kb/2024-01-30/the_potential_risk_with_fstrim.md Outdated Show resolved Hide resolved
kb/2024-01-30/the_potential_risk_with_fstrim.md Outdated Show resolved Hide resolved
kb/2024-01-30/the_potential_risk_with_fstrim.md Outdated Show resolved Hide resolved
kb/2024-01-30/the_potential_risk_with_fstrim.md Outdated Show resolved Hide resolved
kb/2024-01-30/the_potential_risk_with_fstrim.md Outdated Show resolved Hide resolved
kb/2024-01-30/the_potential_risk_with_fstrim.md Outdated Show resolved Hide resolved
kb/2024-01-30/the_potential_risk_with_fstrim.md Outdated Show resolved Hide resolved
kb/2024-01-30/the_potential_risk_with_fstrim.md Outdated Show resolved Hide resolved
kb/2024-01-30/the_potential_risk_with_fstrim.md Outdated Show resolved Hide resolved
bk201
bk201 approved these changes Mar 5, 2024
View reviewed changes
Copy link
Member

@bk201 bk201 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 for consistency and we upper case the first letter of words in the title:

截圖 2024-03-05 上午11 19 24

@Vicente-Cheng
Copy link
Contributor Author

lgtm, but for consistency and we upper case the first letter of words in the title:

截圖 2024-03-05 上午11 19 24

updated! thanks for the reminder

jillian-maroket
jillian-maroket approved these changes Mar 5, 2024
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.

@Vicente-Cheng LGTM overall. Just please apply the suggested wording changes (title and new info).

kb/2024-01-30/the_potential_risk_with_fstrim.md Outdated Show resolved Hide resolved
kb/2024-01-30/the_potential_risk_with_fstrim.md Outdated Show resolved Hide resolved
@Vicente-Cheng
Copy link
Contributor Author

@Vicente-Cheng LGTM overall. Just please apply the suggested wording changes (title and new info).

Hi @jillian-maroket,
Thanks for the explanation, updated to the latest!

@Vicente-Cheng
Copy link
Contributor Author

Hi @ejweber,
Could you help to recheck this topic?
Thanks!

ejweber
ejweber approved these changes Mar 6, 2024
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.

Everything looks correct to me!

One nit, but it shouldn't block merging if you are ready. The fstrim operation will no longer dangerous like this in Longhorn in v1.7.0, v1.6.1, v1.5.4. I know Harvester is not using any of these versions yet, but it may be a good idea to mention. Otherwise, maybe it makes sense to create a Harvester issue to update this doc when Harvester is using one of those versions.

@Vicente-Cheng @bk201
@ejweber @jillian-maroket
KB: add the article for potential risk with fstrim
0547f12 
    - Also mentioned how to avoid this risk.

Signed-off-by: Vicente Cheng <vicente.cheng@suse.com>
Co-authored-by: Kiefer Chang <kiefer.chang@suse.com>
Co-authored-by: Eric Weber <eric.weber@suse.com>
Co-authored-by: Jillian <cjmaroket@outlook.com>
@Vicente-Cheng
Copy link
Contributor Author

Everything looks correct to me!

One nit, but it shouldn't block merging if you are ready. The fstrim operation will no longer dangerous like this in Longhorn in v1.7.0, v1.6.1, v1.5.4. I know Harvester is not using any of these versions yet, but it may be a good idea to mention. Otherwise, maybe it makes sense to create a Harvester issue to update this doc when Harvester is using one of those versions.

Thanks @ejweber. Nice suggestion.
I already update the Longhorn version on this KB.
Once Harvester bumps these longhorn versions. I will update too!

ejweber
ejweber approved these changes Mar 6, 2024
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.

@Vicente-Cheng Vicente-Cheng merged commit fbebad6 into harvester:main Mar 7, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

@ejweber ejweber ejweber approved these changes

@bk201 bk201 bk201 approved these changes

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

Successfully merging this pull request may close these issues.
4 participants
@Vicente-Cheng @bk201 @ejweber @jillian-maroket