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.

Sign up for GitHub

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

Jump to bottom

rec: Move to a stream based socket for the control channel #10930

Merged
merged 3 commits into from
Nov 3, 2021
Merged

rec: Move to a stream based socket for the control channel #10930

merged 3 commits into from
Nov 3, 2021

Conversation

omoerbeek
Copy link
Member

Short description

Nice consequences:

  • No more need to set buffer sizes or have a large read buffer.
  • No more need to create a temporary socket for replies, a session concept is built into SOCK_STREAM sockets and we can just write the reply into the stream.

Survived basic testing here.

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)

rgacogne
rgacogne approved these changes Oct 29, 2021
View reviewed changes
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, I did not test it.

pdns/pdns_recursor.cc Outdated Show resolved Hide resolved
@omoerbeek
Copy link
Member Author

Looks like this unveovered/introduced a bug in command line parsing/passing to the recursor

Oct 29 14:09:43 Received rec_control command 'dump-cacheX' via controlsocket

@omoerbeek
Do not read further than the length we received, the string might
c25917c 
be followed by a passed fd.

Interesting to see that OpenBSD chops up recvs based on the sends,
while Linux is happy to read more than was passed to the corresponding
send call if another send was called after that.
@omoerbeek omoerbeek merged commit afd2b96 into PowerDNS:master Nov 3, 2021
@omoerbeek omoerbeek deleted the rec-control-stream branch November 3, 2021 10:21
@omoerbeek omoerbeek mentioned this pull request Nov 15, 2021
Closed
@OrfeasZ OrfeasZ mentioned this pull request Jan 7, 2022
Merged
3 tasks
@dmgeurts
Copy link

Is there a way to revert/re-enable the old control protocol for 3rd party packages that haven't been updated yet? I'm specifically interested in Telegraf reporting as above.

@rgacogne
Copy link
Member

rgacogne commented Apr 21, 2022 •
edited
Loading

I don't think so, no, sorry :-/
Do you know if there is a specific reason the telegraf plugin does not use the HTTP API instead? I realize this mean actual work but that would be much less brittle.

@dmgeurts
Copy link

No idea, I'm just following their documentation of how to feed Influx with PowerDNS telemetry. Using the HTTP API would mean I don't need to deploy Telegraf to my PowerDNS servers.

@dmgeurts
Copy link

Found this which I'll try instead of using the unix-socket
influxdata/telegraf#812 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rgacogne rgacogne rgacogne approved these changes

Assignees
No one assigned
Labels
enhancement rec
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@omoerbeek @dmgeurts @rgacogne