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

Optional space reservation (thin provisioning) #102

Merged
merged 11 commits into from
Aug 13, 2023

Conversation

Foxite
Copy link
Contributor

@Foxite Foxite commented Aug 9, 2023

Fixes #101

I'm going to test this next, but you can have a look at the code before I'm done. Has been tested.

Also I'm going to send a separate PR in which I will add the parameter to the example config in the helm chart. #103

Summary

I've added an optional parameter to the storage classes, called "reserveSpace", which is true by default. When specified as "false", any datasets created when provisioning PVs using that storage class will not have a RefReservation set.

Checklist

For Code changes

  • Categorize the PR by setting a good title and adding one of the labels:
    kind:bug, kind:enhancement, kind:documentation, kind:change, kind:breaking, kind:dependency
    as they show up in the changelog
  • PR contains the label area:provisioner
  • Link this PR to related issues
  • I have not made any changes in the charts/ directory.

@ccremer ccremer added kind:enhancement New feature or request area:provisioner labels Aug 10, 2023
@ccremer
Copy link
Owner

ccremer commented Aug 10, 2023

Thanks for your contribution!
code-wise LGTM, I like how you keep the default behaviour to avoid breaking change.

Documentation in the Readme is still missing (explain what the implication of thin-provisioning is).

To test it, you can run make test:integration (which creates a 1gb zpool in file in project dir, requires zfs on linux) and see if the properties get attached/removed correctly.

@Foxite
Copy link
Contributor Author

Foxite commented Aug 10, 2023

I have added integration tests for thin provisioning and assertions that the refreservation property is actually correct, in both cases.

To test this it was necessary to upgrade go-zfs, because the currently used version has a bug that makes it impossible to get dataset properties.

Also don't look at the individual commits too much :')

@Foxite Foxite marked this pull request as ready for review August 10, 2023 17:29
Foxite added a commit to Foxite/kubernetes-zfs-provisioner that referenced this pull request Aug 10, 2023
@Foxite
Copy link
Contributor Author

Foxite commented Aug 10, 2023

Just realized I forgot to update the Readme.

@ccremer ccremer merged commit 47825a8 into ccremer:master Aug 13, 2023
3 checks passed
ccremer pushed a commit that referenced this pull request Aug 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Thin provisioning
2 participants