Sentinel Support

[lexxdark]

This includes sentinel support as discussed in issue #22. Based on pull request #406. I have added the discussed fixes in the pull requests and fixed merge conflicts.

[NickCraver]

@lexxdark can you please rebase this without changing all the line endings? Though ?w=1 works around it in GitHub, it still churns git history and messes things up there.

[lexxdark]

Odd, I remember dropping a comment here, but Friday was a full day for me. :">
Will change them. Is there a preference for a type of newlines or the other? The files that I changed, had mixed newlines. :)

[lexxdark]

Tell me if it's ok like this. If not I can convert the touched files to unix new lines (they seem to be in a higher proportion in the files).

Have fun ;)

[NickCraver]

I left comments in #406 asking for help. Basically: we need help with testing here as well. I'll try and get Sentinel servers (enabling current Sentinel tests) as part of the local server spinup. We can then use that server set to test all of the changes here. Anyone who can help on the integration testing front will be much appreciated.

[andyedinborough]

Absolutely. Most of my day is free tomorrow and we're dying to test this out.

FWIW, we have 3 redis Windows servers in our non-production environment already configured to use Sentinel. It's used by our to back session for our websites, synchronize events for our sites and Windows applications. We can give it a thorough pounding.

[NickCraver]

I've pushed configs for testing up (and added them to the test startup suite), I'll try and get testing of existing commands up tomorrow (before reviewing and merging anything new) to make sure we're good on current bits.

[lexxdark]

Not sure how to help with the test part, I will try to see if I can manage something today.
If you have some pointers on how to help with the tests ping me :)

[NickCraver]

I've got tests for Sentinel commands and running in Appveyor (and green), so now need to formulate unit tests for this PR (that the behavior is correct) and see what we can simplify. That's all I've got time for today, but Appeyor was being...fun... which ate a lot of time.

[jordanweak]

Is there any update on this?

[yacineb]

Hi All,

Any update on this PR ? why tests do fail ?

Thks & Regards,

[kunjan-ss]

@lexxdark and @NickCraver can you please provide an update on this. It will be extremely helpful as we're planning to setup HA for Redis in our production environment and would love to use this.

Besides testing this out, if you can share any other blocking issues, we can certainly see how we can help out.

Thank you for all your hard work! 👍

[lexxdark]

On my side there is no update, I have no idea how to implement the testing or where to start researching on it. (referring to how testing is done on this project and also didn't have the time to do a full research)
On our side we are using a modified version of the library, which we will replace when the proper version is available. But we don't have a lot of traffic on the service yet.

As a review until now:

• the way the configuration is implemented should be improved (to also allow proper connection strings for sentinel for example)
• no crashes or issues
• some non-async calls (Thread.Sleep if I remember) within the patch/implementation, which I was a bit afraid to remove without fully understanding how things work there (if the code is thread agnostic or not)

I'm actually doing some integration tests on the reliability of our application when components fail (including redis), which might peer into the real reliability of the patch.

[dfgrosso]

Greetings,

I'm not really familiarized with the concept of a "pull request" and I still don't understand if the sentinel support is actually a feature of some version or not. I'm using the latest version on nuget (1.2.6) and I'm using an approach where I set a list of servers in the connection multiplexer. I keep getting exceptions like the one below when we tries to reach the REDIS servers on the list that aren't the master (I think). I believe the sentinel support with prevent this exceptions as I would be reaching the sentinels instead of the masters.
Thanks in advance.
Best Regards Daniel Grosso (Lisbon, PT)
System.InvalidOperationException: ValueFactory attempted to access the Value property of this instance.

---

[uciprian]

@lexxdark could you share with my account your sentinel implementation, I want to give it a try and contribute as well if I find something, thanks

[dfgrosso]

Hello @lexxdark

What do u need? The redis conf files?

Regards,
Daniel

[uciprian]

to be able to pull KDS:sentinel/master branch

[lexxdark]

The pull request should be public, I will check to make sure. :)
LE: It is public. To be honest, I can't find any direct link from the pull request to the repo and branch.
So to assist here is the link to the branch https://github.com/KDS/StackExchange.Redis/tree/sentinel/master
Ping me on private and I can add changes and/or extra commits

[BalajiYanamadala]

When i connect to sentinel i am getting "faulted: ProtocolFailure on PING" No masters detected. If i connect development redis sentinel is working fine. But when i connect QA redis sentinel, I am facing this issue. Could any one please help me to resolve this issue

[lexxdark]

I managed to run the tests, but right now some of them are failing. About 9 of them on master, commit: 140d741
I just ran RedisConfigs/run-all.cmd and then the tests. Am I doing something wrong, or the branch is broken?
I noticed there is a sentinel already starting for the tests, should I understand that you partially merged this commit, or something to support senhtinel on it?

@BalajiYanamadala: I think that is the error when you point the lib to a non-sentinel redis, when it's expecting a sentinel.

[NickCraver]

@lexxdark I added test nodes for Sentinel in order to make it easier to add tests for adding support, yep. I'm working on the failing tests now but it's a bit tricky to stay green all the time with a I/O heavy integration test suite. I've got the CI build down to 3 but still more to do to get green.

[lexxdark]

About the tests, I'm thinking of adding:

• Connection to sentinel
• Test access to keys in the service
• Sentinel down test
• Service Master down test

Any others?

Is the current way of accessing a Sentinel platform ok? IE:

ConfigurationOptions sentinelOptions = new ConfigurationOptions()
{
    ClientName = $"{_appInfo.AppName}-{connectionKey}-Sentinel",
    TieBreaker = "",
    CommandMap = CommandMap.Sentinel,
    AbortOnConnectFail = false
};

sentinelOptions.EndPoints.Add(sentinelAddress);

ConfigurationOptions redisServiceOptions = new ConfigurationOptions();
redisServiceOptions.ClientName = $"{_appInfo.AppName}-SentinelService";
redisServiceOptions.ServiceName = "master";
redisServiceOptions.AbortOnConnectFail = true;


var sentinelConnection = ConnectionMultiplexer.Connect(sentinelOptions);
var redisConnection = sentinelConnection.GetSentinelMasterConnection(redisServiceOptions);
var db = redisConnection.GetDatabase();
var val = db.GetStringAsync("someKey");

Or should it be like:

ConfigurationOptions sentinelOptions = new ConfigurationOptions()
{
    ClientName = $"{_appInfo.AppName}-{connectionKey}-Sentinel",
    TieBreaker = "",
    ServiceName = "master"
};
var sentinelConnection = ConnectionMultiplexer.Connect(sentinelOptions);
var db = redisConnection.GetDatabase();
var val = db.GetStringAsync("someKey");

Or some similar way of specifying that you are accessing a Sentinel

[NickCraver]

@lexxdark 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.

[mustafacagataytulun]

@lexxdark Are there any progress on tests? I would like to help if I can.

[MichaelPetrinolis]

any news when this will be merged? I could also help.

[NickCraver]

I'm trying to see what's needed to get the functionality in before a master merge (which I'm not sure is realistic). We're about ready for 2.0, and though I'm okay with this being in 2.1 as a follow-up, if I can reasonably adapt it into the 2.0 release, we'll so.

Observations so far: the functionality in 4/5 files in this PR is already in: .SentinelGetSentinelAddresses() is .SentinelSentinels() (slightly different signature, which also returns state information). If we pretend this is rebased, the changes are constrained to consumption of that API in connection multiplexer, which has changed tremendously in the v2 branch (pipelines). I'm taking a look at what all is happening there, if it's compatible, and what we need in terms of integration testing (the main stick point on this PR).

[NickCraver]

Additional observation: this handles a very small subset of the https://redis.io/topics/sentinel pub/sub messages. It seems like we should maybe handle all of those (helper class perhaps), and see if it affects any of the endpoints in our pool. We'd need to correlate on something besides endpoint though, perhaps, or allow such a possibility if possible due to multiple endpoints being valid to a single actual instance. For example Sentinel has the IPv4 but we're connecting on IPv6 or hostname.

Any time we have state stored (which is likely a route to take here), we need to update said state and poll it. If we connect to a sentinel, both of these should be accounted for. That means pub/sub (holistically) and probably something on the heartbeat on the interval, e.g. an additional call on the config check seconds interval trigger if we connected to a Sentinel on the last connect for the multiplexer.

This is me thinking out loud to be sure we address these issues in the solution. While I'd like to save this PR, I'm not sure that's realistic. We would like to have the functionality in here, but the current vs. final would be so drastically different I'm not sure an intermediary is worth fighting for. Going to keep looking at this and thinking about how to handle all the scenarios. I'll chat with Marc as well when he's back Tuesday.

[chester89]

@NickCraver how did that go?

[SeriousM]

Any updates on this issue? Sentinel support is very important!

[Areson]

So, after a long hiatus, I'm going to try and take another stab at getting this working and up-to-date with the current code base. I'm the submitter of PR #406, and unfortunately after doing some initial work on this my place of employment went a different direction and I got pulled away from this. I'm in a position again where Sentinel support is needed so I want to try and take advantage of that. I've noted several items from this thread I'd like to get feedback on:

• Configuration style. It has been noted that the configuration for this isn't really ideal. How should it be setup?
• Missing commands
• New connection multiplexer

If I'm missing anything please let me know.

[Areson]

Wrapping up a first pass on merging the Sentinel code into the latest code. A couple of observations:

• I'm not sure what we can do to clean up the configuration without some more invasive modifications. I'm assuming folks' comments had to do with supporting connection strings, which generally should work OK. The biggest missing item is CommandMap is not supported in connection strings at the moment.
• @NickCraver I'm not convinced that all of the commands that can be subscribe to are essential for an initial implementation. A lot of them are nice for knowing what the Sentinels are doing, but they don't necessarily require the client to do anything. The reason that the original implementation only subscribed to +sentinel and +switch-master was that those did require some action on the part of the client. You could argue that -dup-sentinel does as well, but the implementation of +sentinel does a full refresh of the list of Sentinels, which makes -dup-sentinel possibly redundant. I'd like to see all of these added for monitoring so folks can decide what to do with them if they pop up, but whether or not that should be in the initial PR so we can get something useful out the door is worth discussing.
• I went and reviewed instances there there are Thread.Sleep commands. As I recall those were put in there because occasionally changes in the what node is the master take a few short periods of time before they can be successfully connected to. Short timeouts were added specifically to keep this from failing rapidly multiple times in a row. If these wait times should be configurable, let's add that. Not sure if there is a good way to make them async as they are event handlers.
• Looked over the code to see what it would take to make the ConnectionMultiplexer more user-friendly, e.g. not needing to call another method to get the ConnectionMultiplexer for the current master node. I certainly think it is possible, but again it would be rather invasive since a number of methods would need to be modified to detect what mode the multiplexer is operating in. I'm going to leave that as another point of discussion for now.

I'm hoping that this will generate some discussion prior to me putting together a new PR. I'd like to get some consensus on some of these before going that far.

[mgravell]

The biggest missing item is CommandMap is not supported in connection strings at the moment.

CommandMap has full support; syntax is ${command}=[alias]

Example:

127.0.0.1,$KEYS=FIREME,$SHUTDOWN=

[Areson]

@mgravell Sorry about that...I should have been more clear on what I meant. I was thinking more about the presets being supported in the connection strings, something like CommandMap=Sentinel for the configuration. Programmatically it's easy, e.g. config.CommandMap = CommandMap.Sentinel, but it felt cumbersome to me to enumerate all of those in the connection string.

[mgravell]

Ah, right. We've special-cased some things like twemproxy before, so we can probably do this too if it would be useful.

…

On Thu, 21 Feb 2019, 15:34 Ian Oberst, ***@***.***> wrote: @mgravell <https://github.com/mgravell> Sorry about that...I should have been more clear on what I meant. I was thinking more about the presets being supported in the connection strings, something like CommandMap=Sentinel for the configuration. Programmatically it's easy, e.g. config.CommandMap = CommandMap.Sentinel, but it felt cumbersome to me to enumerate all of those in the connection string. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#692 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABDsE-C3AkMeYrn9mLemn4huiqJXoIvks5vPrx4gaJpZM4O7Qbm> .

[Areson]

Might be to make it faster to spin up a client for Sentinel usage. There are a couple of parameters that have to be set properly to make it work. Looking back over the PR histories I can see multiple cases where that has tripped people up.

[devlounge]

Hi,

Multiple people where I work will need this to work as I've rolled out Redis-HA as a service for them.
Any idea when this will finally get merged?

[Areson]

@devlounge Unclear. This PR and the preceding ones have been around for a bit. If you really need to use this you may need to just build it yourself using this or another PR. I have a fork that applied this PR to the more recent changes in the code base as of February for a friend of mine who was needing it. I haven't had a chance to really test it out myself as I don't actively use Redis at the moment, but it is available for folks to grab if they like.

I'd like to submit an updated PR, but I was waiting to get some feedback from folks on this thread on some of the questions I raised earlier. It's been pretty quiet though.

[janstadt]

@Areson have you heard back with regard to your questions? Like @devlounge (and many other) i am also eager to get this stood up at my company.

[Areson]

@janstadt If it isn't in this thread, then it hasn't been said, at least to me.

[janstadt]

@janstadt If it isn't in this thread, then it hasn't been said, at least to me.

@NickCraver any updates?

[actl-dhruv]

Any update?
@lexxdark did you working on it? where we find latest commit or changes? could share repo?

[davidop]

Any news regarding this issue?

[Areson]

None. It looks like @mrmartan made a new PR, #1067, and updated the code from this. There seems to be newer discussion there but nothing definitive.

EDIT - Nevermind, that is an older thread as well, and not started by @mrmartan. I have a newer version of the code for this on my fork that I put together in Feburary, but I never made another PR as I was pending some feedback.

[NickCraver]

I'm going to close this PR out, not rejecting at all - just closing in favor of the updated #1067 which we're working on the testing for now :)