feat(falkordb): POSIX sh migration and chart 1.0.3 release prep#3
Open
shahar-biron wants to merge 17 commits into
Open
feat(falkordb): POSIX sh migration and chart 1.0.3 release prep#3shahar-biron wants to merge 17 commits into
shahar-biron wants to merge 17 commits into
Conversation
FalkorDB 4.18.3+ deploys on Debian 13 which ships without /bin/bash.
This migrates every lifecycle script and Helm template reference from
bash to POSIX sh so the addon runs on any /bin/sh-based container.
Changes across 30+ shell scripts (scripts/, falkordb-cluster-scripts/,
dataprotection/) and 10 template YAML files:
- Shebangs: #!/bin/bash -> #!/bin/sh
- Bash arrays -> pipe-separated strings
- declare -A associative arrays -> TAB-separated temp-file maps with
_map_get / _map_append / _map_size / _map_keys helpers
- [[ ... ]] / BASH_REMATCH -> POSIX [ ] and sed
- <<< here-strings -> << _EOF_ heredocs
- source -> . (dot-source)
- declare -g/-a -> plain assignments
- ((n++)) -> n=$((n + 1))
- ${var/pat/replace} -> sed-based _mask_password() helper
- IFS=',' read -ra arr -> tr + for loops
- Process substitutions -> temp files
- Template YAML: /bin/bash and - bash -> /bin/sh and - sh
Also adds DEPLOY.md with build, lint, local-test, install, upgrade,
rollback and troubleshooting instructions for the addon chart.
Verified:
- helm lint --strict: 0 failures
- shellcheck --shell=sh --severity=error: 0 errors
- All 35 production scripts in dist/falkordb-1.0.2.tgz start with #!/bin/sh
Co-Authored-By: Oz <oz-agent@warp.dev>
- Replace bash assoc array stubs (declare -gA + map[key]=val) with _map_append calls for initialize_redis_cluster primary/secondary nodes - Replace scale_out stubs using old array pattern with _map_append - Replace scale_in stubs using bash array assignment with plain strings - Replace current_comp_primary_node+=() with string assignment in falkordb_cluster_server_start_spec.sh - Update sentinel_pod_list assertions from array indexes to pipe-separated string in falkordb_sentinel_member_leave_spec.sh Co-Authored-By: Oz <oz-agent@warp.dev>
- Fix _map_size bug in falkordb-cluster-manage.sh - Fix print statements in init_other_components_and_pods_info - Fix port validation in parse_advertised_svc_and_port - Fix sentinel PORT extraction in falkordb-start.sh Co-Authored-By: Oz <oz-agent@warp.dev>
…format - parse_advertised_svc_and_port: use case statement to detect colon in entry since both GNU and BSD cut return the full string (not empty) for 'cut -d: -f2' when no delimiter is present. Entries without ':' now explicitly set port="" so the existing is_empty guard fires. - falkordb_start_spec.sh: update build_sentinel_get_master_addr_by_name_command mocks to produce two-line output (host\nport) matching real Redis sentinel behaviour; single-line space-separated mock caused PORT to be empty (sed -n '2p' on a 1-line string) and tests 28-29 to fail. Co-Authored-By: Oz <oz-agent@warp.dev>
Line 196 had 'export export service_por="6380"' which is both a double-export (SC2316) and a variable name typo; fix to 'export SERVICE_PORT="6380"' to match the assertion that checks $SERVICE_PORT. Co-Authored-By: Oz <oz-agent@warp.dev>
The function reads $service_port (set at script include-time as
${SERVICE_PORT:-6379}), not $SERVICE_PORT. Use lowercase service_port
and an explicit expected value in the assertion so the test actually
validates the non-default port path.
Co-Authored-By: Oz <oz-agent@warp.dev>
Co-Authored-By: Oz <oz-agent@warp.dev>
Co-Authored-By: Oz <oz-agent@warp.dev>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a role-probe and graph-metrics exporters, refactors cluster-manage retry/error handling and cluster-initialization checks, updates Helm templates/values to expose graph metrics and wire the probe, extends unit tests, and bumps the chart version to 1.0.3. ChangesCluster post-provision and probe changes
Sequence DiagramsequenceDiagram
participant Kubelet as Kubelet lifecycle probe
participant RoleProbe as /scripts/falkordb-cluster-role-probe.sh
participant Dbctl as tools/dbctl
participant Redis as redis-cli
Kubelet->>RoleProbe: exec probe
RoleProbe->>Dbctl: dbctl redis getrole
RoleProbe->>Redis: redis-cli cluster info (when ordinal!=0 && reported primary)
Redis-->>RoleProbe: cluster_state
RoleProbe-->>Kubelet: print role / exit
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@addons/falkordb/Chart.yaml`:
- Around line 7-9: Chart metadata in Chart.yaml is inconsistent: the chart
version was bumped to 1.0.3 but appVersion remains "4.14.10" while the PR
targets FalkorDB 4.18.6; update the appVersion field in Chart.yaml from
"4.14.10" to "4.18.6" so the appVersion matches the intended FalkorDB upgrade
and reflects correct chart metadata.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 92f12a83-838f-49b1-a187-39d78b6bc970
⛔ Files ignored due to path filters (1)
dist/falkordb-1.0.3.tgzis excluded by!**/dist/**
📒 Files selected for processing (1)
addons/falkordb/Chart.yaml
Co-Authored-By: Oz <oz-agent@warp.dev>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@addons/falkordb/falkordb-cluster-scripts/falkordb-cluster-manage.sh`:
- Around line 71-73: Validate and normalize the env-derived retry inputs before
doing numeric comparisons: ensure max_retries and retry_interval (local
variables max_retries, retry_interval) are checked to be positive integers and
if not set or non-numeric, default them to safe values (e.g. max_retries=5,
retry_interval=2); convert them to integer form (e.g. via parameter expansion or
integer parsing) so the subsequent retry loop and backoff logic that references
these variables cannot enter a tight loop or behave unpredictably. Apply the
same validation/normalization where those variables are used later in the script
(the retry loop and backoff logic that follows the variable declarations).
In `@addons/falkordb/falkordb-cluster-scripts/falkordb-cluster-role-probe.sh`:
- Around line 16-18: The script uses the non-POSIX keyword "local" for variables
like service_port and cluster_info in falkordb-cluster-role-probe.sh; change
those declarations to POSIX-compatible patterns by removing "local" and either
(a) renaming the variables with a function-specific prefix (e.g.,
probe_service_port, probe_cluster_info) to avoid global collisions, or (b)
switch the shebang to #!/bin/bash if bash is guaranteed; update all other
occurrences (lines ~30-35 and similar in other cluster scripts) to follow the
same approach and ensure KB_SERVICE_PORT/SERVICE_PORT fallback logic remains
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bda7209a-0cd6-4d98-866b-909708ebdf48
⛔ Files ignored due to path filters (1)
dist/falkordb-1.0.3.tgzis excluded by!**/dist/**
📒 Files selected for processing (6)
addons/falkordb/falkordb-cluster-scripts/falkordb-cluster-common.shaddons/falkordb/falkordb-cluster-scripts/falkordb-cluster-manage.shaddons/falkordb/falkordb-cluster-scripts/falkordb-cluster-role-probe.shaddons/falkordb/scripts-ut-spec/falkordb_cluster_common_spec.shaddons/falkordb/scripts-ut-spec/falkordb_cluster_manage_spec.shaddons/falkordb/templates/cmpd-falkordb-cluster.yaml
Comment on lines
+71
to
+73
| local max_retries="$1" | ||
| local retry_interval="$2" | ||
| local function_name="$3" |
There was a problem hiding this comment.
Validate retry inputs before numeric comparisons.
max_retries/retry_interval are env-derived strings; if non-numeric, comparisons/backoff become unreliable (including tight-loop retries). Add numeric validation/defaulting before the loop.
Suggested patch
call_func_with_retry_when_ut_mode_false() {
local max_retries="$1"
local retry_interval="$2"
local function_name="$3"
shift 3
+
+ case "$max_retries" in
+ ''|*[!0-9]*) max_retries=3 ;;
+ esac
+ case "$retry_interval" in
+ ''|*[!0-9]*) retry_interval=5 ;;
+ esac
local retries=0
while true; doAlso applies to: 83-91
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@addons/falkordb/falkordb-cluster-scripts/falkordb-cluster-manage.sh` around
lines 71 - 73, Validate and normalize the env-derived retry inputs before doing
numeric comparisons: ensure max_retries and retry_interval (local variables
max_retries, retry_interval) are checked to be positive integers and if not set
or non-numeric, default them to safe values (e.g. max_retries=5,
retry_interval=2); convert them to integer form (e.g. via parameter expansion or
integer parsing) so the subsequent retry loop and backoff logic that references
these variables cannot enter a tight loop or behave unpredictably. Apply the
same validation/normalization where those variables are used later in the script
(the retry loop and backoff logic that follows the variable declarations).
Comment on lines
+16
to
+18
| local service_port | ||
| local cluster_info | ||
| service_port="${KB_SERVICE_PORT:-${SERVICE_PORT:-6379}}" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify non-POSIX `local` usage in sh scripts for this addon.
rg -n '^[[:space:]]*local[[:space:]]+' addons/falkordb/falkordb-cluster-scripts/*.shRepository: FalkorDB/kubeblocks-addons
Length of output: 28832
🏁 Script executed:
head -5 addons/falkordb/falkordb-cluster-scripts/falkordb-cluster-role-probe.shRepository: FalkorDB/kubeblocks-addons
Length of output: 138
Replace local with alternative POSIX-compatible variable scoping in POSIX sh scripts.
This script uses #!/bin/sh but declares variables with local, which is not POSIX-specified and will fail on stricter /bin/sh implementations (e.g., dash). Use function-scoped naming conventions (e.g., prefixing variables with function name or using explicit cleanup) instead, or switch to #!/bin/bash if bash is guaranteed in the target environment.
Affected: Lines 16-18 and 30-35, plus similar usage throughout other cluster scripts.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@addons/falkordb/falkordb-cluster-scripts/falkordb-cluster-role-probe.sh`
around lines 16 - 18, The script uses the non-POSIX keyword "local" for
variables like service_port and cluster_info in falkordb-cluster-role-probe.sh;
change those declarations to POSIX-compatible patterns by removing "local" and
either (a) renaming the variables with a function-specific prefix (e.g.,
probe_service_port, probe_cluster_info) to avoid global collisions, or (b)
switch the shebang to #!/bin/bash if bash is guaranteed; update all other
occurrences (lines ~30-35 and similar in other cluster scripts) to follow the
same approach and ensure KB_SERVICE_PORT/SERVICE_PORT fallback logic remains
unchanged.
Co-Authored-By: Oz <oz-agent@warp.dev>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
addons/falkordb/falkordb-cluster-scripts/graph-list-metrics-exporter.sh (3)
5-9: ⚡ Quick winConsider validating required environment variables.
The script relies on
REDIS_DEFAULT_USERandREDIS_DEFAULT_PASSWORDbut does not validate they are set. If these are missing (e.g., due to misconfiguration), the script will fail silently or produce unclear errors.✅ Proposed validation check
GRAPH_METRICS_DIR="${GRAPH_METRICS_DIR:-/tmp/graph-metrics}" GRAPH_METRICS_FILE="${GRAPH_METRICS_DIR}/metrics" GRAPH_METRICS_HTTP_PORT="${GRAPH_METRICS_HTTP_PORT:-9122}" GRAPH_METRICS_POLL_INTERVAL_SECONDS="${GRAPH_METRICS_POLL_INTERVAL_SECONDS:-15}" REDIS_PORT="${SERVICE_PORT:-6379}" + +if [ -z "${REDIS_DEFAULT_USER:-}" ] || [ -z "${REDIS_DEFAULT_PASSWORD:-}" ]; then + echo "ERROR: REDIS_DEFAULT_USER and REDIS_DEFAULT_PASSWORD must be set" >&2 + exit 1 +fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@addons/falkordb/falkordb-cluster-scripts/graph-list-metrics-exporter.sh` around lines 5 - 9, The script uses REDIS_DEFAULT_USER and REDIS_DEFAULT_PASSWORD but doesn't validate them; add an explicit check near the top of graph-list-metrics-exporter.sh (around where GRAPH_METRICS_DIR, GRAPH_METRICS_FILE, GRAPH_METRICS_HTTP_PORT, GRAPH_METRICS_POLL_INTERVAL_SECONDS, REDIS_PORT are defined) that tests that REDIS_DEFAULT_USER and REDIS_DEFAULT_PASSWORD are non-empty, print a clear error to stderr (including the variable names) and exit non-zero if either is missing so the failure is explicit and fast.
60-60: ⚖️ Poor tradeoffTrap may not terminate active
ncprocess.When the
nc-based server is used, killing thewhileloop PID (line 59) does not guarantee termination of an activencprocess that may be blocked accepting a connection. Consider sendingSIGTERMto the process group or adding cleanup logic to kill child processes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@addons/falkordb/falkordb-cluster-scripts/graph-list-metrics-exporter.sh` at line 60, The trap currently only kills the saved server_pid which may not terminate a blocked nc child; modify the cleanup to also terminate the nc child/process group by sending SIGTERM to the process group or explicitly killing child PIDs: e.g., start the server loop in its own process group (setsid or set -m) or record nc's PID (nc_pid) and in the trap do kill "${server_pid}" || true; kill -- -${server_pid} || kill "${nc_pid}" || true; exit 0; update the trap line handling TERM and INT to include those additional kill actions so any lingering nc processes are cleaned up.
47-52: ⚡ Quick win
ncportability and blocking concerns.The
nc -l -psyntax varies across implementations (GNU netcat, BSD netcat, BusyBox nc). Some versions use-l portinstead of-l -p port, and the server loop may block indefinitely if a client holds the connection open or ifncisn't available.Consider adding a timeout and documenting the expected
ncvariant, or verifying that the Redis image includes a compatiblenc:while true; do { printf 'HTTP/1.1 200 OK\r\nContent-Type: text/plain; version=0.0.4; charset=utf-8\r\nConnection: close\r\n\r\n' cat "${GRAPH_METRICS_FILE}" - } | nc -l -p "${GRAPH_METRICS_HTTP_PORT}" >/dev/null 2>&1 + } | timeout 5 nc -l -p "${GRAPH_METRICS_HTTP_PORT}" >/dev/null 2>&1 || true done &🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@addons/falkordb/falkordb-cluster-scripts/graph-list-metrics-exporter.sh` around lines 47 - 52, The current loop serving GRAPH_METRICS_FILE on GRAPH_METRICS_HTTP_PORT uses a non-portable nc invocation and can block indefinitely; update the script to (1) detect/verify a compatible nc binary at startup (or document required nc variant), (2) choose nc flags based on detected variant (support both "nc -l -p PORT" and "nc -l PORT" styles), and (3) add connection timeouts/quit flags (e.g. -w / -q or equivalent) so clients cannot keep the server blocked; also add a fallback path (e.g. simple Python http server or socat) if nc is missing to ensure the exporter reliably starts. Ensure references to GRAPH_METRICS_FILE and GRAPH_METRICS_HTTP_PORT remain used by the chosen command.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@addons/falkordb/falkordb-cluster-scripts/graph-list-metrics-exporter.sh`:
- Line 27: The current call to redis-cli exposes credentials via command-line
flags (--user/--pass) in the process list; instead export credentials in
environment variables and call redis-cli without those flags. Remove --user
"${REDIS_DEFAULT_USER}" and --pass "${REDIS_DEFAULT_PASSWORD}" from the
redis-cli invocation in the line that assigns output, and before invoking set
REDISCLI_AUTH="${REDIS_DEFAULT_PASSWORD}" (and, if using ACL users, set
REDISCLI_USER="${REDIS_DEFAULT_USER}") in the environment so redis-cli reads
them automatically; keep tls_args, host/port, and the GRAPH.LIST command
unchanged.
In `@addons/falkordb/templates/cmpd-falkordb-cluster.yaml`:
- Around line 616-639: Add an explicit SERVICE_PORT env to the graph-metrics
container so graph-list-metrics-exporter.sh gets the same port as the main
service: in the graph-metrics container block (container name "graph-metrics")
add an env entry for SERVICE_PORT with the same Helm value used for the main
service port (use the same template variable defined in your chart vars, e.g. {{
$.Values.service.port | quote }} or the exact variable used by the main
container) so the script's ${SERVICE_PORT:-6379} resolves consistently.
In `@addons/falkordb/templates/cmpd-falkordb.yaml`:
- Around line 493-516: The graph-metrics container is missing the SERVICE_PORT
env var used by the script; update the graph-metrics container's env block to
add an entry named SERVICE_PORT whose value is populated from the chart/vars
SERVICE_PORT template variable (the same SERVICE_PORT defined in your chart
vars), e.g. add "- name: SERVICE_PORT" with the template value for SERVICE_PORT
so the graph-list-metrics-exporter.sh sees the correct Redis port in
host-network deployments.
---
Nitpick comments:
In `@addons/falkordb/falkordb-cluster-scripts/graph-list-metrics-exporter.sh`:
- Around line 5-9: The script uses REDIS_DEFAULT_USER and REDIS_DEFAULT_PASSWORD
but doesn't validate them; add an explicit check near the top of
graph-list-metrics-exporter.sh (around where GRAPH_METRICS_DIR,
GRAPH_METRICS_FILE, GRAPH_METRICS_HTTP_PORT,
GRAPH_METRICS_POLL_INTERVAL_SECONDS, REDIS_PORT are defined) that tests that
REDIS_DEFAULT_USER and REDIS_DEFAULT_PASSWORD are non-empty, print a clear error
to stderr (including the variable names) and exit non-zero if either is missing
so the failure is explicit and fast.
- Line 60: The trap currently only kills the saved server_pid which may not
terminate a blocked nc child; modify the cleanup to also terminate the nc
child/process group by sending SIGTERM to the process group or explicitly
killing child PIDs: e.g., start the server loop in its own process group (setsid
or set -m) or record nc's PID (nc_pid) and in the trap do kill "${server_pid}"
|| true; kill -- -${server_pid} || kill "${nc_pid}" || true; exit 0; update the
trap line handling TERM and INT to include those additional kill actions so any
lingering nc processes are cleaned up.
- Around line 47-52: The current loop serving GRAPH_METRICS_FILE on
GRAPH_METRICS_HTTP_PORT uses a non-portable nc invocation and can block
indefinitely; update the script to (1) detect/verify a compatible nc binary at
startup (or document required nc variant), (2) choose nc flags based on detected
variant (support both "nc -l -p PORT" and "nc -l PORT" styles), and (3) add
connection timeouts/quit flags (e.g. -w / -q or equivalent) so clients cannot
keep the server blocked; also add a fallback path (e.g. simple Python http
server or socat) if nc is missing to ensure the exporter reliably starts. Ensure
references to GRAPH_METRICS_FILE and GRAPH_METRICS_HTTP_PORT remain used by the
chosen command.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3dc29e6c-b9df-4909-8786-00be4ad8c557
⛔ Files ignored due to path filters (1)
dist/falkordb-1.0.3.tgzis excluded by!**/dist/**
📒 Files selected for processing (8)
addons/falkordb/falkordb-cluster-scripts/graph-list-metrics-exporter.shaddons/falkordb/scripts/graph-list-metrics-exporter.shaddons/falkordb/templates/_helpers.tpladdons/falkordb/templates/cmpd-falkordb-cluster.yamladdons/falkordb/templates/cmpd-falkordb.yamladdons/falkordb/templates/cmpv-falkordb-cluster.yamladdons/falkordb/templates/cmpv-falkordb.yamladdons/falkordb/values.yaml
| fi | ||
|
|
||
| # shellcheck disable=SC2086 | ||
| output="$(redis-cli --user "${REDIS_DEFAULT_USER}" --pass "${REDIS_DEFAULT_PASSWORD}" ${tls_args} -h 127.0.0.1 -p "${REDIS_PORT}" --raw GRAPH.LIST 2>/dev/null || true)" |
There was a problem hiding this comment.
Credentials exposed in process list.
Passing --user and --pass as command-line arguments to redis-cli exposes credentials in the process list (visible via ps, /proc, logs, etc.). Use REDISCLI_AUTH environment variable instead, which redis-cli reads automatically without exposing the password in the process table.
🔐 Recommended fix to use environment variable for credentials
get_graph_count() {
tls_args=""
if [ "${TLS_ENABLED:-false}" = "true" ]; then
tls_args="--tls --insecure"
fi
+ export REDISCLI_AUTH="${REDIS_DEFAULT_PASSWORD}"
# shellcheck disable=SC2086
- output="$(redis-cli --user "${REDIS_DEFAULT_USER}" --pass "${REDIS_DEFAULT_PASSWORD}" ${tls_args} -h 127.0.0.1 -p "${REDIS_PORT}" --raw GRAPH.LIST 2>/dev/null || true)"
+ output="$(redis-cli --user "${REDIS_DEFAULT_USER}" ${tls_args} -h 127.0.0.1 -p "${REDIS_PORT}" --raw GRAPH.LIST 2>/dev/null || true)"
if [ -z "${output}" ]; then📝 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.
Suggested change
| output="$(redis-cli --user "${REDIS_DEFAULT_USER}" --pass "${REDIS_DEFAULT_PASSWORD}" ${tls_args} -h 127.0.0.1 -p "${REDIS_PORT}" --raw GRAPH.LIST 2>/dev/null || true)" | |
| get_graph_count() { | |
| tls_args="" | |
| if [ "${TLS_ENABLED:-false}" = "true" ]; then | |
| tls_args="--tls --insecure" | |
| fi | |
| export REDISCLI_AUTH="${REDIS_DEFAULT_PASSWORD}" | |
| # shellcheck disable=SC2086 | |
| output="$(redis-cli --user "${REDIS_DEFAULT_USER}" ${tls_args} -h 127.0.0.1 -p "${REDIS_PORT}" --raw GRAPH.LIST 2>/dev/null || true)" | |
| if [ -z "${output}" ]; then |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@addons/falkordb/falkordb-cluster-scripts/graph-list-metrics-exporter.sh` at
line 27, The current call to redis-cli exposes credentials via command-line
flags (--user/--pass) in the process list; instead export credentials in
environment variables and call redis-cli without those flags. Remove --user
"${REDIS_DEFAULT_USER}" and --pass "${REDIS_DEFAULT_PASSWORD}" from the
redis-cli invocation in the line that assigns output, and before invoking set
REDISCLI_AUTH="${REDIS_DEFAULT_PASSWORD}" (and, if using ACL users, set
REDISCLI_USER="${REDIS_DEFAULT_USER}") in the environment so redis-cli reads
them automatically; keep tls_args, host/port, and the GRAPH.LIST command
unchanged.
Comment on lines
+616
to
+639
| - name: graph-metrics | ||
| command: | ||
| - /bin/sh | ||
| - -c | ||
| - /scripts/graph-list-metrics-exporter.sh | ||
| imagePullPolicy: {{ $.Values.graphMetrics.image.pullPolicy | quote }} | ||
| securityContext: | ||
| runAsUser: 59000 | ||
| runAsGroup: 59000 | ||
| allowPrivilegeEscalation: false | ||
| capabilities: | ||
| drop: | ||
| - ALL | ||
| env: | ||
| - name: GRAPH_METRICS_HTTP_PORT | ||
| value: "$(REDIS_GRAPH_METRICS_HTTP_PORT)" | ||
| - name: GRAPH_METRICS_POLL_INTERVAL_SECONDS | ||
| value: {{ $.Values.graphMetrics.pollIntervalSeconds | quote }} | ||
| volumeMounts: | ||
| - name: scripts | ||
| mountPath: /scripts | ||
| ports: | ||
| - name: graph-metrics | ||
| containerPort: 9122 |
There was a problem hiding this comment.
Missing SERVICE_PORT environment variable.
The graph-metrics container runs /scripts/graph-list-metrics-exporter.sh, which references ${SERVICE_PORT:-6379} on line 9 of the script to set REDIS_PORT. However, the graph-metrics container's env section (lines 629-633) does not include SERVICE_PORT. While there's a default fallback to 6379, it would be safer to explicitly pass SERVICE_PORT (defined in vars at line 345) to ensure consistency with the main container's port configuration, especially in host-network scenarios.
📝 Proposed fix to add SERVICE_PORT to env
env:
- name: GRAPH_METRICS_HTTP_PORT
value: "$(REDIS_GRAPH_METRICS_HTTP_PORT)"
- name: GRAPH_METRICS_POLL_INTERVAL_SECONDS
value: {{ $.Values.graphMetrics.pollIntervalSeconds | quote }}
+ - name: SERVICE_PORT
+ value: "$(SERVICE_PORT)"
volumeMounts:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@addons/falkordb/templates/cmpd-falkordb-cluster.yaml` around lines 616 - 639,
Add an explicit SERVICE_PORT env to the graph-metrics container so
graph-list-metrics-exporter.sh gets the same port as the main service: in the
graph-metrics container block (container name "graph-metrics") add an env entry
for SERVICE_PORT with the same Helm value used for the main service port (use
the same template variable defined in your chart vars, e.g. {{
$.Values.service.port | quote }} or the exact variable used by the main
container) so the script's ${SERVICE_PORT:-6379} resolves consistently.
Comment on lines
+493
to
+516
| - name: graph-metrics | ||
| command: | ||
| - /bin/sh | ||
| - -c | ||
| - /scripts/graph-list-metrics-exporter.sh | ||
| imagePullPolicy: {{ $.Values.graphMetrics.image.pullPolicy | quote }} | ||
| securityContext: | ||
| runAsUser: 59000 | ||
| runAsGroup: 59000 | ||
| allowPrivilegeEscalation: false | ||
| capabilities: | ||
| drop: | ||
| - ALL | ||
| env: | ||
| - name: GRAPH_METRICS_HTTP_PORT | ||
| value: "$(REDIS_GRAPH_METRICS_HTTP_PORT)" | ||
| - name: GRAPH_METRICS_POLL_INTERVAL_SECONDS | ||
| value: {{ $.Values.graphMetrics.pollIntervalSeconds | quote }} | ||
| volumeMounts: | ||
| - name: scripts | ||
| mountPath: /scripts | ||
| ports: | ||
| - name: graph-metrics | ||
| containerPort: 9122 |
There was a problem hiding this comment.
Missing SERVICE_PORT environment variable.
The graph-metrics container's script references ${SERVICE_PORT:-6379} to determine the Redis port, but SERVICE_PORT is not explicitly passed in the container's env section (lines 506-510). Although the script defaults to 6379, explicitly passing SERVICE_PORT (defined in vars at line 219) ensures correctness in host-network deployments where the port may differ.
📝 Proposed fix to add SERVICE_PORT to env
env:
- name: GRAPH_METRICS_HTTP_PORT
value: "$(REDIS_GRAPH_METRICS_HTTP_PORT)"
- name: GRAPH_METRICS_POLL_INTERVAL_SECONDS
value: {{ $.Values.graphMetrics.pollIntervalSeconds | quote }}
+ - name: SERVICE_PORT
+ value: "$(SERVICE_PORT)"
volumeMounts:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@addons/falkordb/templates/cmpd-falkordb.yaml` around lines 493 - 516, The
graph-metrics container is missing the SERVICE_PORT env var used by the script;
update the graph-metrics container's env block to add an entry named
SERVICE_PORT whose value is populated from the chart/vars SERVICE_PORT template
variable (the same SERVICE_PORT defined in your chart vars), e.g. add "- name:
SERVICE_PORT" with the template value for SERVICE_PORT so the
graph-list-metrics-exporter.sh sees the correct Redis port in host-network
deployments.
Co-Authored-By: Oz <oz-agent@warp.dev>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
addons/falkordb/falkordb-cluster-scripts/falkordb-cluster-role-probe.sh (2)
30-34:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winReplace
localwith POSIX-compatible variable declarations.Same issue as lines 16-17: the
localkeyword is not POSIX-compliant. Apply the same fix using function-scoped prefixes (e.g.,probe_role_output,probe_role,probe_current_pod_name,probe_pod_ordinal,probe_cluster_state).🔧 Proposed POSIX-compliant fix
run_role_probe() { - local role_output - local role - local current_pod_name - local pod_ordinal - local cluster_state + probe_role_output=$(/tools/dbctl redis getrole 2>/dev/null || true) + probe_role=$(normalize_role "$probe_role_output") - role_output=$(/tools/dbctl redis getrole 2>/dev/null || true) - role=$(normalize_role "$role_output") - - case "$role" in + case "$probe_role" in primary|secondary) ;; *) return 1 ;; esac - current_pod_name="${CURRENT_POD_NAME:-}" - pod_ordinal="${current_pod_name##*-}" + probe_current_pod_name="${CURRENT_POD_NAME:-}" + probe_pod_ordinal="${probe_current_pod_name##*-}" # During bootstrap/recovery, non-zero ordinal pods can briefly report PRIMARY # before replication converges. When cluster_state is not OK, treat that as # degraded and fall back to SECONDARY so role probing does not deadlock the # component in Updating/Failed. - if [ "$role" = "primary" ] && [ "$pod_ordinal" != "0" ]; then - cluster_state=$(get_cluster_state) - if [ "$cluster_state" != "ok" ]; then + if [ "$probe_role" = "primary" ] && [ "$probe_pod_ordinal" != "0" ]; then + probe_cluster_state=$(get_cluster_state) + if [ "$probe_cluster_state" != "ok" ]; then printf '%s' "secondary" return 0 fi fi - printf '%s' "$role" + printf '%s' "$probe_role" return 0 }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@addons/falkordb/falkordb-cluster-scripts/falkordb-cluster-role-probe.sh` around lines 30 - 34, The variables declared with the non-POSIX local keyword (role_output, role, current_pod_name, pod_ordinal, cluster_state) should be converted to POSIX-compatible function-scoped names by removing local and renaming them with a probe_ prefix (e.g., probe_role_output, probe_role, probe_current_pod_name, probe_pod_ordinal, probe_cluster_state) throughout the function where they are set and referenced so the script remains POSIX-compliant; update all occurrences of the original symbols to the new probe_* names in the same function to avoid scope/name conflicts.
16-17:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winReplace
localwith POSIX-compatible variable declarations.This script uses
#!/bin/shand the PR objectives state "POSIX sh migration," but thelocalkeyword is not POSIX-compliant and will fail on strict implementations like dash. This issue was previously flagged but remains unresolved (also affects lines 30-34).Remove
localand use function-scoped naming (e.g.,probe_service_port,probe_cluster_info) to avoid global namespace collisions, or switch to#!/bin/bashif bash is guaranteed.🔧 Proposed POSIX-compliant fix
get_cluster_state() { - local service_port - local cluster_info - service_port="${KB_SERVICE_PORT:-${SERVICE_PORT:-6379}}" + probe_service_port="${KB_SERVICE_PORT:-${SERVICE_PORT:-6379}}" if [ -n "${REDIS_DEFAULT_PASSWORD:-}" ]; then - cluster_info=$(redis-cli ${REDIS_CLI_TLS_CMD:-} -h localhost -p "$service_port" -a "$REDIS_DEFAULT_PASSWORD" cluster info 2>/dev/null || true) + probe_cluster_info=$(redis-cli ${REDIS_CLI_TLS_CMD:-} -h localhost -p "$probe_service_port" -a "$REDIS_DEFAULT_PASSWORD" cluster info 2>/dev/null || true) else - cluster_info=$(redis-cli ${REDIS_CLI_TLS_CMD:-} -h localhost -p "$service_port" cluster info 2>/dev/null || true) + probe_cluster_info=$(redis-cli ${REDIS_CLI_TLS_CMD:-} -h localhost -p "$probe_service_port" cluster info 2>/dev/null || true) fi - printf '%s\n' "$cluster_info" | awk -F: '/^cluster_state:/{print $2; exit}' | tr -d '[:space:]' + printf '%s\n' "$probe_cluster_info" | awk -F: '/^cluster_state:/{print $2; exit}' | tr -d '[:space:]' }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@addons/falkordb/falkordb-cluster-scripts/falkordb-cluster-role-probe.sh` around lines 16 - 17, This script uses the non-POSIX local keyword (e.g., "local service_port" and "local cluster_info") under #!/bin/sh; remove the local declarations and make the variables function-scoped by renaming them to unique names (for example probe_service_port and probe_cluster_info) wherever they are set and read inside the probe functions (also update the other occurrences around lines 30-34), or if you can guarantee bash on target systems change the shebang to #!/bin/bash and keep local. Ensure all references to service_port and cluster_info are updated to the new names to avoid global collisions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@addons/falkordb/falkordb-cluster-scripts/falkordb-cluster-role-probe.sh`:
- Around line 46-48: Check and validate CURRENT_POD_NAME before extracting the
ordinal: ensure current_pod_name is non-empty and contains a hyphen, then
extract pod_ordinal and verify it is a numeric value (e.g., match against
digits); if validation fails, set pod_ordinal to a clear invalid sentinel (or
empty) and log an error/warning so the subsequent degraded-cluster logic (which
uses pod_ordinal) does not mis-fire. Update the code around variables
current_pod_name, pod_ordinal and the degraded-cluster check to use this
validated value and bail or treat as non-leader when invalid.
---
Duplicate comments:
In `@addons/falkordb/falkordb-cluster-scripts/falkordb-cluster-role-probe.sh`:
- Around line 30-34: The variables declared with the non-POSIX local keyword
(role_output, role, current_pod_name, pod_ordinal, cluster_state) should be
converted to POSIX-compatible function-scoped names by removing local and
renaming them with a probe_ prefix (e.g., probe_role_output, probe_role,
probe_current_pod_name, probe_pod_ordinal, probe_cluster_state) throughout the
function where they are set and referenced so the script remains
POSIX-compliant; update all occurrences of the original symbols to the new
probe_* names in the same function to avoid scope/name conflicts.
- Around line 16-17: This script uses the non-POSIX local keyword (e.g., "local
service_port" and "local cluster_info") under #!/bin/sh; remove the local
declarations and make the variables function-scoped by renaming them to unique
names (for example probe_service_port and probe_cluster_info) wherever they are
set and read inside the probe functions (also update the other occurrences
around lines 30-34), or if you can guarantee bash on target systems change the
shebang to #!/bin/bash and keep local. Ensure all references to service_port and
cluster_info are updated to the new names to avoid global collisions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 573d89bf-bb6c-4a93-8daa-543cb66820b9
📒 Files selected for processing (1)
addons/falkordb/falkordb-cluster-scripts/falkordb-cluster-role-probe.sh
Comment on lines
+46
to
+48
| current_pod_name="${CURRENT_POD_NAME:-}" | ||
| pod_ordinal="${current_pod_name##*-}" | ||
|
|
There was a problem hiding this comment.
Validate pod ordinal extraction.
If CURRENT_POD_NAME is empty or malformed (e.g., doesn't contain a hyphen), pod_ordinal could be empty or invalid, causing the degraded-cluster logic at line 53 to trigger incorrectly.
🛡️ Proposed validation
current_pod_name="${CURRENT_POD_NAME:-}"
- pod_ordinal="${current_pod_name##*-}"
+ case "$current_pod_name" in
+ *-[0-9]*)
+ pod_ordinal="${current_pod_name##*-}"
+ ;;
+ *)
+ # If pod name is malformed, assume ordinal 0 to skip degraded-cluster check
+ pod_ordinal="0"
+ ;;
+ esac📝 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.
Suggested change
| current_pod_name="${CURRENT_POD_NAME:-}" | |
| pod_ordinal="${current_pod_name##*-}" | |
| current_pod_name="${CURRENT_POD_NAME:-}" | |
| case "$current_pod_name" in | |
| *-[0-9]*) | |
| pod_ordinal="${current_pod_name##*-}" | |
| ;; | |
| *) | |
| # If pod name is malformed, assume ordinal 0 to skip degraded-cluster check | |
| pod_ordinal="0" | |
| ;; | |
| esac | |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@addons/falkordb/falkordb-cluster-scripts/falkordb-cluster-role-probe.sh`
around lines 46 - 48, Check and validate CURRENT_POD_NAME before extracting the
ordinal: ensure current_pod_name is non-empty and contains a hyphen, then
extract pod_ordinal and verify it is a numeric value (e.g., match against
digits); if validation fails, set pod_ordinal to a clear invalid sentinel (or
empty) and log an error/warning so the subsequent degraded-cluster logic (which
uses pod_ordinal) does not mis-fire. Update the code around variables
current_pod_name, pod_ordinal and the degraded-cluster check to use this
validated value and bail or treat as non-leader when invalid.
Co-Authored-By: Oz <oz-agent@warp.dev>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
addons/falkordb/DEPLOY.md (1)
4-4:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUpdate all chart version references from 1.0.2 to 1.0.3.
The documentation still references chart version
1.0.2throughout (in text descriptions, package commands, file paths, and examples), but the PR objectives state this release bumps the chart to version1.0.3. This inconsistency will cause users to reference incorrect chart filenames and version numbers when following the deployment guide.📝 Suggested fix: Update all version references
Apply a find-and-replace across the file:
1.0.2→1.0.3falkordb-1.0.2.tgz→falkordb-1.0.3.tgzThis affects at minimum:
- Line 4: Chart version in introduction
- Line 8: Section heading
- Line 50: Package output comment
- Line 61: helm template command
- Line 76: helm lint reference
- Line 91: helm install command
- Line 99: tar verification command
- Line 114: helm install example
- Line 122: helm upgrade example
Also applies to: 8-8, 50-50, 61-61, 76-76, 91-91, 99-99, 114-114, 122-122
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@addons/falkordb/DEPLOY.md` at line 4, Replace all occurrences of the chart version string "1.0.2" with "1.0.3" and update corresponding archive names "falkordb-1.0.2.tgz" to "falkordb-1.0.3.tgz" across the document; specifically update textual references and example commands that mention the version (e.g., helm template, helm lint, helm install, helm upgrade, tar verification lines) so examples and package filenames match the bumped chart version.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@addons/falkordb/DEPLOY.md`:
- Line 4: Replace all occurrences of the chart version string "1.0.2" with
"1.0.3" and update corresponding archive names "falkordb-1.0.2.tgz" to
"falkordb-1.0.3.tgz" across the document; specifically update textual references
and example commands that mention the version (e.g., helm template, helm lint,
helm install, helm upgrade, tar verification lines) so examples and package
filenames match the bumped chart version.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 723007d0-dceb-45cb-8d17-0566683b0af8
⛔ Files ignored due to path filters (1)
dist/falkordb-1.0.3.tgzis excluded by!**/dist/**
📒 Files selected for processing (4)
README.mdaddons/falkordb/Chart.yamladdons/falkordb/DEPLOY.mdaddons/falkordb/values.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- addons/falkordb/Chart.yaml
Co-Authored-By: Oz <oz-agent@warp.dev>
Co-Authored-By: Oz <oz-agent@warp.dev>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Validation
Co-Authored-By: Oz oz-agent@warp.dev
Summary by CodeRabbit
Chores
New Features
Bug Fixes / Reliability
Tests
Documentation