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

Storage: Add Dell PowerFlex SDC operation mode #13660

Merged
merged 12 commits into from
Jul 1, 2024

Conversation

roosterfish
Copy link
Contributor

@roosterfish roosterfish commented Jun 25, 2024

Overview

This adds the SDC operation mode to the Dell PowerFlex storage driver.
To discover the presence of the SDC and to request further information from it, Dell's goscaleio library is added to LXD in order to able to import already existing utility functions. It's license is Apache-2.0.
Using this library we don't have to perform any actions against the SDC ourselves. The library is making use of the SDC's /dev/scini device using syscalls.

In addition it refactors the map/unmap functions to be generic so that both the nvme and sdc operation modes are supported.
Furthermore constants are used to identify either nvme or sdc operation mode.

An extension to the test suite is added with canonical/lxd-ci#212.

Limitation

Using the sdc mode currently is limited to images with only three partitions (e.g. ubuntu:jammy). Launching images with more than 3 partitions (e.g. ubuntu:noble) is causing the kernel error sysfs: cannot create duplicate filename '/dev/block/*:*' (triggered by SDC) which breaks the underlying volume for consumption by LXD.

@github-actions github-actions bot added the Documentation Documentation needs updating label Jun 25, 2024
Copy link

Heads up @ru-fu - the "Documentation" label was applied to this issue.

Copy link
Contributor

@ru-fu ru-fu left a comment

Choose a reason for hiding this comment

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

Just some small nitpicks.

doc/reference/storage_powerflex.md Outdated Show resolved Hide resolved
doc/reference/storage_powerflex.md Outdated Show resolved Hide resolved
doc/reference/storage_powerflex.md Outdated Show resolved Hide resolved
@roosterfish roosterfish marked this pull request as ready for review June 27, 2024 12:11
@roosterfish roosterfish marked this pull request as draft June 27, 2024 12:13
@roosterfish roosterfish force-pushed the powerflex_sdc branch 2 times, most recently from 1950d23 to d7928b1 Compare June 27, 2024 12:41
ru-fu
ru-fu previously approved these changes Jun 27, 2024
Copy link
Contributor

@ru-fu ru-fu left a comment

Choose a reason for hiding this comment

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

Thanks, the docs look good now!

@tomponline
Copy link
Member

@roosterfish shall we mark as ready for review?

@roosterfish roosterfish marked this pull request as ready for review June 27, 2024 13:51
@roosterfish
Copy link
Contributor Author

@roosterfish shall we mark as ready for review?

Ready now, I did some rebasing as one of the earlier commits contained stuff that I have removed later on. I am just rerunning the test suites for both nvme and sdc and will report here if both are done.

@roosterfish
Copy link
Contributor Author

@tomponline test suites ran successful for both PowerFlex NVMe/TCP (using default noble image) and SDC (using jammy).

Additionally I have tested SDC in LXD cluster mode, performing live migrations and having both NVMe/TCP and SDC backed LXD storage pools in parallel on the same cluster.

All stable and ready for final review now.

Copy link
Member

@tomponline tomponline left a comment

Choose a reason for hiding this comment

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

Few nits. But overall looks great, thanks!

@tomponline
Copy link
Member

needs a rebase too now

Prepare the function to support both the PowerFlex NVMe/TCP and SDC modes.
This removes the getNVMeMappedDevPath func.

Signed-off-by: Julian Pelizäus <[email protected]>
@roosterfish
Copy link
Contributor Author

I am just rerunning the test suite to double check everything.

This allows using some already existing helper functions for the PowerFlex SDC mode.

Signed-off-by: Julian Pelizäus <[email protected]>
This allows using the Dell PowerFlex SDC approach to consume volumes
from a PowerFlex storage cluster.

Signed-off-by: Julian Pelizäus <[email protected]>
The entire process of creating and deleting NVMe hosts in PowerFlex is already covered
by the the nvme lock that is held as part of mapVolume/unmapVolume.

Signed-off-by: Julian Pelizäus <[email protected]>
The subsystem's NQN contains the PowerFlex system ID not protection domain ID.
This lead the driver to always rerun the nvme-cli connect statement.

As this action is idempotent no error was observed.
But it yields a reverter including a hook to disconnect every time which can lead to problems
if the caller is running the disconnect hook (in this case getMappedDiskPath() during volume imports).

Signed-off-by: Julian Pelizäus <[email protected]>
Signed-off-by: Julian Pelizäus <[email protected]>
Signed-off-by: Julian Pelizäus <[email protected]>
Copy link
Member

@tomponline tomponline left a comment

Choose a reason for hiding this comment

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

LGTM thanks

@tomponline tomponline merged commit c148008 into canonical:main Jul 1, 2024
29 checks passed
@tomponline
Copy link
Member

@roosterfish is there any snap packaging component to this change I need to merge?

@roosterfish
Copy link
Contributor Author

@roosterfish is there any snap packaging component to this change I need to merge?

No change required, all SDC is on the host and LXD is using the /dev/scini host device to interact with it (using Dell's library).

@roosterfish roosterfish deleted the powerflex_sdc branch July 1, 2024 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Documentation needs updating
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants