[no-release-notes] update mini sysbench to use kubernetes - #3056
Merged
Conversation
Contributor
Footnotes
|
Contributor
|
The The flow we use in
Doltgres already has all three of those workflows, and its benchmark dispatch payload is already correctly Doltgres-specific: from_server: doltgres
to_server: doltgres
template_script: ./.github/scripts/performance-benchmarking/get-doltgres-doltgres-job-json.shSuggested changes for this PR:
permissions:
id-token: write
contents: read
- uses: aws-actions/configure-aws-credentials@v4
with:
role-session-name: GitHub_to_AWS_via_FederatedOIDC
role-to-assume: ${{ secrets.AWS_ROLE_TO_ASSUME_CORP }}
aws-region: us-west-2The current For parity with |
Contributor
added 2 commits
August 13, 2026 15:30
coffeegoddd
approved these changes
Aug 13, 2026
coffeegoddd
left a comment
coffeegoddd
left a comment
Contributor
There was a problem hiding this comment.
seems fine to merge and iterate on
jycor
enabled auto-merge (squash)
August 13, 2026 23:09
jycor
disabled auto-merge
August 13, 2026 23:09
jycor
enabled auto-merge (squash)
August 13, 2026 23:10
jycor
changed the title
|
Diff SummaryCoverage spans normal benchmark dispatch and job creation, failure-safe handling of missing inputs and unavailable results, report retrieval and pull-request publication, plus edge cases involving malformed targets, incomplete output, comment updates, and unsafe report content. The run also exercises asynchronous storage behavior and benchmark configuration propagation, revealing broad problems in the changed reporting and job-setup flows. Not safe to merge yet — several PR-attributable failures affect core benchmark reporting and silently alter requested benchmark settings, including a still-failing publication path that can prevent results from appearing or updating reliably. Unrelated pre-existing storage and resource-validation findings are caveats for later, but the attributable failures alone are merge-blocking. Tests run by Ito
Tests that are no longer relevantBelow are tests that previously ran and are no longer relevant:
Additional Findings DetailsThese findings are unrelated to the current changes but were observed during testing. 🟠 Untrusted requests can redirect benchmark work
Evidence Package🟡 Malformed issue numbers can target reports incorrectly
Evidence Package🟡 A benchmark result can reach the wrong pull request
Evidence Package🟡 Delayed benchmark results never get picked up
Evidence PackageTip Reply with @itoqa to send us feedback on this test run. |
![itoqa[bot]](https://avatars.githubusercontent.com/in/2622614?s=60&v=4){kind=small}
|
|
||
| const commentMarker = '<!-- go-run-output-sysbench -->'; | ||
|
|
||
| // List comments on the PR |
There was a problem hiding this comment.
🆕 New Failure: identified in this diff run
Existing benchmark comments cannot be updated
What failed: The workflow finds the marked comment, but the code immediately starts an object fragment with comment_id: comment.id and later contains an unmatched } else {. There is no valid conditional or updateComment request, so an existing marker comment cannot be updated as intended.
Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
- Severity: Medium
- Impact: When a report comment already exists, later benchmark results may fail to publish or may not update that comment. Users may miss the latest report until the workflow is fixed or the old comment is handled manually.
- Steps to Reproduce:
- Run the pull-report workflow with a valid benchmark result and a pull request that already has a comment containing the benchmark marker.
- Let the workflow list the pull request comments and find the marked comment.
- Inspect the publication step and the pull request conversation for the new result.
- Stub / mock content: No stubs, mocks, or bypasses were applied for this test in the recorded run.
- Code Analysis: In
/tmp/output-agent-workspace/repo/.github/workflows/pull-report.yaml, the PR changes the publication script at lines 53-67. Line 53 assignscommentfromcomments.find(...), but line 54 beginscomment_id: comment.idwithout anif (comment)block or anawait github.rest.issues.updateComment({call. Lines 54-58 are therefore not a valid statement, and line 59 adds} else {without a matching opening conditional. The only complete API call in this section is the create-comment call at lines 61-66, which is reached only through the malformed branch. The smallest fix is to wrap lines 54-58 inif (comment) {and add the missingawait github.rest.issues.updateComment({ issue_number, comment_id: comment.id, owner, repo, body }); retain the existing create call in theelsebranch. - Why this is likely a bug: The defect is in production workflow code, not the unavailable local simulator: the source has an orphaned object-property fragment and an unmatched else in the exact path that handles an existing marker comment. This prevents the intended update-in-place behavior from executing for normal repeat benchmark reports. The PR diff directly changed this block, so the targeted fix is to restore the missing conditional and
updateCommentcall rather than redesigning report publication.
Relevant code
.github/workflows/pull-report.yaml:45-67
const { data: comments } = await github.rest.issues.listComments({ ... });
// Check if a comment already exists
const comment = comments.find(comment => comment.body.includes(commentMarker))
comment_id: comment.id,
owner: context.repo.owner,
repo: context.repo.repo,
body: `${commentMarker}\n@${ACTOR} ${FORMAT}\n${resData}`
})
} else {
// Create a new comment
await github.rest.issues.createComment({ ... });.github/workflows/pull-report.yaml:28-41
env:
KEY: ${{ github.event.client_payload.key }}
BUCKET: ${{ github.event.client_payload.bucket }}
...
const issue_number = parseInt(ISSUE_NUMBER, 10);
const fs = require('fs').promises;
const resData = await fs.readFile(`${GITHUB_WORKSPACE}/results.log`, 'utf8');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 — Existing benchmark comments cannot be updated**
**What failed:** The workflow finds the marked comment, but the code immediately starts an object fragment with `comment_id: comment.id` and later contains an unmatched `} else {`. There is no valid conditional or `updateComment` request, so an existing marker comment cannot be updated as intended.
- **Impact:** When a report comment already exists, later benchmark results may fail to publish or may not update that comment. Users may miss the latest report until the workflow is fixed or the old comment is handled manually.
- **Steps to reproduce:**
1. Run the pull-report workflow with a valid benchmark result and a pull request that already has a comment containing the benchmark marker.
2. Let the workflow list the pull request comments and find the marked comment.
3. Inspect the publication step and the pull request conversation for the new result.
- **Stub / mock content:** No stubs, mocks, or bypasses were applied for this test in the recorded run.
- **Code analysis:** In `/tmp/output-agent-workspace/repo/.github/workflows/pull-report.yaml`, the PR changes the publication script at lines 53-67. Line 53 assigns `comment` from `comments.find(...)`, but line 54 begins `comment_id: comment.id` without an `if (comment)` block or an `await github.rest.issues.updateComment({` call. Lines 54-58 are therefore not a valid statement, and line 59 adds `} else {` without a matching opening conditional. The only complete API call in this section is the create-comment call at lines 61-66, which is reached only through the malformed branch. The smallest fix is to wrap lines 54-58 in `if (comment) {` and add the missing `await github.rest.issues.updateComment({ issue_number, comment_id: comment.id, owner, repo, body })`; retain the existing create call in the `else` branch.
- **Why this is likely a bug:** The defect is in production workflow code, not the unavailable local simulator: the source has an orphaned object-property fragment and an unmatched else in the exact path that handles an existing marker comment. This prevents the intended update-in-place behavior from executing for normal repeat benchmark reports. The PR diff directly changed this block, so the targeted fix is to restore the missing conditional and `updateComment` call rather than redesigning report publication.
**Relevant code:**
`.github/workflows/pull-report.yaml:45-67`
~~~yaml
const { data: comments } = await github.rest.issues.listComments({ ... });
// Check if a comment already exists
const comment = comments.find(comment => comment.body.includes(commentMarker))
comment_id: comment.id,
owner: context.repo.owner,
repo: context.repo.repo,
body: `${commentMarker}\n@${ACTOR} ${FORMAT}\n${resData}`
})
} else {
// Create a new comment
await github.rest.issues.createComment({ ... });
~~~
`.github/workflows/pull-report.yaml:28-41`
~~~yaml
env:
KEY: ${{ github.event.client_payload.key }}
BUCKET: ${{ github.event.client_payload.bucket }}
...
const issue_number = parseInt(ISSUE_NUMBER, 10);
const fs = require('fs').promises;
const resData = await fs.readFile(`${GITHUB_WORKSPACE}/results.log`, 'utf8');
~~~
| }); | ||
| const issue_number = parseInt(ISSUE_NUMBER, 10); | ||
|
|
||
| const fs = require('fs').promises; |
There was a problem hiding this comment.
🆕 New Failure: identified in this diff run
Untrusted report text can break review comments
What failed: The workflow could not safely and reliably publish the result text. The downloaded report is inserted directly into the reviewer-facing comment, and the branch for an existing marked comment is missing its if statement and update call, leaving an orphaned object property before an unmatched else.
Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
- Severity: Medium
- Impact: Pull request reports may fail to appear or update, and untrusted report text may trigger unwanted mentions or mislead reviewers through rendered Markdown.
- Steps to Reproduce:
- Provide a results.log file containing an @mention, Markdown or HTML comment text, and more text than the expected report size.
- Trigger the pull-report workflow with a valid pull request number and result object.
- Observe the github-script step while it searches for the report marker and builds the comment body; then inspect whether a comment is created or updated and how the result text is rendered.
- Stub / mock content: No stubs, mocks, or bypasses were applied for this test in the recorded run.
- Code Analysis: The PR changes .github/workflows/pull-report.yaml from a simple createComment call to a marker-based update-or-create flow. At lines 40-41, fs.promises.readFile loads results.log into resData without a size limit, validation, or a policy for handling mention-like or comment-like content. At lines 53-58, comments.find(...) is followed immediately by comment_id, owner, repo, body, and a closing
}); there is noif (comment)statement and nogithub.rest.issues.updateCommentcall. Line 59 then starts} else {, which has no matching conditional in the script as written. The createComment object at lines 61-66 repeats direct${resData}interpolation. The result is a source-level syntax/control-flow defect that can prevent the github-script step from executing, plus an unsafe publication path that would allow untrusted result text to affect notifications and rendered comment structure if the branch were repaired without adding bounded content handling. The smallest practical fix is to restore a validif (comment) { await github.rest.issues.updateComment({ comment_id: comment.id, owner, repo, body }) } else { await github.rest.issues.createComment({ issue_number, owner, repo, body }) }structure, and apply a narrowly scoped maximum size and safe rendering/mention policy to resData before constructing body. - Why this is likely a bug: The expected behavior requires adversarial report text to remain bounded and not control mentions, marker selection, or comment structure. The source always places the complete downloaded file into the comment body, so an @mention can notify an unintended user and Markdown or HTML comment syntax can change what reviewers see. Independently, the malformed lines 53-59 prevent the existing-marker path from being a valid JavaScript conditional and omit the required updateComment request, so repeated reports cannot reliably update the intended comment. The local simulator was unavailable, but this conclusion does not depend on that environment because the defect is visible in the production workflow source and the PR diff directly changed these lines.
Relevant code
.github/workflows/pull-report.yaml:40-43
const fs = require('fs').promises;
const resData = await fs.readFile(`${GITHUB_WORKSPACE}/results.log`, 'utf8');
const commentMarker = '<!-- go-run-output-sysbench -->';
.github/workflows/pull-report.yaml:52-67
// Check if a comment already exists
const comment = comments.find(comment => comment.body.includes(commentMarker))
comment_id: comment.id,
owner: context.repo.owner,
repo: context.repo.repo,
body: `${commentMarker}\n@${ACTOR} ${FORMAT}\n${resData}`
})
} else {
// Create a new comment
await github.rest.issues.createComment({
issue_number: issue_number,
owner: context.repo.owner,
repo: context.repo.repo,
body: `${commentMarker}\n@${ACTOR} ${FORMAT}\n${resData}`
});
}
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 — Untrusted report text can break review comments**
**What failed:** The workflow could not safely and reliably publish the result text. The downloaded report is inserted directly into the reviewer-facing comment, and the branch for an existing marked comment is missing its if statement and update call, leaving an orphaned object property before an unmatched else.
- **Impact:** Pull request reports may fail to appear or update, and untrusted report text may trigger unwanted mentions or mislead reviewers through rendered Markdown.
- **Steps to reproduce:**
1. Provide a results.log file containing an @mention, Markdown or HTML comment text, and more text than the expected report size.
2. Trigger the pull-report workflow with a valid pull request number and result object.
3. Observe the github-script step while it searches for the report marker and builds the comment body; then inspect whether a comment is created or updated and how the result text is rendered.
- **Stub / mock content:** No stubs, mocks, or bypasses were applied for this test in the recorded run.
- **Code analysis:** The PR changes .github/workflows/pull-report.yaml from a simple createComment call to a marker-based update-or-create flow. At lines 40-41, fs.promises.readFile loads results.log into resData without a size limit, validation, or a policy for handling mention-like or comment-like content. At lines 53-58, comments.find(...) is followed immediately by comment_id, owner, repo, body, and a closing `})`; there is no `if (comment)` statement and no `github.rest.issues.updateComment` call. Line 59 then starts `} else {`, which has no matching conditional in the script as written. The createComment object at lines 61-66 repeats direct `${resData}` interpolation. The result is a source-level syntax/control-flow defect that can prevent the github-script step from executing, plus an unsafe publication path that would allow untrusted result text to affect notifications and rendered comment structure if the branch were repaired without adding bounded content handling. The smallest practical fix is to restore a valid `if (comment) { await github.rest.issues.updateComment({ comment_id: comment.id, owner, repo, body }) } else { await github.rest.issues.createComment({ issue_number, owner, repo, body }) }` structure, and apply a narrowly scoped maximum size and safe rendering/mention policy to resData before constructing body.
- **Why this is likely a bug:** The expected behavior requires adversarial report text to remain bounded and not control mentions, marker selection, or comment structure. The source always places the complete downloaded file into the comment body, so an @mention can notify an unintended user and Markdown or HTML comment syntax can change what reviewers see. Independently, the malformed lines 53-59 prevent the existing-marker path from being a valid JavaScript conditional and omit the required updateComment request, so repeated reports cannot reliably update the intended comment. The local simulator was unavailable, but this conclusion does not depend on that environment because the defect is visible in the production workflow source and the PR diff directly changed these lines.
**Relevant code:**
`.github/workflows/pull-report.yaml:40-43`
~~~yaml/javascript
const fs = require('fs').promises;
const resData = await fs.readFile(`${GITHUB_WORKSPACE}/results.log`, 'utf8');
const commentMarker = '<!-- go-run-output-sysbench -->';
~~~
`.github/workflows/pull-report.yaml:52-67`
~~~yaml/javascript
// Check if a comment already exists
const comment = comments.find(comment => comment.body.includes(commentMarker))
comment_id: comment.id,
owner: context.repo.owner,
repo: context.repo.repo,
body: `${commentMarker}\n@${ACTOR} ${FORMAT}\n${resData}`
})
} else {
// Create a new comment
await github.rest.issues.createComment({
issue_number: issue_number,
owner: context.repo.owner,
repo: context.repo.repo,
body: `${commentMarker}\n@${ACTOR} ${FORMAT}\n${resData}`
});
}
~~~
| // The latest commit on the PR branch. | ||
| const toSha = context.payload.pull_request.head.sha; | ||
|
|
||
| const payload = { |
There was a problem hiding this comment.
🆕 New Failure: identified in this diff run
Benchmark duration is lost before the job starts
What failed: The requested benchmark duration reaches the first dispatch payload but is dropped before the Kubernetes job is rendered. The generated job preserves the pull request revisions and other metadata, but it does not contain --sysbench-test-time=20.
Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
- Severity: Medium
- Impact: Benchmark runs can use a different duration than the one requested. This can make performance results unreliable and lead maintainers to make decisions from mismatched data.
- Steps to Reproduce:
- Open or synchronize a pull request with different known base and head commit SHAs.
- Run the automatic benchmark workflow and inspect its repository-dispatch payload; confirm it includes sysbench_test_time=20.
- Follow the payload into the Kubernetes benchmark workflow and render the job using run-benchmarks.sh.
- Inspect the rendered container arguments and confirm that --sysbench-test-time=20 is absent and the TPCC position is being used as the duration position.
- Stub / mock content: No stubs, mocks, or bypasses were applied for this test in the recorded run.
- Code Analysis: The new payload in .github/workflows/sysbench-performance.yaml lines 30-40 sets sysbench_test_time to "20". However, .github/workflows/k8s-benchmark-latency.yaml lines 41-56 exports the other client_payload fields to run-benchmarks.sh and has no SYSBENCH_TEST_TIME entry. Even if that environment mapping were added, run-benchmarks.sh lines 72-85 still calls the template with init-big-repo at argument 10, NOMS_BIN_FORMAT at argument 11, and WITH_TPCC at argument 12; it supplies no duration argument. The changed template in get-doltgres-doltgres-job-json.sh lines 5-22 now treats argument 12 as sysbenchTestTime and argument 13 as withTpcc, and lines 33-39 only emit --sysbench-test-time when argument 12 is non-empty. Consequently, the existing caller's TPCC value is interpreted as duration, while the requested 20-second value is never passed. The smallest practical fix is to map github.event.client_payload.sysbench_test_time to SYSBENCH_TEST_TIME in k8s-benchmark-latency.yaml and pass "$SYSBENCH_TEST_TIME" before "$WITH_TPCC" in run-benchmarks.sh, keeping the template's new positional contract consistent.
- Why this is likely a bug: The test's rendered job contract independently confirms that the duration requested by the pull-request workflow is absent, and source inspection explains the exact positional shift that causes it. This is not a test-only setup issue: the production GitHub Actions and shell files contain the missing handoff. The benchmark can therefore report results for a different duration than the workflow explicitly requested. The direct fix is limited to forwarding the existing payload field and restoring the intended duration-before-TPCC argument order.
Relevant code
.github/workflows/sysbench-performance.yaml:30-40
const payload = {
from_server: "doltgres",
from_version: fromSha,
to_server: "doltgres",
to_version: toSha,
mode: "pullRequest",
issue_number: String(context.issue.number),
init_big_repo: "true",
actor: context.actor,
sysbench_test_time: "20",
template_script: "./.github/scripts/performance-benchmarking/get-doltgres-doltgres-job-json.sh",
};.github/workflows/k8s-benchmark-latency.yaml:41-56
- name: Create Sysbench Performance Benchmarking K8s Job
run: ./.github/scripts/performance-benchmarking/run-benchmarks.sh
env:
FROM_SERVER: ${{ github.event.client_payload.from_server }}
FROM_VERSION: ${{ github.event.client_payload.from_version }}
TO_SERVER: ${{ github.event.client_payload.to_server }}
TO_VERSION: ${{ github.event.client_payload.to_version }}
MODE: ${{ github.event.client_payload.mode }}
ISSUE_NUMBER: ${{ github.event.client_payload.issue_number }}
ACTOR: ${{ github.event.client_payload.actor }}
TEMPLATE_SCRIPT: ${{ github.event.client_payload.template_script }}.github/scripts/performance-benchmarking/run-benchmarks.sh:72-85
source \
"$TEMPLATE_SCRIPT" \
"$jobname" \
"$FROM_SERVER" \
"$FROM_VERSION" \
"$TO_SERVER" \
"$TO_VERSION" \
"$timeprefix" \
"$actorprefix" \
"$format" \
"$issuenumber" \
"$INIT_BIG_REPO" \
"$NOMS_BIN_FORMAT" \
"$WITH_TPCC" > job.json.github/scripts/performance-benchmarking/get-doltgres-doltgres-job-json.sh:18-38
initBigRepo="${10}"
nomsBinFormat="${11}"
sysbenchTestTime="${12}"
withTpcc="${13}"
...
if [ -n "$sysbenchTestTime" ]; then
sysbenchTestTime="\"--sysbench-test-time=$sysbenchTestTime\","
fiEvidence 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 duration is lost before the job starts**
**What failed:** The requested benchmark duration reaches the first dispatch payload but is dropped before the Kubernetes job is rendered. The generated job preserves the pull request revisions and other metadata, but it does not contain --sysbench-test-time=20.
- **Impact:** Benchmark runs can use a different duration than the one requested. This can make performance results unreliable and lead maintainers to make decisions from mismatched data.
- **Steps to reproduce:**
1. Open or synchronize a pull request with different known base and head commit SHAs.
2. Run the automatic benchmark workflow and inspect its repository-dispatch payload; confirm it includes sysbench_test_time=20.
3. Follow the payload into the Kubernetes benchmark workflow and render the job using run-benchmarks.sh.
4. Inspect the rendered container arguments and confirm that --sysbench-test-time=20 is absent and the TPCC position is being used as the duration position.
- **Stub / mock content:** No stubs, mocks, or bypasses were applied for this test in the recorded run.
- **Code analysis:** The new payload in .github/workflows/sysbench-performance.yaml lines 30-40 sets sysbench_test_time to "20". However, .github/workflows/k8s-benchmark-latency.yaml lines 41-56 exports the other client_payload fields to run-benchmarks.sh and has no SYSBENCH_TEST_TIME entry. Even if that environment mapping were added, run-benchmarks.sh lines 72-85 still calls the template with init-big-repo at argument 10, NOMS_BIN_FORMAT at argument 11, and WITH_TPCC at argument 12; it supplies no duration argument. The changed template in get-doltgres-doltgres-job-json.sh lines 5-22 now treats argument 12 as sysbenchTestTime and argument 13 as withTpcc, and lines 33-39 only emit --sysbench-test-time when argument 12 is non-empty. Consequently, the existing caller's TPCC value is interpreted as duration, while the requested 20-second value is never passed. The smallest practical fix is to map github.event.client_payload.sysbench_test_time to SYSBENCH_TEST_TIME in k8s-benchmark-latency.yaml and pass "$SYSBENCH_TEST_TIME" before "$WITH_TPCC" in run-benchmarks.sh, keeping the template's new positional contract consistent.
- **Why this is likely a bug:** The test's rendered job contract independently confirms that the duration requested by the pull-request workflow is absent, and source inspection explains the exact positional shift that causes it. This is not a test-only setup issue: the production GitHub Actions and shell files contain the missing handoff. The benchmark can therefore report results for a different duration than the workflow explicitly requested. The direct fix is limited to forwarding the existing payload field and restoring the intended duration-before-TPCC argument order.
**Relevant code:**
`.github/workflows/sysbench-performance.yaml:30-40`
~~~yaml
const payload = {
from_server: "doltgres",
from_version: fromSha,
to_server: "doltgres",
to_version: toSha,
mode: "pullRequest",
issue_number: String(context.issue.number),
init_big_repo: "true",
actor: context.actor,
sysbench_test_time: "20",
template_script: "./.github/scripts/performance-benchmarking/get-doltgres-doltgres-job-json.sh",
};
~~~
`.github/workflows/k8s-benchmark-latency.yaml:41-56`
~~~yaml
- name: Create Sysbench Performance Benchmarking K8s Job
run: ./.github/scripts/performance-benchmarking/run-benchmarks.sh
env:
FROM_SERVER: ${{ github.event.client_payload.from_server }}
FROM_VERSION: ${{ github.event.client_payload.from_version }}
TO_SERVER: ${{ github.event.client_payload.to_server }}
TO_VERSION: ${{ github.event.client_payload.to_version }}
MODE: ${{ github.event.client_payload.mode }}
ISSUE_NUMBER: ${{ github.event.client_payload.issue_number }}
ACTOR: ${{ github.event.client_payload.actor }}
TEMPLATE_SCRIPT: ${{ github.event.client_payload.template_script }}
~~~
`.github/scripts/performance-benchmarking/run-benchmarks.sh:72-85`
~~~shell
source \
"$TEMPLATE_SCRIPT" \
"$jobname" \
"$FROM_SERVER" \
"$FROM_VERSION" \
"$TO_SERVER" \
"$TO_VERSION" \
"$timeprefix" \
"$actorprefix" \
"$format" \
"$issuenumber" \
"$INIT_BIG_REPO" \
"$NOMS_BIN_FORMAT" \
"$WITH_TPCC" > job.json
~~~
`.github/scripts/performance-benchmarking/get-doltgres-doltgres-job-json.sh:18-38`
~~~shell
initBigRepo="${10}"
nomsBinFormat="${11}"
sysbenchTestTime="${12}"
withTpcc="${13}"
...
if [ -n "$sysbenchTestTime" ]; then
sysbenchTestTime="\"--sysbench-test-time=$sysbenchTestTime\","
fi
~~~
| @@ -18,7 +18,8 @@ format="$8" | |||
| issueNumber="$9" | |||
| initBigRepo="${10}" | |||
There was a problem hiding this comment.
🆕 New Failure: identified in this diff run
Nightly benchmarks use the wrong settings
What failed: Starting a nightly benchmark with TPCC enabled produces a job with --sysbench-test-time=true and no --withTpcc=true option. The launcher accepts and applies the job instead of rejecting the incompatible caller contract.
Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
- Severity: Medium
- Impact: Nightly benchmarks that enable TPCC run with the wrong duration setting and without TPCC. The job still starts, so teams may trust results that do not match the requested benchmark.
- Steps to Reproduce:
- Run the benchmark launcher with the existing twelve-value caller contract and set TPCC to true.
- Capture the generated Kubernetes Job JSON before applying it.
- Inspect the workload arguments and compare the duration and TPCC options with the requested settings.
- Stub / mock content: A local kubectl simulator captured the generated job without contacting a cluster; no production services, credentials, or application data were used.
- Code Analysis: The existing caller in .github/scripts/performance-benchmarking/run-benchmarks.sh:72-85 invokes TEMPLATE_SCRIPT with twelve positional values after the script name, placing NOMS_BIN_FORMAT at position 11 and WITH_TPCC at position 12. The PR changes .github/scripts/performance-benchmarking/get-doltgres-doltgres-job-json.sh:21-22 so position 12 is assigned to sysbenchTestTime and position 13 is assigned to withTpcc. Its rendering logic at :33-39 turns a non-empty sysbenchTestTime into --sysbench-test-time= and only renders --withTpcc when position 13 is present. Therefore the unchanged legacy invocation supplies true to the new duration slot and supplies no TPCC value. The direct reproduction rendered --sysbench-test-time=true, omitted --withTpcc=true, and still returned a successful job creation path. The PR diff explicitly introduced the new positional mapping and rendering; a targeted fix is to update callers to provide both new values or add an arity/contract check that fails clearly before generating the job.
- Why this is likely a bug: This is a production automation defect rather than a harness artifact: the source paths have incompatible positional contracts, and a local direct rendering with the legacy invocation reproduces the exact wrong options. The failure is silent because kubectl accepts the generated job and the launcher reports success, while the benchmark workload no longer matches the caller's request. The fix is narrow: align the caller and template argument positions, or fail early when the old arity is used; no broad benchmark redesign is required.
Relevant code
.github/scripts/performance-benchmarking/run-benchmarks.sh:72-85
source "$TEMPLATE_SCRIPT" ... "$NOMS_BIN_FORMAT" \
"$WITH_TPCC" > job.json.github/scripts/performance-benchmarking/get-doltgres-doltgres-job-json.sh:19-22
initBigRepo="${10}"
nomsBinFormat="${11}"
sysbenchTestTime="${12}"
withTpcc="${13}".github/scripts/performance-benchmarking/get-doltgres-doltgres-job-json.sh:33-39
if [ -n "$sysbenchTestTime" ]; then
sysbenchTestTime="\"--sysbench-test-time=$sysbenchTestTime\","
fi
if [ -n "$withTpcc" ]; then
withTpcc="\"--withTpcc=$withTpcc\",".github/scripts/performance-benchmarking/get-doltgres-doltgres-job-json.sh:101-107
"--results-prefix='$actorPrefix'",
'"$sysbenchTestTime"'
'"$withTpcc"'
'"$initBigRepo"'
'"$nomsBinFormat"'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 — Nightly benchmarks use the wrong settings**
**What failed:** Starting a nightly benchmark with TPCC enabled produces a job with --sysbench-test-time=true and no --withTpcc=true option. The launcher accepts and applies the job instead of rejecting the incompatible caller contract.
- **Impact:** Nightly benchmarks that enable TPCC run with the wrong duration setting and without TPCC. The job still starts, so teams may trust results that do not match the requested benchmark.
- **Steps to reproduce:**
1. Run the benchmark launcher with the existing twelve-value caller contract and set TPCC to true.
2. Capture the generated Kubernetes Job JSON before applying it.
3. Inspect the workload arguments and compare the duration and TPCC options with the requested settings.
- **Stub / mock content:** A local kubectl simulator captured the generated job without contacting a cluster; no production services, credentials, or application data were used.
- **Code analysis:** The existing caller in .github/scripts/performance-benchmarking/run-benchmarks.sh:72-85 invokes TEMPLATE_SCRIPT with twelve positional values after the script name, placing NOMS_BIN_FORMAT at position 11 and WITH_TPCC at position 12. The PR changes .github/scripts/performance-benchmarking/get-doltgres-doltgres-job-json.sh:21-22 so position 12 is assigned to sysbenchTestTime and position 13 is assigned to withTpcc. Its rendering logic at :33-39 turns a non-empty sysbenchTestTime into --sysbench-test-time=<value> and only renders --withTpcc when position 13 is present. Therefore the unchanged legacy invocation supplies true to the new duration slot and supplies no TPCC value. The direct reproduction rendered --sysbench-test-time=true, omitted --withTpcc=true, and still returned a successful job creation path. The PR diff explicitly introduced the new positional mapping and rendering; a targeted fix is to update callers to provide both new values or add an arity/contract check that fails clearly before generating the job.
- **Why this is likely a bug:** This is a production automation defect rather than a harness artifact: the source paths have incompatible positional contracts, and a local direct rendering with the legacy invocation reproduces the exact wrong options. The failure is silent because kubectl accepts the generated job and the launcher reports success, while the benchmark workload no longer matches the caller's request. The fix is narrow: align the caller and template argument positions, or fail early when the old arity is used; no broad benchmark redesign is required.
**Relevant code:**
`.github/scripts/performance-benchmarking/run-benchmarks.sh:72-85`
~~~sh
source "$TEMPLATE_SCRIPT" ... "$NOMS_BIN_FORMAT" \
"$WITH_TPCC" > job.json
~~~
`.github/scripts/performance-benchmarking/get-doltgres-doltgres-job-json.sh:19-22`
~~~sh
initBigRepo="${10}"
nomsBinFormat="${11}"
sysbenchTestTime="${12}"
withTpcc="${13}"
~~~
`.github/scripts/performance-benchmarking/get-doltgres-doltgres-job-json.sh:33-39`
~~~sh
if [ -n "$sysbenchTestTime" ]; then
sysbenchTestTime="\"--sysbench-test-time=$sysbenchTestTime\","
fi
if [ -n "$withTpcc" ]; then
withTpcc="\"--withTpcc=$withTpcc\","
~~~
`.github/scripts/performance-benchmarking/get-doltgres-doltgres-job-json.sh:101-107`
~~~sh
"--results-prefix='$actorPrefix'",
'"$sysbenchTestTime"'
'"$withTpcc"'
'"$initBigRepo"'
'"$nomsBinFormat"'
~~~
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
2 participants
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.
No description provided.