Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 22 additions & 1 deletion build.sh
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ Usage: build.sh [options...]
--configure Run cmake stage (aka configure stage).
--verbose | -v Run verbose build.
--debug Build for debug version.
--debug-on-crash Enable interactive gdb debugging on a valkey-server crash for oss integration tests (single test mode only).
--clean Clean the current build configuration (debug or release).
--format Applies clang-format. (Run in dev container environment to ensure correct clang-format version)
--run-tests Run all tests. Optionally, pass a test name to run: "--run-tests=<test-name>".
Expand Down Expand Up @@ -151,6 +152,13 @@ while [ $# -gt 0 ]; do
print_usage
exit 0
;;
--debug-on-crash)
export DEBUG_ON_CRASH="yes"
BUILD_CONFIG="debug" # Force debug build for better debugging
shift || true
echo "Debug-on-crash enabled - will drop into gdb on valkey-server crash"
echo "Automatically enabling debug build for better debugging symbols"
;;
*)
echo "Unknown argument: ${arg}"
print_usage
Expand Down Expand Up @@ -396,8 +404,21 @@ elif [[ "${INTEGRATION_TEST}" == "yes" ]]; then
fi
export TEST_PATTERN=${TEST_PATTERN}
export INTEG_RETRIES=${INTEG_RETRIES}
params=""
if [[ "${BUILD_CONFIG}" == "debug" ]]; then
params="${params} --debug"
fi
if [[ "${SAN_BUILD}" == "address" ]]; then
params="${params} --asan"
fi
if [[ "${SAN_BUILD}" == "thread" ]]; then
params="${params} --tsan"
fi
if [[ "${DEBUG_ON_CRASH}" == "yes" ]]; then
params="${params} --debug-on-crash"
fi
# Run will run ASan or normal tests based on the environment variable SAN_BUILD
./run.sh
./run.sh ${params}
popd >/dev/null
fi

Expand Down
5 changes: 5 additions & 0 deletions integration/run.sh
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,11 @@ while [ $# -gt 0 ]; do
print_usage
exit 0
;;
--debug-on-crash)
shift || true
export DEBUG_ON_CRASH="yes"
LOG_INFO "Debug-on-crash enabled"
;;
*)
print_usage
exit 1
Expand Down
37 changes: 37 additions & 0 deletions integration/valkey_search_test_case.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,44 @@
import string
import logging
import shutil
import subprocess

LOGS_DIR = "/tmp/valkey-test-framework-files"

if "LOGS_DIR" in os.environ:
LOGS_DIR = os.environ["LOGS_DIR"]


class DebugValkeyServerHandle(ValkeyServerHandle):
"""ValkeyServerHandle that wraps valkey-server with gdb for crash debugging"""

def start(self, wait_for_ping=True, connect_client=True):
if self.server:
raise RuntimeError("Server already started")

# Build server command
server_args = [self.valkey_path]
if self.conf_file:
server_args.append(self.conf_file)
for k, v in self.args.items():
server_args.extend([f"--{k.replace('_', '-')}", str(v)])

# Wrap with gdb if debug-on-crash enabled
if os.environ.get("DEBUG_ON_CRASH") == "yes":
cmd = ["gdb", "--ex", "run", "--args"] + server_args
else:
cmd = server_args

self.server = subprocess.Popen(cmd, cwd=self.cwd)
# Standard connection handling
if connect_client:
self.wait_for_ready_to_accept_connections()
Comment on lines +25 to +48
Copy link
Member

Choose a reason for hiding this comment

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

This duplicates the start code in the testing infrastructure, right? Is that necessary seems like it actually makes it more complicated.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're right about the duplication. We're overriding the start() method from ValkeyServerHandle to wrap the server command with gdb when the debug-on-crash flag is enabled.
There is some duplication but I felt it was simple approach for this use case to automatically start gdb when there is crash. I also noticed in your other PRs you mentioned about saving coredumps for test failures. If this debug-on-crash feature is redundant with your commit, I'm happy to remove it.
Let me know your preference

Copy link
Member

Choose a reason for hiding this comment

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

What I don't like about overriding the start method is that essentially your duplicating it. Over time, the start method is going to accumulate many more parameters, etc. Most likely, the people that make those changes won't even know that there's a duplicate of start that would also need the same changes, leading to the situation where the debug option doesn't work the same as the non-debug option. I recently added the ability to run under valgrind, and faced the same problem but modified the underlying start routine. Is there a reason that this couldn't do the same? See https://github.com/valkey-io/valkey-test-framework/blob/f42f4aa754fd0e26c8b8e52e2a7abd698978856f/src/valkey_test_case.py#L228

if wait_for_ping:
self.connect()

return self.client


class Node:
"""This class represents a valkey server instance, regardless of its role"""

Expand Down Expand Up @@ -148,6 +179,12 @@ def get_config_file_lines(self, testdir, port) -> List[str]:
for example usage."""
raise NotImplementedError

def get_valkey_handle(self):
"""Return debug-enabled valkey handle when debug-on-crash is enabled"""
if os.environ.get("DEBUG_ON_CRASH") == "yes":
return DebugValkeyServerHandle
return ValkeyServerHandle

def start_server(
self,
port: int,
Expand Down
Loading