fix(provisioning): make volumes with fsType=zfs datasets of type filesystem

[cruwe]

Why is this PR required? What issue does it fix?:

fixes #143 -> inconsistent naming of fsType with fallback to ext4 and all datasets created as volumes

What this PR does?:
Reformulating the getter allows volumes to be created with zfs-type, which will result in a "filesystem" dataset.

Does this PR require any upgrade changes?:

I do not think so.

If the changes in this PR are manually verified, list down the scenarios covered::

Without this fix, a dataset as described in #143 is created as a volume. With the fix, it is created as a dataset of type filesystem. Conersly, with type xfs, it is still created as a dataset of type volume. I imagine my testing could have been more thorough, this is what I managed to do to with the time at hand.

Checklist:

• Fixes CreateZFSVolume does not respect fsType=zfs -> dataset #143
• PR Title follows the convention of <type>(<scope>): <subject>
• Has the change log section been updated?
• Commit has unit tests
• Commit has integration tests
• (Optional) Are upgrade changes included in this PR? If not, mention the issue/PR to track:
• (Optional) If documentation changes are required, which issue on https://github.com/openebs/openebs-docs is used to track them:

[pawanpraka1]

@cruwe thanks for this PR, but this is a breaking change and if customers are already using fstype in the storageclass. After this change, that will stop working and it will create ZVOL instead of dataset.

The right way to handle this is make fstype case insensitive. The way kubernetes does this.

[cruwe]

Ok, I understand. So that I read correctly, you propose to make the controller accept both, fstype and fsType?

The change will, I fear, still be breaking, because customers who thought they'd get a filesystem-type dataset actually got a zvol with ext4, I believe. I only noticed because zfs share told me that it is not possible to share a zvol via NFS. I'd imagine the breakage to be smaller, but it is a change of behaviour. How would you deal witht that situation?

In any case, thank you very much for your input, I'll mark that WIP for the time being.

[pawanpraka1]

Ok, I understand. So that I read correctly, you propose to make the controller accept both, fstype and fsType?

The change will, I fear, still be breaking, because customers who thought they'd get a filesystem-type dataset actually got a zvol with ext4, I believe.

I am suggesting something like this : fetch the fstype from req.GetParameters()["fstype"] if not available then from req.GetParameters()["fsType"] or req.GetParameters()["FStype"] or req.GetParameters()["FSTYPE"]. So it will not break as old storageclass will have fstype. The intention is to make it case insensitive and user can use anything.

But I would not worry about making this in-sync with ZFSVolume CRD as it is there just for the display purpose. It serves no purpose other than that.

I would want the controller to understand all possible fstype string. But this is fine since as of now it only supports fstype string.

[cruwe]

I implemented looking up what is meant to be the fstype parameter (semantically) based on your suggestion of precedence.

I have an idea on how to solve GetInsensitiveToCase (should I rename it yo GetInsensitivelyToCase?), I do not know, however, how to deal with UTF-8 when the keys are not in the ASCII-equivalent subset. I'd rather leave that part out. How do you feel about that?

[codecov-commenter]

Codecov Report

Merging #144 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #144   +/-   ##
=======================================
  Coverage   22.90%   22.90%           
=======================================
  Files          14       14           
  Lines         489      489           
=======================================
  Hits          112      112           
  Misses        376      376           
  Partials        1        1           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3078612...b4f4b99. Read the comment docs.

[pawanpraka1]

Nice!! I like the idea of GetInsensitiveToCase. Can we rename it to GetInsensitiveParameters. Let us use this to convert all the parameters into a map of lower case keys and then use lower case keys for the lookup?

[cruwe]

Sure, changed the name. You're right, it increases readability. At worst, it might bring a smile to the inner child of some telenovela watcher, who might imagine a passionate argument btw someone and a parameter who is "just sooo insensitive". As long as we do not have a func GetInsensitiveCharacter(). As we see again, there are only two hard things in computer science: Naming and cache invalidation.

Ok, joking aside, when we want do be insensitive (no pun intended, seriously!) with all parameters, I'd suggest to create a map with case-insensitive keys and do lookups on this map to avoid getting & copying the original map the umpteenth time. I have not yet tried that out and I still need to look at the tests to see if is necessary to adapt some code there.

[pawanpraka1]

@cruwe sounds good.

[cruwe]

In this case, I will run a set of superficial tests this evening and upon successful completion, remove draft status and WIP.

[cruwe]

Hi, I've tested with some common and some perhaps not so common spelling variations, for instance, 'comPREssion', 'Dedup', 'POOLNAME' or even 'fStYpE'.

PVCs derived from this storageclass are created correctly. It is not possible to rectify this sort of creative spelling afterwards due to the storageclass parameters being immutable. That cannot be helped, I fear.

I added some additional comments to explain the reasoning to the casual reader and then squash-rebased to reduce history clutter.

Thank you very much your comments and generally, helping with developing the code. I'll now remove the WIP and draft status. Looking forward to hearing from you with the sharenfs part. Cheers!

[pawanpraka1]

@cruwe can you add a changelog also https://github.com/openebs/zfs-localpv/blob/master/CONTRIBUTING.md#adding-a-changelog. See the example here :- https://github.com/openebs/zfs-localpv/blob/master/changelogs/unreleased/121-pawanpraka1

[cruwe]

@kmova: Done. Thanks for spotting that.
@pawanpraka1: Done. Thanks for pointing out how to do it.

[pawanpraka1]

looks good.