agent: move DNSServer to apiServers #8623
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dnephin
added
theme/internal-cleanup
dnephin
commented
Sep 4, 2020
Comment on lines
-680
to
-731
| a.logger.Info("Started DNS server", | ||
| "address", addr.String(), | ||
| "network", addr.Network(), | ||
| ) |
There was a problem hiding this comment.
After this change, this log message comes from apiServers.Start
Comment on lines
+662
to
+714
| // Unlike the HTTP server, the dns.Server listens and serves in the same call. | ||
| // We could fix this by calling ActivateAndServe instead of ListenAndServe, | ||
| // however that would require duplicating the network switch/case in | ||
| // ListenAndServe. Do we actually support more than one network type? | ||
| // For now we are handling this difference by waiting up to a second for | ||
| // the listener to listen. | ||
| // TODO: fix this by using dns.Server.ActivateAndServe |
There was a problem hiding this comment.
This was the old behaviour as well. I've kept it for now, but I think we can change it in the future. It'll be much easier to test with the DNSServer is better isolated from Agent.
15 tasks
Base automatically changed from
dnephin/better-impl-of-TestAgent.HTTPAddr
to
master
September 17, 2020 15:48
In preparation for moving the dns servers into apiServers. This will allow us to use a standard Reloadable interface for all reloadable types.
And use agent.configReloaders for config reloading
dnephin
force-pushed
the
dnephin/move-dns-server-to-apiServers
branch
from
e1b6b99
to
ca015bc
Compare
dnephin
added
the
pr/no-changelog
dnephin
commented
May 21, 2021
| return apiServer{ | ||
| Protocol: "dns", | ||
| Addr: addr, | ||
| Run: srv.ListenAndServe, |
There was a problem hiding this comment.
I think this will need to log the error from ListenAndServe, otherwise we won't see it until Shutdown.
|
|
|
Closing as this is over two years old. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR continues the work done in #8234 to ensure that the agent shuts down if one of the long running servers fails, instead of continuing in a degraded state. This is very likely even more important for the DNS server because if a process manager is checking the health of the Consul Agent, it may be doing so with the HTTP API, and wouldn't necessarily noticed if the DNS server had failed.