NETOBSERV-983: Change aggregation flow map to hashmap instead perCPU hashmap #118

msherif1234 · 2023-05-05T13:52:23Z

it was brought up by kernel folks that the agent kernel hook assumes that packets from the same flow will always be going to the same cpu core however this isn't guaranteed
that will messup time analysis done by the TC hook since we really don't have perCpu info then there is no need to use perCPUHASH map and we better off with global hashmap

Baseline stats from 1.2:
https://docs.google.com/spreadsheets/d/1P1penjeuOCH9Y7gTDP6uFTpKfG8Uow1gMaCIWB2_-s8/edit#gid=1137245831

New stats from today's run (1.3 + new eBPF image):
https://docs.google.com/spreadsheets/d/17GzV5vwHwananVv8X02T18UIFQ31n-_RUfMrwu-pCDU/edit?usp=sharing

codecov · 2023-05-05T14:07:25Z

Codecov Report

Merging #118 (1ece267) into main (3f192b3) will decrease coverage by 1.21%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##             main     #118      +/-   ##
==========================================
- Coverage   41.80%   40.60%   -1.21%     
==========================================
  Files          30       31       +1     
  Lines        2050     2054       +4     
==========================================
- Hits          857      834      -23     
- Misses       1155     1181      +26     
- Partials       38       39       +1

Flag	Coverage Δ
unittests	`40.60% <50.00%> (-1.21%)`	⬇️

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

Impacted Files	Coverage Δ
pkg/agent/agent.go	`39.20% <ø> (-0.58%)`	⬇️
pkg/ebpf/tracer.go	`0.00% <0.00%> (ø)`
pkg/flow/record.go	`76.00% <ø> (-7.34%)`	⬇️
pkg/flow/account.go	`82.69% <100.00%> (+0.33%)`	⬆️
pkg/flow/tracer_map.go	`83.07% <100.00%> (+2.06%)`	⬆️
pkg/test/tracer_fake.go	`67.85% <100.00%> (ø)`

... and 3 files with indirect coverage changes

msherif1234 · 2023-05-05T14:18:31Z

/ok-to-test

github-actions · 2023-05-05T14:20:29Z

New image: ["quay.io/netobserv/netobserv-ebpf-agent:bce991e"]. It will expire after two weeks.

msherif1234 · 2023-05-09T12:37:43Z

/ok-to-test

github-actions · 2023-05-09T12:39:20Z

New image: ["quay.io/netobserv/netobserv-ebpf-agent:5ee5fa1"]. It will expire after two weeks.

nathan-weinberg · 2023-05-09T18:36:17Z

Perf testing results: https://docs.google.com/spreadsheets/d/17GzV5vwHwananVv8X02T18UIFQ31n-_RUfMrwu-pCDU/edit?usp=sharing

msherif1234 · 2023-05-10T13:31:26Z

/ok-to-test

github-actions · 2023-05-10T13:33:04Z

New image: ["quay.io/netobserv/netobserv-ebpf-agent:b82acd4"]. It will expire after two weeks.

nathan-weinberg · 2023-05-10T18:03:07Z

New perf testing results: https://docs.google.com/spreadsheets/d/1ZPTunXgYYXBeOQqRXiQujUQRjzqPWmZ4j1CcMNfKJyQ/edit?usp=sharing

nathan-weinberg · 2023-05-11T14:32:11Z

Baseline stats with our current eBPF image: https://docs.google.com/spreadsheets/d/1XTSBByY3tsGnBzMLjKsGt1Yvv3GUBRbbOGbcofL7wgQ/edit?usp=sharing

nathan-weinberg · 2023-05-11T17:46:15Z

New test results for b82acd4 image: https://docs.google.com/spreadsheets/d/18Dr0GQ7xpKQiGBBBOSAYJPTqxQoAXOWr-N_8xp0MKrE/edit?usp=sharing

msherif1234 · 2023-05-11T18:58:54Z

pkg/flow/record.go

pkg/flow/tracer_map.go

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

jotak

thanks @msherif1234 !
/lgtm

msherif1234 · 2023-05-31T12:39:29Z

/approve

openshift-ci · 2023-05-31T12:39:34Z

[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

…netobserv#118)" This reverts commit b6e2b87.

…netobserv#118)" This reverts commit b6e2b87. fix Signed-off-by: msherif1234 <mmahmoud@redhat.com>

…#118)" (#172) This reverts commit b6e2b87. fix Signed-off-by: msherif1234 <mmahmoud@redhat.com>

openshift-ci bot added the do-not-merge/work-in-progress label May 5, 2023

msherif1234 requested a review from praveingk May 5, 2023 14:00

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

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

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

msherif1234 changed the title ~~WIP: change aggregation flow map to hashmap instead perCPU hashmap~~ Change aggregation flow map to hashmap instead perCPU hashmap May 9, 2023

openshift-ci bot removed the do-not-merge/work-in-progress label May 9, 2023

jotak mentioned this pull request May 10, 2023

NETOBSERV-1112: Add TCP RTT calculation to ebpf-agent and userspace. #117

Merged

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

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

jotak reviewed May 25, 2023

View reviewed changes

pkg/flow/record.go Show resolved Hide resolved

jotak reviewed May 25, 2023

View reviewed changes

pkg/flow/tracer_map.go Outdated Show resolved Hide resolved

jotak added no-doc This PR doesn't require documentation change on the NetObserv operator no-qe This PR doesn't necessitate QE approval labels May 25, 2023

change aggregation flow map to hashmap instead perCPU hashmap

1ece267

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

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

msherif1234 requested a review from jotak May 25, 2023 16:17

msherif1234 mentioned this pull request May 25, 2023

DONTMERGE: NETOBSERV-983: avoid preallocating huge chunk of memory by default #119

Closed

jotak approved these changes May 26, 2023

View reviewed changes

openshift-ci bot assigned jotak May 26, 2023

openshift-ci bot added the lgtm label May 26, 2023

openshift-ci bot added the approved label May 31, 2023

openshift-merge-robot merged commit b6e2b87 into netobserv:main May 31, 2023

msherif1234 changed the title ~~Change aggregation flow map to hashmap instead perCPU hashmap~~ NETOBSERV-983: Change aggregation flow map to hashmap instead perCPU hashmap Jun 6, 2023

msherif1234 added a commit to msherif1234/netobserv-ebpf-agent that referenced this pull request Aug 31, 2023

Revert "change aggregation flow map to hashmap instead perCPU hashmap (…

2633601

…netobserv#118)" This reverts commit b6e2b87.

msherif1234 added a commit to msherif1234/netobserv-ebpf-agent that referenced this pull request Aug 31, 2023

Revert "change aggregation flow map to hashmap instead perCPU hashmap (…

af8822f

…netobserv#118)" This reverts commit b6e2b87.

msherif1234 added a commit to msherif1234/netobserv-ebpf-agent that referenced this pull request Aug 31, 2023

Revert "change aggregation flow map to hashmap instead perCPU hashmap (…

7ba7fe1

…netobserv#118)" This reverts commit b6e2b87.

msherif1234 added a commit to msherif1234/netobserv-ebpf-agent that referenced this pull request Aug 31, 2023

Revert "change aggregation flow map to hashmap instead perCPU hashmap (…

e6a5200

…netobserv#118)" This reverts commit b6e2b87.

msherif1234 added a commit to msherif1234/netobserv-ebpf-agent that referenced this pull request Aug 31, 2023

Revert "change aggregation flow map to hashmap instead perCPU hashmap (…

6e5aa03

…netobserv#118)" This reverts commit b6e2b87.

msherif1234 added a commit to msherif1234/netobserv-ebpf-agent that referenced this pull request Aug 31, 2023

Revert "change aggregation flow map to hashmap instead perCPU hashmap (…

5191313

…netobserv#118)" This reverts commit b6e2b87.

msherif1234 added a commit to msherif1234/netobserv-ebpf-agent that referenced this pull request Aug 31, 2023

Revert "change aggregation flow map to hashmap instead perCPU hashmap (…

5518398

…netobserv#118)" This reverts commit b6e2b87.

msherif1234 added a commit to msherif1234/netobserv-ebpf-agent that referenced this pull request Sep 5, 2023

Revert "change aggregation flow map to hashmap instead perCPU hashmap (…

62dd76f

…netobserv#118)" This reverts commit b6e2b87.

openshift-merge-robot pushed a commit that referenced this pull request Sep 8, 2023

Revert "change aggregation flow map to hashmap instead perCPU hashmap (…

faf274e

…#118)" (#172) This reverts commit b6e2b87. fix Signed-off-by: msherif1234 <mmahmoud@redhat.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NETOBSERV-983: Change aggregation flow map to hashmap instead perCPU hashmap #118

NETOBSERV-983: Change aggregation flow map to hashmap instead perCPU hashmap #118

msherif1234 commented May 5, 2023 •

edited

Loading

codecov bot commented May 5, 2023 •

edited

Loading

msherif1234 commented May 5, 2023

github-actions bot commented May 5, 2023

msherif1234 commented May 9, 2023

github-actions bot commented May 9, 2023

nathan-weinberg commented May 9, 2023

msherif1234 commented May 10, 2023

github-actions bot commented May 10, 2023

nathan-weinberg commented May 10, 2023

nathan-weinberg commented May 11, 2023 •

edited by msherif1234

Loading

nathan-weinberg commented May 11, 2023

msherif1234 commented May 11, 2023 •

edited

Loading

jotak left a comment

msherif1234 commented May 31, 2023

openshift-ci bot commented May 31, 2023

NETOBSERV-983: Change aggregation flow map to hashmap instead perCPU hashmap #118

NETOBSERV-983: Change aggregation flow map to hashmap instead perCPU hashmap #118

Conversation

msherif1234 commented May 5, 2023 • edited Loading

codecov bot commented May 5, 2023 • edited Loading

Codecov Report

msherif1234 commented May 5, 2023

github-actions bot commented May 5, 2023

msherif1234 commented May 9, 2023

github-actions bot commented May 9, 2023

nathan-weinberg commented May 9, 2023

msherif1234 commented May 10, 2023

github-actions bot commented May 10, 2023

nathan-weinberg commented May 10, 2023

nathan-weinberg commented May 11, 2023 • edited by msherif1234 Loading

nathan-weinberg commented May 11, 2023

msherif1234 commented May 11, 2023 • edited Loading

jotak left a comment

Choose a reason for hiding this comment

msherif1234 commented May 31, 2023

openshift-ci bot commented May 31, 2023

msherif1234 commented May 5, 2023 •

edited

Loading

codecov bot commented May 5, 2023 •

edited

Loading

nathan-weinberg commented May 11, 2023 •

edited by msherif1234

Loading

msherif1234 commented May 11, 2023 •

edited

Loading