Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support to evict DNS and RTT stale entries #163

Merged
merged 1 commit into from
Jul 25, 2023

Conversation

msherif1234
Copy link
Contributor

@msherif1234 msherif1234 commented Jul 22, 2023

Its possible we can have DNS queries w/o any responses or RTT entries w/o replies those entries will be there for ever, since we can't use LRU because we don't know when entries will be deleted and its very large memory footprint we will evict those entries in the user space every time we evict flows hash table by default every 5 seconds.

This is follow up for #162

No CPU or memory obvious impact with this PR
image

@msherif1234
Copy link
Contributor Author

/ok-to-test

@openshift-ci openshift-ci bot added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Jul 22, 2023
@github-actions
Copy link

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

It will expire after two weeks.

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

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

@codecov
Copy link

codecov bot commented Jul 22, 2023

Codecov Report

Merging #163 (3cae3e5) into main (d3f035d) will decrease coverage by 0.43%.
The diff coverage is 23.68%.

@@            Coverage Diff             @@
##             main     #163      +/-   ##
==========================================
- Coverage   39.25%   38.82%   -0.43%     
==========================================
  Files          31       31              
  Lines        2214     2246      +32     
==========================================
+ Hits          869      872       +3     
- Misses       1296     1325      +29     
  Partials       49       49              
Flag Coverage Δ
unittests 38.82% <23.68%> (-0.43%) ⬇️

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

Files Changed Coverage Δ
pkg/ebpf/tracer.go 0.00% <0.00%> (ø)
pkg/agent/agent.go 37.84% <100.00%> (ø)
pkg/flow/tracer_map.go 80.00% <100.00%> (+0.58%) ⬆️
pkg/test/tracer_fake.go 68.96% <100.00%> (+1.10%) ⬆️

@dushyantbehl
Copy link
Contributor

@msherif1234 can we add a check here like clear the entries which are 5s/10s earlier and are still there (which means they are stale I mean)?
we need to check this against Mono clock.

@github-actions github-actions bot removed the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Jul 22, 2023
@msherif1234 msherif1234 force-pushed the evict_feat_map branch 2 times, most recently from 0115f58 to e5c0c39 Compare July 22, 2023 23:01
@msherif1234
Copy link
Contributor Author

/ok-to-test

@openshift-ci openshift-ci bot added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Jul 22, 2023
@github-actions
Copy link

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

It will expire after two weeks.

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

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

@dushyantbehl
Copy link
Contributor

dushyantbehl commented Jul 24, 2023

Thanks @msherif1234 for adding the timeout. I just feel one small thing that since we are handling three maps with one funciton maybe we can make a map manager and handle all three inside of it..
Currently the flow is passed via LookupAndDeleteMap function which acts on bpf metrics and is a bit misleading. WDYT?

other than that looks good to me.

@github-actions github-actions bot removed the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Jul 24, 2023
@msherif1234
Copy link
Contributor Author

Thanks @msherif1234 for adding the timeout. I just feel one small thing that since we are handling three maps with one funciton maybe we can make a map manager and handle all three inside of it.. Currently the flow is passed via LookupAndDeleteMap function which acts on bpf metrics and is a bit misleading. WDYT?

other than that looks good to me.

I created new interface to handle features map I think is more cleaner WDYT ?

Signed-off-by: msherif1234 <mmahmoud@redhat.com>
@msherif1234 msherif1234 changed the title Evict DNS and RTT stale entries everytime we evict flows table Add support to evict DNS and RTT stale entries Jul 24, 2023
@dushyantbehl
Copy link
Contributor

Thanks @msherif1234 for adding the timeout. I just feel one small thing that since we are handling three maps with one funciton maybe we can make a map manager and handle all three inside of it.. Currently the flow is passed via LookupAndDeleteMap function which acts on bpf metrics and is a bit misleading. WDYT?
other than that looks good to me.

I created new interface to handle features map I think is more cleaner WDYT ?

Looks good to me thanks!
/lgtm

@msherif1234
Copy link
Contributor Author

/approve

@openshift-ci
Copy link

openshift-ci bot commented Jul 25, 2023

[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:

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

@openshift-merge-robot openshift-merge-robot merged commit 338d2b2 into netobserv:main Jul 25, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants