-
Notifications
You must be signed in to change notification settings - Fork 35
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
NETOBSERV-1473: migrating netobserv-agent to TCx for new kernels #262
Conversation
@msherif1234: This pull request references NETOBSERV-1408 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 epic to target the "4.16.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. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #262 +/- ##
==========================================
- Coverage 34.04% 32.96% -1.09%
==========================================
Files 47 47
Lines 3836 3959 +123
==========================================
- Hits 1306 1305 -1
- Misses 2444 2568 +124
Partials 86 86
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@msherif1234: This pull request references NETOBSERV-1408 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 epic to target the "4.16.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. |
a931472
to
c649ab6
Compare
@msherif1234: This pull request references NETOBSERV-1408 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 epic to target the "4.16.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. |
81fe201
to
b406e82
Compare
/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=d6040fc make set-agent-image |
@msherif1234: This pull request references NETOBSERV-1408 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 epic to target the "4.16.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. |
@msherif1234: This pull request references NETOBSERV-1408 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 epic to target the "4.16.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. |
@msherif1234: This pull request references NETOBSERV-1408 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 epic to target the "4.16.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. |
@msherif1234: This pull request references NETOBSERV-1408 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 epic to target the "4.16.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. |
@msherif1234: This pull request references NETOBSERV-1408 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 epic to target the "4.16.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. |
@msherif1234: This pull request references NETOBSERV-1408 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 epic to target the "4.16.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. |
@msherif1234: This pull request references NETOBSERV-1408 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 epic to target the "4.16.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. |
@msherif1234: This pull request references NETOBSERV-1408 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 epic to target the "4.16.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. |
@msherif1234: This pull request references NETOBSERV-1408 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 epic to target the "4.16.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. |
@msherif1234: This pull request references NETOBSERV-1408 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 epic to target the "4.16.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. |
b406e82
to
838a768
Compare
3834684
to
8adf9f7
Compare
@msherif1234: This pull request references NETOBSERV-1408 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 epic to target the "4.16.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. |
8adf9f7
to
4c04b7f
Compare
/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=a8f1bf4 make set-agent-image |
ad95e88
to
e338a0d
Compare
e338a0d
to
8e67c8c
Compare
/ok-to-test |
did multiple rebase to keep with latest code, no functional changes |
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=8ce8080 make set-agent-image |
pkg/agent/agent.go
Outdated
alog.WithField("interface", iface).WithError(err). | ||
Warn("can't register flow ebpfFetcher. Ignoring") | ||
return | ||
Warn("can't attach to TCx hook flow ebpfFetcher. fall back to use legacy TC hook") |
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: I would use an Info log rather then Warn as TCx still won't be supported in many configs hence that's still a "normal" behaviour in most cases? (so it doesn't scare the users :-) )
pkg/agent/packets_agent.go
Outdated
plog.WithField("[PCA]interface", iface).WithError(err). | ||
Warn("can't register packet ebpfFetcher. Ignoring") | ||
return | ||
Warn("can't attach to TCx hook packet ebpfFetcher. fall back to use legacy TC hook") |
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.
same here
pkg/ebpf/tracer.go
Outdated
return fmt.Errorf("failed to setns to %s: %w", iface.NetNS, err) | ||
} | ||
} | ||
egrLink, err := link.AttachTCX(link.TCXOptions{ |
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 guess this function should honor the FlowFetcher
enableIngress
and enableEgress
flags, like Register
does?
Also that makes me think, as the agent can already be configured to capture only ingress or only egress via config.Direction
, the filter config.FlowFilterDirection
recently added was probably not necessary? Can we remove it? (not asking that for this PR)
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.
Also that makes me think, as the agent can already be configured to capture only ingress or only egress via config.Direction, the filter config.FlowFilterDirection recently added was probably not necessary? Can we remove it? (not asking that for this PR)
Ok forget about it .. as discussed in slack, these two settings are redundant in ACCEPT filter mode, but they are not in REJECT mode. E.g. we may capture all ingress+egress globally but filter out a small subset of flows that are ingress or egress, for example on a particular port or subnet.
@@ -727,6 +797,38 @@ func (p *PacketFetcher) Register(iface ifaces.Interface) error { | |||
return p.registerIngress(iface, ipvlan) | |||
} | |||
|
|||
func (p *PacketFetcher) AttachTCX(iface ifaces.Interface) error { |
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.
same here, to honor enableIngress/egress
Signed-off-by: Mohamed Mahmoud <[email protected]>
8e67c8c
to
b5f42f2
Compare
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.
code lgtm!
/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=bc5abad make set-agent-image |
/approve |
discussed with @Amoghrd and he is fine running QE verification post merge |
[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 |
Description
Migrate ebpf agent from TC to TCX "TC eXpress" if the kernel support it, this PR fix #230.
zgrep CONFIG_NET_XGRESS /boot/config-$(uname -r) CONFIG_NET_XGRESS=y
using
6.6.2-201.fc39.x86_64
run standalone agent code as well as e2e test and both passed with the new TCX. Waiting for OCP build with rhel9.4 to be able to resume the testing on OCP clusterpkt drop
DNS
RTT
SRIOV secondary interface
PCA
performance and scale test
on older kernel we will get this warning and fall back to the legacy TC hooks
Note: GH use
ubuntu-latest
which is currently6.2.0
so doesn't have TCX support yetDependencies
OCP with rhel9.4 kernel
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.