Allow dd_agent_go_test's default target to take extra gotags - #55347
Allow dd_agent_go_test's default target to take extra gotags#55347chouquette wants to merge 1 commit into
Conversation
extra_gotags gets added to all test targets, but isn't reflected in the geerated per-tags tests naming.
Files inventory check summaryFile checks results against ancestor 37d0bed6: Results for datadog-agent_7.84.0~devel.git.421.ee5f097.pipeline.132973537-1_amd64.deb:No change detected Results for datadog-iot-agent_7.84.0~devel.git.421.ee5f097.pipeline.132973537-1_amd64.deb:No change detected |
Static quality checks✅ Please find below the results from static quality gates 33 successful checks with minimal change (< 2 KiB)
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee5f097caf
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| def dd_agent_go_test( | ||
| name, | ||
| extra_gotags = None, |
There was a problem hiding this comment.
When a package using extra_gotags is placed under # gazelle:dd_agent_go_test off, revertDdAgentGoTests in bazel/rules/go/_gazelle_extension.go:278-289 converts the call to a plain go_test but only removes include_default and gotags_sets. The new macro-only attribute therefore remains on the generated go_test, which does not accept extra_gotags, leaving the BUILD file unable to analyze; delete this attribute during the reversion and cover the transition in the Gazelle extension tests.
Useful? React with 👍 / 👎.
| def dd_agent_go_test( | ||
| name, | ||
| extra_gotags = None, | ||
| gotags_sets = None, |
There was a problem hiding this comment.
The commit modifies a .bzl file, but its validation section only records a downstream build and provides no evidence that bazel run //bazel/buildifier was run. Run the repository formatter and record that validation before merging. bazel/codereview_guideline.mdL9-L13
Useful? React with 👍 / 👎.
Regression DetectorRegression Detector ResultsMetrics dashboard Baseline: 37d0bed Optimization Goals: ✅ No significant changes detected
|
| perf | experiment | goal | Δ mean % | Δ mean % CI | trials | links |
|---|---|---|---|---|---|---|
| ➖ | quality_gate_private_action_runner | memory utilization | +0.84 | [+0.72, +0.96] | 1 | Logs bounds checks dashboard |
| ➖ | dsd_uds_10mb_3k_timestamped_contexts_cpu | % cpu utilization | +0.16 | [-0.08, +0.40] | 1 | Logs |
| ➖ | quality_gate_security_idle | memory utilization | +0.12 | [+0.07, +0.16] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_security_mean_fs_load | memory utilization | +0.05 | [+0.01, +0.08] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_idle_all_features | memory utilization | -0.01 | [-0.05, +0.03] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_idle | memory utilization | -0.09 | [-0.13, -0.05] | 1 | Logs bounds checks dashboard |
| ➖ | dsd_uds_10mb_3k_timestamped_contexts_memory | memory utilization | -0.15 | [-0.35, +0.05] | 1 | Logs |
| ➖ | quality_gate_logs | % cpu utilization | -0.18 | [-1.06, +0.71] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_security_no_fs_load | memory utilization | -0.25 | [-0.33, -0.16] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_metrics_logs | memory utilization | -0.95 | [-1.18, -0.72] | 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 | 172.76MiB ≤ 178MiB | bounds checks dashboard |
| ✅ | quality_gate_idle | total_bytes_received | 10/10 | 747.89KiB ≤ 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 | 521.30MiB ≤ 538MiB | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | total_bytes_received | 10/10 | 1.14MiB ≤ 1.25MiB | bounds checks dashboard |
| ✅ | quality_gate_logs | intake_connections | 10/10 | 19 ≤ 40 | bounds checks dashboard |
| ✅ | quality_gate_logs | memory_usage | 10/10 | 217.83MiB ≤ 229MiB | bounds checks dashboard |
| ✅ | quality_gate_logs | missed_bytes | 10/10 | 0B = 0B | bounds checks dashboard |
| ✅ | quality_gate_logs | total_bytes_received | 10/10 | 263.41MiB ≤ 292MiB | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | cpu_usage | 10/10 | 352.65 ≤ 2000 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | intake_connections | 10/10 | 19 ≤ 40 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | memory_usage | 10/10 | 401.62MiB ≤ 453MiB | 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 | 73.24MiB ≤ 76MiB | bounds checks dashboard |
| ✅ | quality_gate_security_idle | cpu_usage | 10/10 | 25.68 ≤ 100 | bounds checks dashboard |
| ✅ | quality_gate_security_idle | memory_usage | 10/10 | 324.86MiB ≤ 335MiB | bounds checks dashboard |
| ✅ | quality_gate_security_mean_fs_load | cpu_usage | 10/10 | 60.36 ≤ 200 | bounds checks dashboard |
| ✅ | quality_gate_security_mean_fs_load | memory_usage | 10/10 | 305.67MiB ≤ 314MiB | bounds checks dashboard |
| ✅ | quality_gate_security_no_fs_load | cpu_usage | 10/10 | 21.23 ≤ 100 | bounds checks dashboard |
| ✅ | quality_gate_security_no_fs_load | memory_usage | 10/10 | 311.49MiB ≤ 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_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.
- quality_gate_logs, bounds check memory_usage: 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_logs, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_security_mean_fs_load, 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_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_metrics_logs, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check memory_usage: 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 intake_connections: 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_idle_all_features, bounds check memory_usage: 10/10 replicas passed. Gate passed.
| if extra_gotags: | ||
| tags = tags | set(extra_gotags) |
There was a problem hiding this comment.
extra_gotags (and gotags generally) is implemented in rules_go via a Starlark configuration transition, which forks the Bazel configuration for the entire transitive closure of the target's deps/embed, not just the target itself.
That means every helm_test call site that sets extra_gotags =["kustomize_disable_go_plugin_support"] builds sigs.k8s.io/kustomize/api (and everything under it) in a configuration that's isolated from whatever configuration any other target in the repo uses to build the same packages, so there is no action-cache sharing between them.
Since extra_gotags merges into every gotags_sets variant as well as the default target, that fork multiplies across all variants of a given dd_agent_go_test call, all under the same target name. gotags_sets at least makes its fan-out visible via a separate name_<suffix> target.
We're separately chasing Bazel analysis-cache thrashing (#53850, #54963, #55097), so I'd rather this PR avoid adding a new source of configuration forking if we can help it.
There is an alternative that avoids the fork entirely: patch sigs.k8s.io/kustomize/api at the source via go_deps.module_override in deps/go.MODULE.bazel, the same mechanism already used for msgp, godror, coreos/go-systemd, and favadi/protoc-go-inject-tag.
Since internal/plugins/loader/load_go_plugin.go is guarded by //go:build !kustomize_disable_go_plugin_support, with load_go_plugin_disabled.go as the stub fallback, a small patch flipping that constraint (or deleting the plugin-enabled file and dropping the constraint on the stub) makes the disabled behavior unconditional for every consumer, in every configuration, with no gotags/extra_gotags involved and no configuration fork: every target that transitively depends on kustomize/api shares one compiled artifact.
Worth noting: nothing in the repo imports sigs.k8s.io/kustomize/api directly today (go.mod/go.sum show it only as an indirect dependency of test/e2e-framework and test/new-e2e), so nobody exercises its native Go-plugin loading, so patching it unconditionally off should be very low risk.
The only trade-off is that the patch might fail on kustomize version bumps, but that applies to all existing patches in deps/go.MODULE.bazel.
There was a problem hiding this comment.
Thanks a lot for the review and the (very) detailed comment.
I didn't consider patching the package at all, but I agree, it works equally well for our purpose with less side effects, I'll get to it.
There was a problem hiding this comment.
Actually, we may not need a patch in the sense of file. Let me check...
There was a problem hiding this comment.
Oh I meant patching as in "modifying the package" not necessarily a .patch file or similar
There was a problem hiding this comment.
Given that we won't need to change the dd_agent_go_test rule, this PR is moot and I'll close it. I'll push the patch approach directly into #54846
There was a problem hiding this comment.
The best I could manage so far:
There was a problem hiding this comment.
That matches what I had locally (except that instead of removing the file from srcs I gated it with an ignore build tag) but I'm waiting on the target PR to reappear before pushing
### What does this PR do? Patch `sigs.k8s.io/kustomize/api` to always select its Go-plugin disabled loader stub, instead of adding a per-target `extra_gotags` escape hatch to `dd_agent_go_test` (#55347). ### Motivation `kustomize/api`'s plugin loader forces `-Wl,-z,nocopyreloc` on the linker, breaking `github.com/DataDog/zstd`'s copy-relocation-dependent cgo (#54846). #55347 fixes this per test target via `extra_gotags`, but it also flags that the fix is scoped to one target and the same conflict could recur for any future target combining `kustomize/api` with another copy-relocation-sensitive cgo dependency. Patching `kustomize/api` once, at the dependency level, closes that gap instead of requiring every future consumer to rediscover and reapply the same workaround. ### Describe how you validated your changes Built and ran a throwaway target embedding both `kustomize/api`'s `krusty` package and `zstd` with `bazel test`, with no `gotags` override, reproducing and then confirming the fix for the original link failure. The patch applies without exposing `io_k8s_sigs_kustomize_api` via `use_repo`, matching how #54846's `helm_test` actually reaches it: transitively, through `sh_helm_helm_v3`. ### Additional Notes Alas, a `gazelle:build_tags` and `gazelle:exclude` combination **does not work**, since `rules_go`'s compile-time constraint filter re-evaluates `//go:build` tags against each consumer's real `gotags`, independent of what `gazelle` wrote into `srcs`.
What does this PR do?
Allow test targets to take an additional extra_gotags that gets added to all generated test targets.
Motivation
This allows us to get finer grain control over the test dependencies build tags. See #54846 for an existing use case
Describe how you validated your changes
The PR linked just above successfully builds by passing the
kustomize_disable_go_plugin_supportbuild tag when buildingsigs.k8s.io/kustomize/apiAdditional Notes