NETOBSERV-1240: prefer flows with DNS records when checking for dup #174
Conversation
|
@msherif1234: This pull request references NETOBSERV-1240 which is a valid jira issue. In response to this: 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/test-infra repository. |
|
@msherif1234: This pull request references NETOBSERV-1240 which is a valid jira issue. In response to this:
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/test-infra repository. |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #174 +/- ##
==========================================
+ Coverage 39.21% 39.39% +0.17%
==========================================
Files 31 31
Lines 2382 2394 +12
==========================================
+ Hits 934 943 +9
- Misses 1391 1393 +2
- Partials 57 58 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
msherif1234
force-pushed
the
dns_dup_handling
branch
2 times, most recently
from
6e59443
to
38e0d25
Compare
|
/ok-to-test |
openshift-ci
bot
added
the
ok-to-test
|
New image: It will expire after two weeks. To deploy this build, run from the operator repo, assuming the operator is running: USER=netobserv VERSION=1af8426 make set-agent-image |
pkg/flow/deduper.go
Outdated
| @@ -26,6 +26,7 @@ type deduperCache struct { | |||
|
|
|||
| type entry struct { | |||
| key *ebpf.BpfFlowId | |||
| record *Record | |||
There was a problem hiding this comment.
I was thinking about this solution to store the whole record here but it has some downsides / challenges:
- Records are now kept in memory in two places: in the
Accounterand now here - with different eviction timeouts, the one defined here being bigger for good reasons. So we can expect an increased memory usage as those records won't be garbage-collected until they expire from this cache. - If a record was flushed out from
Accounterand still present here while you revert its Duplicate flag withfEntry.record.Duplicate = true, this will actually have no effect downstream, so there are still cases where we'll have duplicates not flagged as such
There was a problem hiding this comment.
I wonder, does the concept of "Weak reference" exists in go? In order to keep this cache with weak refs of records, so that they're still gc'ed after Accounter flush ?
[Edit] It seems it doesn't exist... but we could implement something to have the accounter notifying the deduper on flush, so that all flushed records are niled in the deduper cache. It won't fix my item #2 mentioned above, but should avoid increased memory usage.
There was a problem hiding this comment.
so we are storing pointer to the record not the record itself I shared the memory usage below b4 and after and its about the same
pkg/flow/deduper.go
Outdated
| if justMark { | ||
| fEntry.record.Duplicate = true | ||
| } else { | ||
| fwd = findAndDeleteRecord(fwd, fEntry.record) |
There was a problem hiding this comment.
This won't work if the entry was in cache but already sent previously (via a previous set of records): it won't be in the fwd slice.
The record should be looked for & deleted from the Accounter store I guess
There was a problem hiding this comment.
based on the info in the arch doc my understanding is the dup stage after the account , plus of the record not in the same fwd slice in that case it won't be considered dup right ?
There was a problem hiding this comment.
I'm wrong about the accounter but IMO the problem is still the same: a flow could have been sent out to FLP and its dup comes in later.
the record not in the same fwd slice in that case it won't be considered dup right ?
I think that's wrong and that's why there is a separate cache with its own expiry time: we keep track of flow keys in that cache, even for flows already sent to FLP, so that flows coming later can still be seen as duplicates
msherif1234
force-pushed
the
dns_dup_handling
branch
from
38e0d25
to
603774c
Compare
github-actions
bot
removed
the
ok-to-test
msherif1234
force-pushed
the
dns_dup_handling
branch
2 times, most recently
from
3fc7d7c
to
a2ab479
Compare
|
/ok-to-test |
openshift-ci
bot
added
the
ok-to-test
|
New image: It will expire after two weeks. To deploy this build, run from the operator repo, assuming the operator is running: USER=netobserv VERSION=79005c8 make set-agent-image |
msherif1234
force-pushed
the
dns_dup_handling
branch
from
a2ab479
to
9190365
Compare
github-actions
bot
removed
the
ok-to-test
|
/ok-to-test |
openshift-ci
bot
added
the
ok-to-test
|
New image: It will expire after two weeks. To deploy this build, run from the operator repo, assuming the operator is running: USER=netobserv VERSION=deb3152 make set-agent-image |
msherif1234
force-pushed
the
dns_dup_handling
branch
from
9190365
to
69d61e3
Compare
github-actions
bot
removed
the
ok-to-test
|
/ok-to-test |
openshift-ci
bot
added
the
ok-to-test
|
New image: It will expire after two weeks. To deploy this build, run from the operator repo, assuming the operator is running: USER=netobserv VERSION=9729bb3 make set-agent-image |
|
|
Signed-off-by: msherif1234 <[email protected]>
msherif1234
force-pushed
the
dns_dup_handling
branch
from
69d61e3
to
931946b
Compare
github-actions
bot
removed
the
ok-to-test
|
@memodi can u pls give this a try and see if it fixes the issue ? |
|
/ok-to-test |
openshift-ci
bot
added
the
ok-to-test
|
New image: It will expire after two weeks. To deploy this build, run from the operator repo, assuming the operator is running: USER=netobserv VERSION=2af0f86 make set-agent-image |
|
Verified DNS flows shows up without selecting "Show Duplicates" in UI - cc @skrthomas |
|
/approve |
|
[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 |
…etobserv#174) Signed-off-by: msherif1234 <[email protected]> (cherry picked from commit cca4aa3)
…174) (#176) Signed-off-by: msherif1234 <[email protected]> (cherry picked from commit cca4aa3)
DNS record could be missed because of matching TC flow on different interface
here is example of DNS marked as dup of TC with different interfaces
suggested solution to prefer enriched flow with DNS and mark TC flow as duplicate