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

Fm row fix/v3 #12218

Closed
wants to merge 2 commits into from
Closed

Fm row fix/v3 #12218

wants to merge 2 commits into from

Conversation

victorjulien
Copy link
Member

Flow manager multi instance fixes.

Fix start issue with #12208

In multi instance flow manager setups, each flow manager gets a slice
of the hash table to manage. Due to a logic error in the chunked
scanning of the hash slice, instances beyond the first would always
rescan the same (first) subslice of their slice.

The `pos` variable that is used to keep the state of what the starting
position for the next scan was supposed to be, was treated as if it held
a relative value. Relative to the bounds of the slice. It was however,
holding an absolute position. This meant that when doing it's bounds
check it was always considered out of bounds. This would reset the sub-
slice to be scanned to the first part of the instances slice.

This patch addresses the issue by correctly handling the fact that the
value is absolute.

Bug: OISF#7365.

Fixes: e9d2417 ("flow/manager: adaptive hash eviction timing")
Copy link

codecov bot commented Dec 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.18%. Comparing base (b58b886) to head (3c01abf).
Report is 11 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #12218   +/-   ##
=======================================
  Coverage   83.18%   83.18%           
=======================================
  Files         912      912           
  Lines      257169   257171    +2     
=======================================
+ Hits       213914   213935   +21     
+ Misses      43255    43236   -19     
Flag Coverage Δ
fuzzcorpus 61.00% <0.00%> (-0.01%) ⬇️
livemode 19.41% <100.00%> (+<0.01%) ⬆️
pcap 44.40% <100.00%> (+0.03%) ⬆️
suricata-verify 62.80% <100.00%> (+0.01%) ⬆️
unittests 59.19% <0.00%> (-0.01%) ⬇️

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

Copy link
Member

@inashivb inashivb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests well. LGTM 🚀

@suricata-qa
Copy link

Information:

ERROR: QA failed on SURI_TLPR1_alerts_cmp.

field baseline test %
SURI_TLPR1_stats_chk
.app_layer.flow.ftp 32421 36200 111.66%
.app_layer.flow.dcerpc_tcp 40 43 107.5%
.app_layer.error.http.parser 700 729 104.14%
.app_layer.error.ssh.parser 124 128 103.23%
.ftp.memuse 2906 3102 106.74%

Pipeline 23717

@victorjulien victorjulien added this to the 8.0 milestone Dec 7, 2024
@suricata-qa
Copy link

Information:

ERROR: QA failed on SURI_TLPR1_alerts_cmp.

field baseline test %
SURI_TLPR1_stats_chk
.app_layer.flow.ftp 32421 36200 111.66%
.app_layer.flow.dcerpc_tcp 40 43 107.5%
.app_layer.error.http.parser 700 729 104.14%
.app_layer.error.ssh.parser 124 128 103.23%
.ftp.memuse 2906 3102 106.74%

Pipeline 23720

@victorjulien
Copy link
Member Author

@ct0br0 for the alert top 500 redistribution, how do the numbers look for the ones that dropped out of the top 500? Do they have at least the same number as in master?

@victorjulien
Copy link
Member Author

Merged in #12245, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants