SDN-5007: introducing bfpapplication object #6
Conversation
This file contains 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
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6 +/- ##
==========================================
+ Coverage 26.74% 27.10% +0.35%
==========================================
Files 75 81 +6
Lines 6146 6837 +691
==========================================
+ Hits 1644 1853 +209
- Misses 4337 4800 +463
- Partials 165 184 +19
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
anfredette
reviewed
May 23, 2024
anfredette
reviewed
May 23, 2024
|
In general, I think your bpfapplication CRD looks okay. As I said above, I'm going to have to think about how/whether we could use the existing code some more though. I added a few random thoughts in comments here. |
msherif1234
force-pushed
the
app-crd
branch
5 times, most recently
from
9e603ba
to
755e463
Compare
|
@msherif1234, this pull request is now in conflict and requires a rebase. |
|
Rebased on pr #21. |
msherif1234
changed the title
anfredette
force-pushed
the
app-crd
branch
2 times, most recently
from
14b2026
to
6dbcb97
Compare
|
I fixed some lint errors. |
msherif1234
force-pushed
the
app-crd
branch
2 times, most recently
from
0cf6c68
to
051a49f
Compare
msherif1234
changed the title
If we change the attach point to the one being used in the uprobe sample, it won't require the target. I was going to make this change at some point. |
msherif1234
force-pushed
the
app-crd
branch
3 times, most recently
from
bfbeccc
to
23f279f
Compare
|
@msherif1234, this pull request is now in conflict and requires a rebase. |
Signed-off-by: Mohamed Mahmoud <mmahmoud@redhat.com>
Signed-off-by: Mohamed Mahmoud <mmahmoud@redhat.com>
- Ran make generate for latest BpfApplication CRD change - Handle setting owner and object type - If reconcile is complete for one program/object, continue with the next one - Added unit test for fentry TODO: - Add BpfApplication tests for all program types - Add sample BpfApplication yaml and test on kubernetes - See if there's any way to reduce code duplication in application-program.go Signed-off-by: Andre Fredette <afredette@redhat.com>
Signed-off-by: Andre Fredette <afredette@redhat.com>
Signed-off-by: Andre Fredette <afredette@redhat.com>
- Ran make generate for latest BpfApplication CRD change - Handle setting owner and object type - If reconcile is complete for one program/object, continue with the next one - Added unit test for fentry TODO: - Add BpfApplication tests for all program types - Add sample BpfApplication yaml and test on kubernetes - See if there's any way to reduce code duplication in application-program.go Signed-off-by: Andre Fredette <afredette@redhat.com>
- Handle setting owner and object type - If reconcile is complete with one program/object, continue with the next one These changes have not been tested. Still need to write a unit test Signed-off-by: Andre Fredette <afredette@redhat.com>
When reconciling a *Program object created by an ApplicationProgram, getExistingBpfPrograms needs to return just the programs for the given *Program. However, the current implementation uses the BpfParentProgram label, so that gets all the BpfPrograms created for the ApplicationProgram, which messes up the reconcile logic. This commit adds another label called BpfParentProgram which allows us to get just the BpfPrograms for the current *Program regardless of whether they were created for a single *Program CRD or by a program entry for an ApplicationProgram CRD. Signed-off-by: Andre Fredette <afredette@redhat.com>
Signed-off-by: Mohamed Mahmoud <mmahmoud@redhat.com>
Signed-off-by: Andre Fredette <afredette@redhat.com>
Signed-off-by: Andre Fredette <afredette@redhat.com>
Two changes were needed: - The BpfPrograms needed the internal.BpfApplicationControllerFinalizer instead of the *Program finalizer. - GetDeletionTimestamp() needs to be called on the Owner. In this case, the owner is the BpfApplication. Also moved SetupWithManager() in the BpfApplication operator controller so that it was in the same order as the other controllers. This made a diff easier to read. Signed-off-by: Andre Fredette <afredette@redhat.com>
Signed-off-by: Mohamed Mahmoud <mmahmoud@redhat.com>
Signed-off-by: Mohamed Mahmoud <mmahmoud@redhat.com>
Signed-off-by: Mohamed Mahmoud <mmahmoud@redhat.com>
Adds the attach point to the name of the BpfApplication programs so that BpfPrograms created by multipl programs of the same type are unique. Also - Adds some helper functions for name generation - Fixed an indent issue with the BpfApplication sample yaml Signed-off-by: Andre Fredette <afredette@redhat.com>
astoycos
approved these changes
Jun 20, 2024
There was a problem hiding this comment.
LGTM
Thanks for all the hard work @anfredette @msherif1234!!
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.
Fix bpfman/bpfman#1142