Sentinel: resolve master_host to IP address#2118
Conversation
If we don't resolve hosts to IP addresses, Sentniel compares a slave's masterhost (potentially a hostname) against the master's slave list (only IPs). This commit resolves hostnames provided by replicas so we can string compare IP->IP instead of Hostname->IP. During any failures, Sentinel will use IP addresses to re-parent replicas or rewrite the main config, so the hostnames are for convenience only and never get persisted in the Sentinel config. If the resolve fails, we emit a "-noresolve" event and set the local IP field equal to the non-existing hostname anyway. (Note: since the Redis server is _already_ connected to that hostname, it is *very* unlikely that Sentniel can't also resolve the same hostname because Redis already had to resolve it when it ran `SLAVEOF hostname port`.) Fixes redis#2075 Fixes redis#2032
|
Hello Matt, thanks for the PR. I'm right that here if Sentinel fails to resolve an address it hangs for a very long time becoming unavailable? This should propagate easily to all the Sentinels in the cluster AFAIK, once they scan the master INFO, so the set of Sentinels all stop responding, and if the master fails, no failover is performed. I've the feeling this is not an acceptable solution, we need ability to resolve synchronously but with a short timeout, and if not possible, mark the slave as non usable and provide an error in the Sentinel logs. Alternative solution: just validate the IP address, if resolution is needed, provide an error. Basically if the PR, as I'm guessing, does blocking, non timed out resolution, it is unsafe to merge. |
The timeout is the DNS lookup timeout for reaching the local resolver. On Linux the default timeout is 5 seconds. The only way to do "correct" non-blocking DNS lookups is to either use modern (not-available-everywhere) Linux APIs (getaddrinfo_a) or create async DNS lookup functions in the event loop core: https://github.com/libuv/libuv/blob/v1.x/src/unix/getaddrinfo.c#L140
That is one possibility, but people do like using hostnames. :-\ But, forcing IP-only is also very simple to implement since Redis already has IP-validation functions. :) So, the fix is either allow hostnames — or — explicitly deny hostnames in SLAVEOF (both config and command) to stop an unnecessary Sentinel sync. |
|
Hm... just tested in my Linux system and it's 20 seconds. The INFO output is scanned every 10 seconds, so we would block again and again and again for 5 (or maybe 20) seconds. So basically the system would fail. IMHO it's not black and white: vanilla libc resolver or asynchronous resolution, you can easily implement something that is able to resolve names with milliseconds resolution timeouts. I'm not sure this is enough, I'll check better, but possibly we have also to take some state about the ability to resolve or not a given name, to make sure the system behaves correctly (does not do silly things at least) if the resolution fails. To implement the resolver with reasonable timeouts there are many solutions. Sentinel could spawn a bio.c thread in theory, or a process that does this service for Sentinel via some pipe, or something like that. The problem with that is that the resolving service should be able to cache failures, to avoid blocking again and again on the same name if it was not possible to resolve it lately. IMHO we should fix it properly or deny the feature for now. |
Yeah, for now it's easier to update mattsta@4d522c3 and change We can try proper name resolution later because it'll require extensive rewrites to make things more async in places. |
|
@antirez are there any plans to work with hostnames at the moment? |
|
This issue is addressed by #8282. |
Background
This problem has been reported multiple times on the mailing list and here in GitHub Issues.
People sometimes configure replicas using "SLAVEOF masterhost.internal.corp 6379", then Sentinel shows up, compares the hostname to the IP address on the master, then re-runs SLAVEOF IP PORT even though the slave is already connected to the correct server.
Potential Fixes
The bad "comparing hostname to IP address" issue can be fixed in two places:
master_hostaddressesAlternatively, we could just deny all hostnames by forcing IP-only in the SLAVEOF command. That seems like an extreme fix because hostnames are useful things to have.
This Fix
If we don't resolve hosts to IP addresses, Sentniel compares
a slave's masterhost (potentially a hostname) against the
master's slave list (only IPs).
This commit resolves hostnames provided by replicas so
we can string compare IP->IP instead of Hostname->IP.
During any failures, Sentinel will use IP addresses to
re-parent replicas or rewrite the main config, so the
hostnames are for convenience only and never get persisted
in the Sentinel config.
If a replica goes away and comes back, Sentinel will re-attach it to the master using the master's IP address and not the previous hostname that was configured. Sentinel internally still operates 100% on IP addresses except for comparing the
master_hostfield on replicas against master IP addresses.If the resolve fails, we emit a "-noresolve" event and
set the local IP field equal to the non-existing hostname
anyway. (Note: since the Redis server is already connected
to the hostname it reports in INFO, it is very unlikely that Sentniel can't also
resolve the same hostname because Redis already had to resolve
it when somebody configured
SLAVEOF hostname port.)Also, now
SENTNIEL SLAVES mymasterreportsmaster-hostas the hostname andmaster-host-ipas the resolved IP address of the hostname.Example:
Fixes #2075
Fixes #2032