Skip to content

Conversation

@wmousa
Copy link

@wmousa wmousa commented Dec 17, 2019

The patch allow to ifup/ifdown IPoIB interfaces from network-scripts
The required config is the following
DEVICE=ib0.8005 # ib0 the is IB interface
TYPE=Infiniband # the type is Infiniband
PKEY=yes # indicator that it's IPoIB interface
PKEY_ID=5 # the pkey number

@wmousa wmousa changed the title Adding support for IPoIb interfaces Adding support for IPoIB interfaces Dec 17, 2019
@lnykryn lnykryn requested a review from jamacku December 17, 2019 19:29
Copy link
Member

@jamacku jamacku left a comment

Choose a reason for hiding this comment

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

Please rename ifdown file to ifdown-ib.

net_log $"ERROR: could not add pkey ${PKEY_ID} as ${DEVICE} on dev ${PHYSDEV}"
exit 1
}
fi
Copy link
Member

Choose a reason for hiding this comment

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

Also please set spacing of this and following line to 4 spaces. Thank you.

Copy link
Author

Choose a reason for hiding this comment

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

done

The patch allow to ifup/ifdown IpoIB interfaces from network-scripts
The required config is the following
DEVICE=ib0.8005 # ib0 the it IB interface
TYPE=Infiniband # the type is Infiniband
PKEY=yes        # indicator that it's IPoIB interface
PKEY_ID=5       # the pkey number
@wmousa
Copy link
Author

wmousa commented Dec 24, 2019

Please rename ifdown file to ifdown-ib.

Done

@wmousa
Copy link
Author

wmousa commented Jan 13, 2020

@jamacku @lnykryn, anything to be done here?

@lnykryn
Copy link
Member

lnykryn commented Jan 13, 2020

We were just discussing this with @jamacku on Friday and I have a feeling that this should not be here. At least on rhel7, there is a rdma package with contains ifup-ib and ifdown-ib scripts. So my guess is that you actually want to have this logic in https://github.com/Mellanox/rdma-utils

@adrianchiris
Copy link

Mellanox/rdma-utils is a repository that is used internally in Mellanox it has a bunch of other stuff that is not needed, nor is it maintained.

ipoib configurations is standard and should not be in a vendor specific repo imo.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Adds support for IPoIB (IP over InfiniBand) interfaces to network-scripts, enabling ifup/ifdown operations for InfiniBand network devices with partition keys (PKeys).

  • Implements IPoIB interface creation and configuration logic in ifup script
  • Adds dedicated ifdown-ib script for proper IPoIB interface teardown
  • Integrates IPoIB support into main ifdown script routing logic

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
network-scripts/ifup Adds IPoIB interface detection, kernel module loading, and device creation
network-scripts/ifdown-ib New script for handling IPoIB interface deletion
network-scripts/ifdown Routes IPoIB interfaces to the dedicated ifdown-ib script

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

MATCH='^.+\.[0-9]{1,4}$'
if [[ "${DEVICE}" =~ $MATCH ]]; then
PHYSDEV=${DEVICE%.*}
fi
Copy link

Copilot AI Oct 9, 2025

Choose a reason for hiding this comment

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

The PHYSDEV extraction logic has a flaw: if the DEVICE doesn't match the regex pattern, PHYSDEV remains unset, but it's still used later in line 165. This will cause the ip link add command to fail with an empty link parameter.

Suggested change
fi
fi
if [ -z "${PHYSDEV}" ]; then
net_log $"ERROR: Could not extract physical device from DEVICE='${DEVICE}'"
exit 1
fi

Copilot uses AI. Check for mistakes.
check_device_down ${DEVICE} && { ip -o link set dev ${DEVICE} up; }
else
ip link add dev ${DEVICE} link ${PHYSDEV} type ipoib pkey ${PKEY_ID} || {
(/usr/bin/logger -p daemon.info -t ifup \$"ERROR: could not add
Copy link

Copilot AI Oct 9, 2025

Choose a reason for hiding this comment

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

The dollar sign before the double quote should not be escaped with a backslash. It should be $\" instead of \$\" to properly expand the localized string.

Suggested change
(/usr/bin/logger -p daemon.info -t ifup \$"ERROR: could not add
(/usr/bin/logger -p daemon.info -t ifup $"ERROR: could not add

Copilot uses AI. Check for mistakes.
exit 1
}
fi
exit 0
Copy link

Copilot AI Oct 9, 2025

Choose a reason for hiding this comment

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

This unconditional exit prevents the rest of the ifup script from executing. IPoIB interfaces still need IP configuration, DHCP setup, and other standard network interface initialization that occurs after this block.

Suggested change
exit 0

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants