MasterSlaveTopologyProviders fails to parse valid master host

[szabowexler]

The MasterSlaveTopologyProvider has a regular expression for matching the master host, which is:

^master_host\:([a-zA-Z\,\=\d\.\:]+)$

Unfortunately, this regex doesn't actually match all valid host names, since hostnames may contain hyphens. This won't be an issue when connecting to a master (since slaves are reported as resolved IP addresses), but it is an issue when connecting to an intermediate node (i.e. an instance which itself has a slave, but is also slaved to a master).

In that case, the slave retains the hostname of the master (rather than the IP), which causes the topology provider to throw an exception. I believe this is fixed by changing the regex to be:

^master_host\:([a-zA-Z\,\=\d\.\:\-]+)$

[mp911de]

Hi @szabowexler,

thanks for looking into that. Do you have a case where a master is referenced by its name? AFAIK Redis accepts only IP addresses and refuses to accept hostnames because it does not want to resolve hostnames.

[szabowexler]

We actually had this come up during some routine maintenance at HubSpot. It actually raises another issue, which I'm not sure how to file. We were swapping a master (with the intent to sustain no downtime). The redis docs recommend setting the replacement as a slave to the old master, making it writeable, directing all traffic to the slave, and then once the old master is not serving any traffic, promoting the slave to master. At this time, we found out that:

1. When you SLAVEOF, the hostname is just a fully qualified domain name, so something like mountain-top.subdomain1.subdomain2 is just as valid as an IP address. While the master stores slaves as IP addresses, the slaves stores the master as-is. This means the topology provider will hit the slave, fail to parse, and die.
2. In order to do a master swap with no downtime, you must write to a slave with read-only disabled. Lettuce does not support this at all, at present. I think it probably needs a write routing option (with either MASTER or WRITE_ENABLED_SLAVE_PREFERRED). The former makes sense when swaps may incur downtime, but the latter is required for downtime-free swaps.

[mp911de]

Thanks for the insight. I'll fix the topology provider regarding the hostname parsing. It might be worth opening a ticket in Redis to fix the hostname issue on the slave side so Redis behaves consistently.

The second point is a bit harder to achieve: Lettuce makes a distinction regarding read-write and read-only commands. Only if a read-only command is issued, the slave routing kicks in. This decision is made in com.lambdaworks.redis.masterslave.MasterSlaveChannelWriter but the supporting classes are not visible from outside a package. There's a similar ticket, #338, to expose an SPI so users can customize topology refresh/discovery/and so on.

With exposing the refresh/topology provider/writer classes, you'd be able to subclass MasterSlaveChannelWriter and provide your own read-only/read-write resolution. You'd also be required to signal the topology change on your own. Static Master/Slave runs a refresh only at the very beginning since there's no trigger that indicates a topology change (in contrast to Redis Sentinel).

[mp911de]

That's fixed now.

[szabowexler]

Re: topology, this seems like something lettuce should support. As-is, lettuce is mutually incompatible with the redis recommended downtime free master swap (which is pretty essential for any production deployment of redis).