Implement initial RedisCluster client integration

It seems that we didn't implement tls connection for PhpRedis, and I found this Redis cluster not considering host protocol for masters
Now I work with the AWS ElasticCache Redis cluster, and Encryption in transit is enabled, so it needs to connect with TLS.

I made a simple patch based on 2900947-57.patch →

Add the SSL context parameter
$client = new \RedisCluster($this->getClusterName(), $this->getSeeds(), $this->getTimeout(), $this->getReadTimeout(), $this->getPersistent(), $this->getPassword(), ['verify_peer' => false]);

Now I tested on AWS ElasticCache Redis cluster, it works good.

Patch #49 works fine for me until there's auto-scaling(adding shards) & slot redtribution. I understand with pipeline we will get MOVED server error, but is there a way to handle & retry this? Otherwise it defeats the purpose of going for Cluster Enabled Elasticache if we can't autoscale.

#58 is a huge patch that seems to change file endings or so.

Can we convert this to a merge request? We have gitlab testing now and while the cluster integration won't be able to be tested there unless someone figures out how to set up a cluster with multiple redis docker services, it will at least help to make sure that no existing feature breaks.

1. +++ b/src/Cache/PredisClusterCacheTagsChecksum.php
   @@ -0,0 +1,33 @@
   +  public function invalidateTags(array $tags) {
   +    $keys_to_increment = [];
   +    foreach ($tags as $tag) {
   +      // Only invalidate tags once per request unless they are written again.
   +      if (isset($this->invalidatedTags[$tag])) {
   +        continue;
   +      }
   +      $this->invalidatedTags[$tag] = TRUE;
   +      unset($this->tagCache[$tag]);
   +      $keys_to_increment[] = $this->getTagKey($tag);
   +    }
   +    if ($keys_to_increment) {
   +      $pipe = $this->client->pipeline();
   +      foreach ($keys_to_increment as $key) {
   

   why does this override invalidateTags() and not doInvalidateTags()? Is it possible this predates the refactoring that was done there?

2. +++ b/src/Controller/ReportController.php
   @@ -168,20 +169,19 @@ class ReportController extends ControllerBase {
    
   @@ -192,15 +192,19 @@ class ReportController extends ControllerBase {
   
   @@ -192,15 +192,19 @@ class ReportController extends ControllerBase {
          ],
          'version' => [
            'title' => $this->t('Version'),
   -        'value' => $info['redis_version'] ?? $info['Server']['redis_version'],
   +        'value' => $info['redis_version'],
   +      ],
   +      'mode' => [
   +        'title' => $this->t('Mode'),
   +        'value' => Unicode::ucfirst($info['redis_mode']),
          ],
          'clients' => [
            'title' => $this->t('Connected clients'),
   -        'value' => $info['connected_clients'] ?? $info['Clients']['connected_clients'],
   +        'value' => $info['connected_clients'],
   

   this seems to revert a lot of changes that were done to handle different info structures. There's also a .rej and .org file in the patch.

3. +++ b/src/Flood/PredisCluster.php
   @@ -0,0 +1,11 @@
   +/**
   + * Defines the database flood backend. This is the default Drupal backend.
   + */
   +class PredisCluster extends Predis {
   

   copy paste of an already wrong docblock?

Let me tidy it up and rebase onto the MR.

re #60

1. I'll look into refactoring this
2. All of that logic got pushed into $this->info()
3. Fixed

Hm, it might not run the test because you're not a maintainer, I can't really see a way in the UI to trigger it, clicking on run pipeline asks me but doesn't seem to do anything.

Posted review comments in gitlab.

I've added some functionality to allow setting of some options for both PhpRedisCluster and PredisCluster.
I was missing the 'scheme' option and the timeout options.

As seen here, https://github.com/phpredis/phpredis/blob/develop/redis_cluster.stub.php..., the RedisCluster class allows for a context-array to be passed into the constructor. (It's not documented properly yet, and I can't find the issue I came across that mentions this method argument :) )

With my addition, it's possible to set the scheme to tls like so: $settings['redis.connection']['scheme'] = 'tls';
The options `timeout` and `read_timeout` are also used.

@SpadXIII the latest changes don't seem to match with what's allowing in the RedisCluster constructor.

public function __construct($name, $seeds, $timeout = null, $readTimeout = null, $persistent = false, $auth = null) {}

There's no parameter for an array of context values.

Hi,

Both patches #57 or #58 are not applying any longer with the latest version of the module drupal/redis (1.7.0).

Is anyone else seeing the same ?

On a separate note, we are using this cluster implementation on production for more than 6 months and is working fine for us as per #57.

I encourage maintainers to commit this work so we avoid the hassle re-working this complex patch after every module update.

Thanks

Yes I agree with #69, we've been running this for a long time and the rerolls are getting quite difficult. It would be great if we could commit this in some form and open followup(s) for any remaining changes.

@Berdir - is there some compromise we can come to? What absolutely needs to be done before this gets committed?

The patch are not applying on the latest version of the module, we need to work on this.

@mihaic

Can you please provide version of redis module used where patch #57 working fine.

Caching with the new redis cluster implementation seems to work fine on our Drupal 10!

Only the recently added flush logic ( 🐛 Currently Drush Cr or Cache Clear UI does not flush Redis cache Fixed does not work. If I am not mistaken there are no databases in the cluster implementation - so the check for the database does not make sense for redis cluster right?

Hi,
Since someone requested it, here's the latest patch

This should have been set to NR again with the feedback addressed all the way back in Feb last year.

We have been running this patch for years at this point. If this can't be committed yet, can we please get a detailed list of steps to get it to a point where it can be?

We are using the following settings for our AWS ElasticCache Serverless (Cluster mode on) Redis with TLS enabled.

$settings['redis.connection']['interface'] = 'PhpRedisCluster';
$settings['redis.connection']['seeds'] = [
  "{redis_host}:{redis_port}"
];
$settings['redis.connection']['scheme'] = 'tls';

It is working fine on our end.

Again this is now heavily conflicting with more code committed to 8.x-1.x...

I've gone back through https://git.drupalcode.org/project/redis/-/commits/8.x-1.x/src/Controlle... and added all changes back to the ReportController instead of trying to fix the insane conflicts (due to the refactors to that controller on this branch).

If we can get this in without having to reroll again it would make my year 😅

Here is the re-roll of the MR against 1.8.

If you are using the failover patch ( https://www.drupal.org/node/3102739 → ) along with this change, apply the following patch.

This will need some adjustments for the recent changes that went in 8.x-1.9 (which is being created as I'm writing this), specifically the last write timestamp optimization: 📌 Optimize bin cache tags and last write timetamp Active .

> If we can get this in without having to reroll again it would make my year 😅

Sorry, as we've discussed a while ago, I'm the sole maintainer of this and I have no experience with redis cluster, so it's hard for me to commit to maintaining that.

For the reports page, there are two open issues about problems with the reports page. It would be _great_ if someone would extract the approach from this, which I really like, ensure that the two other issues are also covered (minus the small cluster specific parts). Then we can get that in, which will greatly reduce the conflict potential this has.

That would also help us move into the direction of an idea for a 2.x release of this project that I have, that would introduce an adapter that sits between cache/lock/queue/tag backends and the actual libraries and abstracts the relevant bits of the different implementations into something that each adapter can then customize. I don't know yet if that's feasible but that has the potential to be a major simplification for stuff like this, this info() method could then move into that adapter.

As for the remaining bits of this, this issue has 50 followers and cluster setups implies that there might be budget so I could invest a few hours into setting up a cluster test setup, on CI if possible, and then we could have this covered with tests. Reach out to me if you're interested.