Storage: Add NVMe and SDC storage connectors #14710

MusicDin · 2024-12-20T14:36:38Z

This PR contains cherry-picks from #14700 to include only connectors and PowerFlex changes (without Pure Storage bits)

The aim of the connector is to handle connections with the target storage. Currently, connectors support NVMe, and SDC.

Once PowerFlex will use Connect/Diconnect, the following connector functions can be removed:

ConnectAll
DisconnectAll
SessionID

MusicDin · 2024-12-20T14:45:38Z

As previously discussed with @roosterfish, I have also replaced nvme "disconnect-all" and "connect-all" by connecting to just a specific target. This prevents disconnecting all nvme volumes, which would affect other storage drivers as well. However, needs to be checked that NQN is correctly parsed and passed to the connector.

roosterfish

Thanks for this, looking great already.

I'll grab your branch and make some tests on the PowerFlex cluster for validation. I think the only change that has to be performed upfront is using the actual (Connect|Disconnect)All funcs to resemble the original behavior.
When we have merged the PR I can take care of switching to the Connect/Disconnect instead.

lxd/storage/connectors/connector_nvme.go

lxd/storage/drivers/driver_powerflex_utils.go

lxd/storage/drivers/driver_powerflex.go

lxd/storage/drivers/driver_powerflex_utils.go

MusicDin · 2024-12-20T18:32:30Z

@roosterfish Thanks for the comments. I've addressed all of them, and reverted fixes for ConnectAll -> Connect, and DisconnectAll -> Disconnect to retain the previous implementation.

roosterfish · 2025-01-07T08:43:33Z

I've addressed all of them, and reverted fixes

Thanks, I'll run it through some tests on the PowerFlex env and report back here.

lxd/storage/connectors/utils.go

lxd/storage/drivers/driver_powerflex_utils.go

lxd/storage/connectors/connector.go

lxd/storage/drivers/driver_powerflex_utils.go

tomponline

Nearly there, few small changes to make. Ta

tomponline · 2025-01-28T14:46:45Z

lxd/storage/connectors/utils.go

+		cancel()
+
+		// Wait until all connection attempts have finished.
+		wg.Wait()


once the wait group has finished in the go routine above, the lock is released.
Is it OK that we can call c.Disconnect below after this when potentially another caller has taken the lock and is in the process of connecting again? @roosterfish @MusicDin ?

Mh, in this situation the cleanup call to Disconnect will rip out the connection from another caller because Disconnect obtains the lock and tries to disconnect the session if found. @MusicDin please confirm this.
But it doesn't anymore have the knowledge about whether or not the connection was made on its own behalf.

Maybe a solution could be to have an atomic counter that tracks the "connection demands" for all the vols (grouped by session) and only if this counter is zero Disconnect will actually get rid of the connection?

Hmm, this one is tricky. It could happen. But how long should we keep it?

Having a counter would be a viable solution. But when do we release the initial lock/reduce the counter?

@MusicDin @roosterfish and I discussed it and we'll merge this as-is, and then @roosterfish is going to see if he can address this with his other up coming PowerFlex fixes.

That way we can unblock the Pure driver.

lxd/storage/connectors/utils.go

Signed-off-by: Din Music <[email protected]>

…or lock Signed-off-by: Din Music <[email protected]>

Signed-off-by: Din Music <[email protected]>

…name Signed-off-by: Din Music <[email protected]>

…system Signed-off-by: Din Music <[email protected]>

Previously the mapVolume returned a reverter that disconnected NVMe session in case of an error. Since mapVolume acquires a lock, calling disconnect outside of the mapVolume or unmapVolume functions may result in a race condition because another operation could establish a connection before the reverter is called. In such case, the reverter would carelessly just terminate the newly estabished connection. Therefore, the outerReverter ensures that instead of disconnecting from the volume the unmapVolume is called, which acquires the lock and ensures the disconnect is called only if the session is not in used. Co-authored-by: Julian Pelizäus <[email protected]> Co-authored-by: Din Music <[email protected]> Signed-off-by: Din Music <[email protected]>

Signed-off-by: Din Music <[email protected]>

…inter) Signed-off-by: Din Music <[email protected]>

roosterfish

Ran another bunch of tests on NVMe/SDC with VMs and containers. All good and cleaned up afterwards. LGTM!

tomponline

LGTM thanks!

MusicDin mentioned this pull request Dec 20, 2024

Storage: Fix PowerFlex NVMe subsystem ENOENT #14708

Closed

tomponline requested a review from roosterfish December 20, 2024 14:39

roosterfish requested changes Dec 20, 2024

View reviewed changes

MusicDin force-pushed the feat/storage-connector branch from 67f9bf3 to 4846572 Compare December 20, 2024 18:19

roosterfish requested changes Jan 10, 2025

View reviewed changes

lxd/storage/connectors/utils.go Outdated Show resolved Hide resolved

lxd/storage/connectors/utils.go Outdated Show resolved Hide resolved

MusicDin force-pushed the feat/storage-connector branch from 4846572 to 0e0ad2b Compare January 10, 2025 13:00

tomponline requested a review from roosterfish January 13, 2025 12:53

roosterfish reviewed Jan 15, 2025

View reviewed changes

lxd/storage/drivers/driver_powerflex_utils.go Outdated Show resolved Hide resolved

roosterfish reviewed Jan 15, 2025

View reviewed changes

lxd/storage/connectors/connector.go Outdated Show resolved Hide resolved

MusicDin force-pushed the feat/storage-connector branch 7 times, most recently from e7953f9 to 33ad86e Compare January 17, 2025 12:45

MusicDin mentioned this pull request Jan 20, 2025

Storage: Deduplicate logic between Pure Storage and PowerFlex storage drivers #14700

Closed

2 tasks

MusicDin force-pushed the feat/storage-connector branch 3 times, most recently from 520b075 to eac5ab5 Compare January 21, 2025 11:30

MusicDin marked this pull request as ready for review January 21, 2025 13:38

MusicDin mentioned this pull request Jan 21, 2025

Storage: Add Pure Storage storage driver #14599

Draft

2 tasks

MusicDin force-pushed the feat/storage-connector branch 3 times, most recently from 9388a02 to 0652fc7 Compare January 21, 2025 16:41

MusicDin marked this pull request as draft January 21, 2025 16:47

MusicDin force-pushed the feat/storage-connector branch from 0652fc7 to 4e96af0 Compare January 21, 2025 16:58

tomponline reviewed Jan 28, 2025

View reviewed changes

lxd/storage/drivers/driver_powerflex_utils.go Outdated Show resolved Hide resolved

tomponline reviewed Jan 28, 2025

View reviewed changes

MusicDin dismissed roosterfish’s stale review via 451242b January 28, 2025 13:17

MusicDin force-pushed the feat/storage-connector branch 4 times, most recently from 35c0582 to c426055 Compare January 28, 2025 14:39

tomponline reviewed Jan 28, 2025

View reviewed changes

MusicDin force-pushed the feat/storage-connector branch from c426055 to b969d1d Compare January 28, 2025 15:14

roosterfish reviewed Jan 28, 2025

View reviewed changes

lxd/storage/connectors/utils.go Outdated Show resolved Hide resolved

MusicDin added 3 commits January 29, 2025 12:47

lxd/storage/connectors: Introduce storage connector

826673a

Signed-off-by: Din Music <[email protected]>

lxd/storage/connectors: Helper functions for handling disk device paths

43aa88c

Signed-off-by: Din Music <[email protected]>

lxd/storage/connectors: Add generic connect func to handle multipath

6573a45

Signed-off-by: Din Music <[email protected]>

MusicDin force-pushed the feat/storage-connector branch from b969d1d to 7d53cf4 Compare January 29, 2025 12:57

MusicDin and others added 11 commits January 29, 2025 13:03

lxd/storage/connectors: Add NVMe/TCP connector

17c97c3

Signed-off-by: Din Music <[email protected]>

lxd/storage/connectors: Add SDC connector

b904479

Signed-off-by: Din Music <[email protected]>

lxd/storage/connectors/connector: Get versions for supported connectors

80dc791

Signed-off-by: Din Music <[email protected]>

lxd/storage/drivers/utils: Helper function to resolve server name

0184d9f

Signed-off-by: Din Music <[email protected]>

lxd/storage/drivers/utils: Helper function to acquire storage connect…

61ec719

…or lock Signed-off-by: Din Music <[email protected]>

lxd/storage/drivers/powerflex: Prevent changing powerflex.mode

89dd43a

Signed-off-by: Din Music <[email protected]>

lxd/storage/drivers/powerflex: Use shared function to resolve server …

2e6d560

…name Signed-off-by: Din Music <[email protected]>

lxd/storage/drivers/powerflex: Use connector for handling storage sub…

e4b9029

…system Signed-off-by: Din Music <[email protected]>

lxd/storage/drivers/powerflex: Pre-allocate slices (linter)

c5b9df7

Signed-off-by: Din Music <[email protected]>

lxd/storage/drivers/utils: Remove no longer used waitGone function (l…

343edef

…inter) Signed-off-by: Din Music <[email protected]>

MusicDin force-pushed the feat/storage-connector branch from 7d53cf4 to 343edef Compare January 29, 2025 13:03

roosterfish approved these changes Jan 29, 2025

View reviewed changes

tomponline approved these changes Jan 29, 2025

View reviewed changes

tomponline merged commit 5622894 into canonical:main Jan 29, 2025
28 checks passed

MusicDin deleted the feat/storage-connector branch January 29, 2025 17:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Storage: Add NVMe and SDC storage connectors #14710

Storage: Add NVMe and SDC storage connectors #14710

MusicDin commented Dec 20, 2024 •

edited

Loading

MusicDin commented Dec 20, 2024 •

edited

Loading

roosterfish left a comment •

edited

Loading

MusicDin commented Dec 20, 2024

roosterfish commented Jan 7, 2025

tomponline left a comment

tomponline Jan 28, 2025 •

edited

Loading

roosterfish Jan 28, 2025

MusicDin Jan 28, 2025

MusicDin Jan 28, 2025 •

edited

Loading

tomponline Jan 28, 2025

roosterfish left a comment

tomponline left a comment

Storage: Add NVMe and SDC storage connectors #14710

Storage: Add NVMe and SDC storage connectors #14710

Conversation

MusicDin commented Dec 20, 2024 • edited Loading

MusicDin commented Dec 20, 2024 • edited Loading

roosterfish left a comment • edited Loading

Choose a reason for hiding this comment

MusicDin commented Dec 20, 2024

roosterfish commented Jan 7, 2025

tomponline left a comment

Choose a reason for hiding this comment

tomponline Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

roosterfish Jan 28, 2025

Choose a reason for hiding this comment

MusicDin Jan 28, 2025

Choose a reason for hiding this comment

MusicDin Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

tomponline Jan 28, 2025

Choose a reason for hiding this comment

roosterfish left a comment

Choose a reason for hiding this comment

tomponline left a comment

Choose a reason for hiding this comment

MusicDin commented Dec 20, 2024 •

edited

Loading

MusicDin commented Dec 20, 2024 •

edited

Loading

roosterfish left a comment •

edited

Loading

tomponline Jan 28, 2025 •

edited

Loading

MusicDin Jan 28, 2025 •

edited

Loading