-
Notifications
You must be signed in to change notification settings - Fork 42
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
Restore IDPF devices for renaming rules #95
Conversation
This reverts commit a528f22.
Hi @humlie. Thanks for your PR. I'm waiting for a GoogleCloudPlatform member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/ok-to-test |
/hold Code changes LGTM, we need an associated CIT test case to prevent regressions on C3 metal in the future before merge. We also need access to instances where this will run so we can write an integration test for this. |
src/usr/bin/gce-nic-naming
Outdated
@@ -21,8 +21,8 @@ readonly LOG_TAG=${LOG_TAG:-"gce-nic-naming_$$"} | |||
declare PCI_BUS_PATH='/sys/bus/pci/devices' | |||
declare SYS_PREPEND_PATH='/sys' | |||
# 0x15b3:0x101e is the vendor and device ID for Mellanox CX7 | |||
|
|||
readonly ETHERNET_DEVICES_VENDORS=('15b3:101e') | |||
# 0x8086:0x145c is the vendor and device ID for Intel IDPF |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you some text that this is the VF device ID rather than PF?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Tests that GoogleCloudPlatform/guest-configs#95 doesn't cause NIC name regressions on C3 Metal. PiperOrigin-RevId: 705914177
Tests that GoogleCloudPlatform/guest-configs#95 doesn't cause NIC name regressions on C3 Metal. PiperOrigin-RevId: 705914177
Tests that GoogleCloudPlatform/guest-configs#95 doesn't cause NIC name regressions on C3 Metal. PiperOrigin-RevId: 705992843
src/usr/bin/gce-nic-naming
Outdated
name_builder="rdma${eth_index}" | ||
|
||
if [[ "${SUBSYSTEM}" == "net" ]] && [[ ! -d "${SYS_PREPEND_PATH}${DEVPATH}/device/${PCI_TEST_ITEM}" ]]; then | ||
# If this is a VF device and not an RDMA the race condition is addressed by naming to position based name. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain the race condition, for readers that aren't google internal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
src/usr/bin/gce-nic-naming
Outdated
generate_name "${DEVPATH}" | ||
generated_name=$(generate_name "${DEVPATH}") | ||
|
||
# handle the 2nd load order |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- net
- irdma
right? Can you say this explicitly? Assumptions about load order should be documented for anyone using a custom kernel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
src/usr/bin/gce-nic-naming
Outdated
@@ -14,15 +14,19 @@ | |||
# | |||
# This script is used to generate the network device name for a given device. | |||
|
|||
|
|||
# Input to test if a directory in the PCI tree exists under the device | |||
declare PCI_TEST_ITEM="$1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is needlessly confusing IMO, it isn't actually dynamic. All the cases are static, this only serves to obscure the behavior. Can you instead check for infiniband when "${DRIVER}" == "idpf"
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The driver is not directly passed to the script, I thought this was more flexible for future cases but I can pass the driver instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: a-crate, humlie The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/lgtm |
/unhold |
4172a5c
into
GoogleCloudPlatform:master
This reverts commit a528f22.
Allows IDPF RDMA devices to be renamed at boot time.