optimize filter sampling logic when filtering not enabled #488

msherif1234 · 2024-12-16T11:44:44Z

Description

optimize logic when filtering not enabled to avoid doing packet parsing when its not needed

Dependencies

n/a

Checklist

If you are not familiar with our processes or don't know what to answer in the list below, let us know in a comment: the maintainers will take care of that.

openshift-ci · 2024-12-16T11:44:50Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from msherif1234. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

codecov · 2024-12-16T11:52:51Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 29.85%. Comparing base (bb70b5a) to head (bf642d0).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #488      +/-   ##
==========================================
+ Coverage   29.72%   29.85%   +0.12%     
==========================================
  Files          48       48              
  Lines        4797     4797              
==========================================
+ Hits         1426     1432       +6     
+ Misses       3261     3256       -5     
+ Partials      110      109       -1

Flag	Coverage Δ
unittests	`29.85% <ø> (+0.12%)`	⬆️

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

see 2 files with indirect coverage changes

msherif1234 · 2024-12-16T12:07:53Z

/ok-to-test

jotak · 2024-12-16T12:09:25Z

Thanks, I think it fixes most of the regression but there are still a case not covered: if users previously had a filter and have global sampling, they would still see a performance regression.
IMO to fix the regression entirely we would need to introduce a global config like has_filter_sampling, defined from user-space and passed to the bpf prog, that is set to one if there is at least one filter defined with sampling.

github-actions · 2024-12-16T12:12:49Z

New image:
quay.io/netobserv/netobserv-ebpf-agent:8201443

It will expire after two weeks.

To deploy this build, run from the operator repo, assuming the operator is running:

USER=netobserv VERSION=8201443 make set-agent-image

msherif1234 · 2024-12-16T12:14:16Z

Thanks, I think it fixes most of the regression but there are still a case not covered: if users previously had a filter and have global sampling, they would still see a performance regression. IMO to fix the regression entirely we would need to introduce a global config like has_filter_sampling, defined from user-space and passed to the bpf prog, that is set to one if there is at least one filter defined with sampling.

if u enable filtering that should by itself optimize things I think we can defer this till we better know how the filter smapling will be used we might need to have default value also in case its not set ?

jotak · 2024-12-17T07:37:58Z

@msherif1234
Even though filtering reduces the resource footprint, that's still a regression that we would let go in while we could avoid it quite easily

till we better know how the filter smapling will be used we might need to have default value also in case its not set

The regression that remains is without using filter sampling, let's say someone was filtering for only TCP traffic and applied a global sampling, this too would be concerned with a performance regression

jotak · 2024-12-17T07:49:03Z

@msherif1234 here's my suggestion to fix this entirely: msherif1234#1

msherif1234 · 2024-12-17T18:15:28Z

/ok-to-test

msherif1234 · 2024-12-17T18:26:45Z

/ok-to-test

msherif1234 · 2024-12-17T18:29:23Z

/ok-to-test

github-actions · 2024-12-17T18:31:56Z

New image:
quay.io/netobserv/netobserv-ebpf-agent:c2e2b27

It will expire after two weeks.

To deploy this build, run from the operator repo, assuming the operator is running:

USER=netobserv VERSION=c2e2b27 make set-agent-image

github-actions · 2024-12-17T18:34:08Z

New image:
quay.io/netobserv/netobserv-ebpf-agent:c2e2b27

It will expire after two weeks.

To deploy this build, run from the operator repo, assuming the operator is running:

USER=netobserv VERSION=c2e2b27 make set-agent-image

jotak · 2024-12-18T10:13:35Z

bpf/utils.h

@@ -211,7 +218,7 @@ static inline bool check_and_do_flow_filtering(flow_id *id, u16 flags, u32 drop_
            // we have no matching rules so we update global counter for flows that are not matched by any rule
            increase_counter(FILTER_NOMATCH);
            // we have accept rule but no match so we can't let mismatched flows in the hashmap table.
-            if (action == ACCEPT || action == MAX_FILTER_ACTIONS) {
+            if (action == ACCEPT) {


I guess I see why you removed this, I think it fixes an issue but introduces a new one. In case of no CIDR match, action is neither ACCEPT nor REJECT and previously we would view that as a DROP; now you change that to be a KEEP. I think neither of those options are valid, we need to inspect the filters to decide whether it should return DROP or KEEP.
If users have defined any ACCEPT rules, then a no-match should return DROP
If users have defined any REJECT rules, then a no-match should return KEEP

... and if users have defined a mix of ACCEPT and REJECT, I think it just doesn't make sense because we don't know what to say when no rule is matched?

So at the end of the day, I think it's an API issue: we shouldn't allow to set ACCEPT/REJECT action per rule, it should be a single setting for all rules.

we should be the one define what is the default behavior when no CIDR match happens which is when we get the special action value, in that case I feel keeping the default as before to disallow is more accurate , nothing wrong with the API IMO, we just need to define what is the nomatch action behavior

I don't agree: if we keep as it is today, then there's a whole range of things you can't express, e.g:

Filter: { 1.2.3.4/32: REJECT }

You would expect a flow like that to be kept: src=6.6.6.6, dst=8.8.8.8
But here it would be dropped because action == MAX_FILTER_ACTIONS

(or you need to add extra rules for working that around, but I see that as an unexpected behaviour)

yeah u need to add Allow 0.0.0.0/0

When you add Allow 0.0.0.0/0, you fall into another problem: since the src_ip is checked before the dst_ip, this global-allow will take precedence on a flow like src=6.6.6.6, dst=1.2.3.4 even though there is the reject rule for that IP that in theory should take precedence.
Maybe then you need to always run both lookups for source and dest, and not check dest only when src has no match.

I think we should be more clear on what to expect with this feature, I've started a doc for acceptance criteria: https://docs.google.com/document/d/1YUVcpCdYxRACRVrqEOwir6upMyla5RvvUsZmgP4WvIs/edit?tab=t.0 - I think there's already a couple of issues - things that are not covered

msherif1234 · 2024-12-18T11:48:12Z

/ok-to-test

msherif1234 · 2024-12-18T11:50:50Z

@msherif1234 Even though filtering reduces the resource footprint, that's still a regression that we would let go in while we could avoid it quite easily

till we better know how the filter smapling will be used we might need to have default value also in case its not set

The regression that remains is without using filter sampling, let's say someone was filtering for only TCP traffic and applied a global sampling, this too would be concerned with a performance regression

I understand but my point is when filtering is on u see same perf impact regardless if u hvae sampoling action or not its not worth optimizing this case IMO

github-actions · 2024-12-18T11:52:58Z

New image:
quay.io/netobserv/netobserv-ebpf-agent:ec4a937

It will expire after two weeks.

To deploy this build, run from the operator repo, assuming the operator is running:

USER=netobserv VERSION=ec4a937 make set-agent-image

jotak · 2024-12-18T12:49:36Z

I understand but my point is when filtering is on u see same perf impact regardless if u hvae sampoling action or not its not worth optimizing this case IMO

So we would be introducing a perf regression on purpose, just so it aligns with a case that has worse performance? I'm not sure to agree with that argument... And the optimization is not super complicated.

msherif1234 · 2024-12-18T13:27:18Z

I understand but my point is when filtering is on u see same perf impact regardless if u hvae sampoling action or not its not worth optimizing this case IMO

So we would be introducing a perf regression on purpose, just so it aligns with a case that has worse performance? I'm not sure to agree with that argument... And the optimization is not super complicated.

it seems overkill to customize for special case IMO what if people endup using sampling all the time when they filter they might complain why filtering with smapling has different cpu than with sampling , idk seems too much IMO but not totally against it regardless if its easy or hard to do

jotak · 2024-12-18T15:14:30Z

I don't think that people doing filtering + global sampling are a corner case to ignore. Doing simple filtering with global sampling is precisely meant to reduce resource consumption - the new filtered sampling is a nice feature to fine-tune what you sample, but we can't assume everybody will use that.
I just did a test with global sampling 50 and filter only TCP traffic: before multi-filter PR, CPU reported by bpftop was around 1.95% under stress. With multi-filter + this PR, it climbs to 5.76% => that's almost a 200% increase

Before:

With this PR:

msherif1234 · 2024-12-18T17:05:48Z

/ok-to-test

github-actions · 2024-12-18T17:10:39Z

New image:
quay.io/netobserv/netobserv-ebpf-agent:ae7ce55

It will expire after two weeks.

To deploy this build, run from the operator repo, assuming the operator is running:

USER=netobserv VERSION=ae7ce55 make set-agent-image

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

msherif1234 · 2024-12-18T17:26:33Z

/ok-to-test

github-actions · 2024-12-18T17:31:05Z

New image:
quay.io/netobserv/netobserv-ebpf-agent:06d8a9e

It will expire after two weeks.

To deploy this build, run from the operator repo, assuming the operator is running:

USER=netobserv VERSION=06d8a9e make set-agent-image

msherif1234 · 2024-12-18T18:06:03Z

I don't think that people doing filtering + global sampling are a corner case to ignore. Doing simple filtering with global sampling is precisely meant to reduce resource consumption - the new filtered sampling is a nice feature to fine-tune what you sample, but we can't assume everybody will use that. I just did a test with global sampling 50 and filter only TCP traffic: before multi-filter PR, CPU reported by bpftop was around 1.95% under stress. With multi-filter + this PR, it climbs to 5.76% => that's almost a 200% increase

Before:

With this PR:

I am fine proceeding with the suggested optimization tell we better know how users will endup using this since u have a ready PR we can get mine in then u can followup with ur change WDYT ?

It's possible that flow ids found in the additional map don't match any id from the main flow map, for different reasons. This is especially visible when filtering for drops, as in this case all flows are produced only by the drop hook. In these cases, we must be able to reconstruct the flows from the user space. 3 things were particularly missing: end/start time and eth_protocol. So, they are added back into the additional map. Also, in user space, need to iterate separately over the two maps, to not miss any orphan flow.

jotak · 2024-12-19T13:55:16Z

@msherif1234 can you review this commit ec403e4 ?

msherif1234 · 2024-12-19T14:14:51Z

pkg/tracer/tracer.go

+	for _, id := range ids {
+		countAdditional++
+		if err := m.objects.AdditionalFlowMetrics.LookupAndDelete(&id, &additionalMetrics); err != nil {
+			log.WithError(err).WithField("flowId", id).Warnf("couldn't lookup/delete additional metrics entry")


why the error handling here is different than the other map in fact this more identical to old code so it should be similar no ? specially the check for i===0 ?

you mean, for the switch to legacy mode? I can add that yes. I assumed if there's a need for switching to legacy, it would be done already before entering in that block.. but for sure, there's the case of drops=true which generates 0 regular flow, so ok I can add that for that case

msherif1234 · 2024-12-19T14:17:31Z

bpf/types.h

@@ -106,6 +106,9 @@ typedef struct flow_metrics_t {
 const struct flow_metrics_t *unused2 __attribute__((unused));

 typedef struct additional_metrics_t {
+    u64 start_mono_time_ts;


do we really need this for all features or only the drop case if drop case only we might bundle this in the drop struct ? WDYT ? might be cleaner to have it for all and more future proof just in case not sure

yes I think it's safer to do it for all. It's actually a good thing that the drop=true filter revealed this issue, because it certainly can happen also in other situations, even if that's exceptional. There certainly can be some race condition that makes flows "orphan" in the additional map, without counterpart in main flow map, so we need to fetch &delete them separately if we don't want to risk having them in maps forever. And those "orphan" flows need the timestamp & eth_protocol to be correctly processed

msherif1234 · 2024-12-19T14:55:42Z

/ok-to-test
cc'd @memodi this PR will fix drop filter issue u have seen let me know if i can move it back to review

jotak · 2024-12-19T14:56:06Z

I am fine proceeding with the suggested optimization tell we better know how users will endup using this since u have a ready PR we can get mine in then u can followup with ur change WDYT ?

Ok

msherif1234 · 2024-12-19T14:57:34Z

/lgtm

openshift-ci · 2024-12-19T14:57:36Z

@msherif1234: you cannot LGTM your own PR.

In response to this:

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

jotak · 2024-12-19T14:58:46Z

/lgtm

github-actions · 2024-12-19T15:00:30Z

New image:
quay.io/netobserv/netobserv-ebpf-agent:81f89c0

It will expire after two weeks.

To deploy this build, run from the operator repo, assuming the operator is running:

USER=netobserv VERSION=81f89c0 make set-agent-image

stleerh · 2024-12-19T20:05:03Z

bpf/flows.c

+        }
+        filter_sampling = sampling;
+        do_sampling = 1;
+    }


What should do_sampling be set to if filter is enabled? It is currently left unchanged.

do_sampling is just bool either 0 or 1 sampling rate is what prandom uses

Prior to this code change, do_sampling was always set to 0 or 1 in this function. Now it can be left unchanged if is_filter_enabled() is true. There are a few places in other parts of the code that does something depending on the state of do_sampling. See: https://github.com/search?q=repo%3Anetobserv/netobserv-ebpf-agent%20do_sampling&type=code.

@stleerh I think all code paths are covered to set a value on do_sampling:

if filters aren't enabled (line 85), do_sampling is set to either 0 or 1

if filters are enabled (line 118), do_sampling is also set to either 0 or 1

The execution between line 85 and 118 are just IP/TCP headers extraction, they won't involve checking do_sampling, so that should be fine

Okay, I didn't see that line 85 is not the same as line 118.

/lgtm

stleerh · 2024-12-19T20:09:20Z

bpf/flows.c

+        do_sampling = 1;
+        if (skip) {
+            return TC_ACT_OK;
+        }


This is related to the same question on what do_sampling should be set to since this is part of the same function.

memodi · 2024-12-19T21:29:00Z

/label qe-approved

msherif1234 force-pushed the filter_sampling_opt branch from 9d72433 to 03a957d Compare December 16, 2024 12:07

openshift-ci bot added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Dec 16, 2024

msherif1234 force-pushed the filter_sampling_opt branch from 03a957d to 35857aa Compare December 17, 2024 18:26

github-actions bot removed the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Dec 17, 2024

openshift-ci bot added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Dec 17, 2024

msherif1234 removed the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Dec 17, 2024

openshift-ci bot added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Dec 17, 2024

jotak reviewed Dec 18, 2024

View reviewed changes

msherif1234 force-pushed the filter_sampling_opt branch from 35857aa to dd9ccbd Compare December 18, 2024 11:46

github-actions bot removed the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Dec 18, 2024

openshift-ci bot added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Dec 18, 2024

optimize filter sampling logic when filtering not enabled

1342023

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

msherif1234 force-pushed the filter_sampling_opt branch from ab3250c to 1342023 Compare December 18, 2024 17:26

github-actions bot removed the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Dec 18, 2024

openshift-ci bot added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Dec 18, 2024

github-actions bot removed the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Dec 19, 2024

msherif1234 commented Dec 19, 2024

View reviewed changes

Format, fix test, handle legacy mode

6b3babd

openshift-ci bot added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Dec 19, 2024

openshift-ci bot assigned jotak Dec 19, 2024

openshift-ci bot added the lgtm label Dec 19, 2024

stleerh reviewed Dec 19, 2024

View reviewed changes

openshift-ci bot added the qe-approved QE has approved this pull request label Dec 19, 2024

jotak merged commit 4d8f9d9 into netobserv:main Dec 20, 2024
9 of 10 checks passed

jotak mentioned this pull request Dec 20, 2024

Restore performances in filtering case #496

Open

optimize filter sampling logic when filtering not enabled #488

optimize filter sampling logic when filtering not enabled #488

Conversation

msherif1234 commented Dec 16, 2024

Description

Dependencies

Checklist

openshift-ci bot commented Dec 16, 2024

codecov bot commented Dec 16, 2024 • edited Loading

Codecov Report

msherif1234 commented Dec 16, 2024

jotak commented Dec 16, 2024

github-actions bot commented Dec 16, 2024

msherif1234 commented Dec 16, 2024

jotak commented Dec 17, 2024 • edited Loading

jotak commented Dec 17, 2024

msherif1234 commented Dec 17, 2024

msherif1234 commented Dec 17, 2024

msherif1234 commented Dec 17, 2024

github-actions bot commented Dec 17, 2024

github-actions bot commented Dec 17, 2024

jotak Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jotak Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msherif1234 commented Dec 18, 2024

msherif1234 commented Dec 18, 2024

github-actions bot commented Dec 18, 2024

jotak commented Dec 18, 2024

msherif1234 commented Dec 18, 2024

jotak commented Dec 18, 2024

msherif1234 commented Dec 18, 2024

github-actions bot commented Dec 18, 2024

msherif1234 commented Dec 18, 2024

github-actions bot commented Dec 18, 2024

msherif1234 commented Dec 18, 2024

jotak commented Dec 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msherif1234 commented Dec 19, 2024

jotak commented Dec 19, 2024 • edited Loading

msherif1234 commented Dec 19, 2024

openshift-ci bot commented Dec 19, 2024

jotak commented Dec 19, 2024

github-actions bot commented Dec 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stleerh Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

memodi commented Dec 19, 2024

codecov bot commented Dec 16, 2024 •

edited

Loading

jotak commented Dec 17, 2024 •

edited

Loading

jotak Dec 18, 2024 •

edited

Loading

jotak Dec 18, 2024 •

edited

Loading

jotak commented Dec 19, 2024 •

edited

Loading

stleerh Dec 20, 2024 •

edited

Loading