Added standalone,sentinel and cluster compose files and updated workflow #251
Conversation
WalkthroughThe pull request introduces comprehensive Docker Compose configurations for Redis environments, specifically adding support for standalone, sentinel, and cluster configurations. The changes include creating three new Docker Compose files in the Changes
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #251 +/- ##
=======================================
Coverage 63.08% 63.08%
=======================================
Files 22 22
Lines 531 531
Branches 84 84
=======================================
Hits 335 335
Misses 190 190
Partials 6 6 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (6)
.github/workflows/node.js.yml (2)
38-48: Simplify host file modifications using a loopThe current approach of adding hosts individually can be simplified using a loop for all services.
- name: Add entries to the host file to correctly resolve service names run: | - sudo echo "127.0.0.1 redis-standalone" | sudo tee -a /etc/hosts - sudo echo "127.0.0.1 sentinel-1" | sudo tee -a /etc/hosts - sudo echo "127.0.0.1 sentinel-2" | sudo tee -a /etc/hosts - sudo echo "127.0.0.1 sentinel-3" | sudo tee -a /etc/hosts - sudo echo "127.0.0.1 redis-server-1" | sudo tee -a /etc/hosts - sudo echo "127.0.0.1 redis-server-2" | sudo tee -a /etc/hosts - for i in node{0..5};do - echo "127.0.0.1 $i" | sudo tee -a /etc/hosts - done + services=("redis-standalone" "sentinel-1" "sentinel-2" "sentinel-3" "redis-server-1" "redis-server-2" $(seq -f "node%g" 0 5)) + for service in "${services[@]}"; do + echo "127.0.0.1 $service" | sudo tee -a /etc/hosts + done
50-51: Remove redundant docker-compose installationThe
hoverkraft-tech/compose-actionaction already includes Docker Compose, making this step unnecessary.- - name: install docker-compose client - run: sudo apt update && sudo apt install docker-compose -ydocker/sentinel-compose.yml (1)
107-108: Add network configuration detailsSpecify the network driver and subnet for better control over the Redis Sentinel network.
networks: redis-sentinel: + driver: bridge + ipam: + config: + - subnet: 172.28.0.0/16docker/cluster-compose.yml (3)
145-146: Enhance network configuration for production useThe network configuration lacks important production settings:
- Network driver specification
- Network configuration options
- Network name uniqueness guarantee
Consider enhancing the network configuration:
networks: redis-cluster: + name: ${COMPOSE_PROJECT_NAME:-falkordb}_redis_cluster + driver: overlay # for Swarm mode, or 'bridge' for standalone + driver_opts: + com.docker.network.driver.mtu: 1400 + internal: true # prevent external access🧰 Tools
🪛 yamllint (1.35.1)
[error] 146-146: no new line character at the end of file
(new-line-at-end-of-file)
1-146: Document and enhance the cluster architectureThe cluster configuration would benefit from:
- Documentation explaining:
- Cluster topology (3 masters, 3 replicas)
- Port assignments
- Setup requirements
- Security considerations:
- Redis password/ACL configuration
- Network security
- Configuration flexibility using environment variables
Consider adding a README.md in the docker directory with setup instructions and creating an example .env file:
# Redis Cluster Setup This configuration creates a 6-node Redis cluster with 3 master nodes and 3 replica nodes. ## Configuration Copy `.env.example` to `.env` and adjust the values: ```env REDIS_PASSWORD=your_secure_password REDIS_VERSION=latest REDIS_PORT_START=5000 REDIS_REPLICA_COUNT=1Usage
docker compose -f cluster-compose.yml up -dWould you like me to help create this documentation and environment configuration? <details> <summary>🧰 Tools</summary> <details> <summary>🪛 yamllint (1.35.1)</summary> [warning] 11-11: wrong indentation: expected 6 but found 10 (indentation) --- [error] 146-146: no new line character at the end of file (new-line-at-end-of-file) </details> </details> --- `11-11`: **Fix formatting issues** The following formatting issues were detected: 1. Incorrect indentation in healthcheck section (line 11) 2. Missing newline at end of file (line 146) Apply these formatting fixes: ```diff healthcheck: - test: | + test: | redis-cli --cluster create node0:5000 node1:6000 \ networks: redis-cluster: +Also applies to: 146-146
🧰 Tools
🪛 yamllint (1.35.1)
[warning] 11-11: wrong indentation: expected 6 but found 10
(indentation)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/node.js.yml(1 hunks)docker/cluster-compose.yml(1 hunks)docker/sentinel-compose.yml(1 hunks)docker/standalone-compose.yml(1 hunks)
🧰 Additional context used
🪛 yamllint (1.35.1)
.github/workflows/node.js.yml
[error] 37-37: trailing spaces
(trailing-spaces)
[error] 49-49: trailing spaces
(trailing-spaces)
[error] 58-58: trailing spaces
(trailing-spaces)
docker/cluster-compose.yml
[warning] 11-11: wrong indentation: expected 6 but found 10
(indentation)
[error] 146-146: no new line character at the end of file
(new-line-at-end-of-file)
docker/sentinel-compose.yml
[error] 13-13: trailing spaces
(trailing-spaces)
[error] 45-45: trailing spaces
(trailing-spaces)
[error] 81-81: trailing spaces
(trailing-spaces)
[error] 86-86: trailing spaces
(trailing-spaces)
[error] 100-100: trailing spaces
(trailing-spaces)
| services: | ||
| redis-standalone: | ||
| container_name: redis-standalone | ||
| hostname: redis-standalone | ||
| image: falkordb/falkordb:edge | ||
| ports: | ||
| - 6379:6379 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add essential container configurations for production readiness
The standalone Redis configuration needs several important additions for production deployment:
- Volume persistence to prevent data loss
- Health check for container orchestration
- Resource limits to prevent resource exhaustion
- Restart policy for automatic recovery
services:
redis-standalone:
container_name: redis-standalone
hostname: redis-standalone
image: falkordb/falkordb:edge
ports:
- 6379:6379
+ volumes:
+ - redis-data:/data
+ healthcheck:
+ test: ["CMD", "redis-cli", "ping"]
+ interval: 10s
+ timeout: 5s
+ retries: 3
+ deploy:
+ resources:
+ limits:
+ memory: 1G
+ restart: unless-stopped
+
+volumes:
+ redis-data:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| services: | |
| redis-standalone: | |
| container_name: redis-standalone | |
| hostname: redis-standalone | |
| image: falkordb/falkordb:edge | |
| ports: | |
| - 6379:6379 | |
| services: | |
| redis-standalone: | |
| container_name: redis-standalone | |
| hostname: redis-standalone | |
| image: falkordb/falkordb:edge | |
| ports: | |
| - 6379:6379 | |
| volumes: | |
| - redis-data:/data | |
| healthcheck: | |
| test: ["CMD", "redis-cli", "ping"] | |
| interval: 10s | |
| timeout: 5s | |
| retries: 3 | |
| deploy: | |
| resources: | |
| limits: | |
| memory: 1G | |
| restart: unless-stopped | |
| volumes: | |
| redis-data: |
| redis-standalone: | ||
| container_name: redis-standalone | ||
| hostname: redis-standalone | ||
| image: falkordb/falkordb:edge |
There was a problem hiding this comment.
💡 Codebase verification
Consider using the stable version v4.4.1 instead of 'edge'
Using the edge tag can lead to unexpected behavior as it always pulls the latest build. Based on the available tags from Docker Hub, I recommend using v4.4.1 which is the latest stable version. This will ensure better stability and reproducibility of your deployment.
Example:
image: falkordb/falkordb:v4.4.1🔗 Analysis chain
Consider using a specific version tag instead of 'edge'
Using the edge tag can lead to unexpected behavior as it always pulls the latest build. Consider using a specific version tag for better stability and reproducibility.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check available FalkorDB tags
curl -s https://registry.hub.docker.com/v2/repositories/falkordb/falkordb/tags | jq -r '.results[].name'
Length of output: 206
| - name: Run docker-compose for redis standalone instance | ||
| uses: hoverkraft-tech/[email protected] | ||
| with: | ||
| compose-file: ./docker/standalone-compose.yml | ||
|
|
||
| - name: Run docker-compose for redis replication instance | ||
| uses: hoverkraft-tech/[email protected] | ||
| with: | ||
| compose-file: ./docker/sentinel-compose.yml | ||
|
|
||
| - name: Run docker-compose for redis cluster instance | ||
| uses: hoverkraft-tech/[email protected] | ||
| with: | ||
| compose-file: ./docker/cluster-compose.yml |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add service health checks before running tests
The workflow starts multiple Redis instances but doesn't verify if they're ready before proceeding. Add health checks to ensure services are fully operational.
- name: Run docker-compose for redis standalone instance
uses: hoverkraft-tech/[email protected]
with:
compose-file: ./docker/standalone-compose.yml
+ - name: Wait for standalone Redis to be ready
+ run: |
+ timeout 30s bash -c 'until redis-cli ping; do sleep 1; done'
- name: Run docker-compose for redis replication instance
uses: hoverkraft-tech/[email protected]
with:
compose-file: ./docker/sentinel-compose.yml
+ - name: Wait for sentinel setup to be ready
+ run: |
+ timeout 60s bash -c 'until redis-cli -p 26379 ping; do sleep 1; done'
- name: Run docker-compose for redis cluster instance
uses: hoverkraft-tech/[email protected]
with:
compose-file: ./docker/cluster-compose.yml
+ - name: Wait for cluster to be ready
+ run: |
+ timeout 60s bash -c 'until redis-cli cluster info; do sleep 1; done'Committable suggestion skipped: line range outside the PR's diff.
| touch /data/sentinel.conf | ||
| echo "sentinel monitor master redis-server-1 6380 2" >> /data/sentinel.conf | ||
| echo "sentinel announce-hostnames yes" >> /data/sentinel.conf | ||
| echo "sentinel announce-ip sentinel-1" >> /data/sentinel.conf | ||
| echo "sentinel resolve-hostnames yes" >> /data/sentinel.conf | ||
| echo "sentinel announce-port 26379" >> /data/sentinel.conf | ||
| echo "port 26379" >> /data/sentinel.conf | ||
| redis-server /data/sentinel.conf --sentinel |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance Sentinel configuration with additional parameters
The current Sentinel configuration lacks important parameters for production deployment:
down-after-milliseconds: Timeout to mark node as downfailover-timeout: Time before triggering failoverparallel-syncs: Number of replicas that can sync simultaneously
touch /data/sentinel.conf
echo "sentinel monitor master redis-server-1 6380 2" >> /data/sentinel.conf
+ echo "sentinel down-after-milliseconds master 5000" >> /data/sentinel.conf
+ echo "sentinel failover-timeout master 60000" >> /data/sentinel.conf
+ echo "sentinel parallel-syncs master 1" >> /data/sentinel.conf
echo "sentinel announce-hostnames yes" >> /data/sentinel.conf
echo "sentinel announce-ip sentinel-1" >> /data/sentinel.conf
echo "sentinel resolve-hostnames yes" >> /data/sentinel.conf📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| touch /data/sentinel.conf | |
| echo "sentinel monitor master redis-server-1 6380 2" >> /data/sentinel.conf | |
| echo "sentinel announce-hostnames yes" >> /data/sentinel.conf | |
| echo "sentinel announce-ip sentinel-1" >> /data/sentinel.conf | |
| echo "sentinel resolve-hostnames yes" >> /data/sentinel.conf | |
| echo "sentinel announce-port 26379" >> /data/sentinel.conf | |
| echo "port 26379" >> /data/sentinel.conf | |
| redis-server /data/sentinel.conf --sentinel | |
| touch /data/sentinel.conf | |
| echo "sentinel monitor master redis-server-1 6380 2" >> /data/sentinel.conf | |
| echo "sentinel down-after-milliseconds master 5000" >> /data/sentinel.conf | |
| echo "sentinel failover-timeout master 60000" >> /data/sentinel.conf | |
| echo "sentinel parallel-syncs master 1" >> /data/sentinel.conf | |
| echo "sentinel announce-hostnames yes" >> /data/sentinel.conf | |
| echo "sentinel announce-ip sentinel-1" >> /data/sentinel.conf | |
| echo "sentinel resolve-hostnames yes" >> /data/sentinel.conf | |
| echo "sentinel announce-port 26379" >> /data/sentinel.conf | |
| echo "port 26379" >> /data/sentinel.conf | |
| redis-server /data/sentinel.conf --sentinel |
| redis-server-1: | ||
| image: falkordb/falkordb:edge | ||
| hostname: redis-server-1 | ||
| container_name: redis-server-1 | ||
| ports: | ||
| - 6380:6380 | ||
| networks: | ||
| - redis-sentinel | ||
| entrypoint: /bin/bash | ||
| command: | ||
| - -c | ||
| - | | ||
| touch /data/node.conf | ||
| echo "port 6380" >> /data/node.conf | ||
| echo "replica-announce-ip redis-server-1" >> /data/node.conf | ||
| echo "replica-announce-port 6380" >> /data/node.conf | ||
| echo "loadmodule /FalkorDB/bin/src/falkordb.so" >> /data/node.conf | ||
| redis-server /data/node.conf |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add persistence and security configurations for Redis master
The Redis master configuration needs additional settings for durability and security:
- Data persistence
- Password protection
- Memory limits
touch /data/node.conf
echo "port 6380" >> /data/node.conf
+ echo "requirepass your_redis_password" >> /data/node.conf
+ echo "masterauth your_redis_password" >> /data/node.conf
+ echo "maxmemory 1gb" >> /data/node.conf
+ echo "maxmemory-policy allkeys-lru" >> /data/node.conf
+ echo "appendonly yes" >> /data/node.conf
+ echo "appendfsync everysec" >> /data/node.conf
echo "replica-announce-ip redis-server-1" >> /data/node.conf
echo "replica-announce-port 6380" >> /data/node.conf
echo "loadmodule /FalkorDB/bin/src/falkordb.so" >> /data/node.confAlso, add volume configuration:
image: falkordb/falkordb:edge
hostname: redis-server-1
container_name: redis-server-1
+ volumes:
+ - redis-master-data:/data
ports:
- 6380:6380📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| redis-server-1: | |
| image: falkordb/falkordb:edge | |
| hostname: redis-server-1 | |
| container_name: redis-server-1 | |
| ports: | |
| - 6380:6380 | |
| networks: | |
| - redis-sentinel | |
| entrypoint: /bin/bash | |
| command: | |
| - -c | |
| - | | |
| touch /data/node.conf | |
| echo "port 6380" >> /data/node.conf | |
| echo "replica-announce-ip redis-server-1" >> /data/node.conf | |
| echo "replica-announce-port 6380" >> /data/node.conf | |
| echo "loadmodule /FalkorDB/bin/src/falkordb.so" >> /data/node.conf | |
| redis-server /data/node.conf | |
| redis-server-1: | |
| image: falkordb/falkordb:edge | |
| hostname: redis-server-1 | |
| container_name: redis-server-1 | |
| volumes: | |
| - redis-master-data:/data | |
| ports: | |
| - 6380:6380 | |
| networks: | |
| - redis-sentinel | |
| entrypoint: /bin/bash | |
| command: | |
| - -c | |
| - | | |
| touch /data/node.conf | |
| echo "port 6380" >> /data/node.conf | |
| echo "requirepass your_redis_password" >> /data/node.conf | |
| echo "masterauth your_redis_password" >> /data/node.conf | |
| echo "maxmemory 1gb" >> /data/node.conf | |
| echo "maxmemory-policy allkeys-lru" >> /data/node.conf | |
| echo "appendonly yes" >> /data/node.conf | |
| echo "appendfsync everysec" >> /data/node.conf | |
| echo "replica-announce-ip redis-server-1" >> /data/node.conf | |
| echo "replica-announce-port 6380" >> /data/node.conf | |
| echo "loadmodule /FalkorDB/bin/src/falkordb.so" >> /data/node.conf | |
| redis-server /data/node.conf |
🧰 Tools
🪛 yamllint (1.35.1)
[error] 81-81: trailing spaces
(trailing-spaces)
| @@ -0,0 +1,146 @@ | |||
| services: | |||
| node0: | |||
| image: falkordb/falkordb:edge | |||
There was a problem hiding this comment.
Fix critical issues in healthcheck configuration and image tag
- The
edgetag is not recommended for production use as it may contain unstable changes. Consider using a specific version tag. - The healthcheck has several issues:
- The command
|| echo failed && exit 1doesn't work as intended due to shell operator precedence - No dependency management between nodes
- No wait for other nodes to be ready before cluster creation
- The command
Consider these fixes:
- image: falkordb/falkordb:edge
+ image: falkordb/falkordb:latest # or specific version tag
# ... other configs ...
healthcheck:
test: |
- redis-cli --cluster create node0:5000 node1:6000 \
- node2:7000 node3:8000 node4:9000 node5:10000 --cluster-replicas 1 --cluster-yes || echo failed && exit 1
+ timeout 5 bash -c '
+ until redis-cli -h node1 -p 6000 ping && \
+ redis-cli -h node2 -p 7000 ping && \
+ redis-cli -h node3 -p 8000 ping && \
+ redis-cli -h node4 -p 9000 ping && \
+ redis-cli -h node5 -p 10000 ping; do
+ sleep 1
+ done
+ redis-cli --cluster create node0:5000 node1:6000 \
+ node2:7000 node3:8000 node4:9000 node5:10000 \
+ --cluster-replicas 1 --cluster-yes || exit 1'Also applies to: 10-17
| touch /data/node.conf | ||
| echo "port 5000" >> /data/node.conf | ||
| echo "loadmodule /FalkorDB/bin/src/falkordb.so" >> /data/node.conf | ||
| echo "cluster-preferred-endpoint-type hostname" >> /data/node.conf | ||
| echo "cluster-port 16379" >> /data/node.conf | ||
| echo "cluster-enabled yes" >> /data/node.conf | ||
| echo "cluster-config-file /data/nodes.conf" >> /data/node.conf | ||
| echo "cluster-node-timeout 5000" >> /data/node.conf | ||
| echo "cluster-announce-hostname node0" >> /data/node.conf | ||
| redis-server /data/node.conf |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve configuration management and production readiness
- The node configurations are heavily duplicated. Consider using Docker Compose extensions to define a common template.
- Missing important production configurations:
- No volume mounts for data persistence
- No restart policy defined
- No resource limits
Consider refactoring using this pattern:
x-node-template: &node-template
image: falkordb/falkordb:latest
networks:
- redis-cluster
restart: unless-stopped
volumes:
- redis-data:/data
entrypoint: /bin/bash
command:
- -c
- |
touch /data/node.conf
cat > /data/node.conf << EOF
port ${PORT}
loadmodule /FalkorDB/bin/src/falkordb.so
cluster-preferred-endpoint-type hostname
cluster-port 16379
cluster-enabled yes
cluster-config-file /data/nodes.conf
cluster-node-timeout 5000
cluster-announce-hostname ${HOSTNAME}
EOF
redis-server /data/node.conf
services:
node0:
<<: *node-template
environment:
- PORT=5000
ports:
- "${PORT}:${PORT}"
# ... other specific configs
volumes:
redis-data:Also applies to: 44-53, 66-75, 88-97, 110-119, 133-142
fix #250
Summary by CodeRabbit
New Features
Bug Fixes
Documentation