handle update app object by removing an existing program #38

msherif1234 · 2024-07-03T17:29:43Z

unit-test
1- deploy app example oc create -f config/samples/bpfman.io_v1alpha1_bpfapplication.yaml

oc get bpfprograms 
NAME                                                                                                               TYPE          STATUS         AGE
bpfapplication-sample-kprobe-try-to-wake-up-bpfman-deployment-control-plane-try-to-wake-up                         application   bpfmanLoaded   31s
bpfapplication-sample-tc-ingress-eth0-bpfman-deployment-control-plane-eth0                                         application   bpfmanLoaded   18s
bpfapplication-sample-tracepoint-syscalls-sys-enter-kill-bpfman-deployment-control-plane-syscalls-sys-enter-kill   application   bpfmanLoaded   20s
bpfapplication-sample-uprobe-malloc-bpfman-deployment-control-plane-libc-bpfman-daemon-8ddxs-bpfman                application   bpfmanLoaded   10s
bpfapplication-sample-uprobe-malloc-bpfman-deployment-control-plane-libc-bpfman-daemon-8ddxs-bpfman-agent          application   bpfmanLoaded   13s
bpfapplication-sample-xdp-eth0-bpfman-deployment-control-plane-eth0                                                application   bpfmanLoaded   8s

2- edit app object and remove kprobe oc edit bpfapplication bpfapplication-sample

 oc get bpfprograms 
NAME                                                                                                               TYPE          STATUS         AGE
bpfapplication-sample-tc-ingress-eth0-bpfman-deployment-control-plane-eth0                                         application   bpfmanLoaded   4m30s
bpfapplication-sample-tracepoint-syscalls-sys-enter-kill-bpfman-deployment-control-plane-syscalls-sys-enter-kill   application   bpfmanLoaded   4m32s
bpfapplication-sample-uprobe-malloc-bpfman-deployment-control-plane-libc-bpfman-daemon-8ddxs-bpfman                application   bpfmanLoaded   4m22s
bpfapplication-sample-uprobe-malloc-bpfman-deployment-control-plane-libc-bpfman-daemon-8ddxs-bpfman-agent          application   bpfmanLoaded   4m25s
bpfapplication-sample-xdp-eth0-bpfman-deployment-control-plane-eth0                                                application   bpfmanLoaded   4m20s

3- add back kprobe

oc get bpfprograms 
NAME                                                                                                               TYPE          STATUS         AGE
bpfapplication-sample-kprobe-try-to-wake-up-bpfman-deployment-control-plane-try-to-wake-up                         application   bpfmanLoaded   13s
bpfapplication-sample-tc-ingress-eth0-bpfman-deployment-control-plane-eth0                                         application   bpfmanLoaded   6m52s
bpfapplication-sample-tracepoint-syscalls-sys-enter-kill-bpfman-deployment-control-plane-syscalls-sys-enter-kill   application   bpfmanLoaded   6m54s
bpfapplication-sample-uprobe-malloc-bpfman-deployment-control-plane-libc-bpfman-daemon-8ddxs-bpfman                application   bpfmanLoaded   6m44s
bpfapplication-sample-uprobe-malloc-bpfman-deployment-control-plane-libc-bpfman-daemon-8ddxs-bpfman-agent          application   bpfmanLoaded   6m47s
bpfapplication-sample-xdp-eth0-bpfman-deployment-control-plane-eth0                                                application   bpfmanLoaded   6m42s

codecov · 2024-07-03T17:35:05Z

Codecov Report

Attention: Patch coverage is 7.01754% with 53 lines in your changes missing coverage. Please review.

Project coverage is 26.92%. Comparing base (4a4309b) to head (b3c925c).
Report is 3 commits behind head on main.

Files	Patch %	Lines
controllers/bpfman-agent/common.go	0.00%	28 Missing ⚠️
controllers/bpfman-agent/application-program.go	13.79%	25 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #38      +/-   ##
==========================================
- Coverage   27.08%   26.92%   -0.17%     
==========================================
  Files          81       81              
  Lines        6844     6897      +53     
==========================================
+ Hits         1854     1857       +3     
- Misses       4806     4856      +50     
  Partials      184      184

Flag	Coverage Δ
unittests	`26.92% <7.01%> (-0.17%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

anfredette

Thanks for doing this! It looks pretty good, but I have a few comments.

anfredette · 2024-07-03T17:50:36Z

controllers/bpfman-agent/application-program.go

@@ -74,6 +74,7 @@ func (r *BpfApplicationReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 	}

 	for i, a := range appPrograms.Items {
+		var appProgramMap = make(map[string][]client.Object)


Since we just need to know whether the app program exists, and don't actually need the object, you can make this map[string]bool and just set the entry to true.

anfredette · 2024-07-03T18:00:45Z

controllers/bpfman-agent/common.go

@@ -633,6 +633,31 @@ func (r *ReconcilerCommon) handleProgDelete(
 	return internal.Unchanged, nil
 }



// unloadAndDeletePrograms unloads and deletes BbpPrograms when the owning
// *Program or BpfApplication is not being deleted itself, but something
// has change such that the BpfPrograms are no longer needed.

Suggested change

anfredette · 2024-07-03T18:00:48Z

controllers/bpfman-agent/common.go

@@ -633,6 +633,31 @@ func (r *ReconcilerCommon) handleProgDelete(
 	return internal.Unchanged, nil
 }

+func (r *ReconcilerCommon) unloadAndDeleteApplicationProgramsList(ctx context.Context, bpfDeletedPrograms *bpfmaniov1alpha1.BpfProgramList) error {


Suggested change

func (r *ReconcilerCommon) unloadAndDeleteApplicationProgramsList(ctx context.Context, bpfDeletedPrograms *bpfmaniov1alpha1.BpfProgramList) error {

func (r *ReconcilerCommon) unloadAndDeleteBpfPrograms(ctx context.Context, bpfProgramList *bpfmaniov1alpha1.BpfProgramList) error {

This can also be used by handleProgCreateOrUpdate() to delete the extra BpfPrograms

"bpfDeletedPrograms" doesn't seem right because they aren't deleted yet.

anfredette · 2024-07-03T18:06:30Z

controllers/bpfman-agent/common.go

+			return err
+		}
+
+		r.removeFinalizer(ctx, &bpfProgram, internal.BpfApplicationControllerFinalizer)


I know it's inefficient, but the way the current controller is implemented, every time we make a change to a BpfProgram object, we exit. For example, from handleProgDelete:

if r.removeFinalizer(ctx, &bpfProgram, rec.getFinalizer()) { return internal.Updated, nil }

anfredette · 2024-07-03T18:07:06Z

controllers/bpfman-agent/common.go

+		r.Logger.Info("Deleting bpfProgram", "Name", bpfProgram.Name, "Owner", bpfProgram.GetName())
+		if err := r.Delete(ctx, &bpfProgram, &opts); err != nil {
+			return fmt.Errorf("failed to delete bpfProgram object: %v", err)
+		}


We would return here too on success.

I have some ideas about changing that behavior, but I think we should keep it as is. The reason is that as soon as you make a change, the manager calls Reconcile() again, and if we don't stop, we end up with a Reconcile storm. That's the theory anyway.

so u wanted to delete one program at a time that is why the return ?

That's what ends up happening, but I'd like to look at changing that behavior.

yeah this behavior isn't great I watched app sample configs with 6 hooks takes ~30s to complete

Signed-off-by: Mohamed Mahmoud <[email protected]>

anfredette

LGTM

Signed-off-by: Andre Fredette <[email protected]>

…84197785d29f23b4 [Snyk] Security upgrade golang from 1.6 to 1.23.1

msherif1234 requested a review from anfredette July 3, 2024 17:29

anfredette requested changes Jul 3, 2024

View reviewed changes

msherif1234 force-pushed the bfpapp-delete branch from febdea7 to 379b9d0 Compare July 3, 2024 19:40

msherif1234 requested a review from anfredette July 3, 2024 19:41

msherif1234 force-pushed the bfpapp-delete branch 2 times, most recently from 38e21e8 to 2b538e2 Compare July 3, 2024 20:03

handle update app object by removing an existing program

b3c925c

Signed-off-by: Mohamed Mahmoud <[email protected]>

msherif1234 force-pushed the bfpapp-delete branch from 2b538e2 to b3c925c Compare July 8, 2024 18:13

anfredette approved these changes Jul 8, 2024

View reviewed changes

mergify bot merged commit 2a8ce1b into bpfman:main Jul 8, 2024
12 checks passed

anfredette added a commit to anfredette/bpfman-operator that referenced this pull request Jul 8, 2024

Rebase on pr bpfman#38

76440da

Signed-off-by: Andre Fredette <[email protected]>

anfredette added a commit to anfredette/bpfman-operator that referenced this pull request Jul 8, 2024

Rebase on pr bpfman#38

5b85c5d

Signed-off-by: Andre Fredette <[email protected]>

msherif1234 added a commit to msherif1234/bpfman-operator that referenced this pull request Oct 30, 2024

Merge pull request bpfman#38 from openshift/snyk-fix-d40fffd234bff7dd…

f5e4290

…84197785d29f23b4 [Snyk] Security upgrade golang from 1.6 to 1.23.1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

handle update app object by removing an existing program #38

handle update app object by removing an existing program #38

Uh oh!

msherif1234 commented Jul 3, 2024 •

edited

Loading

Uh oh!

codecov bot commented Jul 3, 2024 •

edited

Loading

Uh oh!

anfredette left a comment

Uh oh!

anfredette Jul 3, 2024

Uh oh!

anfredette Jul 3, 2024

Uh oh!

anfredette Jul 3, 2024

Uh oh!

anfredette Jul 3, 2024

Uh oh!

anfredette Jul 3, 2024

Uh oh!

anfredette Jul 3, 2024

Uh oh!

anfredette Jul 3, 2024

Uh oh!

msherif1234 Jul 3, 2024

Uh oh!

anfredette Jul 3, 2024

Uh oh!

msherif1234 Jul 3, 2024

Uh oh!

anfredette left a comment

Uh oh!

Uh oh!

Uh oh!

		@@ -633,6 +633,31 @@ func (r *ReconcilerCommon) handleProgDelete(
		return internal.Unchanged, nil
		}

	func (r ReconcilerCommon) unloadAndDeleteApplicationProgramsList(ctx context.Context, bpfDeletedPrograms bpfmaniov1alpha1.BpfProgramList) error {
	func (r ReconcilerCommon) unloadAndDeleteBpfPrograms(ctx context.Context, bpfProgramList bpfmaniov1alpha1.BpfProgramList) error {

handle update app object by removing an existing program #38

handle update app object by removing an existing program #38

Uh oh!

Conversation

msherif1234 commented Jul 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jul 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

anfredette left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anfredette left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

msherif1234 commented Jul 3, 2024 •

edited

Loading

codecov bot commented Jul 3, 2024 •

edited

Loading