Address follow-up review feedback on the perf test pipeline - #4497
Draft
mdaigle wants to merge 1 commit into
Draft
Address follow-up review feedback on the perf test pipeline#4497mdaigle wants to merge 1 commit into
mdaigle wants to merge 1 commit into
Conversation
Follow-up to #4473. Picks up the six non-blocking review comments left on that PR plus the one Copilot comment that was never resolved. Security / supply chain: - Pin the azure-kusto-data / azure-kusto-ingest install to an exact version and drop --upgrade. That install runs inside an authenticated AzureCLI@2 context holding an ADX token, so a floating install would execute newly published code with those credentials. Also leaves a TODO for the move to the internal governed feed, since direct public PyPI access is expected to go away. - Pass the sa password to sqlcmd via SQLCMDPASSWORD instead of -P, on both the Linux and Windows harnesses. Process command lines are readable by other users on the box; environment blocks are not. Pipeline reliability: - Raise the ephemeral port range low bound from 1024 to 10000. SQL Server (1433) and sshd (22) live on the same VM, so the connection-churn benchmarks could take a service port as an outbound source port. - Resolve the Python interpreter up front (python3 -> python -> py -3) and fail fast with an accurate message. Windows has no python3, and the Store alias stubs resolve but do not work. Previously this surfaced as "produced no benchmark results" because Get-BenchmarkResultCount swallows the failure. - Report an "unknown" ingestion-failure query result distinctly from "no failures". _dump_failures returns None when the diagnostic query cannot run, which was being reported with reassuring "no failures were reported" wording and hid a repeatable permissions problem. Benchmark methodology: - Pin CPU affinity before the benchmark process starts rather than after. On Windows, narrow the launcher's affinity across Process.Start() so the child inherits it; on Linux, use preexec_fn so the pin lands between fork() and exec(). Setting affinity after the process is running leaves startup, assembly loading and JIT on arbitrary cores, including those reserved for SQL Server. - Stop silently discarding a regression whose benchmark Type cannot be mapped back to a unit. Such a key is never re-run, so its tally is stuck at 1 and the strict-majority test could never confirm it, dropping a real regression. It is now scored against the reps actually performed and logged loudly. Tests: - Add stdlib unit tests for the comparison and best-of-N confirmation logic that the failOnRegression gate depends on. 20 tests, no SQL Server, no BenchmarkDotNet, no ADX required: python3 -m unittest discover -s eng/pipelines/perf/scripts/tests Also adds a removal marker to the baseline-specific csproj workaround, which is otherwise dead weight the moment the baseline version moves. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Hardens the SqlClient perf pipeline harness under eng/pipelines/perf/ by tightening supply-chain inputs, reducing secret exposure, improving reliability of the regression gate, and adding isolated unit tests for the logic that determines pass/fail—without changing what the benchmarks measure or how results are reported.
Changes:
- Pins Azure Data Explorer (Kusto) Python SDK versions in the pipeline and improves Kusto ingestion verification messaging for “unknown” vs. “no failures”.
- Removes
sqlcmd -Ppassword usage (switches toSQLCMDPASSWORD), fixes Windows Python interpreter resolution, and improves CPU affinity pinning timing (pre-start). - Adds stdlib-only Python
unittestcoverage for comparison + best-of-N confirmation/gating logic, plus fixes “unmappable regression” bookkeeping in interleaved mode.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/Microsoft.Data.SqlClient/tests/PerformanceTests/Microsoft.Data.SqlClient.PerformanceTests.csproj | Adds explicit TODO commentary documenting a baseline-specific dependency workaround and its removal criteria. |
| eng/pipelines/perf/sqlclient-perf-pipeline.yml | Pins Kusto SDK install via KustoSdkVersion to improve reproducibility and reduce supply-chain risk. |
| eng/pipelines/perf/scripts/tests/test_perf_scripts.py | Adds isolated unit tests for the perf comparison and regression-confirmation logic. |
| eng/pipelines/perf/scripts/run-perf-tests.sh | Avoids leaking sa password via process args and fixes Linux ephemeral port range to reduce intermittent failures. |
| eng/pipelines/perf/scripts/run-perf-tests.ps1 | Resolves a usable Python 3 interpreter on Windows, avoids password-in-args for sqlcmd, and pins CPU affinity before process start. |
| eng/pipelines/perf/scripts/interleave_perf.py | Pins CPU affinity pre-exec on POSIX; fixes unmappable regression scoring so real regressions aren’t silently downgraded. |
| eng/pipelines/perf/scripts/ingest_kusto.py | Distinguishes “unknown” ingestion failures (diagnostic can’t run) from confirmed “no failures” when verification times out. |
Comment on lines
+132
to
+145
| try: | ||
| proc = subprocess.Popen(cmd, cwd=cwd, env=env, stdout=log, | ||
| stderr=subprocess.STDOUT, preexec_fn=preexec) | ||
| except Exception as exc: # noqa: BLE001 - preexec_fn failure must not abort the run | ||
| if preexec is None: | ||
| raise | ||
| print(f"WARNING: could not pin CPUs {cpus} before exec ({exc}); falling back to " | ||
| f"post-start pinning.", file=sys.stderr) | ||
| proc = subprocess.Popen(cmd, cwd=cwd, env=env, stdout=log, | ||
| stderr=subprocess.STDOUT) | ||
| apply_affinity(proc, cpus) | ||
| else: | ||
| if preexec is None: | ||
| apply_affinity(proc, cpus) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4497 +/- ##
==========================================
- Coverage 64.56% 62.64% -1.92%
==========================================
Files 288 283 -5
Lines 44046 66979 +22933
==========================================
+ Hits 28439 41961 +13522
- Misses 15607 25018 +9411
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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.
Follow-up to #4473, which was merged with six non-blocking review comments outstanding. This picks those up, plus the one Copilot comment on that PR that was never resolved.
Nothing here changes what the pipeline measures or how it reports — it's hardening of the harness, the credentials it handles, and the logic the regression gate depends on.
Security / supply chain
Pin the Kusto SDK install (
sqlclient-perf-pipeline.yml) —pip install --upgrade azure-kusto-data azure-kusto-ingestran unpinned from public PyPI inside theAzureCLI@2task, i.e. in a context already holding a token for the ADX cluster. Any newly published version of those packages or their transitive dependencies would have executed with those credentials, and perf runs would silently change whenever upstream published. Now pinned to6.0.4via a singleKustoSdkVersionvariable, with--upgradedropped.There's also a TODO to repoint this at the internal governed Python feed. Direct access to public PyPI is expected to be withdrawn for the org before long, following npm and NuGet — pinning now means that migration is a feed swap rather than a feed swap plus an unbounded version jump.
Keep the
sapassword out of the process table (run-perf-tests.sh,run-perf-tests.ps1) —sqlcmd -P <password>puts the password in the command line, which is readable by any other user on the box (/proc/<pid>/cmdlineon Linux;Win32_Process/ Process Explorer on Windows). Another process's environment block is not readable, andsqlcmdreadsSQLCMDPASSWORDnatively, so all six call sites now use that.Pipeline reliability
Ephemeral port range (
run-perf-tests.sh) —net.ipv4.ip_local_port_range=1024 65535let the connection-churn benchmarks grab 1433 or 22 as an outbound source port, on a VM where SQL Server and sshd are both listening. That shows up as an intermittent connection failure that reads as a perf anomaly. Raised the low bound to 10000.Python interpreter resolution (
run-perf-tests.ps1) — the Windows harness calledpython3, which is a Unix-ism; on Windows the interpreter ispython.exeor thepylauncher, and a stock image ships App Execution Alias stubs that resolve viaGet-Commandbut only open the Store. The failure was also misleading:Get-BenchmarkResultCountswallows a failed python call and returns 0, so a missing interpreter surfaced as "the run produced no benchmark results". Now resolved once up front (python3→python→py -3), probed for real rather than trusted, and failing fast with an accurate message.Distinguish "unknown" from "no ingestion failures" (
ingest_kusto.py) — this is Copilot's unresolved comment._dump_failures()returnsNonewhen the.show ingestion failuresdiagnostic itself can't run (typically a missing monitoring role), butif failures:treated that identically to a confirmed zero and printed "no ingestion failures were reported... the rows will land shortly". That's factually wrong in theNonecase and hides a repeatable permissions problem behind a soft, reassuring warning. The step still doesn't fail (it genuinely doesn't know), but it now says so and points at the fix.Benchmark methodology
Pin CPU affinity before the process starts, not after (
run-perf-tests.ps1,interleave_perf.py) — both harnesses started the benchmark process and then assigned affinity, leaving process startup, assembly loading, JIT and BenchmarkDotNet's own setup running on arbitrary cores, including the ones reserved for SQL Server. That's exactly the cross-talk the pinning exists to remove.Process.Start()so the child inherits it from its first instruction, then restore it immediately (the child already has the mask, and leaving the harness pinned would constrain the build and collection work that follows). The explicit assignment on the child is kept as a belt-and-braces re-assert and for the log line.preexec_fnso the pin lands betweenfork()andexec(), with a fallback to the old post-start path if that fails. Worth noting this was the interleaved mode, which is the default — so it was less isolated than the legacy sequential path, which correctly wraps the process intaskset.Don't silently drop an unmappable regression (
interleave_perf.py) — if a regressed benchmarkTypecan't be mapped back to a unit, it's never re-run, so its tally is stuck at 1. Scored againstreps=3, the strict-majority testcount * 2 > total_repscould then never pass, so a real regression was quietly downgraded to "unconfirmed" and slipped through the gate — a false negative caused by bookkeeping rather than by the measurement. It's now scored against the number of reps actually performed for it (so it's reported rather than discarded) and logged loudly, since it means the unit list and the reported Type names have drifted apart.Tests
Added
eng/pipelines/perf/scripts/tests/test_perf_scripts.py— 20 stdlibunittesttests over the comparison and best-of-N confirmation logic thatfailOnRegressiondepends on. No SQL Server, no BenchmarkDotNet, no ADX cluster required:Coverage:
_pctzero-baseline handling, everybuild_comparisonstatus boundary (including that the threshold is exclusive, and that a 0-byte baseline allocation isn't dropped), unparseable-report tolerance, and the confirmation verdict — persistent vs. flaky regressions, strict majority, only-candidates-are-re-run, single-rep, improvements never counting as regressions, and the unmappable-regression case above.Validation
Since this is mostly pipeline code that can't be exercised locally, I verified what I could:
python3 -m unittest discover— 20/20 pass, no skipsbash -n run-perf-tests.sh,python3 -m py_compileon all perf scripts — cleanrun-perf-tests.ps1— cleanResolve-Python3from the file via AST and executed it for real: resolves the interpreter, and both call-site invocation styles (piped stdin and argument array) work. Also verified with a fake Store-stubpython3onPATHthat the stub is rejected and the chain falls through to a workingpython.KustoSdkVersionresolvesDeliberately not included
interleave_perf.py_fmt_allocin the interleaved report — @cheenamalhotra replied "Not needed" on Add SqlClient performance test pipeline with baseline comparison, noise reduction, and ADX ingestion #4473, so I've left that call alone.show_perf_results.shandtranslate_results_to_kusto.shwere extracted). The Kusto ingestionAzureCLI@2block is the one that's left. Converting it frominlineScripttoscriptPathchanges the task wiring on a pipeline that can't be validated locally, so I'd rather do that as its own change than bundle it here.mainand is already fixed.Checklist
Suggested release note
None — this is test-infrastructure and pipeline-only, with no effect on the shipped driver.