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

Add linstor storage driver #1621

Open
wants to merge 50 commits into
base: main
Choose a base branch
from
Open

Add linstor storage driver #1621

wants to merge 50 commits into from

Conversation

luissimas
Copy link
Contributor

@luissimas luissimas commented Feb 1, 2025

Description

This PR introduces a new linstor storage driver to enable provisioning of volumes backed by LINSTOR, as discussed in #564.

All storage driver features are implemented except for storage buckets. Storage buckets cannot be implemented natively on LINSTOR, and the most logical approach would be to fallback to the generic implementation like it's done for all drivers other than Ceph. We didn't go in that route due to the fact that today Incus considers that any remote storage driver supports an optimized implementation of storage buckets, but that's not the case for LINSTOR ( https://github.com/lxc/incus/blob/main/internal/server/storage/backend.go#L3827).

Although most driver features are implemented, not all of them are optimized using LINSTOR native capabilities. We fallback to the generic implementation in several cases, including:

  1. Migration across different storage pools
  2. Copying volumes with snapshots
  3. Backups
  4. Refreshing volumes

Closes #564

@luissimas luissimas requested a review from stgraber as a code owner February 1, 2025 00:36
@github-actions github-actions bot added the Documentation Documentation needs updating label Feb 1, 2025
@luissimas
Copy link
Contributor Author

We expect to send integration tests validating the scenarios in the next few days. I think we'll probably need some Linstor setup on the testing environment as well.

@luissimas luissimas force-pushed the linstor branch 2 times, most recently from ea94519 to cf18bb1 Compare February 1, 2025 01:09
@stgraber
Copy link
Member

stgraber commented Feb 2, 2025

There's a bunch of static analysis failure around code styling/formatting that should be pretty easy to handle.

@luissimas luissimas force-pushed the linstor branch 4 times, most recently from 5723ea8 to 6d4dede Compare February 3, 2025 19:54
func (d *Daemon) getLinstor() (*linstor.Client, error) {
// Setup the client if it does not exist
if d.linstor == nil {
err := d.setupLinstor()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m seeing a small race condition leading to a double execution of setupLinstor:

  • getLinstor gets called until after the d.linstor == nil condition is checked
  • setupLinstor gets called before err := d.setupLinstor() gets called
  • setupLinstor gets called again by err := d.setupLinstor().

I’d put a synchronization here…

@luissimas luissimas force-pushed the linstor branch 5 times, most recently from 7c04d43 to 3d7e8c7 Compare February 5, 2025 21:22
@luissimas
Copy link
Contributor Author

Hey @stgraber just a quick update on the state of things. @winiciusallan, @bensmrs and I did a quick call about the current state of the implementation and the next steps. Here are the main takeaways:

  1. @bensmrs is working on the CI tests for the driver
  2. While looking at the tests, we've identified that the lack of support for storage buckets may be a block for merging this, at least from the test's perspective. Can you confirm that?
  3. @winiciusallan will look into how much work would it take to support storage buckets in the linstor driver
  4. Renaming volumes is most likely a block for merging. Our initial approach will be to use the volume ID in the Incus database as the Resource Definition name in LINSTOR. We'll need to check if the ID is available on all code paths that create the volumes. We also need to be aware of the 48 character limit on LINSTOR object names. @bensmrs is probably the best person to tackle this, as he is the most familiar with the Incus code base.
  5. We still need to figure out a good way to validate that all Incus nodes also run the linstor-satellite service and that the node names are consistent between Incus and LINSTOR
  6. I'll finish addressing @bensmrs's comments on this PR and then I'll check if it is possible to have multiple LINSTOR controllers managing the same set of satellites. I'm guessing that is not supported, but if it is, then we should set the controller connection string at the storage pool level rather than in a global config option

@luissimas luissimas force-pushed the linstor branch 3 times, most recently from c38398e to 120ae22 Compare February 6, 2025 11:17
@luissimas luissimas changed the title Add Linstor storage driver Add linstor storage driver Feb 6, 2025
@github-actions github-actions bot added the API Changes to the REST API label Feb 6, 2025
@luissimas luissimas marked this pull request as draft February 6, 2025 21:35
luissimas and others added 29 commits March 23, 2025 11:20
…hot volumes

Signed-off-by: Luís Simas <luis.nascimento@luizalabs.com>
Co-authored-by: Benjamin Somers <benjamin.somers@imt-atlantique.fr>
Signed-off-by: Luís Simas <luis.nascimento@luizalabs.com>
Co-authored-by: Benjamin Somers <benjamin.somers@imt-atlantique.fr>
Signed-off-by: Luís Simas <luis.nascimento@luizalabs.com>
Signed-off-by: Luís Simas <luis.nascimento@luizalabs.com>
Signed-off-by: Benjamin Somers <benjamin.somers@imt-atlantique.fr>
Signed-off-by: Luís Simas <luis.nascimento@luizalabs.com>
…ng of volumes with snapshots

Signed-off-by: Luís Simas <luis.nascimento@luizalabs.com>
Signed-off-by: Benjamin Somers <benjamin.somers@imt-atlantique.fr>
Signed-off-by: Luís Simas <luis.nascimento@luizalabs.com>
Signed-off-by: Luís Simas <luis.nascimento@luizalabs.com>
Signed-off-by: Luís Simas <luis.nascimento@luizalabs.com>
Signed-off-by: Luís Simas <luis.nascimento@luizalabs.com>
Signed-off-by: Benjamin Somers <benjamin.somers@imt-atlantique.fr>
Signed-off-by: Luís Simas <luis.nascimento@luizalabs.com>
Co-authored-by: Benjamin Somers <benjamin.somers@imt-atlantique.fr>
…r resource definitions

Signed-off-by: Luís Simas <luis.nascimento@luizalabs.com>
Co-authored-by: Benjamin Somers <benjamin.somers@imt-atlantique.fr>
…rieving device paths

Signed-off-by: Luís Simas <luis.nascimento@luizalabs.com>
…grating lxc instances

Signed-off-by: Luís Simas <luis.nascimento@luizalabs.com>
…olumes

Signed-off-by: Luís Simas <luis.nascimento@luizalabs.com>
Signed-off-by: Luís Simas <luis.nascimento@luizalabs.com>
Co-authored-by: Benjamin Somers <benjamin.somers@imt-atlantique.fr>
Signed-off-by: Benjamin Somers <benjamin.somers@imt-atlantique.fr>
…volumes

Signed-off-by: Luís Simas <luis.nascimento@luizalabs.com>
…napshots

Signed-off-by: Luís Simas <luis.nascimento@luizalabs.com>
Co-authored-by: Benjamin Somers <benjamin.somers@imt-atlantique.fr>
…umes

Signed-off-by: Luís Simas <luis.nascimento@luizalabs.com>
Signed-off-by: Luís Simas <luis.nascimento@luizalabs.com>
Co-authored-by: Benjamin Somers <benjamin.somers@imt-atlantique.fr>
Among other changes, this commit changes quota sizes to accomodate slightly bigger volumes on LINSTOR and to avoid having tolerance ranges intersect for QUOTA1 and QUOTA2.

Signed-off-by: Benjamin Somers <benjamin.somers@imt-atlantique.fr>
Co-authored-by: Luís Simas <luis.nascimento@luizalabs.com>
Signed-off-by: Benjamin Somers <benjamin.somers@imt-atlantique.fr>
Signed-off-by: Benjamin Somers <benjamin.somers@imt-atlantique.fr>
Co-authored-by: Luís Simas <luis.nascimento@luizalabs.com>
Signed-off-by: Benjamin Somers <benjamin.somers@imt-atlantique.fr>
Signed-off-by: Luís Simas <luis.nascimento@luizalabs.com>
Co-authored-by: Benjamin Somers <benjamin.somers@imt-atlantique.fr>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Changes to the REST API Documentation Documentation needs updating
Development

Successfully merging this pull request may close these issues.

Add a linstor storage driver
4 participants