Adding support for loading TC eBPF programs on existing ingress and HTB qdisc #359
Conversation
jaysheth2
requested review from
sanfern,
charleskbliu0,
jniesz,
dthaler,
ajayR006,
dalalkaran and
pmoroney
as code owners
sanfern
changed the title
kf/kf_unix.go
Outdated
| if iface.Name == ifaceName && qdisc.Kind == "clsact" { | ||
| clsactFound = true | ||
| } | ||
| if iface.Name == ifaceName && qdisc.Kind == "htb" { | ||
| htbFound = true | ||
| htbHandle = qdisc.Msg.Handle | ||
| } | ||
| if iface.Name == ifaceName && qdisc.Kind == "ingress" { | ||
| ingressFound = true | ||
| ingressHandle = qdisc.Msg.Handle | ||
| } |
There was a problem hiding this comment.
Can we convert this into switch statement
| if iface.Name == ifaceName && qdisc.Kind == "clsact" { | |
| clsactFound = true | |
| } | |
| if iface.Name == ifaceName && qdisc.Kind == "htb" { | |
| htbFound = true | |
| htbHandle = qdisc.Msg.Handle | |
| } | |
| if iface.Name == ifaceName && qdisc.Kind == "ingress" { | |
| ingressFound = true | |
| ingressHandle = qdisc.Msg.Handle | |
| } | |
| if iface.Name == ifaceName { | |
| switch qdisc.Kind { | |
| case "clsact": | |
| clsactFound = true | |
| case "htb": | |
| htbFound = true | |
| htbHandle = qdisc.Msg.Handle | |
| case "ingress": | |
| ingressFound = true | |
| ingressHandle = qdisc.Msg.Handle | |
| default: | |
| log.Info("qdisc kind for %s : %v", ifaceName, err) | |
| } | |
| } |
There was a problem hiding this comment.
Fixed in the newer commit
kf/kf_unix.go
Outdated
| if iface.Name == ifaceName && qdisc.Kind == "clsact" { | ||
| clsactFound = true | ||
| } | ||
| if iface.Name == ifaceName && qdisc.Kind == "htb" { | ||
| htbFound = true | ||
| htbHandle = qdisc.Msg.Handle | ||
| } | ||
| if iface.Name == ifaceName && qdisc.Kind == "ingress" { | ||
| ingressFound = true | ||
| ingressHandle = qdisc.Msg.Handle | ||
| } |
There was a problem hiding this comment.
can we convert this to switch statement as above.
There was a problem hiding this comment.
Fixed in the newer commit
kf/kf_unix.go
Outdated
| } | ||
| progFD := uint32(bpfRootProg.FD()) | ||
| // Netlink attribute used in the Linux kernel | ||
| bpfFlag := uint32(tc.BpfActDirect) |
There was a problem hiding this comment.
Is it possible to relocate line 301 and 303 outside of the block in order to prevent duplicated code?
There was a problem hiding this comment.
Fixed in the newer commit
| FD: &progFD, | ||
| Flags: &bpfFlag, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Is it possible to define tc.Attribute{} outside of the block in order to prevent duplicated code?
There was a problem hiding this comment.
As tc.Attribute{} is the field of tc.Object.Attribute, I would prefer to keep the declaration in place for context.
| }, | ||
| }, | ||
| } | ||
| } else if !clsactFound && !ingressFound && !htbFound { |
There was a problem hiding this comment.
IMHO For clarity, this condition block can be relocated to the end as part of the else statement.
| ingressFound = true | ||
| ingressHandle = qdisc.Msg.Handle | ||
| default: | ||
| log.Info().Msgf("Un-supported qdisc kind for interface %s ", ifaceName) |
There was a problem hiding this comment.
Is this going to be too verbose? Or is it useful to a user who is inspecting the logs?
There was a problem hiding this comment.
I believe it will be useful for debugging on the hypervisors.
Signed-off-by: Jay Sheth <jaykumar.sheth@walmart.com>
This PR is the fix for the issue #362
Specifically, in case of Openstack Hypervisors, whenever rate-limit rules are applied (ref: https://wiki.openstack.org/wiki/InstanceResourceQuota#:~:text=quota%3Acpu_period%3D10240000-,Bandwidth,-limits), openstack install HTB and Ingress qdisc. This PR will add support for deploying TC program on the interface with such HTB and Ingress qdisc as well