bin/resque: add support for connecting with a cluster client

[jacobbednarz]

As bin/resque currently works, it pulls in the hostname to use from
the environment using getenv. The problem with this is that you cannot
pass in an array which is what the underlying Redis library uses to
determine whether it initialises a Credis_Client or a Credit_Cluster
for the connection.

This solves that issue by introducing support for passing a comma
separated list of hostnames to REDIS_BACKEND which will be expanded to
an array before passing to Credis_Cluster.

[danhunsaker]

It seems to me that simply having a comma in the host variable would indicate that cluster mode is desired. We can drop some configuration complexity by checking for a comma (stripos($host, ',') !== false), rather than introducing another environment variable.

[jacobbednarz]

We could however, that would mean for single hostname clusters, we'd need to have a wonky looking hostname like redis.local, for it to trigger the clustering client. With this proposal, we keep a single hostname but we flag we explicitly want to use the clustering client. Thoughts?

[danhunsaker]

What is the benefit of using the cluster driver with a single host?

[jacobbednarz]

In my specific use case, it's not a single host but a single hostname that is used for the entire cluster which is load balanced by a Kubernetes service.

[danhunsaker]

I must be missing something. The Cluster driver will still only create one connection per hostname, regardless of how many servers are actually behind that name (neither the Redis extension nor PHP stream connections have access to more than the first IP that a hostname resolves to, as is the norm for any software that doesn't explicitly work around that default behavior of OS-level hostname-lookup functions). So your Cluster still only has a single Client inside, giving you exactly zero benefit to using the driver in the first place, plus a bit of extra function call overhead for your trouble.

[jacobbednarz]

You’re correct about the one connection per hostname but using the cluster client is also about knowing how to find data on the (potentially) sharded cluster. If the regular client attempts a lookup and doesn’t hit the correct node on the first go, it will throw an exception as it (by design) doesn’t follow the redirection from Redis; the cluster client will and successfully fetches the key regardless of which mode has the data.

[danhunsaker]

Interesting. I saw exactly zero code that would handle such a difference in the Cluster driver itself. It doesn't even handle connecting - it makes the regular driver do it. All the Credis Cluster class does is wrap one or more (regular) Credis Client objects, forwarding requests to the specific client object and returning the results without further processing, based on what i just read earlier. So I wonder what mechanism it uses to do that?

In any case, I'm not against supporting the extra variable if we have to for things to function correctly, but i also think it should be an override for the single hostname edge case rather than an on/off switch for the entire feature. Commas in the host string should automatically select the Cluster driver accordingly whether the additional value is set or not.

[jacobbednarz]

How do you feel about supporting either? Single hostnames needing cluster support can add the new environment flag and multiple hostnames separated by a comma also automatically get cluster support? I’m just thinking if I came across a single hostname with a trailing comma in a config file, I’d be confused as to why it was like that. Having an extra comma may also limit the reuse of REDIS_BACKEND as an environment variable as everything that used it would then need to take that into account.

[danhunsaker]

That hybrid approach is what I was trying to describe above, yeah, so I'm on board with that.

[jacobbednarz]

Thanks for clarifying; that wasn't apparent to me. I've added support for comma delimitered hosts in REDIS_BACKEND as well.

[jacobbednarz]

Having a dig further into this, there is something not quite right either with Credis or our cluster. For the test, I'm doing the following:

• Grab the Credis library files

$ curl -o Client.php https://raw.githubusercontent.com/colinmollenhour/credis/master/Client.php
$ curl -o Cluster.php https://raw.githubusercontent.com/colinmollenhour/credis/master/Cluster.php

• Add a simple get from Redis

$ cat repro.php

<?php
require 'Client.php';
require 'Cluster.php';

$cluster = new Credis_Cluster(array(
  array('host' => 'svc.hostname.local', 'port' => 6379),
));

var_dump($cluster->get("example-key");

• Run the PHP file

$ php repro.php

PHP Fatal error:  Uncaught RedisException: MOVED 4701 10.36.230.134:6379 in /tmp/Client.php:1191
Stack trace:
#0 /tmp/Client.php(1191): Redis->get('example-key')
#1 /tmp/Cluster.php(253): Credis_Client->__call('get', Array)
#2 /tmp/repro.php(9): Credis_Cluster->__call('get', Array)
#3 {main}

Next CredisException: MOVED 4701 10.36.230.134:6379 in /tmp/Client.php:1209
Stack trace:
#0 /tmp/Cluster.php(253): Credis_Client->__call('get', Array)
#1 /tmp/repro.php(9): Credis_Cluster->__call('get', Array)
#2 {main}
  thrown in /tmp/Client.php on line 1209

I would expect that this works as the library mentions it support clustering, that this works out of the box. Probably hold on this PR until this is resolved in case I'm holding this wrong.

[jacobbednarz]

Looks like there is something funky with single hostnames as if I explicitly add all the IPs in the cluster instead, this works as expected with the bin/resque modifications. We might be able to ditch the REDIS_CLUSTER_ENABLED environment variable after all.

[jacobbednarz]

@danhunsaker do you happen to have a working example of multiple hostnames with Resque::setBackend? Passing in an array to Resque::setBackend doesn't seem to be working as I would expect, and it looks like only a single host ends up used in Credis.

Resque::setBackend([
  ["host" => "10.0.0.1", "port" => 6379],
  ["host" => "10.0.0.2", "port" => 6379],
]);

[jacobbednarz]

It's worth noting as well, Credis_Cluster doesn't support automatic discovery of other nodes. The only way clustering works is with explicit hostnames so comma separated lists are the only support approach now.

[pprkut]

resque-scheduler has the same block, so support should probably also be added there

[jacobbednarz]

I've finally got this working for our use case however, not as I originally intended.

The kicker is that Credis_Cluster != redis cluster compatible. Credis_Cluster is a poor man's version where it handles all the clients and hashes the keys to find where the data is stored despite none of the hosts knowing about the others.

This was an oversight on my behalf and confused the heck out of everything because I was expecting it to work in the same way where the client would follow the MOVE responses from redis cluster but it did not. We ended up solving our use case using an intermediate proxy that is redis compatible so I'm not going to pursue this PR any further. Someone else is welcome to pick this up if they see value in it but as we won't be using this, I can't in good faith land it without proper testing which I cannot perform in our setup now.

Thanks for your help though!

[mfn]

We ended up solving our use case using an intermediate proxy that is redis compatible

Can you share what proxy? Or is it internal? Thanks

[jacobbednarz]

envoy proxy with the redis cluster extension