-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Add in option to re-resolve DNS names #4793
Conversation
This adds in the ability to re-resolve DNS names used in a connection of an external database. The default is to do nothing new. A new command line options `pool_hostname_update_rate` has been added that will resolve the hostnames used in each pool at the given rate and will force a reconnect if they have changed Signed-off-by: Dan Kozlowski <koz@planetscale.com>
Adding locking around accessing host name and putting back connection to prevent leak Signed-off-by: Dan Kozlowski <koz@planetscale.com>
What's the status here? |
if cp.resolutionFrequency > 0 && | ||
net.ParseIP(cp.info.Host) == nil && | ||
!cp.validAddress(net.ParseIP(r.(*PooledDBConnection).RemoteAddr().String())) { | ||
err := r.(*PooledDBConnection).Reconnect() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little worried that we're passing in the hostname (not the IP) to the mysql client lib, and then we're separately resolving it ourselves and expecting both to match. If for some reason they don't match, we will enter a hot loop of reconnecting. For example, maybe the mysql client lib talks to a different local resolver with its own separate cache, and maybe the caches don't invalidate at the same time.
Would it be possible for us to resolve DNS in only one place (here) and give the mysql client lib a resolved IP address? Or does the mysql client lib need to know the hostname for some reason?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The client doesn't need the hostname, but we compare the actual IP address from connection to the list of all acceptable connections from DNS. If you are ever in a place where you can not know the full list of all acceptable IP's for a given DNS name then this feature will never work for you. So if there is concern about different resolvers giving different IP's you need to turn this off ( which is the default )
In the event of a DNS failover you will enter a hot loop of reconnecting until the pool_hostname_resolve_interval
elapses and then it will sync back up. We could trigger a dns refresh on invalid connection, but that would have to be done while holding the mutex which I think would be more disruptive then just allowing the connections to reconnect
This changes the name of the setting to `pool_hostname_resolve_interval` and adds in a waitgroup to make sure the gofunc finishes before we close the pool Signed-off-by: Dan Kozlowski <koz@planetscale.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with one optional nit.
go/vt/dbconnpool/connection_pool.go
Outdated
ticker *time.Ticker | ||
stop chan struct{} | ||
wg sync.WaitGroup | ||
hostIsIP bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this should be hostIsNotIP
Signed-off-by: Dan Kozlowski <koz@planetscale.com>
This adds in the ability to re-resolve DNS names used in a connection
of an external database. The default is to do nothing new. A new
command line options
pool_hostname_update_rate
has been added thatwill resolve the hostnames used in each pool at the given rate and
will force a reconnect if they have changed
Signed-off-by: Dan Kozlowski koz@planetscale.com