Reland go_shim, replacing go runner by SDK's raw go - #54947
Conversation
go_shim, taking go from the SDK not the launchergo_shim, taking go from the SDK (not the launcher)
Gitlab CI Configuration ChangesChanges Summary
ℹ️ Diff available in the job log. |
This comment has been minimized.
This comment has been minimized.
Files inventory check summaryFile checks results against ancestor a0a423d7: Results for datadog-agent_7.84.0~devel.git.225.730749e.pipeline.131281652-1_amd64.deb:No change detected Results for datadog-iot-agent_7.84.0~devel.git.225.730749e.pipeline.131281652-1_amd64.deb:No change detected |
560772e to
730749e
Compare
go_shim, taking go from the SDK (not the launcher)go_shim, replacing go runner by SDK's raw go
Static quality checks✅ Please find below the results from static quality gates 33 successful checks with minimal change (< 2 KiB)
|
chouetz
left a comment
There was a problem hiding this comment.
Should we add some more paths?
| - changes: | ||
| paths: | ||
| - .gitlab/test/e2e/e2e.yml | ||
| - internal/tools/**/* # Go tools: gotestsum, etc. (incident-59251/59255/59256/59257/59258) |
There was a problem hiding this comment.
Include the shim implementation in E2E change rules
When a follow-up changes
bazel/rules/go_shim/defs.bzlor either wrapper template without also touchinginternal/tools, this rule will not schedule the E2E jobs, even thoughtasks/gotest.pyruns E2E tests through the shimmed//internal/tools:gotestsumtarget. Since the newly introduced shim code controls the working-directory behavior that previously broke E2E, addbazel/rules/go_shim/**/*to this change set so those runtime-only regressions are exercised before reachingmain.Useful? React with 👍 / 👎.
I suppose this remark is valuable
Regression DetectorRegression Detector ResultsMetrics dashboard Baseline: 92f6b05 Optimization Goals: ✅ No significant changes detected
|
| perf | experiment | goal | Δ mean % | Δ mean % CI | trials | links |
|---|---|---|---|---|---|---|
| ➖ | quality_gate_metrics_logs | memory utilization | +1.29 | [+1.04, +1.54] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_security_no_fs_load | memory utilization | +0.10 | [-0.06, +0.25] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_idle | memory utilization | -0.04 | [-0.16, +0.08] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_private_action_runner | memory utilization | -0.06 | [-0.18, +0.06] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_security_mean_fs_load | memory utilization | -0.15 | [-0.23, -0.08] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_security_idle | memory utilization | -0.21 | [-0.31, -0.10] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_idle_all_features | memory utilization | -0.32 | [-0.37, -0.28] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_logs | % cpu utilization | -0.87 | [-1.73, -0.02] | 1 | Logs bounds checks dashboard |
Bounds Checks: ✅ Passed
| perf | experiment | bounds_check_name | replicates_passed | observed_value | links |
|---|---|---|---|---|---|
| ✅ | quality_gate_idle | intake_connections | 10/10 | 4 = 4 | bounds checks dashboard |
| ✅ | quality_gate_idle | memory_usage | 10/10 | 174.47MiB ≤ 178MiB | bounds checks dashboard |
| ✅ | quality_gate_idle | total_bytes_received | 10/10 | 741.79KiB ≤ 819.20KiB | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | intake_connections | 10/10 | 4 = 4 | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | memory_usage | 10/10 | 515.47MiB ≤ 538MiB | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | total_bytes_received | 10/10 | 1.13MiB ≤ 1.25MiB | bounds checks dashboard |
| ✅ | quality_gate_logs | intake_connections | 10/10 | 17 ≤ 40 | bounds checks dashboard |
| ✅ | quality_gate_logs | memory_usage | 10/10 | 205.12MiB ≤ 229MiB | bounds checks dashboard |
| ✅ | quality_gate_logs | missed_bytes | 10/10 | 0B = 0B | bounds checks dashboard |
| ✅ | quality_gate_logs | total_bytes_received | 10/10 | 264.16MiB ≤ 292MiB | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | cpu_usage | 10/10 | 358.43 ≤ 2000 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | intake_connections | 10/10 | 18 ≤ 40 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | memory_usage | 10/10 | 403.13MiB ≤ 439MiB | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | missed_bytes | 10/10 | 0B = 0B | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | total_bytes_received | 10/10 | 0.94GiB ≤ 1.04GiB | bounds checks dashboard |
| ✅ | quality_gate_private_action_runner | memory_usage | 10/10 | 71.84MiB ≤ 76MiB | bounds checks dashboard |
| ✅ | quality_gate_security_idle | cpu_usage | 10/10 | 26.57 ≤ 100 | bounds checks dashboard |
| ✅ | quality_gate_security_idle | memory_usage | 10/10 | 325.65MiB ≤ 335MiB | bounds checks dashboard |
| ✅ | quality_gate_security_mean_fs_load | cpu_usage | 10/10 | 61.74 ≤ 200 | bounds checks dashboard |
| ✅ | quality_gate_security_mean_fs_load | memory_usage | 10/10 | 303.36MiB ≤ 314MiB | bounds checks dashboard |
| ✅ | quality_gate_security_no_fs_load | cpu_usage | 10/10 | 20.45 ≤ 100 | bounds checks dashboard |
| ✅ | quality_gate_security_no_fs_load | memory_usage | 10/10 | 316.20MiB ≤ 343MiB | bounds checks dashboard |
Explanation
Confidence level: 90.00%
Effect size tolerance: |Δ mean %| ≥ 5.00%
Performance changes are noted in the perf column of each table:
- ✅ = significantly better comparison variant performance
- ❌ = significantly worse comparison variant performance
- ➖ = no significant change in performance
A regression test is an A/B test of target performance in a repeatable rig, where "performance" is measured as "comparison variant minus baseline variant" for an optimization goal (e.g., ingress throughput). Due to intrinsic variability in measuring that goal, we can only estimate its mean value for each experiment; we report uncertainty in that value as a 90.00% confidence interval denoted "Δ mean % CI".
For each experiment, we decide whether a change in performance is a "regression" -- a change worth investigating further -- if all of the following criteria are true:
-
Its estimated |Δ mean %| ≥ 5.00%, indicating the change is big enough to merit a closer look.
-
Its 90.00% confidence interval "Δ mean % CI" does not contain zero, indicating that if our statistical model is accurate, there is at least a 90.00% chance there is a difference in performance between baseline and comparison variants.
-
Its configuration does not mark it "erratic".
CI Pass/Fail Decision
✅ Passed. All Quality Gates passed.
- quality_gate_metrics_logs, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check missed_bytes: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check missed_bytes: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_mean_fs_load, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_mean_fs_load, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_idle_all_features, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_idle_all_features, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_idle_all_features, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_security_no_fs_load, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_no_fs_load, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_private_action_runner, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_idle, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_idle, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
### What does this PR do? Relands #54887, which #54925 reverted after it broke E2E jobs on `main`. Recalling that PR: `go_shim` wraps a tool so that `PATH` lookups of `go` reach Bazel's hermetic SDK, and so that the tool runs from the current working directory rather than the runfiles tree, superseding `rules_multitool`'s `cwd()`. It wraps `gotestsum` and `go_mod_tidy_all`, the latter dropping the `$(rlocationpath @rules_go//go)` resolution it had grown for itself and moving to its own package, where the shim taking over `//:go_mod_tidy_all` leaves its `py_binary` the plain name. Changed on top of it: `PATH` now holds the Go SDK's own `go`, resolved through `@rules_go//go:toolchain`, in place of `@rules_go//go`. The wrapper exports `GOROOT` and sets `GOTOOLCHAIN=local` itself rather than leaning on `.bazelrc`, and `RUNFILES_DIR` goes away with the launcher that needed it. ### Motivation Unbreak `dda inv test` on hosts whose ambient `go` differs from the one `go.work` requires, [reported](https://dd.slack.com/archives/C06PBHLD4DQ/p1786618886362859) by @vitkyrka and followed up by @pgimalac and @hush-hush. #54107 routed it through `bazel run //internal/tools:gotestsum`, bringing `gotestsum` under `.bazelrc`'s `--run_env=GOTOOLCHAIN=local` while it still resolved `go` through `PATH`, so every invocation failed with `go.work requires go >= 1.26.5` and exporting `GOTOOLCHAIN=auto` did not help. That fix then broke `main`, because `@rules_go//go` is a launcher rather than a `go`: it assigns `cmd.Dir` from `$BUILD_WORKING_DIRECTORY` on every exec, discarding the directory its caller chose. `gotest-custom` chooses one per test package, so each prebuilt E2E test binary ran at the repository root and no longer found its fixtures, failing on `compose/data`, `usmtest/test_tags.ps1`, `testdataprovision` and `checks/shared-library/files`. The SDK's `go` honors its caller, which is the whole of the difference. ### Describe how you validated your changes On Linux, a child `go` invoked from `test/new-e2e` under the shim reports that module's `go.mod`, where #54887 reported the repository root's. Building a program importing `net/http` through the shim links, confirming the SDK's sources and tools are reachable. The reported reproducer still holds: with a stub announcing itself as go 1.24.0 first in `PATH`, tools reach go1.26.5 and never the stub, and `bazel run //:go_mod_tidy_all -- -diff` exits 0 across every module. ### Additional Notes Prepending to `PATH` has no declarative spelling: `RunEnvironmentInfo` carries static strings only, a key given in both `environment` and `inherited_environment` resolves to the inherited value, and replacing `PATH` outright would hide `sh`, `cc`, `git` and whatever else the toolchain reaches for. Hence a (thin) wrapper reading the environment at runtime. The runfiles mirror `go_bin_for_host`, rules_go's own definition of a runnable `go`: the SDK binary plus its headers, libs, sources and tools. `install_gotestsum` and its per-platform variants keep extracting the raw binary, the shim being runfiles-bound and those tools driving pre-built test binaries through `--raw-command`, never resolving `go`.
730749e to
bb721ce
Compare
What does this PR do?
Reland #54887, which #54925 reverted after it broke E2E jobs on
main.To prevent that from happening again, changes to
bazel/rules/go_shim/**/*andinternal/tools/**now trigger E2E tests unconditionally.Recalling that PR:
go_shimwraps a tool so thatPATHlookups ofgoreach Bazel's hermetic SDK, and so that the tool runs from the current working directory rather than the runfiles tree, supersedingrules_multitool'scwd().It wraps
gotestsumandgo_mod_tidy_all, the latter dropping the$(rlocationpath @rules_go//go)resolution it had grown for itself and moving to its own package, where the shim taking over//:go_mod_tidy_allleaves itspy_binarythe plain name.Changed on top of it:
PATHnow holds the Go SDK's rawgo(unwrapped), resolved through@rules_go//go:toolchain, in place of@rules_go//go(the runner),GOROOTand setsGOTOOLCHAIN=localitself rather than leaning on.bazelrc, andRUNFILES_DIRgoes away with the runner that needed it.Motivation
Unbreak
dda inv teston hosts whose ambientgodiffers from the onego.workrequires,reported by @vitkyrka and followed up by @pgimalac and @hush-hush. #54107 routed it through
bazel run //internal/tools:gotestsum, bringinggotestsumunder.bazelrc's--run_env=GOTOOLCHAIN=localwhile it still resolvedgothroughPATH, so every invocation failed withgo.work requires go >= 1.26.5and exportingGOTOOLCHAIN=autodid not help.That fix then broke
main, because@rules_go//gois a runner rather than ago: it assignscmd.Dirfrom$BUILD_WORKING_DIRECTORYon every exec, discarding the directory its caller chose.gotest-customchooses one per test package, so each prebuilt E2E test binary ran at the repository root and no longer found its fixtures, failing oncompose/data,usmtest/test_tags.ps1,testdataprovisionandchecks/shared-library/files.The SDK's
gohonors its caller, which is the whole of the difference.Describe how you validated your changes
On Linux, a child
goinvoked fromtest/new-e2eunder the shim reports that module'sgo.mod, where #54887 reported the repository root's.Building a program importing
net/httpthrough the shim links, confirming the SDK's sources and tools are reachable.The reported reproducer still holds: with a stub announcing itself as go 1.24.0 first in
PATH, tools reach go1.26.5 and never the stub, andbazel run //:go_mod_tidy_all -- -diffexits 0 across every module.Additional Notes
Prepending to
PATHhas no declarative spelling:RunEnvironmentInfocarries static strings only, a key given in bothenvironmentandinherited_environmentresolves to the inherited value, and replacingPATHoutright would hidesh,cc,gitand whatever else the toolchain reaches for.Hence a (thin) wrapper reading the environment at runtime.
The runfiles mirror
go_bin_for_host, rules_go's own definition of a runnablego: the SDK binary plus its headers, libs, sources and tools.install_gotestsumand its per-platform variants keep extracting the raw binary, the shim being runfiles-bound and those tools driving pre-built test binaries through--raw-command, never resolvinggo.