Skip to content

Conversation

@kurtmc
Copy link
Member

@kurtmc kurtmc commented Oct 8, 2025

No description provided.

@kurtmc kurtmc force-pushed the feature/advanced-readiness-probe branch 4 times, most recently from 908d1ea to 58c1330 Compare October 8, 2025 20:59
@kurtmc kurtmc force-pushed the feature/advanced-readiness-probe branch 2 times, most recently from 411a82b to 7b4cc24 Compare October 16, 2025 00:45
@kurtmc kurtmc force-pushed the feature/advanced-readiness-probe branch from 7b4cc24 to 749eba8 Compare October 16, 2025 00:54
@kurtmc kurtmc requested a review from Copilot October 16, 2025 02:17
Copy link

Copilot AI left a 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
Copy link

Copilot AI Oct 16, 2025

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.

Suggested change
if [ "$uptime" -ge $TIMEOUT_SECONDS ]; then
if [ "$uptime" -ge "$TIMEOUT_SECONDS" ]; then

Copilot uses AI. Check for mistakes.

# Count the number of nodes (each node is one line)
local node_count
node_count=$(echo "$nodes_info" | grep -c "^")
Copy link

Copilot AI Oct 16, 2025

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.

Suggested change
node_count=$(echo "$nodes_info" | grep -c "^")
node_count=$(echo "$nodes_info" | grep -c .)

Copilot uses AI. Check for mistakes.

# 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
Copy link

Copilot AI Oct 16, 2025

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.

Suggested change
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

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants