dnsdist: add pooling support for RemoteLoggerInterface

[esensar]

Short description

This adds a new kind of RemoteLoggerInterface: RemoteLoggerPool. It can take multiple other RemoteLoggerInterfaces and pass data to them in round-robin order by default.

This also adds additional option to newRemoteLogger, newFrameStreamTcpLogger and newFrameStreamUnixLogger: connectionCount, which can be used to generate a pool with multiple connections.

Closes: #14861

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)

[esensar]

There is a lot of noise in pdns/dnsdistdist/dnsdist-lua-bindings-protobuf.cc due to formatting. Let me know if that needs to be fixed or if I should add formatting in a separate PR.

[miodvallat]

Can you split this work in two commits, one with most of the formatting/whitespace changes and another with the actual work? This would indeed help reviewing.

[coveralls]

Pull Request Test Coverage Report for Build 13237466080

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.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 106 of 179 (59.22%) changed or added relevant lines in 4 files are covered.
• 31 unchanged lines in 12 files lost coverage.
• Overall coverage increased (+0.02%) to 64.734%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pdns/remote_logger_pool.cc	6	10	60.0%
pdns/remote_logger_pool.hh	0	13	0.0%
pdns/dnsdistdist/dnsdist-lua-bindings-protobuf.cc	95	122	77.87%
pdns/dnsdistdist/dnsdist-configuration-yaml.cc	5	34	14.71%

Files with Coverage Reduction	New Missed Lines	%
modules/gpgsqlbackend/gpgsqlbackend.cc	1	88.62%
pdns/dnsdistdist/dnsdist-lua-bindings-protobuf.cc	1	76.81%
pdns/recursordist/test-rec-tcounters_cc.cc	2	95.77%
pdns/dnsdistdist/dnsdist-crypto.cc	2	75.72%
pdns/recursordist/aggressive_nsec.cc	2	66.32%
pdns/opensslsigners.cc	2	61.41%
pdns/recursordist/test-syncres_cc2.cc	3	88.85%
pdns/misc.hh	3	87.62%
pdns/recursordist/rec-tcpout.cc	3	61.9%
pdns/iputils.cc	3	56.07%

Totals
Change from base Build 13198501559:	0.02%
Covered Lines:	128325
Relevant Lines:	167185

---

💛 - Coveralls

[miodvallat]

Can't comment about the usefulness, but the code reads ok.

Can you update the meson rules so that the CI gets a chance to pass?

[esensar]

Can't comment about the usefulness, but the code reads ok.

Can you update the meson rules so that the CI gets a chance to pass?

Thank you. I went for a very simple approach for now. There was a discussion of different strategies (for picking the interface(s) to pass data to), but I think these are easy to add onto this, since it really just forwards data to one of the interfaces.

[miodvallat]

One concern, though, is that RemoteLoggerPool::queueData() is not thread-safe. Are you sure that there is no risk of multiple threads attempting to invoke it? If so, you need to protect the d_pool_it value changes with some serialization object.

[esensar]

One concern, though, is that RemoteLoggerPool::queueData() is not thread-safe. Are you sure that there is no risk of multiple threads attempting to invoke it? If so, you need to protect the d_pool_it value changes with some serialization object.

You are right. I have overlooked that. I can see that RemoteLogger has a lock in queueData, so I guess there is that risk, so something similar needs to be done here.

[esensar]

Also, I was wondering if I should change the implementation of toString() for this class. Right now it could produce a very long string, because it is showing information of all the underlying loggers.

The alternative would be to just show number of loggers and stats, but then that would hide the addresses. This also brings me to the name and address implementation. I am not sure what their exact purpose is, so I was not sure how to implement them for this, since it is not guaranteed that all loggers will be the same (it is right now, because the only usage is in generating multiple instances of the same logger, but in general case, it is not).

[rgacogne]

Also, I was wondering if I should change the implementation of toString() for this class. Right now it could produce a very long string, because it is showing information of all the underlying loggers.

I think a short summary, perhaps with aggregated metrics, would be fine. Something like "Pool of {number} remote loggers" plus the metrics?

The alternative would be to just show number of loggers and stats, but then that would hide the addresses.

Yeah, that's fine by me.

This also brings me to the name and address implementation. I am not sure what their exact purpose is, so I was not sure how to implement them for this, since it is not guaranteed that all loggers will be the same (it is right now, because the only usage is in generating multiple instances of the same logger, but in general case, it is not).

As far as I can tell address is unused in our codebase, so it can probably just go away. As for name, I'm fine with just displaying "Pool of {number} remote loggers".

[esensar]

I have added some basic tests for RemotePoolLogger, based on TCP framestream logger. I think it shouldn't result in flaky tests, but running it locally did fail sometimes with address already in use error for some reason. If they turn out to be okay, I can add the same for Unix socket based ones and the protobuf logger too.

[rgacogne]

One tiny nit, but the PR looks good to me, nice work!

[esensar]

One tiny nit, but the PR looks good to me, nice work!

Thank you! Do you think tests for other types of loggers are needed? The pool should work the same for all of them, so not sure if it is worth it to add more tests.

[rgacogne]

Do you think tests for other types of loggers are needed? The pool should work the same for all of them, so not sure if it is worth it to add more tests.

I'm always surprised by how many things that should work the same actually break at some point :) My own experience is that everything that is not tested is at best temporarily working, until it gets broken and no-one notices! So if it's easy to test the other cases I would love the extra tests, but I'm also fine with merging this PR as it is, or even adding the tests myself.

[esensar]

Do you think tests for other types of loggers are needed? The pool should work the same for all of them, so not sure if it is worth it to add more tests.

I'm always surprised by how many things that should work the same actually break at some point :) My own experience is that everything that is not tested is at best temporarily working, until it gets broken and no-one notices! So if it's easy to test the other cases I would love the extra tests, but I'm also fine with merging this PR as it is, or even adding the tests myself.

It should be fairly easy to add in the other tests too, if this one turns out okay, so I could add these too if you want.

[rgacogne]

It should be fairly easy to add in the other tests too, if this one turns out okay, so I could add these too if you want.

If you don't mind it would be awesome, but I'm also OK with merging this PR as it is.

[esensar]

I had some issues with tests for the RemoteLogger. It worked in the end, but I am not sure if it will work well on CI. I will revert it if it fails on CI.