search

openebs / lvm-localpv

Dynamically provision Stateful Persistent Node-Local Volumes & Filesystems for Kubernetes that is integrated with a backend LVM2 data storage stack.
Apache License 2.0
235 stars 92 forks source link

feat(lvm-driver): enable RAID support #259

Open nicholascioli opened 9 months ago

nicholascioli commented 9 months ago

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:

nicholascioli commented 9 months ago

This also technically fixes #208

abhilashshetty04 commented 9 months ago

@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.

Abhinandan-Purkait commented 9 months ago

@nicholascioli Can you please resolve the issues pointed by shellcheck linter, in the files section. Also you would need to rebase it seems.

nicholascioli commented 8 months ago

Sorry, it's been a busy few weeks. I'll look into this again this week.

nicholascioli commented 8 months ago

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?

nicholascioli commented 8 months ago

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.

nicholascioli commented 8 months ago

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!

Abhinandan-Purkait commented 8 months ago

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

Abhinandan-Purkait commented 4 months ago

Hi, @nicholascioli Can you please update us with the plan on this task? Please let us know if we can be of help. Thanks

orville-wright commented 3 months ago

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!!

abhilashshetty04 commented 3 months ago

Hello @nicholascioli , I see few of the check fails with new code. Can you please check? I did a retry.

jaredkipe commented 2 months ago

@orville-wright Interesting news, can you comment on rawfile localpv?