Using Redis with sentinels

[mazuryv]

We use Redis distributed cache with 3 sentinels:
1Master + 1 sentinel in first PC
1Replica + 2 sentinels in second PC
Connection configuration:

    "OrchardCore_Redis": {
      "Configuration": "localhost:26379,password=somePassword,serviceName=redis-orchard,allowAdmin=true", 
      "InstancePrefix": "orchard" // Optional prefix allowing a Redis instance to be shared by different applications.
    }

For using this configuration, we use main branch https://github.com/StackExchange/StackExchange.Redis (previous versions, include last release [2.5.61], throw error about getting information about replica).

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <RootNamespace>OrchardCore.Redis</RootNamespace>
    <!-- NuGet properties-->
    <Title>OrchardCore Redis Abstractions</Title>
    <Description>$(OCFrameworkDescription)

    Abstractions for Redis.</Description>
    <PackageTags>$(PackageTags) OrchardCoreFramework Abstractions</PackageTags>
  </PropertyGroup>

  <ItemGroup>
    <FrameworkReference Include="Microsoft.AspNetCore.App" />
  </ItemGroup>

  <ItemGroup>
    <PackageReference Include="Microsoft.AspNetCore.DataProtection.StackExchangeRedis" />
    <PackageReference Include="Microsoft.Extensions.Caching.StackExchangeRedis" Version="5.0.1" />
    <PackageReference Include="StackExchange.Redis" Version="2.6.34-gc83443aaaf" />
  </ItemGroup>

  <ItemGroup>
    <ProjectReference Include="..\OrchardCore.Abstractions\OrchardCore.Abstractions.csproj" />
  </ItemGroup>

</Project>

Steps to reproduce the behavior:

1. Configure OrchardCore_OrchardCore_Redis
2. Enable feature OrchardCore.Redis.Cache (feature OrchardCore.Redis is enabled as dependency)
3. Error is thrown: "Sentinel: The ConnectionMultiplexer is not a Sentinel connection. Detected as: Stanalone"

How it works in code:

1. OrchardCore.Redis.Startup ConfigureServices(IServiceCollection services) configures RedisOption from configuration["OrchardCore_Redis:Configuration"].
2. When OrchardCore.Redis is started OrchardCore.Redis.Services.RedisServer connects to Redis with ConnectAsync(_options.Value.ConfigurationOptions) (use configured in step 1)
3. When StackExchange.Redis connecting to localhost:26379 (sentinel service), ConfigurationOptions is changing because sentinel service reports about master and replica servers.
4. After connection ConfigurationOptions is set as "[1.1.1.1:6379,2.2.2.2:6379],password=somePassword,serviceName=redis-orchard,allowAdmin=true". This action changes RedisOption.ConfigurationOptions too.
5. When OrchardCore.Redis.Cache is started AddStackExchangeRedisCache use RedisCacheOptionsSetup for connection to Redis.
6. RedisCacheOptionsSetup uses RedisOption.ConfigurationOptions (now it is [1.1.1.1:6379,2.2.2.2:6379],password=somePassword,serviceName=redis-orchard,allowAdmin=true) and when StackExchange.Redis start connection it get first EndPoint (1.1.1.1:6379)
7. Because ConfigurationOptions contains serviceName=redis-orchard, StackExchange.Redis defines connection how sentinel but ServerSelectionStrategy.ServerType (by EndPoint) is Standalone and error is thrown.

My propose:
RedisCacheOptions and RedisOption should be distinct.

1. OrchardCore.Redis.Options.RedisCacheOptionsSetup should be deleted.
2. RedisCacheStartup should configure RedisCacheOptions from configuretion.

    [Feature("OrchardCore.Redis.Cache")]
    public class RedisCacheStartup : StartupBase
    {
        private readonly string _tenant;
        private readonly IShellConfiguration _configuration;
        private readonly ILogger _logger;

        public RedisCacheStartup(ShellSettings shellSettings, IShellConfiguration configuration, ILogger<Startup> logger)
        {
            _tenant = shellSettings.Name;
            _configuration = configuration;
            _logger = logger;
        }

        public override void ConfigureServices(IServiceCollection services)
        {
            if (services.Any(d => d.ServiceType == typeof(IRedisService)))
            {
                try
                {
                    var configurationOptions = ConfigurationOptions.Parse(_configuration["OrchardCore_Redis:Configuration"]);
                    var instancePrefix = _configuration["OrchardCore_Redis:InstancePrefix"];

                    services.Configure<RedisCacheOptions>(options =>
                    {
                        options.ConfigurationOptions = configurationOptions;
                        options.InstanceName = instancePrefix;
                    });
                }
                catch (Exception e)
                {
                    _logger.LogError("'Redis.Cache' features are not active on tenant '{TenantName}' as the 'Configuration' string is missing or invalid: " + e.Message, _tenant);
                    return;
                }

                services.AddStackExchangeRedisCache(o => { });
                services.AddScoped<ITagCache, RedisTagCache>();
            }
        }
    }

In this case all works as expected.

[jtkech]

Yes, just saw this issue about sentinel support dotnet/aspnetcore#28367

Okay, can you start a PR so that we could track the suggestion?

At least we would need to differentiate the InstanceName with the tenant name (an empty string for the Default tenant/shell) as done in RedisCacheOptionsSetup

options.InstanceName = _redisOptions.Value.InstancePrefix + _tenant;

Maybe keep the RedisCacheOptionsSetup and let it inject IShellConfiguration to retrieve config data, and find a way to not have to parse the configuration in 2 places.

Hmm, what we could do is to introduce an OriginalConfigurationOptions field to RedisOptions that would be initialized with the same value, and then just use it in RedisCacheOptionsSetup.

What do you think ;)

[mazuryv]

Thank you for your answer.
After OrchardCore.Redis.Services.RedisServer is connected RedisOptions.ConfigurationOptions is changed already. Clone and copy are not helped.
If I store same options as OriginalConfigurationOptions exception is thrown too.

                var configurationOptions = ConfigurationOptions.Parse(_configuration["OrchardCore_Redis:Configuration"]);
                var instancePrefix = _configuration["OrchardCore_Redis:InstancePrefix"];

                services.Configure<RedisOptions>(options =>
                {
                    options.ConfigurationOptions = configurationOptions;
                    options.OriginalConfigurationOptions = configurationOptions;
                    options.InstancePrefix = instancePrefix;
                });

We should create it twice such as

                var configurationOptions = ConfigurationOptions.Parse(_configuration["OrchardCore_Redis:Configuration"]);
                var originalConfigurationOptions = ConfigurationOptions.Parse(_configuration["OrchardCore_Redis:Configuration"]);
                var instancePrefix = _configuration["OrchardCore_Redis:InstancePrefix"];

                services.Configure<RedisOptions>(options =>
                {
                    options.ConfigurationOptions = configurationOptions;
                    options.OriginalConfigurationOptions = originalConfigurationOptions;
                    options.InstancePrefix = instancePrefix;
                });

Or we can change RedisOptions more deeply.

public class RedisOptions
    {
        /// <summary>
        /// The configuration used to connect to Redis.
        /// </summary>
        public ConfigurationOptions ConfigurationOptions => ConfigurationOptions.Parse(OriginalConfigurationString);//maybe add try/catch there
        /// <summary>
        /// Original configuration string used to connect to Redis
        /// </summary>
        public string OriginalConfigurationString { get; set; }
        /// <summary>
        /// Prefix alowing a Redis instance to be shared.
        /// </summary>
        public string InstancePrefix { get; set; }
    }

and use it like:

                var originalConfigurationString = _configuration["OrchardCore_Redis:Configuration"];
                var instancePrefix = _configuration["OrchardCore_Redis:InstancePrefix"];

                services.Configure<RedisOptions>(options =>
                {
                    options.OriginalConfigurationString = originalConfigurationString;
                    options.InstancePrefix = instancePrefix;
                });

What do you think?

[jtkech]

Can you try

            services.Configure<RedisOptions>(options =>
            {
                options.ConfigurationOptions = configurationOptions.Clone();
                options.OriginalConfigurationOptions = configurationOptions;
                options.InstancePrefix = instancePrefix;
            });

[mazuryv]

The same error is thrown.

                var configurationOptions = ConfigurationOptions.Parse(_configuration["OrchardCore_Redis:Configuration"]);
                var instancePrefix = _configuration["OrchardCore_Redis:InstancePrefix"];

                services.Configure<RedisOptions>(options =>
                {
                    options.ConfigurationOptions = configurationOptions.Clone();
                    options.OriginalConfigurationOptions = configurationOptions;
                    options.InstancePrefix = instancePrefix;
                });

        public void Configure(RedisCacheOptions options)
        {
            options.InstanceName = _redisOptions.Value.InstancePrefix + _tenant;
            options.ConfigurationOptions = _redisOptions.Value.OriginalConfigurationOptions;
        }

Looks like Clone is not work as expected.

[mazuryv]

Maybe will be better use public string Configuration { get; set; } as in ```Microsoft.Extensions.Caching.StackExchangeRedis.RedisCacheOptions````

[jtkech]

Okay, maybe a not so deep clone, thanks for trying it.

Will see tomorrow.

Hmm maybe the 2 parsings solution in the main Redis feature startup ConfigureServices(), where we are sure that both are only executed in this ConfigureServices() when building the tenant container.

[jtkech]

@mazuryv

Okay, so for me your best suggestion is to just add the 2 following lines.

            var originalConfigurationOptions = ConfigurationOptions.Parse(_configuration[...]);
            ...

            services.Configure<RedisOptions>(options =>
            {
                ...
                options.OriginalConfigurationOptions = originalConfigurationOptions;
                ...
            });

And then just use OriginalConfigurationOptions in RedisCacheOptionsSetup

Would you like to suggest a PR?

[mazuryv]

I think that

    public class RedisOptions
    {
        /// <summary>
        /// The configuration used to connect to Redis.
        /// </summary>
        public ConfigurationOptions ConfigurationOptions => ConfigurationOptions.Parse(Configuration);
        /// <summary>
        /// Original configuration string used to connect to Redis
        /// </summary>
        public string Configuration { get; set; }
        /// <summary>
        /// Prefix alowing a Redis instance to be shared.
        /// </summary>
        public string InstancePrefix { get; set; }
    }

is better, because ConfigurationOptions always get correct configuration.
If we use OriginalConfigurationOptions and connect to it, ConfigurationOptions and OriginalConfigurationOptions are broken, and we cannot use RedisOptions in our code.
In this case, I need OriginalConfigurationOptions1, OriginalConfigurationOptions2...

[jtkech]

Okay I got it, will take a look at the PR, thanks