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

Features/dropqueuedpackets #882

Merged

Conversation

vvfedorenko
Copy link
Contributor

This pull request adds sock_queue_timeout configuration options which stores the amount of seconds spend in the socket queue after which the packet is considered old and should be dropped. The reason to drop is because client won't be waiting for the reply anymore and there is no reason to process the query.
There are also 2 metrics added:

  • num.queries_timed_out to store the amount of dropped packets
  • query.queue_time_us.max to collect the max time that was found for packets to wait in the socket queue (this can be easily converted to histogram if needed)

Remove config parser/lexer code as it's rebuilded every time but can
break adding new config options.
Also clean up the code base to avoid mixing actual code changes and lint
issues.

Signed-off-by: Vadim Fedorenko <vadfed@meta.com>
Copy link
Member

@wcawijngaards wcawijngaards left a comment

Choose a reason for hiding this comment

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

Thank you for the nice code and some code cleanup, that implements the timestamping feature! The code looks nice, I have but a few smaller suggestions.

Is it okay if I put the banner over the new files util/timeval_func.c and util/timeval_func.h, that contain the moved timeval functions from other places in the source code? I mean to put the license text on the file, like we have for other files. And the Copyright statement, for NLnet Labs above the file as well, like for other files. That makes it similar to the other code we have. NLnet Labs wants to have the copyright and also the license on the files to maintain the project. I think it is nice to clean up the code with them.

util/config_file.c Outdated Show resolved Hide resolved
util/netevent.c Outdated Show resolved Hide resolved
util/netevent.c Outdated Show resolved Hide resolved
@vvfedorenko
Copy link
Contributor Author

Yep, sure, you can put statements, there is no new code in these files, it's just copy-paste and cleanup.
Let me rebase and update the PR with the suggestions applied

There are several definitions of the same functions manipulating timeval
structures. Let's move them to separate file and arrange the code
preperly.

Signed-off-by: Vadim Fedorenko <vadfed@meta.com>
Add special field in comm_point to store the software receive timestamp
for every particular UDP packet. Aux data parser is updated to read
values and the whole callback is switched to use recvmsg form.

Signed-off-by: Vadim Fedorenko <vadfed@meta.com>
Add sock_queue_timeout config option to have queue timeout configurable.

Signed-off-by: Vadim Fedorenko <vadfed@meta.com>
Add counter `num_queries_timed_out` meaning queries that were sitting in the
socket queue and waiting to being processed too long. There is no reason
to process such queries, so let's drop it in the very beginning of the
pipeline.

Signed-off-by: Vadim Fedorenko <vadfed@meta.com>
Add new statistic value to know the size of the queue in microseconds.

Signed-off-by: Vadim Fedorenko <vadfed@meta.com>
@wcawijngaards
Copy link
Member

With the adjustments, I'll approve and merge this. Thank you for the additional feature and cleanup!

After that, I will likely make a couple of smaller changes; spelling error, documentation mentions, and Changelog entry, to adjust the code a little. Thanks for the header license change, that is what I wanted.

@wcawijngaards wcawijngaards merged commit 7081b03 into NLnetLabs:master Apr 26, 2023
wcawijngaards added a commit that referenced this pull request Apr 26, 2023
  util/timeval_func.c and util/timeval_func.h. Man page entries and
  example entry.
@vvfedorenko vvfedorenko deleted the features/dropqueuedpackets branch April 26, 2023 17:20
jedisct1 added a commit to jedisct1/unbound that referenced this pull request May 25, 2023
* nlnet/master: (39 commits)
  - Fix unbound-dnstap-socket time fraction conversion for printout.
  - Fix unbound-dnstap-socket printout when no query is present.
  - Fix to remove unused variables from RPZ clientip data structure.
  - Fix RPZ removal of client-ip, nsip, nsdname triggers from IXFR.
  - Fix to print debug log for ancillary data with correct IP address.
  - Fix NLnetLabs#888: [FR] Use kernel timestamps for dnstap.
  - Fix warning in windows compile, in set_recvtimestamp.
  - Fix doxygen in addr_to_nat64 header definition.
  - Fix to remove unused whitespace from acx_nlnetlabs.m4 and config.h.
  - Fix NLnetLabs#885: Error: util/configlexer.c: No such file or directory,   adds error messages explaining to install flex and bison.
  Changelog entry for NLnetLabs#722: - Merge NLnetLabs#722 from David 'eqvinox' Lamparter: NAT64 support. - For NLnetLabs#722: minor fixes, formatting, refactoring.
  - For NLnetLabs#722: Minor fixes, formatting and refactoring.
  - Fix RPZ IP responses with trigger rpz-drop on cache entries, that   they are dropped.
  Changelog for NLnetLabs#860
  Remove msg_del_for_0ttl, call msg_cache_remove directly
  - Fix for NLnetLabs#882: document variable to stop doxygen warning.
  - Fix for NLnetLabs#882: small changes, date updated in Copyright for   util/timeval_func.c and util/timeval_func.h. Man page entries and   example entry.
  stats: add query max wait time metric
  stats: add counter for timed out queries
  config: add sock_queue_timeout configuration
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants