feat(lvm-driver): enable RAID support #259
Conversation
|
This also technically fixes #208 |
|
@nicholascioli , Thanks for raising the PR. I did see you had mentioned your approach with respect to StorageClass changes in #164 . Could you create a little more detailed design document for this please. |
abhilashshetty04
requested review from
abhilashshetty04,
Abhinandan-Purkait and
avishnu
|
@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. |
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. Tests have been added to cover the main supported RAID types (e.g. raid0, raid1, raid5, raid6, and raid10), but technically any valid LVM RAID type should work as well. Fixes openebs#164 Signed-off-by: Nicholas Cioli <[email protected]>
nicholascioli
force-pushed
the
164-lvm-raid
branch
from
5111243
to
32dec67
Compare
|
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! |
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. 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. |
|
Let's check the feasibility to have this change merged as Beta-grade for the v4.2 release. |
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 newlvcreateoptionsparameter.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
lvcreatecommand, 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:
<type>(<scope>): <subject>