lvm-localpv
lvm-localpv copied to clipboard
openebs
feat(lvm-driver): enable RAID support
Pull Request template
Please, go through these steps before you submit a PR.
Why is this PR required? What issue does it fix?:
This PR adds support for RAID options when creating StorageClasses that translate to built-in LVM2 RAID types and options. This change also opens the door to allowing more niche LVM arguments through the new lvcreateoptions parameter.
What this PR does?:
Add support for LVM2 RAID types and parameters, with sane defaults for backwards compatibility. lvm-driver now assumes that a non-specified RAID type corresponds to the previous default of linear RAID, where data is packed onto disk until it runs out of space, continuing to the next as necessary.
Does this PR require any upgrade changes?:
It should not, as it assumes linear defaults if no raid type is specified.
If the changes in this PR are manually verified, list down the scenarios covered::
Changes are tested against both unit and integration tests.
Any additional information for your reviewer? :
The original issue brought up that care should be taken when allowing extra arguments to the lvcreate command, but I didn't see a history of sanitizing the input to the CLI commands anywhere else, so I'm not sure if more care should be given to that specific option. It seems that the underlying command runner for go does not spawn it in a shell, so it might not be necessary.
I had to update the base docker image to bring in a newer version of LVM2 that supports both dm integrity and raid options. I used the first version of alpine past the previous base image version that supported the needed arguments, but let me know if you would prefer to just update to the newest version available instead.
Also, documentation needs to be updated to explain the changes.
Checklist:
- [x] Fixes #164
- [x] PR Title follows the convention of
<type>(<scope>): <subject> - [x] Has the change log section been updated?
- [x] Commit has unit tests
- [x] 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:
@nicholascioli , Thanks for raising the PR. I did see you had mentioned your approach with respect to StorageClass changes in https://github.com/openebs/lvm-localpv/issues/164 . Could you create a little more detailed design document for this please.
@nicholascioli Can you please resolve the issues pointed by shellcheck linter, in the files section. Also you would need to rebase it seems.
Sorry, it's been a busy few weeks. I'll look into this again this week.
Hey, so most of the errors / warnings from the shellcheck are from pre-existing code. Should I still fix them as part of this PR?
Also, the bdd test seems to have failed because it couldn't resize the volume? Is the filesystem allocated to the worker small? It could be that the raid test adds too many disk images which end up filling the available disk space, but that's just a guess.
I've added some docs to the design folder. Let me know if I should also add it to the general docs folder as well!
Hey, so most of the errors / warnings from the shellcheck are from pre-existing code. Should I still fix them as part of this PR?
Yes, it would be great if you can fix them as well. Thanks
Hi, @nicholascioli Can you please update us with the plan on this task? Please let us know if we can be of help. Thanks
Hi @nicholascioli I run Product Mgmt for the OpenEBS project. You've put a lot of impressive & detailed work into your PR and it received great reviews from our ENG team. Overall positive & exciting.
As part of our new 2024 Roadmap strategy, LVM-localPV is now getting a major focus in the OpenEBS product as it's being unified into the core OpenEBS platform as a key central component, rather than being managed as a separate external Data-Engine.
Additionally, LVM-LocalPV now has ~50,000+ Daily Active Users and +150,000 product installations. So it's now a very well validated part of the platform. Hence we're are unifying it into the main product.
We'd really like to work on developing your PR and integrating the design into the new unified platform. @abhilashshetty04 is a key guy on our team doing this, and he's reviewed you PR in detail.
Are you still interested and willing to support your PR? and move it forward to be a key part of the product?
We'd really love that!!
Hello @nicholascioli , I see few of the check fails with new code. Can you please check? I did a retry.
@orville-wright Interesting news, can you comment on rawfile localpv?
@jaredkipe Are you looking for anything specific info about rawfile-localPV? Please elaborate so we could help. @avishnu How do we pursue this PR further?
Let's check the feasibility to have this change merged as Beta-grade for the v4.2 release.
Is this thing stuck? Can we help?
Hey @jochenseeber yeah seems like it, we haven't really heard from the contributor in a while. It would be good to have this taken up. Thanks. Any thoughts @dsharma-dc @tiagolobocastro ?
Yes that would be very helpful indeed :+1: Might be worth looking into the status of the VG as well, I don't see any status for raid, unless it's already there. IE when something goes wrong, how does user get visibility?
This is being moved to OpenEBS 4.3 release as there has not been any progress.
There has been no further activity on this PR. Moving out-of-scope from OpenEBS v4.3.
Hey @jochenseeber Thanks for your prior contributions on LVM, are you still willing to take a stab at finishing this? This would be a good addition. Thanks!
Hey @jochenseeber Thanks for your prior contributions on LVM, are you still willing to take a stab at finishing this? This woild be a good addition. Thanks!
Still planning to do so, but not a lot of time at the moment…
Hey @jochenseeber Thanks for your prior contributions on LVM, are you still willing to take a stab at finishing this? This woild be a good addition. Thanks!
Still planning to do so, but not a lot of time at the moment…
Great! Thanks, looking forward to your changes.