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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] persistence.removeSnapshotsDuringFilesystemTrim Helm variable is unreferenced #7909

Closed
ejweber opened this issue Feb 9, 2024 · 3 comments
Assignees
Labels
backport/1.4.5 backport/1.5.5 backport/1.6.1 kind/bug require/backport Require backport. Only used when the specific versions to backport have not been definied. require/qa-review-coverage Require QA to review coverage
Milestone

Comments

@ejweber
Copy link
Contributor

ejweber commented Feb 9, 2024

Describe the bug

https://github.com/longhorn/longhorn/pull/4919/files added two new Helm variables related to filesystem trim:

  • defaultSettings.removeSnapshotsDuringFilesystemTrim is referenced in chart/templates/default-setting.yaml.
  • persistence.removeSnapshotsDuringFilesystemTrim should probably be referenced in chart/templates/storageclass.yaml, but actually isn't referenced anywhere.

Expected behavior

Either persistence.removeSnapshotsDuringFilesystemTrim should be referenced or it should be removed.

To be honest, I'm not sure of the value it provides. It makes sense to be able to specify unmapMarkSnapChainRemoved in user-created StorageClasses, since that setting may or may not be desired for different workloads. However, if a user just wants to enable or disable the behavior by default, the default setting should suffice.

@ejweber ejweber added kind/bug require/qa-review-coverage Require QA to review coverage require/backport Require backport. Only used when the specific versions to backport have not been definied. labels Feb 9, 2024
@ejweber ejweber added this to the v1.7.0 milestone Feb 16, 2024
@ejweber ejweber self-assigned this Feb 16, 2024
@longhorn-io-github-bot
Copy link

longhorn-io-github-bot commented Feb 16, 2024

Pre Ready-For-Testing Checklist

  • Where is the reproduce steps/test steps documented?

Default case:

  1. Do not set any values.
  2. Helm install/upgrade.
  3. Check the longhorn StorageClass for unmapMarkSnapChainRemoved: ignored.

Non-default case:

  1. Set the value of persistence.removeSnapshotsDuringFilesystemTrim to enabled in `values.yaml.
  2. Helm install/upgrade.
  3. Check the longhorn StorageClass for unmapMarkSnapChainRemoved: enabled.

@ejweber
Copy link
Contributor Author

ejweber commented Feb 16, 2024

Thanks @yardenshoham for the submission. I reviewed and approved the PR, but will need another maintainer to merge it. I'll help shepherd it through the QA and backport process.

@roger-ryao
Copy link

Verified on master-head 20240417

The test steps

#7909 (comment)

Result Passed

Default case

  1. master-head
  2. v1.6.1 upgrade to master-head

Non-default case

  1. master-head
  2. v1.6.1 upgrade to master-head

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/1.4.5 backport/1.5.5 backport/1.6.1 kind/bug require/backport Require backport. Only used when the specific versions to backport have not been definied. require/qa-review-coverage Require QA to review coverage
Projects
None yet
Development

No branches or pull requests

3 participants