support requirepass redis sentinel #30352
Conversation
|
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
|
|
This is cool, this issue was raised by a few users. @jonathanjuursema @dintorf @TomYork1986 - you were involved in related Can you please help the community and verify if the fix solves the problems you raised? That would boost our confidence up that we can merge this one. |
|
Most certainly! I'll try to make some time for this today, otherwise I'll block some time to test this in our organization next week! :) |
Would be cool. I would hold on with merging that one then - but mark it for 2.6 so that we will come back to it (BTW. @ORuteMa can you please fix the static checks ? |
Of course I can, I will fix it soon. After installing the pre-commit tool on my local machine, I encountered some network issues and couldn't finish the checks :( |
|
Approved the CI run now 🤞 |
|
Hey! I've set aside some time today to work on this, but I could not finish a test. We're deploying Airflow using the Docker container, and as far as I can tell this has not been built into an image yet. So I've used our current pipeline to build an image off of this branch, and use that image as the base for our own image. I've also tried to look into how, using the changes in this PR, I'm supposed to correctly configure Redis Sentinel. In my original issue (comment link) I've talked over a few ways which I thought might work (leaning on some stuff I found externally as well). Having said that, the test environment in our organisation is now up to date and I have that image built off of this branch. If you can tell me what the syntax is for configuring Sentinel I should be able to give it a go relatively quickly. In our case, the following apply:
We deploy everything in containers and use environment variables for configuration. Hoping to hear from you! |
Hi! Thanks for your work. I will share my config below. @jonathanjuursema In my case, I configured the Regarding sentinel with TLS, I think it is supported, and you can find more details from celery/celery#6647. Therefore, in my PR, I have changed line 85 to support the URL of the If you have the time, could you please test whether the sentinel works properly when SSL is enabled? My Redis deployment is only protected by a password, so I would like to ensure that the SSL configuration is functional. |
|
@jonathanjuursema - did you manage it to work? The build is green, but I am hesitating to merge that one, as I am not sure if this is general enough. Possibly also adding documentation describing how to use it (based on your experience and maybe with @ORuteMa review and help could alleviate some of the concerns? I am afraid in the current form it is a bit too difficult to discover how to do. Would it be possible to add such docs (@ORuteMa and @jonathanjuursema maybe you can join forces on that one ?). |
|
Hey! I'm very sorry for not updating here. Unfortunately we have shifted priorities a while back. I was able to squeeze in some time for the previous quick test, but I haven't had sufficient time to dive into this again so far. It's not worth much, but I still have this on my todo list to pick up whenever things calm down. Having said that, our org wants wants me to focus on other things for the time being - so I wouldn't wait for me. :( I'm sorry I can't be of more help at this moment. |
Maybe I should add some docs to describe |
Thank you. I will mark it as "Require changes" just to not merge it accidentally before. |
potiuk
left a comment
potiuk
left a comment
There was a problem hiding this comment.
Waiting for some docs describing the new featture.
|
@potiuk I have added some docs to describe sentinel_kwargs, I am not sure if |
It should be 2.7.0 - we do not add features in patchlevel versions. Also - one small request, can you mention that Redis sentinel is supported in https://airflow.apache.org/docs/apache-airflow/stable/core-concepts/executor/celery.html and maybe add an example configuration for it there? That's what I was looking for as "discoverabiliyt" by having just parameter decribing kwargs, it's not obvious that sentinel is supported and if we describe it in celery docs with example it might be much easier for people to find out |
Sure. I will add a example for sentinel. |
|
@potiuk Hi there, Thank you for your time and help, and please let me know if there's anything I can do to assist. |
| # visibility_timeout = | ||
|
|
||
| # The sentinel_kwargs parameter allows passing additional options to the Sentinel client. | ||
| # In a typical scenario where Redis Sentinel is used as the broker and Redis servers are |
There was a problem hiding this comment.
You should add them description in config.yml -> this is the source of truth for this defailt config but also it is used to generate documentation automatically, so the comment you added here, should be added in config.yml and then pre-commit should automatically add it above and you should not need to add it manually here.
There was a problem hiding this comment.
Actually, I don't add it manually. When I make modifications in config.yml and do a git commit, the pre-commit hook prevents my merge request and automatically generates this content in my git stage. Only after that, I can successfully commit without any pre-commit issues.
There was a problem hiding this comment.
The pre-commit prevents you doing it and modifes it and ask you to commit the changes it has done when it add.
You need to:
- add changes to ,yml
- commit and let pre-commit add it to the config
- add the generated config changes it to the staging
- commit again
There was a problem hiding this comment.
Ah ... stupid me. yes you re right! I missed that it is example generated from the other part. Sorry for the confusion.
Yes. It's a problem with latest botocore which we are handling separately (it affecst a subset of prs that are changing common files like config,yml for example, so you are unlucky, but you should not bother, we can merge it even if we won't fix it in main. |
|
Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions. |
4 participants
In my deployment, I am using a celery worker with Redis Sentinel configured as a broker, with both Redis and Sentinel having password authentication enabled. To configure the password in sentinel_kwargs, I encountered an error when I added it to the celery_broker_transport_options section. However, I was able to resolve the issue by using conf.getjson() to load the sentinel_kwargs configuration, and it now works without any issues.
Closes: #28010