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 nvme-connect-nbft.service, and minor fixes #1

Merged
merged 5 commits into from
May 24, 2023

Conversation

mwilck
Copy link
Collaborator

@mwilck mwilck commented May 23, 2023

This PR adds a service unit for starting up NBFT-defined NVMe connections after switching root (https://bugzilla.suse.com/show_bug.cgi?id=1211647), and an udev rule that avoids the later renaming of nbft${X} interfaces by any interface naming policy that the system may have in place (also bug 1211647).

Also, it adds some small bug fixes, and a convenience patch for local compilation of the SLE15-SP5 branch.

Fixes: 13b0b73 ("fabrics: Add old nbft compat commands with backwards compatibilty")
Signed-off-by: Martin Wilck <mwilck@suse.com>
@mwilck
Copy link
Collaborator Author

mwilck commented May 24, 2023

Upstream: linux-nvme#1953 ("fix conditions in nvmf-autoconnect.service" only)

@igaw
Copy link
Collaborator

igaw commented May 24, 2023

The ConditionPathExists=|! is supposed work as OR condition. See also linux-nvme#1893 (comment)

@igaw
Copy link
Collaborator

igaw commented May 24, 2023

The ConditionPathExists=|! is supposed work as OR condition. See also linux-nvme#1893 (comment)

I understood the StockOverflow article wrong. The OR condition is just the `Condition...=|' part. Fixed upstream.

@@ -0,0 +1,2 @@
# Avoid renaming nbft$X interfaces
SUBSYSTEM=="net", ACTION=="add", ENV{INTERFACE}=="nbft*", NAME:="%E{INTERFACE}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I thought the we should not use ACTION==add anymore. Instead something like ACTION!=remove so that change is also handled (can't find the reference anymore though).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's because for block devices we need to handle both add and change. I think it's different for net devices, it particular for rules that are used to change the device name, like this one. The existing rules I've seen for network device naming all use add only.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is from Lennart on this topic:

rules files should always use a logic where instead of listing positives, they should list negatives. i.e. create your SYMLINKS= props in all cases but in "remove". This is what udev's/systemd's own rules files do, and is what should be done in 3rd party files too.

and after looking through systemd's udev rules I see the mentioned pattern:

# do not edit this file, it will be overwritten on update

SUBSYSTEM!="net", GOTO="net_setup_link_end"

IMPORT{builtin}="path_id"

ACTION=="remove", GOTO="net_setup_link_end"

IMPORT{builtin}="net_setup_link"

NAME=="", ENV{ID_NET_NAME}!="", NAME="$env{ID_NET_NAME}"

LABEL="net_setup_link_end"

Copy link
Collaborator

Choose a reason for hiding this comment

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

as we running low on time, let's go with your version and fix this up later.

@igaw
Copy link
Collaborator

igaw commented May 24, 2023

The OR condition is just the `Condition...=|' part. Fixed upstream.

Could you update the commit message to include the upstream commit id. FWIW, I am using git cherry-pick -s -x for the back ports.

@igaw
Copy link
Collaborator

igaw commented May 24, 2023

Should the last to patches also be routed via upstream? Doesn't really look SUSE specific.

The unit should be started if either config.json or discovery.conf
exists. The current syntax would start it if either one does not
exist. Fix it.

Fixes: eec7634 ("fabrics: Trigger auto connect if config.json exists")
Signed-off-by: Martin Wilck <mwilck@suse.com>
(cherry picked from commit 9803d92)
This makes it less error-prone to test compilation on a local system.

Signed-off-by: Martin Wilck <mwilck@suse.com>
Create a separate unit file for connecting to NBFT-defined subsystems.
This unit is intended to be called in "post-up" scripts from network
management software if an interface defined in the HFI section of the
NBFT is brought up (L3-configured).

In simple scenarios with just one HFI, this won't be necessary because the
interface must be brought up in the initramfs already. But in multipath
scenarios, the initramfs may choose not to wait for every HFI to come up, and
thus it may be necessary to bring up the secondary connection(s) later on.

Signed-off-by: Martin Wilck <mwilck@suse.com>
In the initramfs, the interface naming is taken care of by dracut.
But there are various network-interface-naming policies in place which
may attempt to rename the interface, causing confusion and possibly
wrong interface parameters.

Add an udev rule that avoids renaming any network interface that
has been assigned a name nbft$N, which is by convention the naming
scheme that is used for NBFT device in the initramfs.

Note: The simpler 'NAME:="%k"' directive doesn't work because udev rejects
it ('Ignoring NAME="%k", as it will take no effect.'). The ":=" syntax makes
sure the interface isn't renamed any more by later rules. "INTERFACE" is set
by the kernel in the "add" uevent for a network interface.

Signed-off-by: Martin Wilck <mwilck@suse.com>
@mwilck
Copy link
Collaborator Author

mwilck commented May 24, 2023

Could you update the commit message to include the upstream commit id. FWIW, I am using git cherry-pick -s -x for the back ports.

done

@mwilck
Copy link
Collaborator Author

mwilck commented May 24, 2023

Should the last to patches also be routed via upstream? Doesn't really look SUSE specific.

I've created a draft PR: linux-nvme#1954
But I want to discuss this in the Timberland group first.

@igaw
Copy link
Collaborator

igaw commented May 24, 2023

Should the last to patches also be routed via upstream? Doesn't really look SUSE specific.

I've created a draft PR: linux-nvme#1954 But I want to discuss this in the Timberland group first.

Okay, got it. So we are going to integrate this version until we have it sorted up in upstream.

@igaw igaw merged this pull request into SLE15-SP5-next May 24, 2023
@igaw igaw deleted the SLE15-SP5-next-mw branch May 24, 2023 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants