-
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 <ak.karis@gmail.com>
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 <ak.karis@gmail.com>
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 <ak.karis@gmail.com>
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 <ak.karis@gmail.com>
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 <ak.karis@gmail.com>
af2a346 to
bbd6448
Compare
The PR bpfman#485 changed Priority from int32 to *int32 in both spec (AttachInfo) and status (AttachInfoState) types. However, the status field should remain int32 because the controller always resolves a concrete priority value after reconciliation. The field is annotated +required but was declared as *int32 with json:"priority,omitempty" - these are contradictory: a required field should not be a pointer with omitempty, as that allows it to be absent from the serialised output. This commit reverts only the status types back to int32 whilst keeping the spec types as *int32 (which correctly allows distinguishing between "not set" and "explicitly set to 0").
The PR bpfman#485 changed Priority from int32 to *int32 in both spec (AttachInfo) and status (AttachInfoState) types. However, the status field should remain int32 because the controller always resolves a concrete priority value after reconciliation. The field is annotated +required but was declared as *int32 with json:"priority,omitempty" - these are contradictory: a required field should not be a pointer with omitempty, as that allows it to be absent from the serialised output. This commit reverts only the status types back to int32 whilst keeping the spec types as *int32 (which correctly allows distinguishing between "not set" and "explicitly set to 0"). Signed-off-by: Andrew McDermott <amcdermo@redhat.com>
The PR bpfman#485 changed Priority from int32 to *int32 in both spec (AttachInfo) and status (AttachInfoState) types. However, the status field should remain int32 because the controller always resolves a concrete priority value after reconciliation. The field is annotated +required but was declared as *int32 with json:"priority,omitempty" - these are contradictory: a required field should not be a pointer with omitempty, as that allows it to be absent from the serialised output. This commit reverts only the status types back to int32 whilst keeping the spec types as *int32 (which correctly allows distinguishing between "not set" and "explicitly set to 0"). Signed-off-by: Andrew McDermott <amcdermo@redhat.com>
frobware
left a comment
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.
Should status be a pointer too? The spec field correctly uses *int32 to distinguish "not set" from "zero value", but I think the status field should remain int32 because:
- Semantic mismatch: Status represents what was actually applied, not user intent. The controller always resolves a concrete priority value (either from spec or the default 1000), so status is never "unset".
- Annotation contradiction: The status Priority field is marked +required but uses *int32 with omitempty. This is contradictory - omitempty allows the field to be absent in JSON, but +required says it must always be present. For required fields that always have a value, a non-pointer type is more appropriate.
- API clarity: Using a pointer for status implies the value might be absent, which creates confusion about the controller's behaviour. A non-pointer type makes it clear that status always reflects a resolved, concrete value.
I did some follow-up work in #492 which reverts status to be a non-pointer type while keeping spec as *int32. The PR includes testing evidence showing priority: 0 is correctly preserved in both spec and status.
| // +kubebuilder:validation:Minimum=0 | ||
| // +kubebuilder:validation:Maximum=1000 | ||
| Priority int32 `json:"priority"` | ||
| Priority *int32 `json:"priority,omitempty"` |
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.
If we say that the status reflects what was actually applied to the system—since the controller always resolves a concrete priority value, whether it’s the user’s explicit value or the default of 1000—then the status field can never be "unset". It should always have a value.
The field is annotated +required but declared as *int32 with json:"priority,omitempty" - these are contradictory: a required field should not be a pointer with omitempty, as that allows it to be absent from the serialised output.
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