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

dnsdist: dump more packet cache #14658

Merged
merged 8 commits into from
Oct 18, 2024
Merged

dnsdist: dump more packet cache #14658

merged 8 commits into from
Oct 18, 2024

Conversation

phonedph1
Copy link
Contributor

Idea from #14649

Short description

Include more readily accessible info from the packet cache.

Potentially need to change documentation to remove 'summary of the cache', or make the full packet stuff an optional argument.

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled this code
  • tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)

@coveralls
Copy link

coveralls commented Sep 11, 2024

Pull Request Test Coverage Report for Build 10975112632

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 1 of 8 (12.5%) changed or added relevant lines in 2 files are covered.
  • 1187 unchanged lines in 20 files lost coverage.
  • Overall coverage decreased (-0.004%) to 64.704%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pdns/dnsdistdist/dnsdist-lua-bindings-packetcache.cc 1 2 50.0%
pdns/dnsdistdist/dnsdist-cache.cc 0 6 0.0%
Files with Coverage Reduction New Missed Lines %
pdns/recursordist/sortlist.cc 2 72.94%
pdns/distributor.hh 2 51.86%
pdns/libssl.cc 3 59.01%
pdns/rcpgenerator.cc 3 90.14%
pdns/misc.cc 3 63.32%
pdns/shuffle.cc 4 39.71%
pdns/dnsdistdist/dnsdist-carbon.cc 6 63.51%
pdns/recursordist/test-syncres_cc1.cc 8 89.87%
pdns/dnsdistdist/dnsdist-tcp.cc 8 75.79%
pdns/recursordist/rec_channel.cc 8 65.88%
Totals Coverage Status
Change from base Build 10793790851: -0.004%
Covered Lines: 124808
Relevant Lines: 162224

💛 - Coveralls

@rgacogne
Copy link
Member

CIFuzz is failing because it now needs the Base64Encode symbol, which means we need to dnsdist-crypto.cc dnsdist-crypto.hh to fuzz_target_dnsdistcache_SOURCES in Makefile.am.

Copy link
Member

@rgacogne rgacogne left a comment

Choose a reason for hiding this comment

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

Code looks good to me, thanks a lot! I would indeed prefer to make the full packet stuff optional. Please let me know if you want me to take care of that.

@phonedph1
Copy link
Contributor Author

Code looks good to me, thanks a lot! I would indeed prefer to make the full packet stuff optional. Please let me know if you want me to take care of that.

I think I can in the next few days but if it's going to take you like 5 minutes feel free to just go for it :)

@rgacogne
Copy link
Member

I think I can in the next few days but if it's going to take you like 5 minutes feel free to just go for it :)

I'm in no rush and happy to leave it to you :)

@phonedph1
Copy link
Contributor Author

I'm in no rush and happy to leave it to you :)

I think it does what we want now :)

@rgacogne rgacogne self-requested a review September 19, 2024 07:53
@Habbie
Copy link
Member

Habbie commented Sep 20, 2024

i asked @phonedph1 to add an sdig hint to the doc before merge

@rgacogne
Copy link
Member

Looks good to me, thanks! The CI failure was unrelated, I restarted it 🤞

@omoerbeek
Copy link
Member

Looks good to me, thanks! The CI failure was unrelated, I restarted it 🤞

"Restart failed jobs" failed because artifacts are gone, will do a full restart.

@rgacogne rgacogne merged commit 09410c4 into PowerDNS:master Oct 18, 2024
79 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.

5 participants