NETOBSERV-1732: add logic to lkup all previous tc filters and remove them #360
Conversation
|
@msherif1234: This pull request references NETOBSERV-1732 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.17.0" version, but no target version was set. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
/ok-to-test |
openshift-ci
bot
added
the
ok-to-test
|
New image: It will expire after two weeks. To deploy this build, run from the operator repo, assuming the operator is running: USER=netobserv VERSION=29b9188 make set-agent-image |
github-actions
bot
removed
the
ok-to-test
|
/ok-to-test |
openshift-ci
bot
added
the
ok-to-test
|
New image: It will expire after two weeks. To deploy this build, run from the operator repo, assuming the operator is running: USER=netobserv VERSION=846d29a make set-agent-image |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #360 +/- ##
=======================================
Coverage ? 32.67%
=======================================
Files ? 48
Lines ? 3593
Branches ? 0
=======================================
Hits ? 1174
Misses ? 2317
Partials ? 102
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
@msherif1234 I thought it was already managed e.g. here: https://github.com/netobserv/netobserv-ebpf-agent/blob/main/pkg/ebpf/tracer.go#L308 , what's the difference exactly with the existing deletions? Is it because previously it would only delete for the current interfaces, but not if interfaces have changed in the meantime? |
current logic won't work as when the pod restart the FD value won't be the same as old program so the existing code won't ever kick in https://github.com/netobserv/netobserv-ebpf-agent/blob/main/pkg/ebpf/tracer.go#L304 |
| } | ||
|
|
||
| for _, f := range filters { | ||
| if err := netlink.FilterDel(f); err != nil { |
There was a problem hiding this comment.
not sure how likely it is to get an error here, but to be more resilient you could accumulate all errors in a slice and use errors.Join : that would avoid returning after an error in case there's still filters to delete in the list.
There was a problem hiding this comment.
I can use Aggregrate error too which does the same like errors join
pkg/ebpf/tracer.go
Outdated
| if strings.HasPrefix(bpfFilter.Name, tcEgressFilterName) { | ||
| { | ||
| egressDevs = append(egressDevs, l) | ||
| } | ||
| } |
There was a problem hiding this comment.
Nit: unnecessary {}
| if strings.HasPrefix(bpfFilter.Name, tcEgressFilterName) { | |
| { | |
| egressDevs = append(egressDevs, l) | |
| } | |
| } | |
| if strings.HasPrefix(bpfFilter.Name, tcEgressFilterName) { | |
| egressDevs = append(egressDevs, l) | |
| } |
…them Signed-off-by: Mohamed Mahmoud <mmahmoud@redhat.com>
github-actions
bot
removed
the
ok-to-test
|
/ok-to-test |
openshift-ci
bot
added
the
ok-to-test
|
New image: It will expire after two weeks. To deploy this build, run from the operator repo, assuming the operator is running: USER=netobserv VERSION=a705627 make set-agent-image |
|
/label no-qe |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: msherif1234 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 |
…them (netobserv#360) Signed-off-by: Mohamed Mahmoud <mmahmoud@redhat.com>
…them (#360) Signed-off-by: Mohamed Mahmoud <mmahmoud@redhat.com>
Description
Today if ebpf agent pod is killed with
SIGKILLthe clean up code never get a chance to remove all TC filters and qdisc installedthis PR check all previously installed TC filters and remove them
Dependencies
n/a
Checklist
If you are not familiar with our processes or don't know what to answer in the list below, let us know in a comment: the maintainers will take care of that.