-
Notifications
You must be signed in to change notification settings - Fork 24
Convert the Priority field from int32 to *int32 across all BPF program types (TC, TCX, XDP) and their cluster variants #485
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
base: main
Are you sure you want to change the base?
Conversation
0be6ca4 to
6b87e7d
Compare
|
With this fix, applications can now have priority 0: |
6b87e7d to
25cf476
Compare
3e45d88 to
5cd629f
Compare
Convert the Priority field from int32 to *int32 across all BPF program types (TC, TCX, XDP) and their cluster variants. This change: - Removes the default value of 1000 from kubebuilder annotations - Makes the Priority field truly optional in the API - Updates all AttachInfo and AttachInfoState structs - Adds a GetPriority helper function to handle nil pointer cases - Updates all reconcilers to use the helper function - Regenerates CRDs and deepcopy code - Updates tests to use ptr.To() for priority values This allows users to omit the priority field when they want to use bpfman's default behavior, rather than always setting an explicit value. It also follows API best practices to set the default value via the controller, and not via the CRD definition. Signed-off-by: Andreas Karis <[email protected]>
Convert cluster and namespace BPF application tests to table-driven structure for better maintainability and test coverage. Extract common test functionality into reusable helper functions: - createFakeClusterReconciler() and createFakeNamespaceReconciler() for setup - runClusterReconciler() and runNamespaceReconciler() for execution - verifyClusterBpfApplicationState() and verifyNamespaceBpfApplicationState() - verifyClusterBpfProgramState() and verifyNamespaceBpfProgramState() Inline program definitions within test cases, simplify reconciler setup in GetBpfAppState tests, and add comprehensive verification of program status across multiple reconciliation cycles. Signed-off-by: Andreas Karis <[email protected]>
5cd629f to
9f3186c
Compare
Extract the NetnsCache map and getNetnsId method from ReconcilerCommon into a new NetNsCache interface with ReconcilerNetNsCache implementation. This enables proper unit testing by allowing tests to inject a MockNetNsCache instead of relying on actual filesystem operations. Changes: - Define NetNsCache interface with GetNetNsId and Reset methods - Implement ReconcilerNetNsCache with the original caching logic - Update all reconcilers to use NetNsCache.GetNetNsId() instead of ReconcilerCommon.getNetnsId() - Add MockNetNsCache for testing with predefined namespace mappings - Initialize NetNsCache in main.go and test setup Signed-off-by: Andreas Karis <[email protected]>
Add unit tests for XDP, TC, and TCX programs to verify that: - nil priority defaults to 1000 - explicit priority values are correctly set - zero priority is allowed The tests cover both cluster-scoped and namespace-scoped BPF applications, ensuring the priority field is properly handled and reflected in the program status. Signed-off-by: Andreas Karis <[email protected]>
2cb3797 to
af2a346
Compare
| func TestTcGoCounter(t *testing.T) { | ||
| t.Log("deploying tc counter program") | ||
| require.NoError(t, clusters.KustomizeDeployForCluster(ctx, env.Cluster(), tcGoCounterKustomize)) | ||
| addCleanup(func(context.Context) 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.
Note: functions added via addCleanup are run exactly once, and that at the end of all tests. The problem with that is that this will conflict when multiple tests add the same cleanup. Instead, use t.Cleanup which runs after each test. It's also probably not necessary to throw an error on cleanup, therefore don't..?
Add integration tests to verify BPF program links are correctly ordered by priority across XDP, TC, and TCX program types. The new verification framework validates link ordering on each cluster node by comparing ClusterBpfApplicationState data against actual bpfman daemon state. Signed-off-by: Andreas Karis <[email protected]>
af2a346 to
bbd6448
Compare
Summary
Convert the
Priorityfield fromint32to*int32across all BPF program types (TC, TCX, XDP) and their cluster variants.Fixes #484
Motivation
Making
Prioritya pointer allows users to set the priority to 0, or to use the default behavior. This provides better API semantics and allows bpfman to apply its own default priority logic when the field is omitted. Following OpenShift API guide best practices, the default value is now set via the controller rather than the CRD definition.Changes
Core API Changes (commit 7a20559)
API Changes: Changed
Priorityfield type fromint32to*int32in:ClTcAttachInfo/ClTcAttachInfoStateClTcxAttachInfo/ClTcxAttachInfoStateClXdpAttachInfo/ClXdpAttachInfoStateTcAttachInfo/TcAttachInfoStateTcxAttachInfo/TcxAttachInfoStateXdpAttachInfo/XdpAttachInfoStateKubebuilder Annotations: Removed
+kubebuilder:default:=1000annotations - the default is now set by the controller rather than via the API, following OpenShift API guide best practicesHelper Function: Added
GetPriority()helper inpkg/helpersto handle nil pointer cases and provide default value of 1000Controller Updates: Updated all reconcilers to use
GetPriority()helper:cl_tc_program.go,cl_tcx_program.go,cl_xdp_program.gons_tc_program.go,ns_tcx_program.go,ns_xdp_program.goGenerated Code: Updated CRDs, deepcopy functions, and CSV manifests
Tests: Updated all test files to use
ptr.To()for priority valuesTest Infrastructure Improvements (commits 7c42d03, 229e32f)
Table-Driven Tests (commit 7c42d03): Refactored BPF application tests to use table-driven pattern with reusable helper functions:
createFakeClusterReconciler()/createFakeNamespaceReconciler()for setuprunClusterReconciler()/runNamespaceReconciler()for executionverifyClusterBpfApplicationState()/verifyNamespaceBpfApplicationState()for validationverifyClusterBpfProgramState()/verifyNamespaceBpfProgramState()for status checksMockable Network Namespace Cache (commit 229e32f): Extracted NetnsCache into a mockable interface:
NetNsCacheinterface withGetNetNsId()andReset()methodsReconcilerNetNsCachewith original caching logicMockNetNsCachefor testing with predefined namespace mappingsPriority Field Testing (commit 5cd629f)
Testing