Skip to content
Merged
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
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ echo '
"alert_recipients": "'$ACTOR_EMAIL'"
},
"labels": {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

View replay

🆕 New Failure: identified in this diff run

Medium severity Pull-request benchmark cannot start

What failed: The benchmark job data is not valid JSON, so the pull-request benchmark cannot start.

Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
  • Severity: Medium Medium severity
  • Impact: Pull-request performance benchmarks cannot start because the generated job is invalid, so maintainers cannot get benchmark results for the change.
  • Steps to Reproduce:
    1. Prepare a benchmark-latency payload for Doltgres to Doltgres with pullRequest mode and a sysbench duration of 20.
    2. Run .github/scripts/performance-benchmarking/get-doltgres-doltgres-job-json.sh with valid benchmark metadata and capture its output.
    3. Run jq against the complete rendered Job document.
    4. Observe that jq reports an expected separator error at the label object and exits with status 5.
  • Stub / mock content: The run used local-only repository-dispatch and Kubernetes submission interception with strict JSON validation, so no external cluster or live service was contacted. The benchmark template itself was rendered directly with representative pull-request values.
  • Code Analysis: The PR adds the first property in the pod-template label object at .github/scripts/performance-benchmarking/get-doltgres-doltgres-job-json.sh:66, but it does not add the comma required before the existing property at line 67. The shell template therefore emits the adjacent text "app": "performance-benchmarking" followed immediately by "k8s-liquidata-inc-monitored-job": "created-by-static-config", which is invalid JSON. The template command itself exits successfully because it only prints text; the downstream submission path in run-benchmarks.sh:72-88 writes that text to job.json and passes it to kubectl, so the malformed document prevents Job creation. The smallest practical fix is to add a comma after the app label at line 66, then retain the strict JSON parse check in the test coverage.
  • Why this is likely a bug: A valid Kubernetes Job must be represented by valid JSON before kubectl can apply it. The direct local render with valid Doltgres pull-request inputs exited 0 but jq reported Expected separator between values at line 18 column 43, matching the missing comma in the production template. Because the new label is required by the intended benchmark metadata while the existing monitored-job label must remain, both properties need to be comma-separated; adding that comma is a targeted fix for the observed failure.
Relevant code

.github/scripts/performance-benchmarking/get-doltgres-doltgres-job-json.sh:65-68

        "labels": {
          "app": "performance-benchmarking"
          "k8s-liquidata-inc-monitored-job": "created-by-static-config"
        }

.github/scripts/performance-benchmarking/run-benchmarks.sh:72-90

source "$TEMPLATE_SCRIPT" ... > job.json

out=$(KUBECONFIG="$KUBECONFIG" kubectl apply -f job.json || true)
Evidence Package
Copy prompt for an agent
Ito QA identified the following failure during automated PR testing. Please investigate and propose a fix.

**Medium severity — Pull-request benchmark cannot start**

**What failed:** The benchmark job data is not valid JSON, so the pull-request benchmark cannot start.

- **Impact:** Pull-request performance benchmarks cannot start because the generated job is invalid, so maintainers cannot get benchmark results for the change.
- **Steps to reproduce:**
  1. Prepare a benchmark-latency payload for Doltgres to Doltgres with pullRequest mode and a sysbench duration of 20.
  2. Run .github/scripts/performance-benchmarking/get-doltgres-doltgres-job-json.sh with valid benchmark metadata and capture its output.
  3. Run jq against the complete rendered Job document.
  4. Observe that jq reports an expected separator error at the label object and exits with status 5.
- **Stub / mock content:** The run used local-only repository-dispatch and Kubernetes submission interception with strict JSON validation, so no external cluster or live service was contacted. The benchmark template itself was rendered directly with representative pull-request values.
- **Code analysis:** The PR adds the first property in the pod-template label object at .github/scripts/performance-benchmarking/get-doltgres-doltgres-job-json.sh:66, but it does not add the comma required before the existing property at line 67. The shell template therefore emits the adjacent text `"app": "performance-benchmarking"` followed immediately by `"k8s-liquidata-inc-monitored-job": "created-by-static-config"`, which is invalid JSON. The template command itself exits successfully because it only prints text; the downstream submission path in run-benchmarks.sh:72-88 writes that text to job.json and passes it to kubectl, so the malformed document prevents Job creation. The smallest practical fix is to add a comma after the app label at line 66, then retain the strict JSON parse check in the test coverage.
- **Why this is likely a bug:** A valid Kubernetes Job must be represented by valid JSON before kubectl can apply it. The direct local render with valid Doltgres pull-request inputs exited 0 but jq reported `Expected separator between values at line 18 column 43`, matching the missing comma in the production template. Because the new label is required by the intended benchmark metadata while the existing monitored-job label must remain, both properties need to be comma-separated; adding that comma is a targeted fix for the observed failure.

**Relevant code:**

`.github/scripts/performance-benchmarking/get-doltgres-doltgres-job-json.sh:65-68`

~~~sh
        "labels": {
          "app": "performance-benchmarking"
          "k8s-liquidata-inc-monitored-job": "created-by-static-config"
        }
~~~

`.github/scripts/performance-benchmarking/run-benchmarks.sh:72-90`

~~~sh
source "$TEMPLATE_SCRIPT" ... > job.json

out=$(KUBECONFIG="$KUBECONFIG" kubectl apply -f job.json || true)
~~~

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

View replay

🆕 New Failure: identified in this diff run

Medium severity Benchmark jobs reject valid identity data

What failed: The benchmark request is rejected before a job is created, even though its revision, issue, and duration values are supplied.

Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
  • Severity: Medium Medium severity
  • Impact: Pull-request benchmark jobs are rejected before creation, so teams cannot get benchmark results or matching report metadata for this path.
  • Steps to Reproduce:
    1. Run the Doltgres benchmark wrapper in pull-request mode with distinct from and to revisions, issue number 3088, and sysbench duration 37.
    2. Inspect the generated Job document and confirm that the requested revision, issue, and duration arguments are present in the raw output.
    3. Parse the complete generated document with jq or another strict JSON parser.
    4. Observe the parse error at the pod labels and the wrapper exit before a valid Job or result record is created.
  • Stub / mock content: The check used a local-only dispatch wrapper and Kubernetes interception to validate the rendered payload without contacting a cluster; no production services or customer data were used.
  • Code Analysis: The current template uses a single-quoted shell heredoc-style echo and interpolates the label object at lines 65-68. Line 66 is the PR-added property, "app": "performance-benchmarking", but it has no comma before line 67's existing "k8s-liquidata-inc-monitored-job" property. The resulting object contains adjacent JSON members and fails strict parsing (Expected separator between values / Expecting ',' delimiter). The template function itself exits 0 because it only prints text; the failure is detected later by the local validator or by Kubernetes when run-benchmarks.sh submits job.json. The smallest fix is to add the missing comma after the app property, then retain the existing wrapper validation and submission flow.
  • Why this is likely a bug: This is a deterministic syntax defect in production benchmark-dispatch code, not only a failed assertion. The same template output fails independently under strict JSON parsing for a distinctive request, while the PostgreSQL-to-Doltgres template remains parseable in the comparable release check. Because run-benchmarks.sh passes the rendered document directly to kubectl, the missing separator makes the Doltgres pull-request benchmark path unusable until the comma is restored. The PR diff directly added the property that introduced the invalid syntax, so the minimal remediation is to add that separator or revert the added label line.
Relevant code

.github/scripts/performance-benchmarking/get-doltgres-doltgres-job-json.sh:65-68

        "labels": {
          "app": "performance-benchmarking"
          "k8s-liquidata-inc-monitored-job": "created-by-static-config"
        }

.github/scripts/performance-benchmarking/run-benchmarks.sh:72-93

source "$TEMPLATE_SCRIPT" ... > job.json
out=$(KUBECONFIG="$KUBECONFIG" kubectl apply -f job.json || true)
if [ "$out" != "job.batch/$jobname created" ]; then
  exit 1
fi
Evidence Package
Copy prompt for an agent
Ito QA identified the following failure during automated PR testing. Please investigate and propose a fix.

**Medium severity — Benchmark jobs reject valid identity data**

**What failed:** The benchmark request is rejected before a job is created, even though its revision, issue, and duration values are supplied.

- **Impact:** Pull-request benchmark jobs are rejected before creation, so teams cannot get benchmark results or matching report metadata for this path.
- **Steps to reproduce:**
  1. Run the Doltgres benchmark wrapper in pull-request mode with distinct from and to revisions, issue number 3088, and sysbench duration 37.
  2. Inspect the generated Job document and confirm that the requested revision, issue, and duration arguments are present in the raw output.
  3. Parse the complete generated document with jq or another strict JSON parser.
  4. Observe the parse error at the pod labels and the wrapper exit before a valid Job or result record is created.
- **Stub / mock content:** The check used a local-only dispatch wrapper and Kubernetes interception to validate the rendered payload without contacting a cluster; no production services or customer data were used.
- **Code analysis:** The current template uses a single-quoted shell heredoc-style echo and interpolates the label object at lines 65-68. Line 66 is the PR-added property, `"app": "performance-benchmarking"`, but it has no comma before line 67's existing `"k8s-liquidata-inc-monitored-job"` property. The resulting object contains adjacent JSON members and fails strict parsing (`Expected separator between values` / `Expecting ',' delimiter`). The template function itself exits 0 because it only prints text; the failure is detected later by the local validator or by Kubernetes when run-benchmarks.sh submits job.json. The smallest fix is to add the missing comma after the app property, then retain the existing wrapper validation and submission flow.
- **Why this is likely a bug:** This is a deterministic syntax defect in production benchmark-dispatch code, not only a failed assertion. The same template output fails independently under strict JSON parsing for a distinctive request, while the PostgreSQL-to-Doltgres template remains parseable in the comparable release check. Because run-benchmarks.sh passes the rendered document directly to kubectl, the missing separator makes the Doltgres pull-request benchmark path unusable until the comma is restored. The PR diff directly added the property that introduced the invalid syntax, so the minimal remediation is to add that separator or revert the added label line.

**Relevant code:**

`.github/scripts/performance-benchmarking/get-doltgres-doltgres-job-json.sh:65-68`

~~~shell
        "labels": {
          "app": "performance-benchmarking"
          "k8s-liquidata-inc-monitored-job": "created-by-static-config"
        }
~~~

`.github/scripts/performance-benchmarking/run-benchmarks.sh:72-93`

~~~shell
source "$TEMPLATE_SCRIPT" ... > job.json
out=$(KUBECONFIG="$KUBECONFIG" kubectl apply -f job.json || true)
if [ "$out" != "job.batch/$jobname created" ]; then
  exit 1
fi
~~~

"app": "performance-benchmarking"
"k8s-liquidata-inc-monitored-job": "created-by-static-config"
}
},
Expand Down
Comment thread
jycor marked this conversation as resolved.
Comment thread
jycor marked this conversation as resolved.
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@

set -e

if [ "$#" -lt 9 ]; then
echo "Usage: ./get-job-json.sh <jobname> <fromServer> <fromVersion> <toServer> <toVersion> <timeprefix> <actorprefix> <format> <issueNumber> <initBigRepo> <nomsBinFormat> <withTpcc>"
if [ "$#" -lt 10 ]; then
Comment thread
jycor marked this conversation as resolved.
echo "Usage: ./get-job-json.sh <jobname> <fromServer> <fromVersion> <toServer> <toVersion> <timeprefix> <actorprefix> <format> <issueNumber> <initBigRepo> <nomsBinFormat> <sysbenchTestTime> <withTpcc>"
exit 1
fi

Expand All @@ -18,7 +18,8 @@ format="$8"
issueNumber="$9"
initBigRepo="${10}"
nomsBinFormat="${11}"
withTpcc="${12}"
sysbenchTestTime="${12}"
Comment thread
jycor marked this conversation as resolved.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

View All Evidence

🆕 New Failure: identified in this diff run

Medium severity Invalid benchmark durations are accepted

What failed: The job template accepted abc, 0, -1, and a very large duration. It returned success and included each value as the benchmark time instead of stopping with a validation error.

Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
  • Severity: Medium Medium severity
  • Impact: Operators can submit a benchmark with an invalid or unbounded duration, which may run an unintended workload and use excessive resources instead of showing a clear validation error.
  • Steps to Reproduce:
    1. Run .github/scripts/performance-benchmarking/get-postgres-doltgres-job-json.sh with otherwise valid arguments and set the duration argument to abc.
    2. Repeat with duration values 0, -1, and 999999999999999999999999.
    3. Parse each generated Job and inspect its container arguments.
    4. Run the local submission wrapper for each generated result and record the exit status.
  • Stub / mock content: A local submission wrapper and local test values were used; no production cluster or customer data was accessed.
  • Code Analysis: At line 21, positional argument 12 is assigned directly to sysbenchTestTime. The PR-added block at lines 34-36 checks only whether the string is non-empty, then constructs the argument string --sysbench-test-time=. It does not require decimal digits, reject zero or negative values, or enforce a maximum duration. The generated argument is inserted at line 109, and run-benchmarks.sh passes SYSBENCH_TEST_TIME from the workflow environment at line 85 before applying the resulting file with kubectl at line 88. Therefore the malformed value is not merely displayed incorrectly: it can flow from dispatch/workflow input into a submitted benchmark Job. The smallest fix is to validate the value before formatting it, requiring the intended positive integer range and exiting with a clear error when the check fails; the exact upper bound should match the benchmark contract.
  • Why this is likely a bug: The test expected invalid or unsafe durations to be rejected before expensive work was scheduled, but the template returned exit code 0 for all four probes and emitted the exact invalid values as benchmark arguments. This behavior is supported by the source path: the only guard is a non-empty-string check, and the caller submits the rendered file through kubectl. A benchmark duration is a resource limit, so accepting non-numeric, non-positive, or unbounded values can cause an unintended workload rather than a harmless formatting difference. The PR diff directly added the pass-through formatting at lines 34-36, so adding bounded input validation at that boundary is a focused remediation.
Relevant code

.github/scripts/performance-benchmarking/get-postgres-doltgres-job-json.sh:21

sysbenchTestTime="${12}"

.github/scripts/performance-benchmarking/get-postgres-doltgres-job-json.sh:34-36

if [ -n "$sysbenchTestTime" ]; then
  sysbenchTestTime="\"--sysbench-test-time=$sysbenchTestTime\","
fi

.github/scripts/performance-benchmarking/get-postgres-doltgres-job-json.sh:107-110

"--results-prefix='$actorprefix'",
              '"$sysbenchTestTime"'
              '"$withTpcc"'

.github/scripts/performance-benchmarking/run-benchmarks.sh:83-88

"$NOMS_BIN_FORMAT"    \
+  "$SYSBENCH_TEST_TIME" \
+  "$WITH_TPCC" > job.json

out=$(KUBECONFIG="$KUBECONFIG" kubectl apply -f job.json || true)
Evidence Package
Copy prompt for an agent
Ito QA identified the following failure during automated PR testing. Please investigate and propose a fix.

**Medium severity — Invalid benchmark durations are accepted**

**What failed:** The job template accepted abc, 0, -1, and a very large duration. It returned success and included each value as the benchmark time instead of stopping with a validation error.

- **Impact:** Operators can submit a benchmark with an invalid or unbounded duration, which may run an unintended workload and use excessive resources instead of showing a clear validation error.
- **Steps to reproduce:**
  1. Run .github/scripts/performance-benchmarking/get-postgres-doltgres-job-json.sh with otherwise valid arguments and set the duration argument to abc.
  2. Repeat with duration values 0, -1, and 999999999999999999999999.
  3. Parse each generated Job and inspect its container arguments.
  4. Run the local submission wrapper for each generated result and record the exit status.
- **Stub / mock content:** A local submission wrapper and local test values were used; no production cluster or customer data was accessed.
- **Code analysis:** At line 21, positional argument 12 is assigned directly to sysbenchTestTime. The PR-added block at lines 34-36 checks only whether the string is non-empty, then constructs the argument string --sysbench-test-time=<value>. It does not require decimal digits, reject zero or negative values, or enforce a maximum duration. The generated argument is inserted at line 109, and run-benchmarks.sh passes SYSBENCH_TEST_TIME from the workflow environment at line 85 before applying the resulting file with kubectl at line 88. Therefore the malformed value is not merely displayed incorrectly: it can flow from dispatch/workflow input into a submitted benchmark Job. The smallest fix is to validate the value before formatting it, requiring the intended positive integer range and exiting with a clear error when the check fails; the exact upper bound should match the benchmark contract.
- **Why this is likely a bug:** The test expected invalid or unsafe durations to be rejected before expensive work was scheduled, but the template returned exit code 0 for all four probes and emitted the exact invalid values as benchmark arguments. This behavior is supported by the source path: the only guard is a non-empty-string check, and the caller submits the rendered file through kubectl. A benchmark duration is a resource limit, so accepting non-numeric, non-positive, or unbounded values can cause an unintended workload rather than a harmless formatting difference. The PR diff directly added the pass-through formatting at lines 34-36, so adding bounded input validation at that boundary is a focused remediation.

**Relevant code:**

`.github/scripts/performance-benchmarking/get-postgres-doltgres-job-json.sh:21`

~~~sh
sysbenchTestTime="${12}"
~~~

`.github/scripts/performance-benchmarking/get-postgres-doltgres-job-json.sh:34-36`

~~~sh
if [ -n "$sysbenchTestTime" ]; then
  sysbenchTestTime="\"--sysbench-test-time=$sysbenchTestTime\","
fi
~~~

`.github/scripts/performance-benchmarking/get-postgres-doltgres-job-json.sh:107-110`

~~~sh
"--results-prefix='$actorprefix'",
              '"$sysbenchTestTime"'
              '"$withTpcc"'
~~~

`.github/scripts/performance-benchmarking/run-benchmarks.sh:83-88`

~~~sh
"$NOMS_BIN_FORMAT"    \
+  "$SYSBENCH_TEST_TIME" \
+  "$WITH_TPCC" > job.json

out=$(KUBECONFIG="$KUBECONFIG" kubectl apply -f job.json || true)
~~~

withTpcc="${13}"
precision="1"
tpccRegex="tpcc%"

Expand All @@ -30,6 +31,10 @@ if [ -n "$nomsBinFormat" ]; then
nomsBinFormat="\"--noms-bin-format=$nomsBinFormat\","
fi

if [ -n "$sysbenchTestTime" ]; then
sysbenchTestTime="\"--sysbench-test-time=$sysbenchTestTime\","
fi

if [ -n "$withTpcc" ]; then
withTpcc="\"--withTpcc=$withTpcc\","
fi
Expand Down Expand Up @@ -101,6 +106,7 @@ echo '
"--region=us-west-2",
"--results-dir='$timeprefix'",
"--results-prefix='$actorprefix'",
'"$sysbenchTestTime"'
Comment thread
jycor marked this conversation as resolved.
'"$withTpcc"'
'"$initBigRepo"'
'"$nomsBinFormat"'
Expand Down
25 changes: 13 additions & 12 deletions .github/scripts/performance-benchmarking/run-benchmarks.sh
Comment thread
jycor marked this conversation as resolved.
Comment thread
jycor marked this conversation as resolved.
Original file line number Diff line number Diff line change
Expand Up @@ -70,18 +70,19 @@ fi
issuenumber=${ISSUE_NUMBER:-"-1"}

source \
Comment thread
jycor marked this conversation as resolved.
Comment thread
jycor marked this conversation as resolved.
"$TEMPLATE_SCRIPT" \
"$jobname" \
"$FROM_SERVER" \
"$FROM_VERSION" \
"$TO_SERVER" \
"$TO_VERSION" \
"$timeprefix" \
"$actorprefix" \
"$format" \
"$issuenumber" \
"$INIT_BIG_REPO" \
"$NOMS_BIN_FORMAT" \
"$TEMPLATE_SCRIPT" \
"$jobname" \
"$FROM_SERVER" \
"$FROM_VERSION" \
"$TO_SERVER" \
"$TO_VERSION" \
"$timeprefix" \
"$actorprefix" \
"$format" \
"$issuenumber" \
"$INIT_BIG_REPO" \
"$NOMS_BIN_FORMAT" \
"$SYSBENCH_TEST_TIME" \
Comment thread
jycor marked this conversation as resolved.
"$WITH_TPCC" > job.json

out=$(KUBECONFIG="$KUBECONFIG" kubectl apply -f job.json || true)
Expand Down
1 change: 1 addition & 0 deletions .github/workflows/k8s-benchmark-latency.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ jobs:
KUBECONFIG: "./kubeconfig"
INIT_BIG_REPO: ${{ github.event.client_payload.init_big_repo }}
Comment thread
jycor marked this conversation as resolved.
NOMS_BIN_FORMAT: "__DOLT__"
SYSBENCH_TEST_TIME: ${{ github.event.client_payload.sysbench_test_time }}
TEMPLATE_SCRIPT: ${{ github.event.client_payload.template_script }}

# TODO: skipping tpcc with Doltgres and Postgres, not currently working
Expand Down
10 changes: 6 additions & 4 deletions .github/workflows/performance-benchmarks-pull-report.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,11 @@ jobs:
if: ${{ needs.validate-commentor.outputs.valid == 'true' }}
outputs:
benchmark: ${{ steps.set_benchmark.outputs.benchmark }}
Comment thread
jycor marked this conversation as resolved.
sysbench-test-time: ${{ steps.set_benchmark.outputs.sysbench-test-time }}
Comment thread
jycor marked this conversation as resolved.
comment-body: ${{ steps.set_body.outputs.body }}
steps:
- name: Check for Deploy Trigger
uses: dolthub/pull-request-comment-trigger@master
uses: dolthub/pull-request-comment-trigger@v2
id: check
with:
trigger: '#benchmark'
Expand All @@ -40,14 +41,15 @@ jobs:
id: set_benchmark
run: |
echo "benchmark=true" >> $GITHUB_OUTPUT
echo "sysbench-test-time=20" >> $GITHUB_OUTPUT

performance:
runs-on: ubuntu-22.04
needs: [validate-commentor, check-comments]
if: ${{ needs.check-comments.outputs.benchmark == 'true' }}
name: Trigger Benchmark Latency K8s Workflow
steps:
- uses: dolthub/pull-request-comment-branch@v4
- uses: dolthub/pull-request-comment-branch@v3
id: comment-branch
with:
repo_token: ${{ secrets.GITHUB_TOKEN }}
Expand All @@ -57,8 +59,8 @@ jobs:
with:
github-token: ${{ secrets.GITHUB_TOKEN }}
script: core.setOutput("pull_number", JSON.stringify(context.issue.number));
- uses: peter-evans/repository-dispatch@v2.0.0
- uses: peter-evans/repository-dispatch@v3
with:
token: ${{ secrets.REPO_ACCESS_TOKEN }}
event-type: benchmark-latency
client-payload: '{"from_server": "doltgres", "from_version": "${{ github.sha }}", "to_server": "doltgres", "to_version": "${{ steps.comment-branch.outputs.head_sha }}", "mode": "pullRequest", "issue_number": "${{ steps.get_pull_number.outputs.pull_number }}", "init_big_repo": "true", "actor": "${{ github.actor }}", "template_script": "./.github/scripts/performance-benchmarking/get-doltgres-doltgres-job-json.sh"}'
client-payload: '{"from_server": "doltgres", "from_version": "${{ github.sha }}", "to_server": "doltgres", "to_version": "${{ steps.comment-branch.outputs.head_sha }}", "mode": "pullRequest", "issue_number": "${{ steps.get_pull_number.outputs.pull_number }}", "init_big_repo": "true", "actor": "${{ github.actor }}", "sysbench_test_time": "${{ needs.check-comments.outputs.sysbench-test-time }}", "template_script": "./.github/scripts/performance-benchmarking/get-doltgres-doltgres-job-json.sh"}'
Loading