-
Notifications
You must be signed in to change notification settings - Fork 24
Follow-up to PR #485: Use non-pointer type for status Priority field #492
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
Open
frobware
wants to merge
11
commits into
bpfman:main
Choose a base branch
from
frobware:issue484-followup
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
@frobware, this pull request is now in conflict and requires a rebase. |
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>
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>
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>
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>
With the AttachInfoState Priority field reverted to int32, update the reconcilers to: - Use the Priority value directly when populating status (no longer need GetPriorityPointer) - Compare Priority values directly in findLink functions (no longer need GetPriority wrapper for int32 comparisons) - Use GetPriority only when reading from spec (which remains *int32) Signed-off-by: Andrew McDermott <amcdermo@redhat.com>
With the status Priority field now int32, the GetPriorityPointer helper is no longer needed. Remove it along with its unused ptr import. Signed-off-by: Andrew McDermott <amcdermo@redhat.com>
Remove require.NotNil checks and pointer dereferences for status Priority fields which are now int32. The GetPriority helper is still needed when comparing against spec types which remain *int32. Signed-off-by: Andrew McDermott <amcdermo@redhat.com>
Move the default attach priority value to the API package as a public constant, providing a single authoritative definition. The helper function now references this constant rather than a private value. This ensures the default is defined alongside the types that use it and can be referenced by documentation and external consumers. Signed-off-by: Andrew McDermott <amcdermo@redhat.com>
Update the bundle CRD manifests to reflect the status Priority field being required (int32) rather than optional (*int32). Signed-off-by: Andrew McDermott <amcdermo@redhat.com>
22a2938 to
336d155
Compare
2 tasks
Contributor
Author
|
/hold |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This is follow-on work from PR #485 which addressed issue #484 (allowing
priority: 0for BPF programs). PR #485 correctly changed the specPriorityfield fromint32to*int32, but also changed the status field to a pointer type. This PR explores and implements using a non-pointer type for the status field instead.Problem with pointer type in status
The status
Priorityfield was annotated+requiredbut declared as*int32withjson:"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.Since the controller always resolves a concrete priority value (either the user's explicit value or the default 1000), the status field should be
int32. The status represents what was actually applied to the system, and that is never "unset".Changes
AttachInfoStatePriority fields back toint32for all program types (XDP, TC, TCX and their cluster variants)GetPriority()only for spec fields, use direct int32 values for statusGetPriorityPointer()which is no longer neededDefaultAttachPriorityas a public API constant for the default value (1000)Design
*int32int32Cluster Testing
Test 1: Explicit priority: 0
Created a ClusterBpfApplication with
priority: 0:Result - Spec preserved priority: 0:
Result - Status shows priority: 0:
Result - bpfman confirms Priority: 0:
Test 2: Default priority (not specified)
Created a ClusterBpfApplication without specifying priority:
Result - Status shows priority: 1000 (default applied by controller):
Schema verification
The status field is now correctly marked as required.