Enhance bpf interface autodetection #9498
Conversation
marvin-tigera
added
release-note-required
|
|
| @@ -1122,87 +1131,37 @@ func (m *bpfEndpointManager) onInterfaceUpdate(update *ifaceStateUpdate) { | |||
| return | |||
| } | |||
|
|
|||
| // Should be safe without the lock since there shouldn't be any active background threads | |||
There was a problem hiding this comment.
why did you move this down here?
There was a problem hiding this comment.
I take the lock in cleanupOldAttach to get the policyIdx info from the state. Hence moved the lock in the calling function below the block where we call cleanupOldAttach
| if _, ok := m.hostIfaceToSlaveDevices[netiface.Name]; !ok { | ||
| m.hostIfaceToSlaveDevices[netiface.Name] = set.New[string]() | ||
| log.Errorf("Failed to get interface information via netlink '%s'", update.Name) | ||
| curIfaceType = IfaceTypeL3 |
There was a problem hiding this comment.
why do we default to L3?
There was a problem hiding this comment.
when we don't get the link information, I default the type to L3 and if the interface matches in dataIfacePattern, I change it to Data.
felix/dataplane/linux/bpf_ep_mgr.go
Outdated
| masterIndex int | ||
| parentIndex int |
There was a problem hiding this comment.
can the interface have both master and parent in the tree? It feels to me that that the system ifaces can have either or, but not both and it is just a naming convention. I wonder if it would make thing simple if we maintained only a parent for both in our internal tree. Or maybe I am confused since there is just one parent pointer.
There was a problem hiding this comment.
master and parent index are treated differently. With the refactoring I just have the masterIndex. This is used only for cases when the interface moves out of a bond. The interface will be a child of the bond and when it moves out, it needs to be added as a standalone interface.
felix/dataplane/linux/bpf_ep_mgr.go
Outdated
|
|
||
| if attrs.MasterIndex == 0 && attrs.ParentIndex == 0 { | ||
| eintf := m.findHostIfaceByIndex(attrs.Index) | ||
| if eintf != nil { |
There was a problem hiding this comment.
so a device with this index already exists. If you think it is the same device, why would you rewrite its name etc, but not the parent. children etc?
There was a problem hiding this comment.
I just rewrite the name so that I don't overwrite the children information. For example, when felix gets the child information first, we create the child node and also the parent node with master index. Now when felix gets the update of the parent interface, the interface with that index is already there in the tree. Just need to update the name.
felix/dataplane/linux/bpf_ep_mgr.go
Outdated
| @@ -305,6 +314,7 @@ type bpfEndpointManager struct { | |||
| ifStateMap *cachingmap.CachingMap[ifstate.Key, ifstate.Value] | |||
| removeOldJumps bool | |||
| legacyCleanUp bool | |||
| hostIfaces map[int]*bpfHostIface | |||
There was a problem hiding this comment.
it seems that hostIfaces are the roots of the trees, but then you use findHostIfaceByIndex that is returning ifaces that are not in this map, but within the trees. It is quite confusing.
What if this map was called hepIfaces or hostIfaceTrees or whatever would show that it is not just a plain interface
There was a problem hiding this comment.
changed it to hostIfaceTrees and introduced a new type bpfIfaceTrees
felix/dataplane/linux/bpf_ep_mgr.go
Outdated
| link, err := m.dp.getIfaceLink(masterIfName) | ||
| if err != nil { | ||
| log.Errorf("Error getting link for interface %s, err = %v", masterIfName, err) | ||
| return slaveDevices |
There was a problem hiding this comment.
should this be like a nil or something?
There was a problem hiding this comment.
Have changed to return an empty slice of string.
felix/dataplane/linux/bpf_ep_mgr.go
Outdated
| m.hostIfaces[attrs.MasterIndex] = masterIface | ||
| } | ||
| } | ||
| delete(m.hostIfaces, attrs.Index) |
There was a problem hiding this comment.
why are you deleting it if you just added it and it could be the new root?
There was a problem hiding this comment.
When adding an interface with masterIndex !=0 (which means the intf is a slave intf), we need to delete the interface from the tree before adding it because the interface might have moved from standalone to a bond. Hence the deletion.
| for _, child := range hostIf.children { | ||
| if child.name != "" { | ||
| return false | ||
| } | ||
| } | ||
| return true |
There was a problem hiding this comment.
| for _, child := range hostIf.children { | |
| if child.name != "" { | |
| return false | |
| } | |
| } | |
| return true | |
| return len(hostIf.children) == 0 |
perhaps?
There was a problem hiding this comment.
In our FVs, eth0 is a veth which has a parent index. Hence the parent interface will be added as a child but it wont have a name, since felix wont get the update. This results in len(hostIf.children) > 0 but its a invalid child. Hence I did not do this way.
| if val.masterIndex != 0 { | ||
| h.deleteIface(intf.name) | ||
| } else { | ||
| // We might have created this interface. |
felix/dataplane/linux/bpf_ep_mgr.go
Outdated
| // Better to delete the interface here. Now the interface is a slave interface. | ||
| // Its highly likely that it has moved from standalone to a slave. Hence delete the | ||
| // interface from the tree and add it again. |
There was a problem hiding this comment.
| // Better to delete the interface here. Now the interface is a slave interface. | |
| // Its highly likely that it has moved from standalone to a slave. Hence delete the | |
| // interface from the tree and add it again. | |
| // Now the interface is a slave interface, perhaps with a different master. So delete the interface and add it again. |
felix/dataplane/linux/bpf_ep_mgr.go
Outdated
| } | ||
| } | ||
|
|
||
| // addIfaceWithParent handles adding interface with parentIndex to the tree. |
There was a problem hiding this comment.
| // addIfaceWithParent handles adding interface with parentIndex to the tree. | |
| // addIfaceWithParent add in-tree parent of the parentIndex interface. |
sridhartigera
force-pushed
the
bpf-iface-autodetect
branch
from
6aa4eca to
464a5f4
Compare
sridhartigera
force-pushed
the
bpf-iface-autodetect
branch
from
464a5f4 to
bb0c230
Compare
Description
This PR enhances the host interface auto-detection logic for the eBPF dataplane.
with the new approach, BPF endpoint manager builds a set of trees based on the netlink information of each host interface.
For example, if we have 2 interfaces in a bond and vlan on top of the bond, vlan interface is at the root of the tree, bond is at level 1 and physical interfaces are at the leaves. In the case of single standalone interfaces, each interface is a single node tree. Attaching Tc,XDP programs follows the below logic
In the above process, XDP and Tc programs are removed from the interfaces when the interface moves down the tree.
xdp attached to physical interface and tc to bond.vlan
Related issues/PRs
Todos
Release Note
Reminder for the reviewer
Make sure that this PR has the correct labels and milestone set.
Every PR needs one
docs-*label.docs-pr-required: This change requires a change to the documentation that has not been completed yet.docs-completed: This change has all necessary documentation completed.docs-not-required: This change has no user-facing impact and requires no docs.Every PR needs one
release-note-*label.release-note-required: This PR has user-facing changes. Most PRs should have this label.release-note-not-required: This PR has no user-facing changes.Other optional labels:
cherry-pick-candidate: This PR should be cherry-picked to an earlier release. For bug fixes only.needs-operator-pr: This PR is related to install and requires a corresponding change to the operator.