-
Notifications
You must be signed in to change notification settings - Fork 113
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
introduce a recheck for the VF interface name #720
introduce a recheck for the VF interface name #720
Conversation
Thanks for your PR,
To skip the vendors CIs use one of:
|
cc @zeeke @adrianchiris |
632c827
to
ad0b2f7
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
Pull Request Test Coverage Report for Build 9613496848Details
💛 - Coveralls |
pkg/host/internal/sriov/sriov.go
Outdated
return err == nil, nil | ||
|
||
// try to get the interface name again and check if the name changed | ||
vfName = s.networkHelper.TryGetInterfaceName(pciAddr) |
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 problem is that TryGetInterfaceName(pciAddr)
might return eth0
value that is not unique for the system, and LinkByName('eth0')
might return a different VF.
I'm not sure this would solve the problem 🤔
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.
so the idea here was
- getInterface name
- vfLink object
- getInterface name again
and we have two options here:
if the second time it's still eth0 this means the vfLink object we did before contains the right mac address(even if the VF will change name).
if the vf change is name we will try again
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.
I think the problem is this:
suppose in the system there are two different VFs named eth0
(0000:19:00.5 and 0000:19:00.6) and we are calling VFIsReady("0000:19:00.5")
vfName = s.networkHelper.TryGetInterfaceName("0000:19:00.5")
-> eth0
vfLink, err = s.netlinkLib.LinkByName(vfName)
-> might return 0000:19:00.6
then
vfName = s.networkHelper.TryGetInterfaceName(pciAddr)
still returns eth0
and the function will return the .6 netlink reference, instead of the .5 one. WDYT?
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.
oh ok now I get your point the thing is that is not possible on a given time we can only have one interface with the same name that from the kernel lock system. some more info about that from our kernel developer
The interface name is assigned by the kernel during netdevice_register(). It is while holding the lock (rtnl_lock) and the kernel performs a name collision check.
It is not possible to have two interfaces with the same name, but when eth0 is renamed to e.g. enp12s23, another device can use eth0 again, but not at the same time (rtnl_lock). The name of the device can be changed later (e.g. with the help of udev and this is also easily done, but again there is rtnl_lock which prevents collisions in names).
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.
Oh, now it's much more clear, thanks.
So the scenario we are hitting here is a timeline like this:
- Kernel create VF for 0000:d8:00.2 and call it
eth0
- udev rule change the name of
0000:d8:00.2
aseth0 -> eno1vf0
- kernel create VF for 0000:d8:00.3 and call it
eth0
So, before your changes it may happens the race condition:
- Kernel create VF for 0000:d8:00.2 and call it
eth0
a. VFIsReady(0000:d8:00.2)
b. TryGetInterfaceName(0000:d8:00.2) -> eth0 - udev rule change the name of
0000:d8:00.2
aseth0 -> eno1vf0
- kernel create VF for 0000:d8:00.3 and call it
eth0
c. LinkByName(eth0) -> link for VF .3
And your proposing changes correctly address this case by
d. TryGetInterfaceName(0000:d8:00.2) -> eno1vf0
e. eth0
!= eno1vf0
f. repeat the PollImmediate loop
is that correct?
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.
Right!
discussed in community meeting. we should modify the way we get the Link device by relying on interface index instead of name. interface index is found under sysfs |
ad0b2f7
to
8615c37
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
/hold waiting for k8snetworkplumbingwg/sriov-network-device-plugin#569 THIS PR CONTAINS A REPLACE IN GO MODS! |
8615c37
to
980e62f
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
980e62f
to
bc4a88e
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
pkg/host/internal/network/network.go
Outdated
return netDevName | ||
} | ||
|
||
// TryGetInterfaceName tries to find the SR-IOV virtual interface index base on pci address | ||
func (n *network) TryGetInterfaceIndex(pciAddr string) int { | ||
ifIndex, err := n.dputilsLib.GetNetIndex(pciAddr) |
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.
you may get the wrong index in case of switched since implementation returns the first netdev interface index found.
in TryGetInterfaceNames , if in switchdev we return the "uplink representor" netdev. since in switchdev all representors (VF representors and uplink representor )will be found under the PF /net folder.
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.
in device plugin we also have GetPFNames()[1] which also takes switchdev into account
bc4a88e
to
70f94bc
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
Pull Request Test Coverage Report for Build 9700937218Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9699009559Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9682784028Details
💛 - Coveralls |
/hold cancel |
I switched the implementation to call the tryGetInterfaceName in the new function that way we don't need any new function in the sriov network device plugin. |
pkg/host/internal/sriov/sriov.go
Outdated
@@ -189,12 +189,13 @@ func (s *sriov) VFIsReady(pciAddr string) (netlink.Link, error) { | |||
var err error | |||
var vfLink netlink.Link | |||
err = wait.PollImmediate(time.Second, 10*time.Second, func() (bool, error) { | |||
vfName := s.networkHelper.TryGetInterfaceName(pciAddr) | |||
vfLink, err = s.netlinkLib.LinkByName(vfName) | |||
vfIndex := s.networkHelper.TryGetInterfaceIndex(pciAddr) |
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.
Do we want to explicitly handle -1 result?
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.
we didn't handle the empty interface name on TryGetInterfaceName so I was thinking it could be the same but I don't really care :)
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
pkg/host/internal/network/network.go
Outdated
return netDevName | ||
} | ||
|
||
// TryGetInterfaceName tries to find the SR-IOV virtual interface index base on pci address |
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.
nit: TryGetInterfaceIndex
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
pkg/host/internal/network/network.go
Outdated
return netDevName | ||
} | ||
|
||
// TryGetInterfaceName tries to find the SR-IOV virtual interface index base on pci address | ||
func (n *network) TryGetInterfaceIndex(pciAddr string) int { | ||
ifName := n.TryToGetVirtualInterfaceName(pciAddr) |
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.
not related to this PR but shouldnt we merge the added logic in TryToGetVirtualInterfaceName into TryGetInterfaceName ?
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.
sorry I used the wrong function here
pkg/host/internal/network/network.go
Outdated
} | ||
|
||
netDir := filepath.Join(consts.SysBusPciDevices, pciAddr, "net", ifName) | ||
if _, err := os.Lstat(netDir); err != nil { |
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.
do we really need this check ?
between here and L118 the path may no longer be valid when netdev name changes via udev right after this spot.
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.
sounds right :)
pkg/host/internal/network/network.go
Outdated
return netDevName | ||
} | ||
|
||
// TryGetInterfaceName tries to find the SR-IOV virtual interface index base on pci address | ||
func (n *network) TryGetInterfaceIndex(pciAddr string) int { |
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.
any chance to add UT for this one ?
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 both for this function and the vfIsReady function
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.
ATM we dont use this method in a "Try" manner but rather fail on error (-1 returned)
WDYT about changing this one to: GetInterfaceIndex(pciAddr string) (int, error)
?
i think its better checking error obj than checking for -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.
sure done :)
70f94bc
to
aa5e1a1
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
Pull Request Test Coverage Report for Build 9760276703Details
💛 - Coveralls |
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.
final comment from my side otherwise LGTM
aa5e1a1
to
50db112
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
Pull Request Test Coverage Report for Build 9761609394Details
💛 - Coveralls |
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.
LGTM
pkg/host/internal/network/network.go
Outdated
return netDevName | ||
} | ||
|
||
// TryGetInterfaceIndex tries to find the SR-IOV virtual interface index base on pci address |
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.
nit: please update comment :)
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 :)
pkg/host/internal/network/network.go
Outdated
indexFile := filepath.Join(vars.FilesystemRoot, consts.SysBusPciDevices, pciAddr, "net", ifName, "ifindex") | ||
ifIndex, err := os.ReadFile(indexFile) | ||
if err != nil { | ||
log.Log.Error(err, "TryGetInterfaceIndex(): failed to read ifindex file", "indexFile", indexFile) |
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.
nit: please update log msg
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 :)
pkg/host/internal/network/network.go
Outdated
|
||
intIfIndex, err := strconv.Atoi(strings.TrimSpace(string(ifIndex))) | ||
if err != nil { | ||
log.Log.Error(err, "TryGetInterfaceIndex(): failed to parse ifindex file content", "ifIndex", string(ifIndex)) |
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.
nit: please update log msg
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 :)
@@ -238,4 +238,37 @@ var _ = Describe("Network", func() { | |||
Expect(n.GetNetDevNodeGUID("0000:4b:00.3")).To(Equal("1122:3344:5566:7788")) | |||
}) | |||
}) | |||
Context("TryGetInterfaceIndex", func() { |
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.
nit: please update 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.
done :)
50db112
to
ff1d3aa
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
Pull Request Test Coverage Report for Build 9766049420Details
💛 - Coveralls |
@@ -24,6 +24,8 @@ type NetlinkLib interface { | |||
LinkSetVfPortGUID(link Link, vf int, portguid net.HardwareAddr) error | |||
// LinkByName finds a link by name and returns a pointer to the object. | |||
LinkByName(name string) (Link, error) | |||
// LinkByName finds a link by index and returns a pointer to the object. |
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.
just noticed this one :(. can you fix the typo
pkg/host/internal/network/network.go
Outdated
return netDevName | ||
} | ||
|
||
// GetInterfaceIndex tries to find the SR-IOV virtual interface index base on pci address |
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 function is not limited to SRIOV virtual interfaces as far as i can tell.
how about:
// GetInterfaceIndex returns network interface index base on pci address or error if occurred
It's possible to have a race in the VFIsReady function. vf netdevice can have a default eth0 device name and be the time we call the netlink syscall to get the device information eth0 can be a different device. this cause duplicate mac allocation on vf admin mac address Signed-off-by: Sebastian Sch <[email protected]>
ff1d3aa
to
f8ce428
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
Pull Request Test Coverage Report for Build 9774215462Details
💛 - Coveralls |
done with all the function names :P can you please take another look? |
It's possible to have a race in the VFIsReady function. vf netdevice can have a default eth0 device name and be the time we call the netlink syscall to get the device information eth0 can be a different device.
this cause duplicate mac allocation on vf admin mac address