Sentinel Support Derived from pr-692

[shadim]

This PR is derived from PR-692 and have been merged with the latest master commit.
Things that have been done:

1. review code for PR-692
2. fixed potential infinite loop in the code
3. Adapt code to success build with the latest master commit
4. Manual testing on 3 Sentinel nodes and 3 Redis nodes (connection and failover)

Usage:

                ConfigurationOptions sentinelConfig = new ConfigurationOptions();
                sentinelConfig.ServiceName = "mymaster";
                sentinelConfig.EndPoints.Add("192.168.99.102", 26379);
                sentinelConfig.EndPoints.Add("192.168.99.102", 26380);
                sentinelConfig.EndPoints.Add("192.168.99.102", 26381);
                sentinelConfig.TieBreaker = "";
                sentinelConfig.DefaultVersion = new Version(4, 0, 11);                 
                // its important to set the Sentinel commands supported
                sentinelConfig.CommandMap = CommandMap.Sentinel;

                // Get sentinel connection
                ConnectionMultiplexer sentinelConnection = ConnectionMultiplexer.Connect(sentinelConfig, Console.Out);
                // Create master service configuration
                ConfigurationOptions masterConfig = new ConfigurationOptions { ServiceName = "mymaster" };
                // Get master Redis connection
                var redisMasterConnection = sentinelConnection.GetSentinelMasterConnection(masterConfig);

                ...
               IDatabase db = redisMasterConnection.GetDatabase();                
               db.StringSet(key, value);
               ...
               string value1 = db.StringGet(key);

[gkorland]

@mgravell

[kooldude2010]

Need some help, noob here. Downloaded the code and built it and added reference to Redis.Stackexchange.dll to my own project. I am not able to see GetSentinelMasterConnection method. What am I doing wrong?

Any help would be greatly appreciated.

[shadim]

Please ensure that you are follow the example usage from commit conv.

On Thu, Feb 28, 2019 at 18:30 kooldude2010 ***@***.***> wrote: Need some help, noob here. Downloaded the code and built it and added reference to Redis.Stackexchange.dll to my own project. I am not able to see GetSentinelMasterConnection method. What am I doing wrong? Any help would be greatly appreciated. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1067 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABF7Dqh-oa_5E1HF4XJMTTa37D9mL-Yjks5vSAQ0gaJpZM4bIQzt> .

-- *Shadi MassalhaEmail: sm@ <shady.massalha@gmail.com>apps.mygbiz.com <http://apps.mygbiz.com>Skype: shadi.massalhaMobile #: +972-52-553-4466Fax #: +972-73-726-7768*

[jrsearles]

I am finding with this PR that redis slaves are never used, even when requests are set to prefer them. Digging through the code it appears that slaves are never added to the serverSnapshot. Is this an oversight?

[gkorland]

@mgravell any chance you got to review it?

[shadim]

I am finding with this PR that Redis slaves are never used, even when requests are set to prefer them. Digging through the code it appears that slaves are never added to the serverSnapshot. Is this an oversight?

by design GetSentinelMasterConnection should point to the master see the following architecture
http://taswar.zeytinsoft.com/wp-content/uploads/2018/03/redis-sentinel.png

there is no oversight to use the slaves you should do something like the following snippet:

            var Server = sentinelConnection.GetServer(<any sentinel ip>, <port>);
            var slaves = Server.SentinelSlaves(ServiceName);
            var config = new ConfigurationOptions
            {                 
                TieBreaker = "";
                ServiceName = TestConfig.Current.SentinelSeviceName,
                SyncTimeout = 5000
            };

            foreach(var kv in slaves)
            {
                config.EndPoints.Add(kv.ToDictionary()["name"]);
            }
            var readonlyConn = ConnectionMultiplexer.Connect(config);

I had added a new unit test for the above case see ReadOnlyConnectionSlavesTest.

[mrmartan]

ConnectionMultiplexer.ConnectImplAsync does not call ConnectionMultiplexer.InitializeSentinel

And shouldn't there also be ConnectionMultiplexer.GetSentinelMasterConnectionAsync?

[mrmartan]

Regarding slaves and

there is no oversight to use the slaves you should do something like the following snippet

I seem to fail to grasp the idea. Can you elaborate how this is envisioned to be used?
From #692:

The main test besides those would be master/slave working correctly - e.g. one ConnectionMultiplexer configured via Sentinel following and working after a Sentinel failover is issued.

Using GetSentinelMasterConnection how do I add slaves to the connection returned? I want this multiplexer to work the same way as non-sentinel enabled one, ie. handles reconfiguration through sentinel to connect to Redis instances and respects CommandFlags (Prefer/Demand/Master/Slave). There seems to be a lot of work expected to be done on the GetSentinelMasterConnection user side.

[shadim]

Regarding slaves and

there is no oversight to use the slaves you should do something like the following snippet

I seem to fail to grasp the idea. Can you elaborate on how this is envisioned to be used?
From #692:

The main test besides those would be master/slave working correctly - e.g. one ConnectionMultiplexer configured via Sentinel following and working after a Sentinel failover is issued.

Using GetSentinelMasterConnection how do I add slaves to the connection returned? I want this multiplexer to work the same way as non-sentinel enabled one, ie. handles reconfiguration through sentinel to connect to Redis instances and respects CommandFlags (Prefer/Demand/Master/Slave). There seems to be a lot of work expected to be done on the GetSentinelMasterConnection user side.

I agree it's easy to add the slaves also in the GetSentinelMasterConnection but, I am not sure if this match the Redis client design. because GetSentinelMasterConnection should point to the master only.
if there is a read operation that should be done against the slaves should have a new connection for this purpose of load balancing.

anyway, I will examine if its correct to do this change in GetSentinelMasterConnection or create a new method GetSentinelSlaveConnection.

[mrmartan]

Should we discuss this here or under #692?

I come from a place where we used Redis Cluster and that is more or less transparent for this client. You throw at it all the cluster nodes and it "just works." Even with Sentinel when you point the client at Redis nodes (those Sentinel managed) it works until there is some Sentinel action. And the connection multiplexer very much understands which nodes are masters and which are slaves and sends commands according to CommandFlags. So the ultimate goal (for me at least) would be to give connection string pointing to Sentinel nodes to ConnectionMultiplexer.Connect() and be returned Sentinel-managed ConnectionMultiplexer instance that just works.

Also what #692 (and by extension you) is doing amounts to a breaking change. With this Sentinel support (through GetSentinelMasterConnection) one would not be able to use Sentinel with libraries depending on StackExchange.Redis unless those libraries change, e.g. Microsoft.Extensions.Caching.StackExchangeRedis

[jastBytes]

I would love to see this working in the near future. Is there something I can do to make this happen soon?

[gkorland]

@mgravell is anything blocking this PR?

[gkorland]

@NickCraver

[NickCraver]

@gkorland Yep - there's still zero testing on any of the Sentinel bits thus far - it's not something we use or prioritize, so it especially needs testing. It's something we've brought up in all the other Sentinel PRs thus far.

[shadim]

but, we have completed the missing testing.
what kind of testing did you missing in this pr?

[NickCraver]

@shadim Ah indeed! I completely missed this from being collapsed - let me try and pull this local and remedy a few things :) It'll need Marc's eyes as well but let me get tests in shape on Linux, master merged, etc. - let's see if I can push to that branch...

[shadim]

@shadim this is what I have locally at the moment running all servers:

Can you confirm what you're seeing local?

I imagine this is a state issue based on bad assumptions in the tests of "clean slate" which is true on the CI server on a fresh checkout, but not locally where you get state, e.g. the contents of the .conf for a sentinel monitor may look like this on checkout:

port 26381
sentinel monitor mymaster 127.0.0.1 7010 1
sentinel down-after-milliseconds mymaster 1000
sentinel failover-timeout mymaster 2000
sentinel config-epoch mymaster 0
dir "../Temp" 

But locally after a run, it'll look like (and future test runs will start with):

port 26381
sentinel myid cbfdf24284a91f65b29bdcc22210489b9311c3a5
sentinel deny-scripts-reconfig yes
sentinel monitor mymaster 127.0.0.1 7010 1
sentinel down-after-milliseconds mymaster 1000
dir "/mnt/c/git/StackExchange/StackExchange.Redis/tests/RedisConfigs/Temp"
# Generated by CONFIG REWRITE
maxclients 4064
protected-mode no
sentinel failover-timeout mymaster 2000
sentinel config-epoch mymaster 0
sentinel leader-epoch mymaster 1
sentinel known-replica mymaster 127.0.0.1 7011
sentinel known-sentinel mymaster 127.0.0.1 26380 a2602c71b26af58d8496fc497461af281b4fd40c
sentinel known-sentinel mymaster 127.0.0.1 26379 bdc76ccaaf8833537b87afc7dcb1b7719b37a64f
sentinel current-epoch 1

I'm out for some errands and docs today, but poking at git resetting those locally to see if that is indeed the issue and then we can go about resetting state at the start of the test properly (as failover tests do - see Failover.cs constructor).

If you could sanity check me on having inconsistent or bad test results locally, that'd be a huge help! We want to get that green before moving forward here and getting eyes/thoughts from @mgravell on approach.

@NickCraver no in my local environment I get all the test pass, I think you are right this because of bad assumptions in the tests of "clean slate"

[jastBytes]

I would like to contribute, but did not manage to make the solution work on Linux and from what I've read in the documentation testing is only supported on Windows in general.

[jded76]

I'd like to contribute on this too, if you need any help.
I tried the sentinel tests and they are running fine, no matter how many times i stopped the servers and running start-sentinel.cmd again, or if run the tests without stopping the servers.

I looked at the tests, and there is no assumption of who is the master or the slave server.
The pattern they use in general is get the current master, make a fail-over, check that the new master is different from the previous. So I don't think that anything is needed in the Sentinel.cs constructor.

@NickCraver Maybe the output of the servers will help you determine what is the exact problem of the failures.
One strange thing that I noticed in your config is that before the "Generated by CONFIG REWRITE" line, you have the ID of the server (sentinel myid cbfdf24284a91f65b29bdcc22210489b9311c3a5). In my configs this setting does not exist. And I noticed that the IDs of the servers is changing after every run of the batch file (start-sentinel.cmd).
Can you check that the IDs of your servers are different on every config, because if not this will cause conflict (see here)?

[jded76]

@NickCraver is this still alive? Can I help with something to get things done?

[NickCraver]

I've been trying to get tests on CI stable (including getting an upgrade to our CI plan) - now trying consistent builds here. I'm also testing some community contributions on Docker to get a clean slate test environment more readily available for all.

I've got CI mostly green now which was the hard part - kicking builds here and trying to pull in the Docker approach as another testing method on any platform for all. That'll make this much easier to solve...the changing state on local machines makes this cluster state-dependent type of PR especially hard to determine flakiness of (it's likely my cluster).

[ejsmith]

@NickCraver I'm having a really hard time trying to create a clean set of changes ahead of this PR for the docker support. I have a fork based on https://github.com/shadim/StackExchange.Redis that I have made changes to here: https://github.com/ejsmith/StackExchange.Redis My git skills are failing me. It seems like this entire PR isn't a ton of changes and it would be nice to have a clean PR with minimal changes rebased onto your current master, but I haven't been able to figure out how to do that without just doing a diff and creating a brand new PR.

[ejsmith]

I created this PR to the PR. :-) shadim#3

[NickCraver]

Getting this in for the 2.1.x release, thanks for the work here everyone! We've got more testing in, Docker ability to test this locally better, and are generally in a good state. We can make more refinements here as needed.

[gliljas]

Were the "transparency issues" raised by @mrmartan addressed?

[mgravell]

Yes; there's a new build on myget. We've been dogfooding it at Stack Overflow without problem. We'll probably push it to nuget on Monday.

…

On Sun, 29 Mar 2020, 18:16 Gunnar Liljas, ***@***.***> wrote: Were the "transparency issues raised" by @mrmartan <https://github.com/mrmartan> addressed? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1067 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAEHMEOBQSDW4MVABDDNE3RJ567HANCNFSM4GZBBTWQ> .

[emi662002]

This PR is derived from PR-692 and have been merged with the latest master commit.
Things that have been done:

1. review code for PR-692
2. fixed potential infinite loop in the code
3. Adapt code to success build with the latest master commit
4. Manual testing on 3 Sentinel nodes and 3 Redis nodes (connection and failover)

Usage:

                ConfigurationOptions sentinelConfig = new ConfigurationOptions();
                sentinelConfig.ServiceName = "mymaster";
                sentinelConfig.EndPoints.Add("192.168.99.102", 26379);
                sentinelConfig.EndPoints.Add("192.168.99.102", 26380);
                sentinelConfig.EndPoints.Add("192.168.99.102", 26381);
                sentinelConfig.TieBreaker = "";
                sentinelConfig.DefaultVersion = new Version(4, 0, 11);                 
                // its important to set the Sentinel commands supported
                sentinelConfig.CommandMap = CommandMap.Sentinel;

                // Get sentinel connection
                ConnectionMultiplexer sentinelConnection = ConnectionMultiplexer.Connect(sentinelConfig, Console.Out);
                // Create master service configuration
                ConfigurationOptions masterConfig = new ConfigurationOptions { ServiceName = "mymaster" };
                // Get master Redis connection
                var redisMasterConnection = sentinelConnection.GetSentinelMasterConnection(masterConfig);

                ...
               IDatabase db = redisMasterConnection.GetDatabase();                
               db.StringSet(key, value);
               ...
               string value1 = db.StringGet(key);

Is this the right way to configure sentinels? I'm quite confused as i see this in the documentation:

If you specify a serviceName in the connection string, it will trigger sentinel mode. This example will connect to a sentinel server on the local machine using the default sentinel port (26379), discover the current master server for the mymaster service and return a managed connection pointing to that master server that will automatically be updated if the master changes:

var conn = ConnectionMultiplexer.Connect("localhost,serviceName=mymaster");

My specific problem:
When I use the code from this post with GetSentinelMasterConnection in combination with RedisXmlRepository I get an error:
'This operation has been disabled in the command-map and cannot be used: LRANGE'

[ejsmith]

When you call var conn = ConnectionMultiplexer.Connect("localhost,serviceName=mymaster"); that is returning a managed connection to the Redis servers (not the sentinel). So you don’t need to call GetSentinelMasterConnection afterward.

[emi662002]

When you call var conn = ConnectionMultiplexer.Connect("localhost,serviceName=mymaster"); that is returning a managed connection to the Redis servers (not the sentinel). So you don’t need to call GetSentinelMasterConnection afterward.

Indeed! thanks!