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: Fix PowerFlex NVMe subsystem ENOENT #14708

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

roosterfish
Copy link
Contributor

In case no NVMe subsystem is connected, it appears that the /sys/devices/virtual/nvme-subsystem might not exist.

This can be due to a newer kernel version (saw this happen on 5.15.0-130-generic).
The fix is to check for ENOENT and only try to parse the subsystems in case the directory exists.

Looks this is already fixed in https://github.com/canonical/lxd/pull/14700/files#diff-715b401f96a8a50ff84a548172cb47b0c2ded1c49ce881ee56a2377319927a97R69.

Copy link
Member

@MusicDin MusicDin left a comment

Choose a reason for hiding this comment

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

Thanks, I should adjust that for Pure Storage as well.

There is one linter issue for pre-allocation of errorStrings slice. In addition, I just left one comment about reverting connection (although it has no impact).


if stderr != "" {
return nil, fmt.Errorf("Failed connecting to PowerFlex NVMe/TCP subsystem: %s", stderr)
revert.Add(func() { _ = d.disconnectNVMeSubsys() })
Copy link
Member

@MusicDin MusicDin Dec 20, 2024

Choose a reason for hiding this comment

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

While this reverted is currently never executed, I think we should not even try to revert connection if it fails. Instead we should rely on the caller to revert volume mapping because if there are other volumes connected it would simply disconnect them. On the other side, the unmap will take care that it gets disconnected only if there is no other volumes connected.

@tomponline
Copy link
Member

tomponline commented Dec 20, 2024

@roosterfish @MusicDin can we separate the pure driver from the changes in #14700 so we can include this in LXD (including the fix from this PR) in LXD 5.21.3 before pure lands?

@MusicDin
Copy link
Member

I've cherry-picked storage connector and PowerFlex changes from #14700 in #14710

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants