TCX ebpf hook support #102

msherif1234 · 2024-08-13T12:23:36Z

fixes #92

define few APIs for TCX program type
add TCX operator and agent controller and ut
add TCX support to bpfapplication controller
add TCX intg test

codecov · 2024-08-13T12:27:54Z

Codecov Report

Attention: Patch coverage is 27.29084% with 365 lines in your changes missing coverage. Please review.

Project coverage is 27.60%. Comparing base (7e4a74b) to head (aa3840d).
Report is 10 commits behind head on main.

Files with missing lines	Patch %	Lines
...client/clientset/typed/apis/v1alpha1/tcxprogram.go	0.00%	97 Missing ⚠️
...entset/typed/apis/v1alpha1/fake/fake_tcxprogram.go	0.00%	63 Missing ⚠️
controllers/bpfman-agent/tcx-program.go	60.46%	43 Missing and 8 partials ⚠️
controllers/bpfman-operator/tcx-program.go	25.92%	38 Missing and 2 partials ⚠️
apis/v1alpha1/zz_generated.deepcopy.go	58.90%	28 Missing and 2 partials ⚠️
controllers/bpfman-agent/application-program.go	0.00%	27 Missing ⚠️
...lient/externalversions/apis/v1alpha1/tcxprogram.go	0.00%	21 Missing ⚠️
pkg/client/apis/v1alpha1/tcxprogram.go	0.00%	16 Missing ⚠️
cmd/bpfman-agent/main.go	0.00%	6 Missing ⚠️
cmd/bpfman-operator/main.go	0.00%	6 Missing ⚠️
... and 4 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #102      +/-   ##
==========================================
- Coverage   27.61%   27.60%   -0.02%     
==========================================
  Files          81       87       +6     
  Lines        6999     7499     +500     
==========================================
+ Hits         1933     2070     +137     
- Misses       4877     5228     +351     
- Partials      189      201      +12

Flag	Coverage Δ
unittests	`27.60% <27.29%> (-0.02%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

anfredette

Looks good except for the couple of comments. I think we should consider adding a priority option even if we don't support it right away. I like the idea of using labels to define an order, but I just don't see how it would work for tcx programs included in a BpfApplication CRD.

apis/v1alpha1/tcxProgram_types.go

mergify · 2024-08-20T13:06:45Z

@msherif1234, this pull request is now in conflict and requires a rebase.

config/samples/bpfman.io_v1alpha1_tcx_pass_tcprogram.yaml

This PR introduces support for TCX based on the current implementation in the astoycos Aya TCX branch. While minor API changes are anticipated before Aya TCX merges, these are expected to be cosmetic and should not significantly affect bpfman. The PR implements a priority-based API, similar to the existing TC API, with a key difference: TCX does not support "proceed_on" configuration as it's not available in the kernel API. Flow continuation is determined by the return value of the current program. It's important to note that neither the Aya nor the kernel APIs directly support priority. They utilize a relative positioning system, requiring new TCX programs to be placed relative to existing ones. To maintain usability across multiple independent applications, we’ve preserved a TC-like priority API in bpfman, which internally maps to Aya’s relative-ordering API. This PR also adds an integration test and example programs for TCX, which have been successfully tested with Mohammed’s TCX bpfman-operator PR (bpfman/bpfman-operator#102). Additionally, due to TCX's requirement for kernel version 6.6 or higher, GitHub Actions have been updated to use ubuntu-24.04. Signed-off-by: Andre Fredette <[email protected]>

Signed-off-by: Mohamed Mahmoud <[email protected]>

msherif1234 · 2024-10-22T10:27:28Z

intg test seems to broken because of bpfman/bpfman#1324

anfredette · 2024-10-22T13:22:13Z

It definitely was, but I think I backed out the changes that were causing the issue. I could be wrong, but I suspect that it's now failing because bpfman/bpfman#1222 added tcx to the app counter example.

…

On Tue, Oct 22, 2024 at 6:27 AM Mohamed S. Mahmoud ***@***.***> wrote: intg test seems to broken because of bpfman/bpfman#1324 <bpfman/bpfman#1324> — Reply to this email directly, view it on GitHub <#102 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHY7XZB5NCWF57JVHRW2S3Z4YSCNAVCNFSM6AAAAABMOFAXMOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMRYHEYDEMZXHA> . You are receiving this because you commented.Message ID: ***@***.***>

Signed-off-by: Mohamed Mahmoud <[email protected]>

anfredette · 2024-10-23T13:01:51Z

test/integration/tcx_test.go

+
+	pods, err := env.Cluster().Client().CoreV1().Pods(tcxGoCounterUserspaceNs).List(ctx, metav1.ListOptions{LabelSelector: "name=go-tcx-counter"})
+	require.NoError(t, err)
+	gotcCounterPod := pods.Items[0]


Should gotcCounterPod be gotcxCounterPod?

anfredette · 2024-10-23T13:02:17Z

test/integration/tcx_test.go

+		require.NoError(t, err)
+		t.Logf("counter pod log %s", output.String())
+
+		return doTcCheck(t, output)


doTcxCheck?

It seems like we need a doTcxCheck() function. This is probably working because the tc test is also running and there are logs from that test.

I reused the Tc check since both look for the same string might be better to have one for tcx will do

Doesn't the TCX program have "TCX" in the log instead of "TC"?

Actually, I just looked, and it doesn't. I think it should, though. That's probably another reason I changed the TC, TCX, and XDP logs in the app counter example.

We can do that in a separate pr, though.

Or not. They're all in separate pods, so I guess it's clear. Sorry for the stream of consciousness as I remember how all this stuff works :-).

anfredette · 2024-10-23T13:08:12Z

apis/v1alpha1/tcxProgram_types.go

+// +kubebuilder:printcolumn:name="Status",type=string,JSONPath=`.status.conditions[0].reason`
+// +kubebuilder:printcolumn:name="Direction",type=string,JSONPath=`.spec.direction`,priority=1
+// +kubebuilder:printcolumn:name="InterfaceSelector",type=string,JSONPath=`.spec.interfaceselector`,priority=1
+// +kubebuilder:printcolumn:name="Position",type=string,JSONPath=`.spec.position`,priority=1


Include the following as in Tc:
// +kubebuilder:printcolumn:name="Priority",type=string,JSONPath=.spec.priority,priority=1

ok I had b4 we added pri to tcx will include

controllers/bpfman-operator/tcx-program.go

anfredette

Mostly a few nits. I think the only real issue is the doTcxCheck() comment. Otherwise, this is looking good.

Signed-off-by: Mohamed Mahmoud <[email protected]>

msherif1234 · 2024-10-23T13:43:26Z

Mostly a few nits. I think the only real issue is the doTcxCheck() comment. Otherwise, this is looking good.
This isn't an issue since both log the same strings but I created one for tcx check to be more specific

anfredette

/LGTM

chore(deps): update konflux references

anfredette requested changes Aug 13, 2024

View reviewed changes

apis/v1alpha1/tcxProgram_types.go Outdated Show resolved Hide resolved

apis/v1alpha1/tcxProgram_types.go Show resolved Hide resolved

msherif1234 force-pushed the tcx branch from bf67bcb to d2cc56e Compare August 13, 2024 14:48

msherif1234 requested a review from anfredette August 13, 2024 14:48

msherif1234 marked this pull request as draft August 13, 2024 22:08

msherif1234 force-pushed the tcx branch 3 times, most recently from 7fd6718 to 3a850b2 Compare August 14, 2024 11:22

msherif1234 changed the title ~~WIP: initial APIs for new TCX ebpf hook~~ WIP: TCX ebpf hook support Aug 14, 2024

msherif1234 force-pushed the tcx branch 3 times, most recently from 4290d9a to d0889d6 Compare August 14, 2024 19:49

mergify bot added the needs-rebase label Aug 20, 2024

msherif1234 force-pushed the tcx branch from d0889d6 to 7bd79b8 Compare August 20, 2024 14:20

mergify bot removed the needs-rebase label Aug 20, 2024

msherif1234 force-pushed the tcx branch 7 times, most recently from 89fc221 to 9e361a8 Compare August 22, 2024 00:21

anfredette reviewed Aug 27, 2024

View reviewed changes

config/samples/bpfman.io_v1alpha1_tcx_pass_tcprogram.yaml Outdated Show resolved Hide resolved

This was referenced Aug 28, 2024

Add Support for TCX Programs bpfman/bpfman#1222

Merged

Revert automatic bpfman image loading into KIND cluster #113

Merged

msherif1234 force-pushed the tcx branch from 9e361a8 to 57f5846 Compare August 29, 2024 20:11

msherif1234 force-pushed the tcx branch from 57f5846 to a2e91dd Compare September 8, 2024 23:29

msherif1234 force-pushed the tcx branch from a2e91dd to db9f809 Compare October 8, 2024 20:34

msherif1234 force-pushed the tcx branch from db9f809 to 3619a12 Compare October 11, 2024 18:23

msherif1234 added 4 commits October 21, 2024 07:50

WIP: initial APIS for TCX ebpf program types

d28de93

Signed-off-by: Mohamed Mahmoud <[email protected]>

Add controllers for TCX

0d7718c

Signed-off-by: Mohamed Mahmoud <[email protected]>

Intg with bpfman tcx PR

9af7dbf

Signed-off-by: Mohamed Mahmoud <[email protected]>

Add TCX example to bundle csv

4ad964d

Signed-off-by: Mohamed Mahmoud <[email protected]>

msherif1234 force-pushed the tcx branch from 3619a12 to 4ad964d Compare October 21, 2024 11:52

msherif1234 changed the title ~~WIP: TCX ebpf hook support~~ TCX ebpf hook support Oct 21, 2024

msherif1234 marked this pull request as ready for review October 21, 2024 21:40

msherif1234 requested a review from anfredette October 21, 2024 21:42

msherif1234 force-pushed the tcx branch from b842318 to 9900085 Compare October 22, 2024 14:10

update bpfman dependency to pick TCX support

c96f816

Signed-off-by: Mohamed Mahmoud <[email protected]>

msherif1234 force-pushed the tcx branch from 9900085 to c96f816 Compare October 22, 2024 17:57

msherif1234 added 2 commits October 22, 2024 16:39

fix intg test for application object

5c35e17

Signed-off-by: Mohamed Mahmoud <[email protected]>

change GH workflows from ubuntu latest to ubuntu-24.04

875131f

Signed-off-by: Mohamed Mahmoud <[email protected]>

anfredette reviewed Oct 23, 2024

View reviewed changes

controllers/bpfman-operator/tcx-program.go Outdated Show resolved Hide resolved

anfredette requested changes Oct 23, 2024

View reviewed changes

Address review comments

aa3840d

Signed-off-by: Mohamed Mahmoud <[email protected]>

msherif1234 requested a review from anfredette October 23, 2024 13:42

anfredette approved these changes Oct 23, 2024

View reviewed changes

mergify bot merged commit 0c0ba31 into bpfman:main Oct 23, 2024
16 checks passed

msherif1234 pushed a commit to msherif1234/bpfman-operator that referenced this pull request Nov 19, 2024

Merge pull request bpfman#102 from openshift/konflux/references/main

9305273

chore(deps): update konflux references

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TCX ebpf hook support #102

TCX ebpf hook support #102

msherif1234 commented Aug 13, 2024 •

edited

Loading

codecov bot commented Aug 13, 2024 •

edited

Loading

anfredette left a comment

mergify bot commented Aug 20, 2024

msherif1234 commented Oct 22, 2024

anfredette commented Oct 22, 2024 via email

anfredette Oct 23, 2024

msherif1234 Oct 23, 2024

anfredette Oct 23, 2024

anfredette Oct 23, 2024

msherif1234 Oct 23, 2024

anfredette Oct 23, 2024

anfredette Oct 23, 2024

anfredette Oct 23, 2024

anfredette Oct 23, 2024

anfredette Oct 23, 2024

msherif1234 Oct 23, 2024

anfredette left a comment

msherif1234 commented Oct 23, 2024

anfredette left a comment

TCX ebpf hook support #102

TCX ebpf hook support #102

Conversation

msherif1234 commented Aug 13, 2024 • edited Loading

codecov bot commented Aug 13, 2024 • edited Loading

Codecov Report

anfredette left a comment

Choose a reason for hiding this comment

mergify bot commented Aug 20, 2024

msherif1234 commented Oct 22, 2024

anfredette commented Oct 22, 2024 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anfredette left a comment

Choose a reason for hiding this comment

msherif1234 commented Oct 23, 2024

anfredette left a comment

Choose a reason for hiding this comment

msherif1234 commented Aug 13, 2024 •

edited

Loading

codecov bot commented Aug 13, 2024 •

edited

Loading