Skip to content

Fix coverage threshold bug and harden benchmark scripts configurability and security#90

Draft
Copilot wants to merge 2 commits into
mainfrom
copilot/fix-coverage-calculation-issue
Draft

Fix coverage threshold bug and harden benchmark scripts configurability and security#90
Copilot wants to merge 2 commits into
mainfrom
copilot/fix-coverage-calculation-issue

Conversation

Copy link
Copy Markdown

Copilot AI commented Apr 13, 2026

Several bugs and hardcoded values in the pre-commit hook and benchmark scripts reduced reliability and flexibility. Coverage comparison was broken due to a scaling mismatch, SSH connections were vulnerable to MITM, and key operational parameters were not configurable.

.githooks/pre-commit

  • Fix coverage check: c * 100 vs threshold 5000 was inconsistent — simplify to printf "%d", c and compare against 50

benchmarks/scripts/system_utils.py

  • Replace AutoAddPolicy() with load_system_host_keys() + RejectPolicy() to prevent MITM attacks
  • Replace Ed25519Key with PKey.from_path() to support RSA, ECDSA, and Ed25519 keys generically
  • Make connection timeout a parameter (default 30)

benchmarks/scripts/mysql_utils.py

  • Replace print with logging.getLogger(__name__) for proper log level control

benchmarks/scripts/cache_cleaning_workers.py

  • Extract build_cache_clean_command() to deduplicate disk/memory cache command construction
  • Add --container-name-pattern (default ^presto_workers) to make Docker filter configurable
  • Add --sshuser (default centos), --worker-http-port (default 8080) as CLI args
  • Add --cleanup-disk-cache, --cleanup-os-cache, --cleanup-memory-cache flags; when none specified, all three run (preserving existing behavior)
  • Exit early with clear error message when DB returns no cluster details or no worker IPs
# Before: silent empty run if no results
worker_public_ips = []
if results:
    worker_public_ips = get_workers_public_ips(results[0][0])

# After: fail fast with actionable message
if not results:
    print(f"No cluster details found for cluster '{cluster_name}'. Exiting without cleanup.")
    sys.exit(1)
Original prompt
Please apply the following diffs and create a pull request.
Once the PR is ready, give it a title based on the messages of the fixes being applied.

[{"message":"The coverage calculation multiplies by 100 but compares against 5000 (representing 50%). This is inconsistent with typical coverage values which are already percentages. If `COVERAGE` is '75.5', this calculates 7550, but the threshold check expects values like 5000 for 50%. The multiplication should be by 1 (or removed) if comparing against a threshold of 50, or the threshold should be 50 if kept as-is.","fixFiles":[{"filePath":".githooks/pre-commit","diff":"diff --git a/.githooks/pre-commit b/.githooks/pre-commit\n--- a/.githooks/pre-commit\n+++ b/.githooks/pre-commit\n@@ -39,8 +39,8 @@\n echo \"Checking coverage...\"\n COVERAGE=$(go tool cover -func=coverage.out 2>/dev/null | grep total | awk '{print $3}' | sed 's/%//')\n if [ -n \"$COVERAGE\" ]; then\n-  COVERAGE_INT=$(awk -v c=\"$COVERAGE\" 'BEGIN { printf \"%d\", c * 100 }')\n-  if [ \"$COVERAGE_INT\" -lt 5000 ]; then\n+  COVERAGE_INT=$(awk -v c=\"$COVERAGE\" 'BEGIN { printf \"%d\", c }')\n+  if [ \"$COVERAGE_INT\" -lt 50 ]; then\n     echo \"Coverage ${COVERAGE}% is below 50% threshold\"\n     FAILED=1\n   else\n"}]},{"message":"The threshold comparison uses 5000, which would represent 50% only if the coverage value is multiplied by 100. However, this creates confusion as standard coverage percentages don't require this scaling. The threshold should be 50 if `COVERAGE_INT` represents a whole number percentage, or the calculation in line 42 should be adjusted.","fixFiles":[{"filePath":".githooks/pre-commit","diff":"diff --git a/.githooks/pre-commit b/.githooks/pre-commit\n--- a/.githooks/pre-commit\n+++ b/.githooks/pre-commit\n@@ -39,8 +39,8 @@\n echo \"Checking coverage...\"\n COVERAGE=$(go tool cover -func=coverage.out 2>/dev/null | grep total | awk '{print $3}' | sed 's/%//')\n if [ -n \"$COVERAGE\" ]; then\n-  COVERAGE_INT=$(awk -v c=\"$COVERAGE\" 'BEGIN { printf \"%d\", c * 100 }')\n-  if [ \"$COVERAGE_INT\" -lt 5000 ]; then\n+  COVERAGE_INT=$(awk -v c=\"$COVERAGE\" 'BEGIN { printf \"%d\", c }')\n+  if [ \"$COVERAGE_INT\" -lt 50 ]; then\n     echo \"Coverage ${COVERAGE}% is below 50% threshold\"\n     FAILED=1\n   else\n"}]},{"message":"The docker filter pattern 'name=^presto_workers' may not match the intended container if the actual container name differs. This could result in the command failing silently or targeting the wrong container. Verify the exact container naming convention or make it configurable.","fixFiles":[{"filePath":"benchmarks/scripts/cache_cleaning_workers.py","diff":"diff --git a/benchmarks/scripts/cache_cleaning_workers.py b/benchmarks/scripts/cache_cleaning_workers.py\n--- a/benchmarks/scripts/cache_cleaning_workers.py\n+++ b/benchmarks/scripts/cache_cleaning_workers.py\n@@ -13,8 +13,14 @@\n             worker_ips.append(value)\n     return worker_ips\n \n-def cleanup_worker_disk_cache(worker_public_ips, login_user, ssh_key_path, worker_http_port):\n-    ssd_cache_clean_command = f\"docker exec $(docker ps -q --filter 'name=^presto_workers') curl -sS -X GET http://localhost:{worker_http_port}/v1/operation/server/clearCache?type=ssd\"\n+def build_cache_clean_command(worker_http_port, cache_type, container_name_pattern):\n+    return (\n+        f\"docker exec $(docker ps -q --filter 'name={container_name_pattern}') \"\n+        f\"curl -sS -X GET http://localhost:{worker_http_port}/v1/operation/server/clearCache?type={cache_type}\"\n+    )\n+\n+def cleanup_worker_disk_cache(worker_public_ips, login_user, ssh_key_path, worker_http_port, container_name_pattern):\n+    ssd_cache_clean_command = build_cache_clean_command(worker_http_port, \"ssd\", container_name_pattern)\n     for worker_ip in worker_public_ips:\n         execute_ssh_command(worker_ip, login_user, ssh_key_path, ssd_cache_clean_command)\n     print(\"cleanup_worker_disk_cache is successful!\")\n@@ -26,8 +32,8 @@\n             execute_ssh_command(worker_ip, login_user, ssh_key_path, command)\n     print(\"cleanup_worker_os_cache is successful!\")\n \n-def cleanup_worker_memory_cache(worker_public_ips, login_user, ssh_key_path, worker_http_port):\n-    memory_cache_clean_command = f\"docker exec $(docker ps -q --filter 'name=^presto_workers') curl -sS -X GET http://localhost:{worker_http_port}/v1/operation/server/clearCache?type=memory\"\n+def cleanup_worker_memory_cache(worker_public_ips, login_user, ssh_key_path, worker_http_port, container_name_pattern):\n+    memory_cache_clean_command = build_cache_clean_command(worker_http_port, \"memory\", container_name_pattern)\n     for worker_ip in worker_public_ips:\n         execute_ssh_command(worker_ip, login_user, ssh_key_path, memory_cache_clean_command)\n     print(\"cleanup_worker_memory_cache is successful!\")\n@@ -38,6 +44,7 @@\n     parser.add_argument('--mysql', required=True, help='Mysql database details')\n     parser.add_argument('-...

…ging module, and harden SSH in benchmark scripts

Agent-Logs-Url: https://github.com/prestodb/pbench/sessions/c90caf7f-fdb0-47b9-ae4d-30eac8b06fb4

Co-authored-by: ethanyzhang <6643318+ethanyzhang@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix inconsistent coverage calculation in pre-commit hook Fix coverage threshold bug and harden benchmark scripts configurability and security Apr 13, 2026
Copilot AI requested a review from ethanyzhang April 13, 2026 19:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants