Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix command line for Redis liveness probe #576

Merged
merged 1 commit into from
Apr 21, 2023
Merged

Fix command line for Redis liveness probe #576

merged 1 commit into from
Apr 21, 2023

Conversation

mpihlak
Copy link
Contributor

@mpihlak mpihlak commented Mar 20, 2023

Fixes always succeeding liveness probe described in #575

Changes proposed on the PR:

  • Pass the PING command to redis-cli after credentials to avoid auth failures
  • Explicitly check for a PONG response, so that error status is properly returned

@mpihlak mpihlak requested a review from a team as a code owner March 20, 2023 21:05
@samof76
Copy link
Contributor

samof76 commented Mar 21, 2023

@mpihlak why do we do a grep, isn't exit status 1 not returned when ping fails?

@mpihlak
Copy link
Contributor Author

mpihlak commented Mar 21, 2023

@mpihlak why do we do a grep, isn't exit status 1 not returned when ping fails?

redis-cli is not returning proper exit status in this case:

/data $ redis-cli -h localhost -p 6379 ping --user pinger --pass pingpass --no-auth-warning
(error) NOAUTH Authentication required.
/data $ echo $?
0

There's also this issue open on the Redis side and some other issues mentioning error statuses not properly returned.

@ese
Copy link
Member

ese commented Apr 20, 2023

Thanks @mpihlak Could you merge with master to fix CI?

@mpihlak
Fix command line for Redis liveness probe
8e94166 
redis-cli requires the command argument to be passed after the
credentials. Also (at least on 6.x versions) it does not properly return
an error status on authentication failures. This causes the liveness
probe to always return success. Fix this by checking for a PONG response
to a PING.
@mpihlak mpihlak force-pushed the fix-redis-liveness-probe branch from f358664 to 8e94166 Compare April 21, 2023 06:59
@mpihlak
Copy link
Contributor Author

mpihlak commented Apr 21, 2023

Rebased to master and fixed a couple of tests. Should be good now.

@ese ese merged commit 33b2339 into spotahome:master Apr 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ese ese ese approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mpihlak @samof76 @ese