NETOBSERV-1545: Expose a counter for BPF hashmap update packets drop #304

msherif1234 · 2024-03-25T12:55:13Z

Description

cilium doesn't seem to have a way to read globals from ebpf program I will keep this PR as draft till we have a way to read global from userspace

https://cilium.slack.com/archives/C027KBX679U/p1711370379468299

so perCPU array map will be used to hold the global counter and userspace will read, aggregate and update the metrics

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-robot · 2024-03-25T12:55:18Z

openshift-ci-robot · 2024-03-26T10:47:41Z

openshift-ci-robot · 2024-03-26T11:47:52Z

@msherif1234: This pull request references NETOBSERV-1545 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set.

In response to this:

Description

cilium doesn't seem to have a way to read globals from ebpf program I will keep this PR as draft till we have a way to read global from userspace

https://cilium.slack.com/archives/C027KBX679U/p1711370379468299

so perCPU array map will be used to hold the global counter and userspace will read, aggregate and update the metrics

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.

Will this change affect NetObserv / Network Observability operator? If not, you can ignore the rest of this checklist.

Is this PR backed with a JIRA ticket? If so, make sure it is written as a title prefix (in general, PRs affecting the NetObserv/Network Observability product should be backed with a JIRA ticket - especially if they bring user facing changes).

Does this PR require product documentation?

If so, make sure the JIRA epic is labelled with "documentation" and provides a description relevant for doc writers, such as use cases or scenarios. Any required step to activate or configure the feature should be documented there, such as new CRD knobs.

Does this PR require a product release notes entry?

If so, fill in "Release Note Text" in the JIRA.

Is there anything else the QE team should know before testing? E.g: configuration changes, environment setup, etc.

If so, make sure it is described in the JIRA ticket.

QE requirements (check 1 from the list):

Standard QE validation, with pre-merge tests unless stated otherwise.

Regression tests only (e.g. refactoring with no user-facing change).

No QE (e.g. trivial change with high reviewer's confidence, or per agreement with the QE team).

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 openshift-eng/jira-lifecycle-plugin repository.

msherif1234 · 2024-03-26T12:03:09Z

/ok-to-test

github-actions · 2024-03-26T12:04:55Z

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

It will expire after two weeks.

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

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

msherif1234 · 2024-03-26T13:55:47Z

/ok-to-test

codecov-commenter · 2024-03-26T13:56:35Z

Codecov Report

Attention: Patch coverage is 0% with 18 lines in your changes are missing coverage. Please review.

Project coverage is 33.88%. Comparing base (a5bcf49) to head (a2ba1b4).

❗ Current head a2ba1b4 differs from pull request most recent head fde93e1. Consider uploading reports for the commit fde93e1 to get more accurate results

Files	Patch %	Lines
pkg/ebpf/tracer.go	0.00%	16 Missing ⚠️
pkg/ebpf/bpf_x86_bpfel.go	0.00%	1 Missing ⚠️
pkg/ebpf/tracer_legacy.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #304      +/-   ##
==========================================
- Coverage   34.04%   33.88%   -0.16%     
==========================================
  Files          47       47              
  Lines        3836     3854      +18     
==========================================
  Hits         1306     1306              
- Misses       2444     2462      +18     
  Partials       86       86

Flag	Coverage Δ
unittests	`33.88% <0.00%> (-0.16%)`	⬇️

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.

github-actions · 2024-03-26T13:57:37Z

New image:
quay.io/netobserv/netobserv-ebpf-agent:8a1158f

It will expire after two weeks.

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

USER=netobserv VERSION=8a1158f make set-agent-image

msherif1234 · 2024-03-26T14:05:40Z

To emulate error condition :

set sampling rate to 1
set cacheMaxFlows to 100
run ./hey-ho.sh -r 5 -d 3 -z 10m -n 4 -q 2 -p -b

msherif1234 · 2024-03-26T14:22:17Z

/gh pr ready

jotak · 2024-03-28T12:32:57Z

bpf/flows.c

+                bpf_printk("error updating flow %d\n", ret);
+            }
+            // Update global counter for hashmap update errors
+            error_counter_p = bpf_map_lookup_elem(&global_counters, &key);


to make sure if I understand that: you created global_counters as a map for generic purpose, ie. today it contains drop counters at key 0 but potentially later we may add more counters at different indexes, is this correct?
Perhaps if so you could define a constant DROP_COUNTER_KEY = 0 or something like that?

yes and I am already thinking of using this to count filtered flows :) at different index as well as ovs dbg I will add const here and the golang side Thanks

jotak · 2024-03-28T12:35:26Z

pkg/ebpf/tracer.go

+// ReadGlobalCounter reads the global counter and updates hashmap update error counter metrics
+func (m *FlowFetcher) ReadGlobalCounter(met *metrics.Metrics) {
+	var allCPUValue []uint32
+	key := uint32(0)


here as well this key could be a constant like dropCounterKey = 0
It would make it more obvious that we can add more keys for more counters

jotak · 2024-03-28T12:44:15Z

pkg/ebpf/tracer.go

+	}
+	// aggregate all the counters
+	for _, counter := range allCPUValue {
+		met.Errors.WithErrorName("flow-fetcher", "CannotUpdateHashMapCounter").Add(float64(counter))


There's already a metric for drops, could we use it instead?

Suggested change

met.Errors.WithErrorName("flow-fetcher", "CannotUpdateHashMapCounter").Add(float64(counter))

met.DroppedFlowsCounter.WithSourceAndReason("flow-fetcher", "CannotUpdateHashMapCounter").Add(float64(counter))

SG will use it then

Signed-off-by: Mohamed Mahmoud <mmahmoud@redhat.com>

jotak

thanks @msherif1234 !

msherif1234 · 2024-03-28T15:14:54Z

/approve

openshift-ci · 2024-03-28T15:14:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: msherif1234

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [msherif1234]

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

openshift-ci bot added the do-not-merge/work-in-progress label Mar 25, 2024

openshift-ci-robot added the jira/valid-reference label Mar 25, 2024

msherif1234 marked this pull request as draft March 25, 2024 12:55

msherif1234 force-pushed the update_err_counter branch from c59108c to d43a69d Compare March 26, 2024 11:46

msherif1234 changed the title ~~WIP: NETOBSERV-1545: Expose a counter of BPF drops~~ NETOBSERV-1545: Expose a counter of BPF drops Mar 26, 2024

msherif1234 requested a review from jotak March 26, 2024 11:47

msherif1234 force-pushed the update_err_counter branch 2 times, most recently from ba261e4 to c0b7a79 Compare March 26, 2024 12:02

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

msherif1234 removed the do-not-merge/work-in-progress label Mar 26, 2024

msherif1234 force-pushed the update_err_counter branch from c0b7a79 to a2ba1b4 Compare March 26, 2024 13:55

github-actions bot removed the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Mar 26, 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 Mar 26, 2024

msherif1234 marked this pull request as ready for review March 26, 2024 14:24

msherif1234 changed the title ~~NETOBSERV-1545: Expose a counter of BPF drops~~ NETOBSERV-1545: Expose a counter for BPF hashmap update packets drop Mar 26, 2024

jotak reviewed Mar 28, 2024

View reviewed changes

msherif1234 force-pushed the update_err_counter branch from a2ba1b4 to fde93e1 Compare March 28, 2024 13:07

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

msherif1234 requested a review from jotak March 28, 2024 13:07

NETOBSERV-1545: Expose a counter of BPF drops

b25759a

Signed-off-by: Mohamed Mahmoud <mmahmoud@redhat.com>

msherif1234 force-pushed the update_err_counter branch from fde93e1 to b25759a Compare March 28, 2024 13:12

jotak approved these changes Mar 28, 2024

View reviewed changes

openshift-ci bot assigned jotak Mar 28, 2024

openshift-ci bot added the lgtm label Mar 28, 2024

openshift-ci bot added the approved label Mar 28, 2024

openshift-merge-bot bot merged commit b63f1dd into netobserv:main Mar 28, 2024
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NETOBSERV-1545: Expose a counter for BPF hashmap update packets drop #304

NETOBSERV-1545: Expose a counter for BPF hashmap update packets drop #304

msherif1234 commented Mar 25, 2024 •

edited

openshift-ci-robot commented Mar 25, 2024 •

edited by openshift-ci bot

Description

Dependencies

Checklist

openshift-ci-robot commented Mar 26, 2024 •

edited by openshift-ci bot

Description

Dependencies

Checklist

openshift-ci-robot commented Mar 26, 2024 •

edited by openshift-ci bot

Description

Dependencies

Checklist

msherif1234 commented Mar 26, 2024

github-actions bot commented Mar 26, 2024

msherif1234 commented Mar 26, 2024

codecov-commenter commented Mar 26, 2024 •

edited

github-actions bot commented Mar 26, 2024

msherif1234 commented Mar 26, 2024 •

edited

msherif1234 commented Mar 26, 2024

jotak Mar 28, 2024 •

edited

msherif1234 Mar 28, 2024

jotak Mar 28, 2024 •

edited

jotak Mar 28, 2024

msherif1234 Mar 28, 2024

jotak left a comment

msherif1234 commented Mar 28, 2024

openshift-ci bot commented Mar 28, 2024

	met.Errors.WithErrorName("flow-fetcher", "CannotUpdateHashMapCounter").Add(float64(counter))
	met.DroppedFlowsCounter.WithSourceAndReason("flow-fetcher", "CannotUpdateHashMapCounter").Add(float64(counter))

NETOBSERV-1545: Expose a counter for BPF hashmap update packets drop #304

NETOBSERV-1545: Expose a counter for BPF hashmap update packets drop #304

Conversation

msherif1234 commented Mar 25, 2024 • edited

Description

Dependencies

Checklist

openshift-ci-robot commented Mar 25, 2024 • edited by openshift-ci bot

Description

Dependencies

Checklist

openshift-ci-robot commented Mar 26, 2024 • edited by openshift-ci bot

Description

Dependencies

Checklist

openshift-ci-robot commented Mar 26, 2024 • edited by openshift-ci bot

Description

Dependencies

Checklist

msherif1234 commented Mar 26, 2024

github-actions bot commented Mar 26, 2024

msherif1234 commented Mar 26, 2024

codecov-commenter commented Mar 26, 2024 • edited

Codecov Report

github-actions bot commented Mar 26, 2024

msherif1234 commented Mar 26, 2024 • edited

msherif1234 commented Mar 26, 2024

jotak Mar 28, 2024 • edited

Choose a reason for hiding this comment

msherif1234 Mar 28, 2024

Choose a reason for hiding this comment

jotak Mar 28, 2024 • edited

Choose a reason for hiding this comment

jotak Mar 28, 2024

Choose a reason for hiding this comment

msherif1234 Mar 28, 2024

Choose a reason for hiding this comment

jotak left a comment

Choose a reason for hiding this comment

msherif1234 commented Mar 28, 2024

openshift-ci bot commented Mar 28, 2024

msherif1234 commented Mar 25, 2024 •

edited

openshift-ci-robot commented Mar 25, 2024 •

edited by openshift-ci bot

openshift-ci-robot commented Mar 26, 2024 •

edited by openshift-ci bot

openshift-ci-robot commented Mar 26, 2024 •

edited by openshift-ci bot

codecov-commenter commented Mar 26, 2024 •

edited

msherif1234 commented Mar 26, 2024 •

edited

jotak Mar 28, 2024 •

edited

jotak Mar 28, 2024 •

edited