More concise naming scheme for BpfPrograms #37
Conversation
|
The new names look like the following: |
anfredette
force-pushed
the
bpfprogram-name
branch
from
7031bd2
to
2bc7d6c
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #37 +/- ##
==========================================
+ Coverage 26.92% 27.40% +0.48%
==========================================
Files 81 81
Lines 6897 6965 +68
==========================================
+ Hits 1857 1909 +52
- Misses 4856 4870 +14
- Partials 184 186 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
anfredette
force-pushed
the
bpfprogram-name
branch
from
2bc7d6c
to
76440da
Compare
Create a shorter unique name for BpfProgram objects. For *Programs, the name is <*Program name>-<random string>. For BpfApplications, the unique name is <BpfApplication name>-<bpf program type>-<random string>. Then, the identifying info for BpfProgram that used to be in the name is stored in the BpfProgram object. Fixes: bpfman#28 Signed-off-by: Andre Fredette <[email protected]>
Signed-off-by: Andre Fredette <[email protected]>
anfredette
force-pushed
the
bpfprogram-name
branch
from
76440da
to
5b85c5d
Compare
| existingBpfPrograms[bpfProg.GetName()] = bpfProg | ||
| key := bpfProgKey{ | ||
| appProgId: bpfProg.GetLabels()[internal.AppProgramId], | ||
| attachPoint: bpfProg.GetAnnotations()[internal.BpfProgramAttachPoint], |
There was a problem hiding this comment.
just curious why u need both progId and attachement point as key, progId should be unique right ?
There was a problem hiding this comment.
Not exactly. I'm trying to use appProgId to uniquely identify a single program item within a BpfApplication. For this, I'm currently using program type plus all or a portion of the attach point. In some cases (Fentry, Fexit, Kprobe) it's the whole attach point. In the case of uprobes, it's the FunctionName (I think I should also add the target), but it can actually be attached to the same point in multiple containers (each one results in a separate BpfProgram object). In the case of tracepoint, it's the first tracepoint on the list. In the case of XDP, it's the first interface on the list. In the case of TC, it's the first interface on the list plus the direction. This kind of works because we've said that there will only ever be one program of a given type per attach point. However, that may not hold true for everyone, and for the program types where there is a list of attach points, I wonder if we'd ever have two programs of the same type that have different lists, but the first item is the same. That probably won't happen with our applications, but I don't know if we can rule it out. My assumption is that we'll need to change something in the future to make this 100% unique. One Idea I have is to have the operator controller generate a unique ID for each program. Another is to require that the user provide a unique name/id for each program in a BpfApplication.
There was a problem hiding this comment.
So, to summarize, the appProgId should uniquely identify a program item in a BpfApplication, and the following set of info uniquely identifies a BpfProgram object that was created for it:
Labels:
- internal.BpfProgramOwner
- internal.AppProgramId
- internal.K8sHostLabel
Annotation:
- internal.BpfProgramAttachPoint
I put the attach point in an annotation because we don't need to use it in a search, and it can be a longer string if needed.
There was a problem hiding this comment.
if u have more than one app we can't be sure they will provide unique id better to be done under the hood IMO
There was a problem hiding this comment.
I agree, but I've been thinking about it and it might be a little tricky. It’s easy to generate appProgramIds the first time. For example, we could just use the index of the program item in the list. However, if the BpfApplication CRD is updated, and the user adds a new program, and/or deletes an existing one, I think we want to leave the unchanged programs alone. I.e., we don’t want to unload and reload everything. We just want to handle the changes. To do this, I think we want to keep the same appProgramId for the existing programs and generate new ones for any new programs. To do this, I think we'll need to compare the old and new CRD versions and keep the existing appProgramId for the programs that are the same, and generate new ones for any new programs. To do this, we’ll need to identify what constitutes a new program vs. an existing program. For example, the CRD can be updated with some new attach points for an XDP program. We probably want to be able to keep the existing programs in place and only reconcile the changes.
I was thinking that what I have in this PR is a good enough for now and provides the structure for a more complicated solution later.
|
/LGTM |
…/ocp-bpfman-operator-bundle Update ocp-bpfman-operator-bundle to 69e5089
Create a shorter unique name for
BpfProgramobjects. For *Programs, the name is<*Program name>-<random string>. ForBpfApplications, the unique name is<BpfApplication name>-<bpf program type>-<random string>.Then, the identifying info for BpfProgram that used to be in the name is stored in the BpfProgram object.
Fixes: #28