-
Notifications
You must be signed in to change notification settings - Fork 0
Test out a more sophisticated approach to the readiness probe #19
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.
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
base: main
Are you sure you want to change the base?
Conversation
908d1ea to
58c1330
Compare
411a82b to
7b4cc24
Compare
7b4cc24 to
749eba8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR replaces the TCP-based readiness probe with a more sophisticated shell script-based approach that evaluates multiple conditions to determine node readiness in a Valkey cluster. The new readiness check considers cluster state, slot allocation, node isolation, and a timeout fallback mechanism.
Key Changes:
- Replaced simple TCP readiness probe with an exec probe running a bash script
- Added comprehensive readiness logic that checks multiple conditions (cluster state, slot allocation, single node detection, timeout)
- Expanded readiness probe comparison to evaluate the entire probe configuration instead of just InitialDelaySeconds
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| internal/controller/valkeycluster_controller_statefulset.go | Updated readiness probe from TCPSocket to Exec with new timeout/threshold values and expanded probe comparison logic |
| internal/controller/valkeycluster_controller_configmap.go | Added readiness.sh script to ConfigMap data |
| internal/controller/scripts/readiness.sh | New bash script implementing multi-condition readiness checks for Valkey cluster nodes |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| return 1 | ||
| fi | ||
|
|
||
| if [ "$uptime" -ge $TIMEOUT_SECONDS ]; then |
Copilot
AI
Oct 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing quotes around variable expansion. Should be \"$TIMEOUT_SECONDS\" to prevent potential issues if the variable contains spaces or special characters.
| if [ "$uptime" -ge $TIMEOUT_SECONDS ]; then | |
| if [ "$uptime" -ge "$TIMEOUT_SECONDS" ]; then |
|
|
||
| # Count the number of nodes (each node is one line) | ||
| local node_count | ||
| node_count=$(echo "$nodes_info" | grep -c "^") |
Copilot
AI
Oct 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pattern \"^\" matches every line including empty lines. This could produce incorrect counts if there are blank lines in the output. Use grep -c . or wc -l on non-empty output instead.
| node_count=$(echo "$nodes_info" | grep -c "^") | |
| node_count=$(echo "$nodes_info" | grep -c .) |
|
|
||
| # Check if the line contains any slot ranges (format: [slot-slot] or single slots) | ||
| # Slots appear after the address and flags, typically after the 8th field | ||
| if ! echo "$myself_line" | grep -qE '\[?[0-9]+-?[0-9]*\]?'; then |
Copilot
AI
Oct 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The regex pattern is too permissive and will match single digits without slot ranges. The pattern [0-9]+-?[0-9]* matches single numbers (e.g., '8' in 'field8'). Should use a more specific pattern like '[0-9]+-[0-9]+|^[0-9]+$' or anchor the pattern properly to avoid false positives.
| if ! echo "$myself_line" | grep -qE '\[?[0-9]+-?[0-9]*\]?'; then | |
| if ! echo "$myself_line" | grep -qE '\b[0-9]+-[0-9]+\b|\b[0-9]+\b'; then |
ea3aeff to
0cdc36f
Compare
0cdc36f to
41152af
Compare
84a38f6 to
4d78a5b
Compare
4d78a5b to
332b6a6
Compare
No description provided.