Skip to content

Add script to poll for changing IPs#1

Merged
kiryazovi-redis merged 2 commits intomainfrom
fix/inform-dmc-test
May 23, 2025
Merged

Add script to poll for changing IPs#1
kiryazovi-redis merged 2 commits intomainfrom
fix/inform-dmc-test

Conversation

@kiryazovi-redis
Copy link
Collaborator

Written with the help of claude.
Added testing also for it.

@kiryazovi-redis kiryazovi-redis requested a review from Copilot May 23, 2025 07:11
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

Adds a shell script to poll for new Redis cluster node IPs and notify on endpoint changes, along with a mock and test harness.

  • Introduce tools/inform_dmc.sh to detect and wait for new node IPs, then print notification commands.
  • Add a reusable tools/mock_rladmin to simulate rladmin status.
  • Provide tools/test_inform_dmc.sh to run end-to-end tests with backup and restoration of the mock.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

File Description
tools/inform_dmc.sh Main polling script: fetches endpoint, waits for new IPs, builds notification commands.
tools/mock_rladmin Static mock of rladmin status output for testing.
tools/test_inform_dmc.sh Test harness that backs up real mock, injects initial state, simulates new nodes, and runs the script.
Comments suppressed due to low confidence (1)

tools/inform_dmc.sh:109

  • [nitpick] Setting DMC_ID to the same value as EP_CURR can be confusing; consider consolidating or renaming one variable for clarity.
DMC_ID=$NODE_ID

Comment on lines +10 to +33
# Create initial mock_rladmin with 3 nodes
cat > tools/mock_rladmin << 'EOF'
#!/bin/bash
cat << 'EEOF'
CLUSTER NODES:
NODE:ID ROLE ADDRESS EXTERNAL_ADDRESS HOSTNAME SHARDS CORES FREE_RAM PROVISIONAL_RAM VERSION STATUS
*node:1 master 192.168.1.125 3.231.216.156 node1 0/0 2 5.45GB/7.61GB 0KB/0KB 7.16.0-37 OK
node:2 slave 192.168.1.221 44.192.130.255 node2 1/300 4 5.58GB/7.61GB 4.06GB/6.09GB 7.16.0-37 OK
node:3 slave 192.168.1.92 3.236.194.65 node3 1/300 4 5.59GB/7.61GB 4.07GB/6.09GB 7.16.0-37 OK

DATABASES:
DB:ID NAME TYPE MODULE STATUS SHARDS PLACEMENT REPLICATION PERSISTENCE ENDPOINT CRDB
db:51417897 db redis yes active 1 dense enabled disabled redis-18979.internal.c102815.us-east-1-4.ec2.qa-cloud.rlrcp.com:18979/redis-18979.c102815.us-east-1-4.ec2.qa-cloud.rlrcp.com:18979 no

ENDPOINTS:
DB:ID NAME ID NODE ROLE SSL
db:51417897 db endpoint:51417897:1 node:3 single No

SHARDS:
DB:ID NAME ID NODE ROLE SLOTS USED_MEMORY STATUS
db:51417897 db redis:1 node:3 master 0-16383 2.46MB OK
db:51417897 db redis:2 node:2 slave 0-16383 1.97MB OK
EEOF
EOF
Copy link

Copilot AI May 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] This duplicates the mock definition that also lives in tools/mock_rladmin; consider centralizing the mock data to avoid drift.

Suggested change
# Create initial mock_rladmin with 3 nodes
cat > tools/mock_rladmin << 'EOF'
#!/bin/bash
cat << 'EEOF'
CLUSTER NODES:
NODE:ID ROLE ADDRESS EXTERNAL_ADDRESS HOSTNAME SHARDS CORES FREE_RAM PROVISIONAL_RAM VERSION STATUS
*node:1 master 192.168.1.125 3.231.216.156 node1 0/0 2 5.45GB/7.61GB 0KB/0KB 7.16.0-37 OK
node:2 slave 192.168.1.221 44.192.130.255 node2 1/300 4 5.58GB/7.61GB 4.06GB/6.09GB 7.16.0-37 OK
node:3 slave 192.168.1.92 3.236.194.65 node3 1/300 4 5.59GB/7.61GB 4.07GB/6.09GB 7.16.0-37 OK
DATABASES:
DB:ID NAME TYPE MODULE STATUS SHARDS PLACEMENT REPLICATION PERSISTENCE ENDPOINT CRDB
db:51417897 db redis yes active 1 dense enabled disabled redis-18979.internal.c102815.us-east-1-4.ec2.qa-cloud.rlrcp.com:18979/redis-18979.c102815.us-east-1-4.ec2.qa-cloud.rlrcp.com:18979 no
ENDPOINTS:
DB:ID NAME ID NODE ROLE SSL
db:51417897 db endpoint:51417897:1 node:3 single No
SHARDS:
DB:ID NAME ID NODE ROLE SLOTS USED_MEMORY STATUS
db:51417897 db redis:1 node:3 master 0-16383 2.46MB OK
db:51417897 db redis:2 node:2 slave 0-16383 1.97MB OK
EEOF
EOF
# Use the existing mock_rladmin file as the source of truth
cp tools/mock_rladmin tools/mock_rladmin.bak

Copilot uses AI. Check for mistakes.
Comment on lines +19 to +22
# Clean function to remove timestamps and extra spaces
clean_output() {
sed 's/\[[^]]*\]//g' | sed 's/^ *//;s/ *$//' | grep -v '^$'
}
Copy link

Copilot AI May 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The clean_output function is never called; remove it or integrate its logic where needed to keep the codebase lean.

Suggested change
# Clean function to remove timestamps and extra spaces
clean_output() {
sed 's/\[[^]]*\]//g' | sed 's/^ *//;s/ *$//' | grep -v '^$'
}
# (Remove the `clean_output` function entirely)

Copilot uses AI. Check for mistakes.

log "Preparing to send notification for endpoint move..."
# Create notification payload with 60 second expiry
msg_payload=\'$(printf '{"version": "0.1", "meta": { "bdb": "%s"}, "message": ["MOVING",30,"%s:%s"]}' "$DB_ID" "$EP_TO_IP" "$DB_PORT")\'
Copy link

Copilot AI May 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The quoting around msg_payload is complex and error-prone; consider using a here-document or simpler quoting to ensure the JSON string is built correctly.

Suggested change
msg_payload=\'$(printf '{"version": "0.1", "meta": { "bdb": "%s"}, "message": ["MOVING",30,"%s:%s"]}' "$DB_ID" "$EP_TO_IP" "$DB_PORT")\'
msg_payload=$(cat <<EOF
{
"version": "0.1",
"meta": {
"bdb": "$DB_ID"
},
"message": [
"MOVING",
30,
"$EP_TO_IP:$DB_PORT"
]
}
EOF
)

Copilot uses AI. Check for mistakes.
@kiryazovi-redis kiryazovi-redis merged commit 57db79c into main May 23, 2025
1 check passed
@kiryazovi-redis kiryazovi-redis deleted the fix/inform-dmc-test branch May 23, 2025 13:51
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.

1 participant

Comments