Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdaptive export is rewritten into a node-local operator that applies ClickHouse schema, consumes kubescape events, writes attribution rows, and drives streaming, passthrough, and control paths. Supporting packages add hashing, parsing, storage, Pixie adapters, and load-test tooling. ChangesAdaptive Export Operator
Estimated review effort🎯 5 (Critical) | ⏱️ ~90+ minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ConstanzeTU
pushed a commit
that referenced
this pull request
Jun 9, 2026
The dx image build pipeline lives in entlein/dx itself (PR #53, branch feat/bazel-release): bazel-based with @px external pin to pixie's ae-prod tip, pushes to docker.io/entlein/dx-daemon on a release/dx/v* tag in the dx repo. The pixie-side buildx workflow this reverts duplicated that intent in the wrong repo + the wrong build system (docker buildx instead of bazel + pl_go_image macros) + the wrong registry (ghcr.io/k8sstormcenter instead of docker.io/entlein).
entlein
marked this pull request as ready for review
June 9, 2026 08:46
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/vizier_release.yaml:
- Line 18: The workflow uses an unrecognized self-hosted runner label in the
runs-on field ("oracle-vm-16cpu-64gb-x86-64"), which causes actionlint to fail;
either add this exact label to your actionlint self-hosted labels configuration
(the config key that lists allowed self-hosted labels) or change the runs-on
value in the workflow to a label already listed; update the runs-on entry in the
workflow or the actionlint config so that the label
"oracle-vm-16cpu-64gb-x86-64" is accepted by actionlint.
In `@src/vizier/services/adaptive_export/cmd/main.go`:
- Around line 506-514: Replace the raw http.ListenAndServe call with an explicit
http.Server instance configured with sensible timeouts (e.g., ReadTimeout,
ReadHeaderTimeout, WriteTimeout, IdleTimeout) and Addr set from CONTROL_ADDR and
Handler from ctrlSrv.Handler(); start it in the goroutine via
server.ListenAndServe() and keep the existing error handling (compare against
http.ErrServerClosed) and logging behavior for the control.New(activeSet, nil)
control surface. Ensure timeouts are chosen to protect the management surface
from slow clients and retain the existing log.WithField("addr", addr).Info and
log.WithError(err).Error flows.
In `@src/vizier/services/adaptive_export/internal/clickhouse/integration_test.go`:
- Around line 145-152: The test currently reuses a single 30s context
(ctx/cancel) for both Apply and VerifyPixieSchema causing deadline races; change
to create and use separate contexts (with their own timeouts and cancels) for
a.Apply(ctxApply) and a.VerifyPixieSchema(ctxVerify) so Apply cannot exhaust the
timeout needed for VerifyPixieSchema, ensuring you call the corresponding
cancel() after each operation; locate the calls to Apply and VerifyPixieSchema
in the test and replace the shared ctx/cancel usage with distinct contexts for
each call.
In `@src/vizier/services/adaptive_export/internal/clickhouse/schema.sql`:
- Around line 416-430: The ReplacingMergeTree for table adaptive_attribution
uses ORDER BY (hostname, anomaly_hash) which can collapse rows across different
namespace/pod values; either widen the primary key to include namespace and pod
(e.g., change ORDER BY to include namespace, pod) so the merge key is unique per
(hostname, namespace, pod, anomaly_hash) or ensure anomaly_hash is computed to
include namespace and pod; inspect the anomaly hash construction in
src/vizier/services/adaptive_export/internal/anomaly/hash.go (and any function
that builds anomaly_hash) and then update either the CREATE TABLE ORDER BY
clause or the hash construction so the join path (hostname, namespace, pod,
time) cannot lose rows.
In `@src/vizier/services/adaptive_export/internal/control/server_test.go`:
- Around line 115-129: Update TestBadInputRejected to include negative cases
that assert 400 responses for invalid time/window bounds: add do(...) calls that
post to the same endpoints using the test helper do with JSON payloads that
include (1) t_end <= 0 (e.g. {"pod":"p","table":"t","query_id":"x","t_end":0}),
(2) window [0,0] (e.g. {"pod":"p","table":"t","query_id":"x","window":[0,0]}),
and (3) window with end <= start (e.g.
{"pod":"p","table":"t","query_id":"x","window":[10,5]}), and assert each
returned r.StatusCode == http.StatusBadRequest; place these alongside the
existing checks in TestBadInputRejected so request-shape validation is locked by
tests.
In `@src/vizier/services/adaptive_export/internal/control/server.go`:
- Around line 110-159: The three handlers (handleStart, handleStop, handleQuery)
are currently unauthenticated; add an authorization check at the top of each
handler to reject unauthenticated/unauthorized callers before any state change
or query is performed. Implement or call a helper (e.g., s.requireAuth(r, w) or
s.authenticator.Authenticate(r)) that validates credentials (Authorization
header, token, mTLS, or IP restriction to localhost) and returns a boolean; if
it fails write an appropriate 401/403 response and return. Apply this check in
handleStart, handleStop, and handleQuery (before decode/processing and before
using s.set or s.runner) or refactor into a middleware wrapper used by all three
handlers. Ensure unauthorized requests never reach s.set.Upsert, s.set.Remove,
or s.runner.OrderQuery.
- Around line 116-154: The start and query handlers accept invalid times: add
validation in handleStart (after decode of targetReq) to reject default/zero
t_end by checking req.TEnd > 0 and return HTTP 400 if not; and in handleQuery
(after decode of queryReq) validate the Window values before calling
s.runner.OrderQuery—ensure req.Window has length 2, both entries > 0, and
req.Window[0] < req.Window[1], returning HTTP 400 for any violation; keep using
the existing req types (targetReq, queryReq) and the same response codes (400
for bad requests) and only proceed to s.set.Upsert or s.runner.OrderQuery when
validations pass.
In `@src/vizier/services/adaptive_export/internal/controller/controller.go`:
- Around line 26-29: Update the package header comment in controller.go to
reflect current behavior: remove or modify the lines claiming the controller
"does NOT execute PxL itself, does NOT write pixie observation rows, and does
NOT manage retention scripts" and instead state that the controller does execute
PxL and writes Pixie rows via pushPixieRows (and clarify its relationship to
retention scripts if applicable). Make sure to reference the controller package
and the pushPixieRows function in the comment so the contract accurately
describes the implementation.
- Around line 325-347: If c.sink.Write(ctx, []sink.AttributionRow{snapshot})
returns an error, do not call c.cfg.OnAttribution or spawn pushPixieRows: treat
persistence failure as fatal for downstream fan-out. Modify the block around
c.sink.Write so that when err != nil you log the error and skip both the
c.cfg.OnAttribution(snapshot.Namespace, snapshot.Pod, snapshot.TEnd) call and
the goroutine that calls c.pushPixieRows(ctx, snapshot) / manipulates c.inFlight
for hash; only execute OnAttribution and the spawn path when Write succeeds.
- Around line 243-255: Rehydrate restores c.active but never restarts the rev-1
fan-out goroutines; after populating c.active in Rehydrate, and after releasing
c.mu (do not spawn goroutines while holding the lock), detect rev-1 mode (e.g.,
c.mode or c.cfg.Mode) and for each restored row start the fan-out loop by
launching pushPixieRows in a goroutine with the row's AnomalyHash (ensure you
pass the hash as a value to avoid loop-variable capture and do not hold c.mu
while spawning).
In `@src/vizier/services/adaptive_export/internal/passthrough/passthrough.go`:
- Around line 133-147: The external calls l.q.Query(ctx, src) and
l.s.WritePixieRows(ctx, table, rows) must be executed with bounded per-table
contexts instead of the parent ctx to avoid hanging the sweep/tick loop; wrap
each call in a context.WithTimeout (use a sensible timeout value or an existing
field like l.tableTimeout if present) and defer cancel() before calling Query
and before calling WritePixieRows, propagate the new ctx into those calls, and
handle context.DeadlineExceeded as a warning similar to other errors so Run/tick
can continue promptly without blocking the select loop.
- Around line 81-84: The constructor (New) returns &Loop{... cfg: cfg} while
retaining cfg.Tables by reference; make a defensive copy of the slice before
storing it in Loop so external mutation or concurrent changes don't affect the
loop. Locate the New function and the Loop struct, and replace direct assignment
of cfg (or cfg.Tables) with a shallow copy where you allocate a new slice and
copy elements from cfg.Tables into it (use the new slice when setting cfg.Tables
stored in Loop), ensuring both the default-assignment branch (when
len(cfg.Tables)==0) and the caller-provided branch copy the slice before
constructing Loop.
In `@src/vizier/services/adaptive_export/internal/pixieapi/pixieapi.go`:
- Around line 87-89: The direct-mode dial code currently enforces the old
env/address gate and calls WithDisableTLSVerification, so change the direct
connection branch to use WithDirectTLSSkipVerify() instead and remove the
special-case guard that returns an error when opts.VizierAddr contains
"cluster.local" and PX_DISABLE_TLS != "1"; locate the direct-dial path that
inspects opts.VizierAddr and constructs the pxapi client (where
WithDisableTLSVerification is used) and replace that option with
WithDirectTLSSkipVerify(), ensuring other non-direct constructors still use the
existing TLS options.
In `@src/vizier/services/adaptive_export/internal/sink/fastencode.go`:
- Around line 188-195: The comment in the time.Time branch is incorrect about
avoiding an intermediate allocation: x.UTC().Format(...) returns a string which
is then passed to buf.WriteString, so this does allocate; update the comment
near the case time.Time / buf.WriteString / x.UTC().Format(...) to state that
Format returns a string (causing an intermediate allocation) and remove the
misleading claim about AppendFormat, or optionally mention that avoiding
allocation would require using a byte-append approach (e.g., time.AppendFormat)
if desired.
In `@src/vizier/services/adaptive_export/internal/streaming/filter.go`:
- Around line 194-196: When handling the receive from deltaCh in the select,
ensure you call disarm() before returning so any armed timer is stopped and
won't later fire and block on pendingC; update the case handling the
"<-u.deltaCh" branch to call disarm() (the same routine used elsewhere to
stop/cleanup the timer) and then return, guaranteeing the timer goroutine is
cleaned up when ActiveSet shuts down.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 38e6ea52-8b8e-437c-a44f-4fcf35986e67
📒 Files selected for processing (75)
.github/workflows/vizier_release.yamlskaffold/skaffold_vizier.yamlsrc/api/go/pxapi/opts.gosrc/stirling/source_connectors/socket_tracer/testing/container_images/BUILD.bazelsrc/vizier/services/adaptive_export/BUILD.bazelsrc/vizier/services/adaptive_export/cmd/BUILD.bazelsrc/vizier/services/adaptive_export/cmd/main.gosrc/vizier/services/adaptive_export/internal/activeset/BUILD.bazelsrc/vizier/services/adaptive_export/internal/activeset/activeset.gosrc/vizier/services/adaptive_export/internal/activeset/activeset_test.gosrc/vizier/services/adaptive_export/internal/anomaly/BUILD.bazelsrc/vizier/services/adaptive_export/internal/anomaly/hash.gosrc/vizier/services/adaptive_export/internal/anomaly/hash_bench_test.gosrc/vizier/services/adaptive_export/internal/anomaly/hash_test.gosrc/vizier/services/adaptive_export/internal/clickhouse/BUILD.bazelsrc/vizier/services/adaptive_export/internal/clickhouse/apply.gosrc/vizier/services/adaptive_export/internal/clickhouse/apply_test.gosrc/vizier/services/adaptive_export/internal/clickhouse/columns_test.gosrc/vizier/services/adaptive_export/internal/clickhouse/ddl.gosrc/vizier/services/adaptive_export/internal/clickhouse/ddl_test.gosrc/vizier/services/adaptive_export/internal/clickhouse/insert.gosrc/vizier/services/adaptive_export/internal/clickhouse/insert_test.gosrc/vizier/services/adaptive_export/internal/clickhouse/integration_test.gosrc/vizier/services/adaptive_export/internal/clickhouse/schema.sqlsrc/vizier/services/adaptive_export/internal/config/BUILD.bazelsrc/vizier/services/adaptive_export/internal/config/definition.gosrc/vizier/services/adaptive_export/internal/control/BUILD.bazelsrc/vizier/services/adaptive_export/internal/control/server.gosrc/vizier/services/adaptive_export/internal/control/server_test.gosrc/vizier/services/adaptive_export/internal/controller/BUILD.bazelsrc/vizier/services/adaptive_export/internal/controller/controller.gosrc/vizier/services/adaptive_export/internal/controller/controller_test.gosrc/vizier/services/adaptive_export/internal/e2e/BUILD.bazelsrc/vizier/services/adaptive_export/internal/e2e/e2e_test.gosrc/vizier/services/adaptive_export/internal/kubescape/BUILD.bazelsrc/vizier/services/adaptive_export/internal/kubescape/extract.gosrc/vizier/services/adaptive_export/internal/kubescape/extract_test.gosrc/vizier/services/adaptive_export/internal/passthrough/BUILD.bazelsrc/vizier/services/adaptive_export/internal/passthrough/passthrough.gosrc/vizier/services/adaptive_export/internal/passthrough/passthrough_test.gosrc/vizier/services/adaptive_export/internal/pixie/pixie.gosrc/vizier/services/adaptive_export/internal/pixieapi/BUILD.bazelsrc/vizier/services/adaptive_export/internal/pixieapi/pixieapi.gosrc/vizier/services/adaptive_export/internal/pixieapi/pixieapi_test.gosrc/vizier/services/adaptive_export/internal/pxl/BUILD.bazelsrc/vizier/services/adaptive_export/internal/pxl/pxl.gosrc/vizier/services/adaptive_export/internal/pxl/queryfor.gosrc/vizier/services/adaptive_export/internal/pxl/queryfor_bench_test.gosrc/vizier/services/adaptive_export/internal/pxl/queryfor_test.gosrc/vizier/services/adaptive_export/internal/pxl/tables.gosrc/vizier/services/adaptive_export/internal/pxl/tables_test.gosrc/vizier/services/adaptive_export/internal/sink/BUILD.bazelsrc/vizier/services/adaptive_export/internal/sink/clickhouse.gosrc/vizier/services/adaptive_export/internal/sink/clickhouse_test.gosrc/vizier/services/adaptive_export/internal/sink/encode_bench_test.gosrc/vizier/services/adaptive_export/internal/sink/fastencode.gosrc/vizier/services/adaptive_export/internal/sink/fastencode_test.gosrc/vizier/services/adaptive_export/internal/sink/integration_test.gosrc/vizier/services/adaptive_export/internal/streaming/BUILD.bazelsrc/vizier/services/adaptive_export/internal/streaming/filter.gosrc/vizier/services/adaptive_export/internal/streaming/filter_test.gosrc/vizier/services/adaptive_export/internal/streaming/integration_test.gosrc/vizier/services/adaptive_export/internal/streaming/notifier.gosrc/vizier/services/adaptive_export/internal/streaming/notifier_test.gosrc/vizier/services/adaptive_export/internal/streaming/scanner.gosrc/vizier/services/adaptive_export/internal/streaming/scanner_test.gosrc/vizier/services/adaptive_export/internal/streaming/supervisor.gosrc/vizier/services/adaptive_export/internal/streaming/writer.gosrc/vizier/services/adaptive_export/internal/trigger/BUILD.bazelsrc/vizier/services/adaptive_export/internal/trigger/clickhouse.gosrc/vizier/services/adaptive_export/internal/trigger/clickhouse_test.gosrc/vizier/services/adaptive_export/internal/trigger/fingerprint_bench_test.gosrc/vizier/services/adaptive_export/internal/trigger/integration_test.gosrc/vizier/services/adaptive_export/internal/trigger/watermark.gosrc/vizier/services/adaptive_export/internal/trigger/watermark_test.go
💤 Files with no reviewable changes (2)
- src/vizier/services/adaptive_export/internal/config/definition.go
- src/vizier/services/adaptive_export/internal/pxl/pxl.go
Comment on lines
+145
to
+152
| ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) | ||
| defer cancel() | ||
| // Apply first so the test is order-independent w.r.t. TestApply_Live. | ||
| if err := a.Apply(ctx); err != nil { | ||
| t.Fatalf("Apply (precondition): %v", err) | ||
| } | ||
| if err := a.VerifyPixieSchema(ctx); err != nil { | ||
| t.Fatalf("VerifyPixieSchema: %v", err) |
There was a problem hiding this comment.
Use separate contexts for Apply and VerifyPixieSchema in the live verify test.
At Line 145, a single 30s timeout is shared across both operations. On slower clusters, Apply can consume most/all budget and make VerifyPixieSchema fail with deadline errors unrelated to schema correctness.
Suggested fix
- ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
- defer cancel()
- // Apply first so the test is order-independent w.r.t. TestApply_Live.
- if err := a.Apply(ctx); err != nil {
+ ctxApply, cancelApply := context.WithTimeout(context.Background(), 60*time.Second)
+ defer cancelApply()
+ // Apply first so the test is order-independent w.r.t. TestApply_Live.
+ if err := a.Apply(ctxApply); err != nil {
t.Fatalf("Apply (precondition): %v", err)
}
- if err := a.VerifyPixieSchema(ctx); err != nil {
+ ctxVerify, cancelVerify := context.WithTimeout(context.Background(), 60*time.Second)
+ defer cancelVerify()
+ if err := a.VerifyPixieSchema(ctxVerify); err != nil {
t.Fatalf("VerifyPixieSchema: %v", err)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) | |
| defer cancel() | |
| // Apply first so the test is order-independent w.r.t. TestApply_Live. | |
| if err := a.Apply(ctx); err != nil { | |
| t.Fatalf("Apply (precondition): %v", err) | |
| } | |
| if err := a.VerifyPixieSchema(ctx); err != nil { | |
| t.Fatalf("VerifyPixieSchema: %v", err) | |
| ctxApply, cancelApply := context.WithTimeout(context.Background(), 60*time.Second) | |
| defer cancelApply() | |
| // Apply first so the test is order-independent w.r.t. TestApply_Live. | |
| if err := a.Apply(ctxApply); err != nil { | |
| t.Fatalf("Apply (precondition): %v", err) | |
| } | |
| ctxVerify, cancelVerify := context.WithTimeout(context.Background(), 60*time.Second) | |
| defer cancelVerify() | |
| if err := a.VerifyPixieSchema(ctxVerify); err != nil { | |
| t.Fatalf("VerifyPixieSchema: %v", err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/vizier/services/adaptive_export/internal/clickhouse/integration_test.go`
around lines 145 - 152, The test currently reuses a single 30s context
(ctx/cancel) for both Apply and VerifyPixieSchema causing deadline races; change
to create and use separate contexts (with their own timeouts and cancels) for
a.Apply(ctxApply) and a.VerifyPixieSchema(ctxVerify) so Apply cannot exhaust the
timeout needed for VerifyPixieSchema, ensuring you call the corresponding
cancel() after each operation; locate the calls to Apply and VerifyPixieSchema
in the test and replace the shared ctx/cancel usage with distinct contexts for
each call.
Comment on lines
+115
to
+129
| func TestBadInputRejected(t *testing.T) { | ||
| srv := New(&fakeExporter{}, &fakeRunner{}) | ||
| // missing pod | ||
| if r := do(t, srv, http.MethodPost, "/export/start", `{"namespace":"n"}`); r.StatusCode != http.StatusBadRequest { | ||
| t.Fatalf("start no-pod = %d, want 400", r.StatusCode) | ||
| } | ||
| // malformed json | ||
| if r := do(t, srv, http.MethodPost, "/export/stop", `{not json`); r.StatusCode != http.StatusBadRequest { | ||
| t.Fatalf("stop bad-json = %d, want 400", r.StatusCode) | ||
| } | ||
| // query missing table | ||
| if r := do(t, srv, http.MethodPost, "/query", `{"pod":"p","query_id":"x","window":[1,2]}`); r.StatusCode != http.StatusBadRequest { | ||
| t.Fatalf("query no-table = %d, want 400", r.StatusCode) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Extend bad-input tests for t_end and window bounds.
Please add negative cases for t_end<=0, window=[0,0], and window with end<=start so request-shape validation is locked in by tests.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/vizier/services/adaptive_export/internal/control/server_test.go` around
lines 115 - 129, Update TestBadInputRejected to include negative cases that
assert 400 responses for invalid time/window bounds: add do(...) calls that post
to the same endpoints using the test helper do with JSON payloads that include
(1) t_end <= 0 (e.g. {"pod":"p","table":"t","query_id":"x","t_end":0}), (2)
window [0,0] (e.g. {"pod":"p","table":"t","query_id":"x","window":[0,0]}), and
(3) window with end <= start (e.g.
{"pod":"p","table":"t","query_id":"x","window":[10,5]}), and assert each
returned r.StatusCode == http.StatusBadRequest; place these alongside the
existing checks in TestBadInputRejected so request-shape validation is locked by
tests.
Comment on lines
+133
to
+147
| rows, err := l.q.Query(ctx, src) | ||
| if err != nil { | ||
| log.WithError(err).WithField("table", table).Warn("ADAPTIVE_PASSTHROUGH: pixie query failed") | ||
| continue | ||
| } | ||
| if len(rows) == 0 { | ||
| log.WithField("table", table).Debug("ADAPTIVE_PASSTHROUGH: 0 rows") | ||
| continue | ||
| } | ||
| if err := l.s.WritePixieRows(ctx, table, rows); err != nil { | ||
| log.WithError(err).WithFields(log.Fields{ | ||
| "table": table, | ||
| "rows": len(rows), | ||
| }).Warn("ADAPTIVE_PASSTHROUGH: sink write failed") | ||
| continue |
There was a problem hiding this comment.
Bound external query/write calls with per-table timeouts.
On Line 133 and Line 142, external calls run with the parent context only. A hung dependency call can stall the whole sweep and delay shutdown because Run cannot re-enter the select loop until tick returns. Add bounded contexts per table operation.
Suggested fix
@@
- rows, err := l.q.Query(ctx, src)
+ tableCtx, cancel := context.WithTimeout(ctx, l.cfg.Refresh)
+ rows, err := l.q.Query(tableCtx, src)
+ cancel()
if err != nil {
log.WithError(err).WithField("table", table).Warn("ADAPTIVE_PASSTHROUGH: pixie query failed")
continue
}
@@
- if err := l.s.WritePixieRows(ctx, table, rows); err != nil {
+ writeCtx, cancel := context.WithTimeout(ctx, l.cfg.Refresh)
+ err = l.s.WritePixieRows(writeCtx, table, rows)
+ cancel()
+ if err != nil {
log.WithError(err).WithFields(log.Fields{
"table": table,
"rows": len(rows),
}).Warn("ADAPTIVE_PASSTHROUGH: sink write failed")
continue
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/vizier/services/adaptive_export/internal/passthrough/passthrough.go`
around lines 133 - 147, The external calls l.q.Query(ctx, src) and
l.s.WritePixieRows(ctx, table, rows) must be executed with bounded per-table
contexts instead of the parent ctx to avoid hanging the sweep/tick loop; wrap
each call in a context.WithTimeout (use a sensible timeout value or an existing
field like l.tableTimeout if present) and defer cancel() before calling Query
and before calling WritePixieRows, propagate the new ctx into those calls, and
handle context.DeadlineExceeded as a warning similar to other errors so Run/tick
can continue promptly without blocking the select loop.
Comment on lines
+188
to
+195
| case time.Time: | ||
| // Same format normalisePixieValue uses for the encoding/json | ||
| // path — CH DateTime64 string input shape. | ||
| buf.WriteByte('"') | ||
| // AppendFormat reuses the buf's underlying bytes; no | ||
| // intermediate string allocation. | ||
| buf.WriteString(x.UTC().Format("2006-01-02 15:04:05.000000000")) | ||
| buf.WriteByte('"') |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Minor comment inaccuracy about string allocation.
The comment on lines 192-193 states "AppendFormat reuses the buf's underlying bytes; no intermediate string allocation" but the code calls x.UTC().Format(...) which returns a string, then passes it to WriteString. This allocates a string intermediate.
time.Time doesn't have an AppendFormat method, so using Format is the correct approach here, but the comment is misleading.
📝 Suggested comment correction
case time.Time:
// Same format normalisePixieValue uses for the encoding/json
// path — CH DateTime64 string input shape.
buf.WriteByte('"')
- // AppendFormat reuses the buf's underlying bytes; no
- // intermediate string allocation.
+ // Format() allocates a string; time.Time has no AppendFormat method.
buf.WriteString(x.UTC().Format("2006-01-02 15:04:05.000000000"))
buf.WriteByte('"')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| case time.Time: | |
| // Same format normalisePixieValue uses for the encoding/json | |
| // path — CH DateTime64 string input shape. | |
| buf.WriteByte('"') | |
| // AppendFormat reuses the buf's underlying bytes; no | |
| // intermediate string allocation. | |
| buf.WriteString(x.UTC().Format("2006-01-02 15:04:05.000000000")) | |
| buf.WriteByte('"') | |
| case time.Time: | |
| // Same format normalisePixieValue uses for the encoding/json | |
| // path — CH DateTime64 string input shape. | |
| buf.WriteByte('"') | |
| // Format() allocates a string; time.Time has no AppendFormat method. | |
| buf.WriteString(x.UTC().Format("2006-01-02 15:04:05.000000000")) | |
| buf.WriteByte('"') |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/vizier/services/adaptive_export/internal/sink/fastencode.go` around lines
188 - 195, The comment in the time.Time branch is incorrect about avoiding an
intermediate allocation: x.UTC().Format(...) returns a string which is then
passed to buf.WriteString, so this does allocate; update the comment near the
case time.Time / buf.WriteString / x.UTC().Format(...) to state that Format
returns a string (causing an intermediate allocation) and remove the misleading
claim about AppendFormat, or optionally mention that avoiding allocation would
require using a byte-append approach (e.g., time.AppendFormat) if desired.
Author
|
Author
|
Build request: TTL Pushed one commit to Bug: the trigger polls Fix ( Ask: please build a TTL |
|
@entlein — picked up the build request. Working on it now from One heads-up while you wait: the These are stale test-side expectations, not a regression in your fix logic — they just look for raw Image first, then I can either send you a follow-up patch for those two tests or you can fix them in your next commit — your call. |
|
@entlein — TTL image ready for your rig:
kubectl -n pl set image deployment/adaptive-export adaptive-export=ttl.sh/ae-700821d3b-2026-06-16:24h
kubectl -n pl rollout status deployment/adaptive-export --timeout=2m
kubectl -n pl logs deployment/adaptive-export --tail=30 | grep -iE 'normalize|watermark|trigger'Re-run your sustained-injection rig (the one that produced 25/25 ticks → 0 attribution rows pre-fix) and ping back with the post-fix tick/row counts. Reminder: the two |
Author
|
Harness + generator source added to this PR ( The fixture-isolation load-test harness that found + validates the F8 fix now lives at
Second ask (build VM): please turn |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/vizier/services/adaptive_export/internal/trigger/clickhouse.go (1)
267-317:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset
seenAtBoundarywhen the watermark advances.Line 292 promotes
watermarkinside the loop, so by Line 308maxSeen > watermarkis normally false after progress. The old boundary set is then extended instead of replaced, causingseenAtBoundaryto grow for every new boundary over a long-running operator.Proposed fix
- nextSeen := map[string]bool{} + startWatermark := watermark + nextSeen := map[string]bool{} @@ - if maxSeen > watermark { + if maxSeen > startWatermark { watermark = maxSeen seenAtBoundary = nextSeen dirty = true } else if maxSeen == watermark {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vizier/services/adaptive_export/internal/trigger/clickhouse.go` around lines 267 - 317, The issue is that `seenAtBoundary` grows unbounded because the watermark can be advanced mid-loop at line 292 (`watermark = evn`), making the condition `maxSeen > watermark` false at line 308, which causes the code to extend the boundary set (lines 310-312) instead of replacing it. When the watermark is promoted during the loop iteration, the boundary set should be reset to reflect the new watermark position. Add logic to reset `seenAtBoundary` whenever the watermark is advanced at line 292, ensuring that old boundary entries do not accumulate across different watermark boundaries. This ensures that `seenAtBoundary` only contains fingerprints relevant to the current watermark value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/vizier/services/adaptive_export/cmd/main.go`:
- Around line 353-360: The code currently accepts zero as a valid value for the
push refresh interval when parsing the envPushRefreshSec environment variable,
but the documented modes only support positive seconds (for push refresh) or
negative values (for single-shot mode). Add validation to reject the case where
n equals zero after the strconv.Atoi call, treating it as an invalid
configuration (either by returning an error or logging a warning and skipping
the configuration). The current else block that handles positive values should
be modified to explicitly check that n is greater than zero, or a separate check
should reject n when it equals zero before attempting to assign it to
ctlCfg.PushRefreshInterval.
In `@src/vizier/services/adaptive_export/internal/e2e/loadtest_test.go`:
- Around line 184-188: The loop starting at line 184 breaks prematurely when
sufficient tables are present but before each table has accumulated the expected
number of rows. The condition on line 187 checks only c.attribution and the
count of tables in c.rowsByTable, but does not verify the actual row counts
within each table. Add a condition to gate the break statement to also check
that each table's row count meets the expected perTable count threshold,
ensuring all expected rows have landed before exiting the wait loop.
In
`@src/vizier/services/adaptive_export/internal/trigger/clickhouse_internal_test.go`:
- Around line 64-67: The variable gotQuery is being written from the httptest
server handler goroutine and read later from the test goroutine without
synchronization, causing a data race. Replace the direct variable assignment
with either a buffered channel (e.g., make a channel to receive the query
string) or a sync.Mutex to protect concurrent access to gotQuery. Use the
channel or mutex in both the handler function where gotQuery is set and wherever
it is read after the server processes the request to ensure proper
synchronization between goroutines.
In `@src/vizier/services/adaptive_export/internal/trigger/clickhouse.go`:
- Around line 282-303: The current implementation uses client-side deduplication
to skip rows already seen at the current watermark boundary, but when multiple
rows share the same normalized event_time and exceed the PollLimit, the query
continues returning the same boundary rows, causing rows beyond the limit to
never be emitted. Fix this by implementing either a server-side page cursor or
tie-breaker in the ClickHouse query to provide deterministic ordering beyond the
normalized event_time (such as using a unique row identifier or sequence
number), or by modifying the pagination logic to fully drain all rows with an
equal watermark value before advancing the watermark to the next distinct
event_time. This ensures no rows are skipped due to pagination limits when
multiple rows share the same boundary time value.
---
Outside diff comments:
In `@src/vizier/services/adaptive_export/internal/trigger/clickhouse.go`:
- Around line 267-317: The issue is that `seenAtBoundary` grows unbounded
because the watermark can be advanced mid-loop at line 292 (`watermark = evn`),
making the condition `maxSeen > watermark` false at line 308, which causes the
code to extend the boundary set (lines 310-312) instead of replacing it. When
the watermark is promoted during the loop iteration, the boundary set should be
reset to reflect the new watermark position. Add logic to reset `seenAtBoundary`
whenever the watermark is advanced at line 292, ensuring that old boundary
entries do not accumulate across different watermark boundaries. This ensures
that `seenAtBoundary` only contains fingerprints relevant to the current
watermark value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d20e5a1f-ad2f-4cb9-b2f3-8e97a52e56b0
📒 Files selected for processing (4)
src/vizier/services/adaptive_export/cmd/main.gosrc/vizier/services/adaptive_export/internal/e2e/loadtest_test.gosrc/vizier/services/adaptive_export/internal/trigger/clickhouse.gosrc/vizier/services/adaptive_export/internal/trigger/clickhouse_internal_test.go
Comment on lines
+353
to
360
| if v := strings.TrimSpace(os.Getenv(envPushRefreshSec)); v != "" { | ||
| if n, err := strconv.Atoi(v); err == nil { | ||
| if n < 0 { | ||
| ctlCfg.PushRefreshInterval = -1 | ||
| log.Info(envPushRefreshSec + "<0 — single-shot pull mode (one pull per anomaly window)") | ||
| } else { | ||
| ctlCfg.PushRefreshInterval = time.Duration(n) * time.Second | ||
| } |
There was a problem hiding this comment.
Reject ADAPTIVE_PUSH_REFRESH_SEC=0 instead of installing a zero interval.
The documented modes are unset/default, positive seconds, and negative single-shot. Accepting 0 can hand the controller a zero refresh interval, which risks a tight loop or ticker panic depending on downstream handling.
Proposed fix
if v := strings.TrimSpace(os.Getenv(envPushRefreshSec)); v != "" {
if n, err := strconv.Atoi(v); err == nil {
- if n < 0 {
+ switch {
+ case n < 0:
ctlCfg.PushRefreshInterval = -1
log.Info(envPushRefreshSec + "<0 — single-shot pull mode (one pull per anomaly window)")
- } else {
+ case n == 0:
+ log.WithField("value", v).Warn(envPushRefreshSec + " must be positive or negative; using default refresh")
+ default:
ctlCfg.PushRefreshInterval = time.Duration(n) * time.Second
}
} else {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if v := strings.TrimSpace(os.Getenv(envPushRefreshSec)); v != "" { | |
| if n, err := strconv.Atoi(v); err == nil { | |
| if n < 0 { | |
| ctlCfg.PushRefreshInterval = -1 | |
| log.Info(envPushRefreshSec + "<0 — single-shot pull mode (one pull per anomaly window)") | |
| } else { | |
| ctlCfg.PushRefreshInterval = time.Duration(n) * time.Second | |
| } | |
| if v := strings.TrimSpace(os.Getenv(envPushRefreshSec)); v != "" { | |
| if n, err := strconv.Atoi(v); err == nil { | |
| switch { | |
| case n < 0: | |
| ctlCfg.PushRefreshInterval = -1 | |
| log.Info(envPushRefreshSec + "<0 — single-shot pull mode (one pull per anomaly window)") | |
| case n == 0: | |
| log.WithField("value", v).Warn(envPushRefreshSec + " must be positive or negative; using default refresh") | |
| default: | |
| ctlCfg.PushRefreshInterval = time.Duration(n) * time.Second | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/vizier/services/adaptive_export/cmd/main.go` around lines 353 - 360, The
code currently accepts zero as a valid value for the push refresh interval when
parsing the envPushRefreshSec environment variable, but the documented modes
only support positive seconds (for push refresh) or negative values (for
single-shot mode). Add validation to reject the case where n equals zero after
the strconv.Atoi call, treating it as an invalid configuration (either by
returning an error or logging a warning and skipping the configuration). The
current else block that handles positive values should be modified to explicitly
check that n is greater than zero, or a separate check should reject n when it
equals zero before attempting to assign it to ctlCfg.PushRefreshInterval.
Comment on lines
+184
to
+188
| deadline := time.Now().Add(3 * time.Second) | ||
| for time.Now().Before(deadline) { | ||
| c := measure(stub.sqls(), stub.bodies()) | ||
| if c.attribution >= 1 && len(c.rowsByTable) >= wantTables { | ||
| break |
There was a problem hiding this comment.
Wait for expected row counts, not just table presence.
Line 187 can break as soon as each table has any insert, then Line 192 cancels the controller before the expected rows necessarily land. Gate readiness on perTable counts to avoid intermittent short reads.
Proposed fix
deadline := time.Now().Add(3 * time.Second)
for time.Now().Before(deadline) {
c := measure(stub.sqls(), stub.bodies())
- if c.attribution >= 1 && len(c.rowsByTable) >= wantTables {
+ ready := c.attribution >= 1
+ for tbl, want := range perTable {
+ if want > 0 && c.rowsByTable[tbl] < want {
+ ready = false
+ break
+ }
+ }
+ if ready {
break
}
time.Sleep(5 * time.Millisecond)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| deadline := time.Now().Add(3 * time.Second) | |
| for time.Now().Before(deadline) { | |
| c := measure(stub.sqls(), stub.bodies()) | |
| if c.attribution >= 1 && len(c.rowsByTable) >= wantTables { | |
| break | |
| deadline := time.Now().Add(3 * time.Second) | |
| for time.Now().Before(deadline) { | |
| c := measure(stub.sqls(), stub.bodies()) | |
| ready := c.attribution >= 1 | |
| for tbl, want := range perTable { | |
| if want > 0 && c.rowsByTable[tbl] < want { | |
| ready = false | |
| break | |
| } | |
| } | |
| if ready { | |
| break | |
| } | |
| time.Sleep(5 * time.Millisecond) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/vizier/services/adaptive_export/internal/e2e/loadtest_test.go` around
lines 184 - 188, The loop starting at line 184 breaks prematurely when
sufficient tables are present but before each table has accumulated the expected
number of rows. The condition on line 187 checks only c.attribution and the
count of tables in c.rowsByTable, but does not verify the actual row counts
within each table. Add a condition to gate the break statement to also check
that each table's row count meets the expected perTable count threshold,
ensuring all expected rows have landed before exiting the wait loop.
|
@entlein — second ask done. Branch What's in the commit:
Local verify: FYI — open issues on this PR that aren't mine to fix but you'll want to address:
Image (ttl.sh/ae-700821d3b-2026-06-16:24h) is still up if your rig is still soak-testing — let me know if you need a fresh build after you incorporate any of the above. |
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/e2e_test/adaptive_export_loadtest/harness/ae_config.sh`:
- Line 18: The shell scripts lack the -e flag in their set commands, which means
failed commands do not cause the scripts to exit immediately, allowing
subsequent commands to run even after failures. In
src/e2e_test/adaptive_export_loadtest/harness/ae_config.sh at line 18, change
the set command from set -uo pipefail to set -euo pipefail so that failed k
operations abort immediately. In
src/e2e_test/adaptive_export_loadtest/harness/deploy_ae.sh at line 23, change
the set command within the remote heredoc from set -uo pipefail to set -euo
pipefail so that failed kubectl commands stop deployment before __AE_DEPLOYED__
is emitted.
In `@src/e2e_test/adaptive_export_loadtest/harness/build_gen_image.sh`:
- Around line 15-18: The build script uses a fixed hardcoded path
`/tmp/aeload-ctx.tgz` for the temporary archive, which can be overwritten by
parallel test runs and cause incorrect build context. Replace the fixed
`/tmp/aeload-ctx.tgz` path with a dynamically generated unique temporary file
path (e.g., using mktemp or a similar mechanism that includes a timestamp or
random suffix). Update both occurrences of the path: the one in the tar command
that creates the archive and the one in the labctl cp command that copies it to
the dev-machine.
In `@src/e2e_test/adaptive_export_loadtest/harness/deploy_ae.sh`:
- Line 51: The script unconditionally deletes /tmp/keys.env on line 51 even
though the script itself never creates this file, which can unintentionally
remove a user's existing file. Remove the rm command that deletes /tmp/keys.env,
or if cleanup is necessary, ensure the script only removes temporary files it
actually creates by using a script-controlled temporary file location instead of
relying on shared system directories.
- Around line 22-49: The bash heredoc executed via labctl ssh in the
deploy_ae.sh script uses set -uo pipefail without the -e flag, which means
failed kubectl commands are silently ignored and the script continues executing,
potentially emitting __AE_DEPLOYED__ even when deployment failed. Add the -e
flag to the set command in the heredoc block (currently set -uo pipefail) to
change it to set -euo pipefail, which will cause the script to exit immediately
on any command failure and achieve fail-fast behavior.
In `@src/e2e_test/adaptive_export_loadtest/harness/exp_control.sh`:
- Around line 58-59: The issue spans two files where critical injector failures
are being masked. In
src/e2e_test/adaptive_export_loadtest/harness/exp_control.sh at lines 58-59, the
first inj command with rule R0001 uses || true which silently ignores failures;
remove this and require both the R0001 and R0010 inj commands to succeed
together so that any failure properly triggers the INJECT_FAIL case. In
src/e2e_test/adaptive_export_loadtest/harness/exp_e8.sh at lines 58-64, modify
the sustained-tick injection handling to treat failed injections as explicit
tick failures instead of allowing them to be masked or reported as STALL
conditions.
- Around line 66-67: The rollout restart and rollout status commands for the
adaptive-export deployment are both suppressing failures with `|| true`, which
allows the E6 idempotency check to pass even when the restart condition fails to
execute properly. Remove the `|| true` failure suppression from both the rollout
restart and rollout status command lines so that any failures in these commands
will cause the script to fail and prevent false test passes.
In `@src/e2e_test/adaptive_export_loadtest/harness/lib.sh`:
- Around line 73-77: The chq() function doesn't validate whether the ClickHouse
query succeeded before returning its output, allowing error responses to be
inadvertently converted to numeric values by downstream tr -dc '0-9' filtering.
Modify the chq() function to detect and reject ClickHouse error responses,
checking the response output for error indicators before returning, and ensure
all calling code validates query success before applying numeric parsing with tr
-dc '0-9'.
In `@src/e2e_test/adaptive_export_loadtest/harness/run.sh`:
- Line 11: The set command in the run.sh script is missing the `-e` flag, which
prevents the script from exiting immediately when commands fail, allowing phase
failures to be masked. Add the `-e` flag to the set command to enable fail-fast
behavior for proper phase orchestration. This change should be applied to all
set statements in the script where this pattern appears.
In `@src/e2e_test/adaptive_export_loadtest/harness/stats.py`:
- Around line 40-57: The logic for determining the reproducibility verdict in
the final print statement is flawed when there are zero PASS rows. When all rows
fail the passcol check, every metric will encounter the "if not vals" condition
and continue without setting repro_ok to False, causing the verdict to
incorrectly state "EXACTLY REPRODUCIBLE" even though no valid PASS data was
evaluated. To fix this, introduce a flag to track whether any PASS rows were
actually found and processed. Initialize this flag before the loop over metrics,
set it to True only when we successfully process a metric with valid values
(after the "if not vals" check), and update the final verdict condition to also
check that this flag is True so the verdict is only "EXACTLY REPRODUCIBLE" when
both: (1) at least one PASS row was found, and (2) all metrics with data show no
variation (repro_ok remains True).
In
`@src/e2e_test/adaptive_export_loadtest/tools/loadgen/cmd/cleanloadgen/main.go`:
- Around line 171-175: The database query in the loop starting at line 171
(where db.QueryRow("SELECT 1") is called) executes without a context deadline,
which can cause the generator to block indefinitely if the database connection
hangs. Add a context with an appropriate timeout to the QueryRow call to ensure
queries do not hang indefinitely. Use context.WithTimeout or
context.WithDeadline to create a time-bounded context before executing each
db.QueryRow operation in the loop.
In `@src/e2e_test/adaptive_export_loadtest/tools/loadgen/cmd/httpsink/main.go`:
- Around line 26-29: The HTTP server initialization in the main function is
missing timeout configuration and has improper error handling. Add ReadTimeout,
WriteTimeout, and IdleTimeout fields to the http.Server struct to prevent
resource exhaustion and stalls. Additionally, modify the error handling for
srv.ListenAndServe() to check if the error is http.ErrServerClosed and only
panic on actual errors, since http.ErrServerClosed is an expected error that
occurs during graceful shutdown and should not cause a panic.
In `@src/e2e_test/adaptive_export_loadtest/tools/loadgen/Dockerfile`:
- Around line 11-12: Two consecutive RUN instructions for the go build commands
(cleanloadgen and httpsink) are creating unnecessary intermediate layers.
Consolidate these two separate RUN statements into a single RUN instruction by
joining them with the && operator and using a backslash for line continuation.
This reduces the Docker image layer count and improves build efficiency.
In `@src/e2e_test/adaptive_export_loadtest/tools/loadgen/go.mod`:
- Line 3: The go.mod file currently specifies an unpinned version with "go
1.22", which allows any patch release to be used during builds. Add a toolchain
directive in go.mod with a specific patch version such as "toolchain go1.22.x"
(replacing x with the actual patch number) to pin the exact Go patch level and
ensure consistent, reproducible builds while mitigating exposure to any stdlib
advisories in older 1.22 patch releases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 85a889e9-e088-4ee2-997b-b9643bd28d26
📒 Files selected for processing (23)
.bazelignoresrc/e2e_test/adaptive_export_loadtest/CONTRACTS.mdsrc/e2e_test/adaptive_export_loadtest/FINDINGS_AND_BACKLOG.mdsrc/e2e_test/adaptive_export_loadtest/README.mdsrc/e2e_test/adaptive_export_loadtest/fixtures/EXPERIMENTS.mdsrc/e2e_test/adaptive_export_loadtest/harness/ae_config.shsrc/e2e_test/adaptive_export_loadtest/harness/build_gen_image.shsrc/e2e_test/adaptive_export_loadtest/harness/deploy_ae.shsrc/e2e_test/adaptive_export_loadtest/harness/exp_control.shsrc/e2e_test/adaptive_export_loadtest/harness/exp_e5.shsrc/e2e_test/adaptive_export_loadtest/harness/exp_e8.shsrc/e2e_test/adaptive_export_loadtest/harness/inject.shsrc/e2e_test/adaptive_export_loadtest/harness/lib.shsrc/e2e_test/adaptive_export_loadtest/harness/run.shsrc/e2e_test/adaptive_export_loadtest/harness/stats.pysrc/e2e_test/adaptive_export_loadtest/k8s/00-sinks.yamlsrc/e2e_test/adaptive_export_loadtest/k8s/gen-pod.tmpl.yamlsrc/e2e_test/adaptive_export_loadtest/tools/loadgen/Dockerfilesrc/e2e_test/adaptive_export_loadtest/tools/loadgen/cmd/cleanloadgen/main.gosrc/e2e_test/adaptive_export_loadtest/tools/loadgen/cmd/httpsink/main.gosrc/e2e_test/adaptive_export_loadtest/tools/loadgen/go.modsrc/vizier/services/adaptive_export/internal/trigger/clickhouse_test.gosrc/vizier/services/adaptive_export/internal/trigger/watermark_test.go
| # window written exactly once. | ||
| # Also disables async_insert on the ingest user so row counts are stable at read | ||
| # time (per the AE per-PG fixes), and applies the PL_CLOUD_ADDR :443 fix. | ||
| set -uo pipefail |
There was a problem hiding this comment.
Shell fail-fast is inconsistently configured, enabling false-success runs after command failures.
src/e2e_test/adaptive_export_loadtest/harness/ae_config.sh#L18-L18: switch toset -euo pipefailso failedkoperations abort immediately.src/e2e_test/adaptive_export_loadtest/harness/deploy_ae.sh#L23-L23: switch remote heredoc toset -euo pipefailso failedkubectlcommands stop deployment before__AE_DEPLOYED__is emitted.
📍 Affects 2 files
src/e2e_test/adaptive_export_loadtest/harness/ae_config.sh#L18-L18(this comment)src/e2e_test/adaptive_export_loadtest/harness/deploy_ae.sh#L23-L23
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/e2e_test/adaptive_export_loadtest/harness/ae_config.sh` at line 18, The
shell scripts lack the -e flag in their set commands, which means failed
commands do not cause the scripts to exit immediately, allowing subsequent
commands to run even after failures. In
src/e2e_test/adaptive_export_loadtest/harness/ae_config.sh at line 18, change
the set command from set -uo pipefail to set -euo pipefail so that failed k
operations abort immediately. In
src/e2e_test/adaptive_export_loadtest/harness/deploy_ae.sh at line 23, change
the set command within the remote heredoc from set -uo pipefail to set -euo
pipefail so that failed kubectl commands stop deployment before __AE_DEPLOYED__
is emitted.
Comment on lines
+15
to
+18
| tar -C "$SRC" --exclude='.git' --exclude='harness/__pycache__' -czf /tmp/aeload-ctx.tgz . | ||
| echo "[build] cp to $PG dev-machine" | ||
| labctl cp -m dev-machine /tmp/aeload-ctx.tgz "$PG:/tmp/aeload-ctx.tgz" </dev/null | ||
|
|
There was a problem hiding this comment.
Use a unique temp archive path to avoid cross-run collisions.
A fixed /tmp/aeload-ctx.tgz can be clobbered by parallel runs and produce incorrect build context.
Suggested fix
-echo "[build] packing $SRC"
-tar -C "$SRC" --exclude='.git' --exclude='harness/__pycache__' -czf /tmp/aeload-ctx.tgz .
+echo "[build] packing $SRC"
+CTX_TGZ="$(mktemp /tmp/aeload-ctx.XXXXXX.tgz)"
+trap 'rm -f "$CTX_TGZ"' EXIT
+tar -C "$SRC" --exclude='.git' --exclude='harness/__pycache__' -czf "$CTX_TGZ" .
echo "[build] cp to $PG dev-machine"
-labctl cp -m dev-machine /tmp/aeload-ctx.tgz "$PG:/tmp/aeload-ctx.tgz" </dev/null
+labctl cp -m dev-machine "$CTX_TGZ" "$PG:/tmp/aeload-ctx.tgz" </dev/null📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| tar -C "$SRC" --exclude='.git' --exclude='harness/__pycache__' -czf /tmp/aeload-ctx.tgz . | |
| echo "[build] cp to $PG dev-machine" | |
| labctl cp -m dev-machine /tmp/aeload-ctx.tgz "$PG:/tmp/aeload-ctx.tgz" </dev/null | |
| echo "[build] packing $SRC" | |
| CTX_TGZ="$(mktemp /tmp/aeload-ctx.XXXXXX.tgz)" | |
| trap 'rm -f "$CTX_TGZ"' EXIT | |
| tar -C "$SRC" --exclude='.git' --exclude='harness/__pycache__' -czf "$CTX_TGZ" . | |
| echo "[build] cp to $PG dev-machine" | |
| labctl cp -m dev-machine "$CTX_TGZ" "$PG:/tmp/aeload-ctx.tgz" </dev/null |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/e2e_test/adaptive_export_loadtest/harness/build_gen_image.sh` around
lines 15 - 18, The build script uses a fixed hardcoded path
`/tmp/aeload-ctx.tgz` for the temporary archive, which can be overwritten by
parallel test runs and cause incorrect build context. Replace the fixed
`/tmp/aeload-ctx.tgz` path with a dynamically generated unique temporary file
path (e.g., using mktemp or a similar mechanism that includes a timestamp or
random suffix). Update both occurrences of the path: the one in the tar command
that creates the archive and the one in the labctl cp command that copies it to
the dev-machine.
Comment on lines
+22
to
+49
| OUT=$(labctl ssh "$PG" -m dev-machine -- bash -s <<EOF 2>&1 || true | ||
| set -uo pipefail | ||
| . /tmp/keys.env | ||
| [[ -n "\${PX_API_KEY:-}" ]] || { echo "NO_PX_API_KEY"; exit 1; } | ||
| kubectl get ns pl >/dev/null 2>&1 || kubectl create ns pl | ||
| # secret (api-key + clickhouse-dsn) + SA — never echo the key (stdin yaml). | ||
| kubectl -n pl create secret generic pl-adaptive-export-secrets \ | ||
| --from-literal=pixie-api-key="\$PX_API_KEY" \ | ||
| --from-literal=clickhouse-dsn='${CH_DSN}' \ | ||
| --dry-run=client -o yaml | kubectl apply -f - >/dev/null | ||
| kubectl -n pl get sa pl-adaptive-export-service-account >/dev/null 2>&1 || kubectl -n pl create sa pl-adaptive-export-service-account | ||
| # PL_CLOUD_ADDR :443 fix (AE crashloops / 0 writes without it). | ||
| CUR=\$(kubectl -n pl get cm pl-cloud-config -o jsonpath='{.data.PL_CLOUD_ADDR}' 2>/dev/null || true) | ||
| if [[ -n "\$CUR" && "\$CUR" != *:* ]]; then | ||
| kubectl -n pl patch cm pl-cloud-config --type merge -p "{\"data\":{\"PL_CLOUD_ADDR\":\"\${CUR}:443\"}}" >/dev/null | ||
| echo "PL_CLOUD_ADDR patched -> \${CUR}:443" | ||
| fi | ||
| kubectl apply -f /tmp/ae-rendered.yaml >/dev/null | ||
| # Single-shot load-test mode (AFTER=5 < 30s refresh → one pull on any image; | ||
| # PUSH_REFRESH=-1 is the explicit equivalent on a rebuilt image). | ||
| kubectl -n pl set env ds/adaptive-export \ | ||
| ADAPTIVE_SKIP_APPLY=false ADAPTIVE_PUSH_PIXIE_ROWS=true \ | ||
| ADAPTIVE_PUSH_REFRESH_SEC=-1 ADAPTIVE_WINDOW_BEFORE_SEC=120 ADAPTIVE_WINDOW_AFTER_SEC=5 \ | ||
| EXPORT_MODE=auto >/dev/null | ||
| kubectl -n pl rollout status ds/adaptive-export --timeout=180s 2>&1 | tail -1 || echo "rollout slow" | ||
| echo "AE_PODS=\$(kubectl -n pl get pods -l name=adaptive-export --no-headers 2>/dev/null | grep -c Running)" | ||
| echo __AE_DEPLOYED__ | ||
| EOF |
There was a problem hiding this comment.
Make the remote deployment block fail-fast.
The SSH heredoc omits -e, so failed kubectl operations may be ignored and the script can still emit __AE_DEPLOYED__.
Suggested fix
-set -uo pipefail
+set -euo pipefail📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| OUT=$(labctl ssh "$PG" -m dev-machine -- bash -s <<EOF 2>&1 || true | |
| set -uo pipefail | |
| . /tmp/keys.env | |
| [[ -n "\${PX_API_KEY:-}" ]] || { echo "NO_PX_API_KEY"; exit 1; } | |
| kubectl get ns pl >/dev/null 2>&1 || kubectl create ns pl | |
| # secret (api-key + clickhouse-dsn) + SA — never echo the key (stdin yaml). | |
| kubectl -n pl create secret generic pl-adaptive-export-secrets \ | |
| --from-literal=pixie-api-key="\$PX_API_KEY" \ | |
| --from-literal=clickhouse-dsn='${CH_DSN}' \ | |
| --dry-run=client -o yaml | kubectl apply -f - >/dev/null | |
| kubectl -n pl get sa pl-adaptive-export-service-account >/dev/null 2>&1 || kubectl -n pl create sa pl-adaptive-export-service-account | |
| # PL_CLOUD_ADDR :443 fix (AE crashloops / 0 writes without it). | |
| CUR=\$(kubectl -n pl get cm pl-cloud-config -o jsonpath='{.data.PL_CLOUD_ADDR}' 2>/dev/null || true) | |
| if [[ -n "\$CUR" && "\$CUR" != *:* ]]; then | |
| kubectl -n pl patch cm pl-cloud-config --type merge -p "{\"data\":{\"PL_CLOUD_ADDR\":\"\${CUR}:443\"}}" >/dev/null | |
| echo "PL_CLOUD_ADDR patched -> \${CUR}:443" | |
| fi | |
| kubectl apply -f /tmp/ae-rendered.yaml >/dev/null | |
| # Single-shot load-test mode (AFTER=5 < 30s refresh → one pull on any image; | |
| # PUSH_REFRESH=-1 is the explicit equivalent on a rebuilt image). | |
| kubectl -n pl set env ds/adaptive-export \ | |
| ADAPTIVE_SKIP_APPLY=false ADAPTIVE_PUSH_PIXIE_ROWS=true \ | |
| ADAPTIVE_PUSH_REFRESH_SEC=-1 ADAPTIVE_WINDOW_BEFORE_SEC=120 ADAPTIVE_WINDOW_AFTER_SEC=5 \ | |
| EXPORT_MODE=auto >/dev/null | |
| kubectl -n pl rollout status ds/adaptive-export --timeout=180s 2>&1 | tail -1 || echo "rollout slow" | |
| echo "AE_PODS=\$(kubectl -n pl get pods -l name=adaptive-export --no-headers 2>/dev/null | grep -c Running)" | |
| echo __AE_DEPLOYED__ | |
| EOF | |
| OUT=$(labctl ssh "$PG" -m dev-machine -- bash -s <<EOF 2>&1 || true | |
| set -euo pipefail | |
| . /tmp/keys.env | |
| [[ -n "\${PX_API_KEY:-}" ]] || { echo "NO_PX_API_KEY"; exit 1; } | |
| kubectl get ns pl >/dev/null 2>&1 || kubectl create ns pl | |
| # secret (api-key + clickhouse-dsn) + SA — never echo the key (stdin yaml). | |
| kubectl -n pl create secret generic pl-adaptive-export-secrets \ | |
| --from-literal=pixie-api-key="\$PX_API_KEY" \ | |
| --from-literal=clickhouse-dsn='${CH_DSN}' \ | |
| --dry-run=client -o yaml | kubectl apply -f - >/dev/null | |
| kubectl -n pl get sa pl-adaptive-export-service-account >/dev/null 2>&1 || kubectl -n pl create sa pl-adaptive-export-service-account | |
| # PL_CLOUD_ADDR :443 fix (AE crashloops / 0 writes without it). | |
| CUR=\$(kubectl -n pl get cm pl-cloud-config -o jsonpath='{.data.PL_CLOUD_ADDR}' 2>/dev/null || true) | |
| if [[ -n "\$CUR" && "\$CUR" != *:* ]]; then | |
| kubectl -n pl patch cm pl-cloud-config --type merge -p "{\"data\":{\"PL_CLOUD_ADDR\":\"\${CUR}:443\"}}" >/dev/null | |
| echo "PL_CLOUD_ADDR patched -> \${CUR}:443" | |
| fi | |
| kubectl apply -f /tmp/ae-rendered.yaml >/dev/null | |
| # Single-shot load-test mode (AFTER=5 < 30s refresh → one pull on any image; | |
| # PUSH_REFRESH=-1 is the explicit equivalent on a rebuilt image). | |
| kubectl -n pl set env ds/adaptive-export \ | |
| ADAPTIVE_SKIP_APPLY=false ADAPTIVE_PUSH_PIXIE_ROWS=true \ | |
| ADAPTIVE_PUSH_REFRESH_SEC=-1 ADAPTIVE_WINDOW_BEFORE_SEC=120 ADAPTIVE_WINDOW_AFTER_SEC=5 \ | |
| EXPORT_MODE=auto >/dev/null | |
| kubectl -n pl rollout status ds/adaptive-export --timeout=180s 2>&1 | tail -1 || echo "rollout slow" | |
| echo "AE_PODS=\$(kubectl -n pl get pods -l name=adaptive-export --no-headers 2>/dev/null | grep -c Running)" | |
| echo __AE_DEPLOYED__ | |
| EOF |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/e2e_test/adaptive_export_loadtest/harness/deploy_ae.sh` around lines 22 -
49, The bash heredoc executed via labctl ssh in the deploy_ae.sh script uses set
-uo pipefail without the -e flag, which means failed kubectl commands are
silently ignored and the script continues executing, potentially emitting
__AE_DEPLOYED__ even when deployment failed. Add the -e flag to the set command
in the heredoc block (currently set -uo pipefail) to change it to set -euo
pipefail, which will cause the script to exit immediately on any command failure
and achieve fail-fast behavior.
| echo __AE_DEPLOYED__ | ||
| EOF | ||
| ) | ||
| rm -f /tmp/keys.env 2>/dev/null || true |
There was a problem hiding this comment.
Avoid deleting an unrelated local /tmp/keys.env.
This script never creates a local /tmp/keys.env; unconditional deletion can remove a user’s existing file.
Suggested fix
-rm -f /tmp/keys.env 2>/dev/null || true📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| rm -f /tmp/keys.env 2>/dev/null || true |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/e2e_test/adaptive_export_loadtest/harness/deploy_ae.sh` at line 51, The
script unconditionally deletes /tmp/keys.env on line 51 even though the script
itself never creates this file, which can unintentionally remove a user's
existing file. Remove the rm command that deletes /tmp/keys.env, or if cleanup
is necessary, ensure the script only removes temporary files it actually creates
by using a script-controlled temporary file location instead of relying on
shared system directories.
Comment on lines
+58
to
+59
| inj --ns aeload --pod "$filt" --rule R0001 --pid 1234 --comm java --event-time "$T" --same-time || true | ||
| inj --ns aeload --pod "$filt" --rule R0010 --pid 1234 --comm java --event-time "$T" --same-time || { echo "$rep,$EXP,,,,,,,INJECT_FAIL"|tee -a "$OUT"; continue; } |
There was a problem hiding this comment.
Shared root cause: critical injector failures are being masked, invalidating experiment semantics.
src/e2e_test/adaptive_export_loadtest/harness/exp_control.sh#L58-L59: require both E4 boundary-collision writes to succeed; otherwise mark rep asINJECT_FAILand continue.src/e2e_test/adaptive_export_loadtest/harness/exp_e8.sh#L58-L64: treat failed sustained-tick injection as explicit tick failure instead of allowing it to appear asSTALL.
📍 Affects 2 files
src/e2e_test/adaptive_export_loadtest/harness/exp_control.sh#L58-L59(this comment)src/e2e_test/adaptive_export_loadtest/harness/exp_e8.sh#L58-L64
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/e2e_test/adaptive_export_loadtest/harness/exp_control.sh` around lines 58
- 59, The issue spans two files where critical injector failures are being
masked. In src/e2e_test/adaptive_export_loadtest/harness/exp_control.sh at lines
58-59, the first inj command with rule R0001 uses || true which silently ignores
failures; remove this and require both the R0001 and R0010 inj commands to
succeed together so that any failure properly triggers the INJECT_FAIL case. In
src/e2e_test/adaptive_export_loadtest/harness/exp_e8.sh at lines 58-64, modify
the sustained-tick injection handling to treat failed injections as explicit
tick failures instead of allowing them to be masked or reported as STALL
conditions.
Comment on lines
+171
to
+175
| for i := 0; i < pgN; i++ { | ||
| var one int | ||
| if err := db.QueryRow("SELECT 1").Scan(&one); err != nil { | ||
| fatalf("pg query %d/%d failed: %v", i+1, pgN, err) | ||
| } |
There was a problem hiding this comment.
Add per-query timeout for PG statements.
Lines 171-175 issue DB queries without context deadlines; a stuck sink can block the generator indefinitely.
Suggested fix
for i := 0; i < pgN; i++ {
var one int
- if err := db.QueryRow("SELECT 1").Scan(&one); err != nil {
+ qctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+ err := db.QueryRowContext(qctx, "SELECT 1").Scan(&one)
+ cancel()
+ if err != nil {
fatalf("pg query %d/%d failed: %v", i+1, pgN, err)
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for i := 0; i < pgN; i++ { | |
| var one int | |
| if err := db.QueryRow("SELECT 1").Scan(&one); err != nil { | |
| fatalf("pg query %d/%d failed: %v", i+1, pgN, err) | |
| } | |
| for i := 0; i < pgN; i++ { | |
| var one int | |
| qctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) | |
| err := db.QueryRowContext(qctx, "SELECT 1").Scan(&one) | |
| cancel() | |
| if err != nil { | |
| fatalf("pg query %d/%d failed: %v", i+1, pgN, err) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/e2e_test/adaptive_export_loadtest/tools/loadgen/cmd/cleanloadgen/main.go`
around lines 171 - 175, The database query in the loop starting at line 171
(where db.QueryRow("SELECT 1") is called) executes without a context deadline,
which can cause the generator to block indefinitely if the database connection
hangs. Add a context with an appropriate timeout to the QueryRow call to ensure
queries do not hang indefinitely. Use context.WithTimeout or
context.WithDeadline to create a time-bounded context before executing each
db.QueryRow operation in the loop.
Comment on lines
+26
to
+29
| srv := &http.Server{Addr: addr, Handler: mux} | ||
| if err := srv.ListenAndServe(); err != nil { | ||
| panic(err) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Harden the HTTP server with timeouts and graceful shutdown error handling.
Use explicit timeouts and ignore http.ErrServerClosed to avoid avoidable stalls/panics.
Suggested fix
+import "errors"
...
- srv := &http.Server{Addr: addr, Handler: mux}
- if err := srv.ListenAndServe(); err != nil {
+ srv := &http.Server{
+ Addr: addr,
+ Handler: mux,
+ ReadHeaderTimeout: 5 * time.Second,
+ ReadTimeout: 10 * time.Second,
+ WriteTimeout: 10 * time.Second,
+ IdleTimeout: 30 * time.Second,
+ }
+ if err := srv.ListenAndServe(); err != nil && !errors.Is(err, http.ErrServerClosed) {
panic(err)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| srv := &http.Server{Addr: addr, Handler: mux} | |
| if err := srv.ListenAndServe(); err != nil { | |
| panic(err) | |
| } | |
| srv := &http.Server{ | |
| Addr: addr, | |
| Handler: mux, | |
| ReadHeaderTimeout: 5 * time.Second, | |
| ReadTimeout: 10 * time.Second, | |
| WriteTimeout: 10 * time.Second, | |
| IdleTimeout: 30 * time.Second, | |
| } | |
| if err := srv.ListenAndServe(); err != nil && !errors.Is(err, http.ErrServerClosed) { | |
| panic(err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/e2e_test/adaptive_export_loadtest/tools/loadgen/cmd/httpsink/main.go`
around lines 26 - 29, The HTTP server initialization in the main function is
missing timeout configuration and has improper error handling. Add ReadTimeout,
WriteTimeout, and IdleTimeout fields to the http.Server struct to prevent
resource exhaustion and stalls. Additionally, modify the error handling for
srv.ListenAndServe() to check if the error is http.ErrServerClosed and only
panic on actual errors, since http.ErrServerClosed is an expected error that
occurs during graceful shutdown and should not cause a panic.
Comment on lines
+11
to
+12
| RUN go build -trimpath -ldflags="-s -w" -o /out/cleanloadgen ./cmd/cleanloadgen | ||
| RUN go build -trimpath -ldflags="-s -w" -o /out/httpsink ./cmd/httpsink |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consolidate consecutive RUN instructions to reduce layer count.
Lines 11–12 can be combined into a single RUN statement to reduce the intermediate image layer.
♻️ Proposed consolidation
-RUN go build -trimpath -ldflags="-s -w" -o /out/cleanloadgen ./cmd/cleanloadgen
-RUN go build -trimpath -ldflags="-s -w" -o /out/httpsink ./cmd/httpsink
+RUN go build -trimpath -ldflags="-s -w" -o /out/cleanloadgen ./cmd/cleanloadgen && \
+ go build -trimpath -ldflags="-s -w" -o /out/httpsink ./cmd/httpsink🧰 Tools
🪛 Hadolint (2.14.0)
[info] 12-12: Multiple consecutive RUN instructions. Consider consolidation.
(DL3059)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/e2e_test/adaptive_export_loadtest/tools/loadgen/Dockerfile` around lines
11 - 12, Two consecutive RUN instructions for the go build commands
(cleanloadgen and httpsink) are creating unnecessary intermediate layers.
Consolidate these two separate RUN statements into a single RUN instruction by
joining them with the && operator and using a backslash for line continuation.
This reduces the Docker image layer count and improves build efficiency.
| @@ -0,0 +1,5 @@ | |||
| module aeload | |||
|
|
|||
| go 1.22 | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Go version declarations in repo =="
fd -HI 'go.mod|go.work|Dockerfile|.*\.tool-versions|.*\.yaml|.*\.yml' -t f | while read -r f; do
rg -n --no-heading \
'(^go [0-9]+\.[0-9]+$)|(^toolchain go[0-9]+\.[0-9]+(\.[0-9]+)?$)|(golang:[0-9]+\.[0-9]+(\.[0-9]+)?)|(go-version:)|(GO_VERSION)' \
"$f" || true
doneRepository: k8sstormcenter/pixie
Length of output: 561
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locating Dockerfile with golang:1.22-bookworm =="
rg -n 'golang:1.22-bookworm' --type dockerfile
echo -e "\n== Checking if loadgen module is built in CI =="
fd -t f -e 'yml' -e 'yaml' | xargs rg -l 'loadgen|adaptive_export' || echo "No CI workflows found referencing loadgen"
echo -e "\n== Contents of loadgen go.mod =="
cat -n src/e2e_test/adaptive_export_loadtest/tools/loadgen/go.mod 2>/dev/null || echo "File not accessible directly, checking structure..."
echo -e "\n== Checking for toolchain declarations in go.mod files =="
find . -name go.mod -type f | while read f; do
echo "=== $f ==="
head -5 "$f"
doneRepository: k8sstormcenter/pixie
Length of output: 169
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Finding all Dockerfiles =="
fd -H 'Dockerfile' -t f | head -20
echo -e "\n== Searching for golang:1.22-bookworm in all files =="
rg 'golang:1.22-bookworm' -n
echo -e "\n== Checking go.mod files for toolchain declarations =="
fd 'go\.mod' -t f | while read f; do
if grep -q 'toolchain\|go 1\.22' "$f"; then
echo "=== $f ==="
cat -n "$f" | head -10
fi
done
echo -e "\n== Checking CI workflows for loadgen/adaptive_export references =="
fd '\.(yml|yaml)$' -t f -H | xargs grep -l 'loadgen\|adaptive_export' 2>/dev/null || echo "No matches found"Repository: k8sstormcenter/pixie
Length of output: 1728
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Full content of loadgen Dockerfile =="
cat -n src/e2e_test/adaptive_export_loadtest/tools/loadgen/Dockerfile
echo -e "\n== Searching for GitHub Actions workflows =="
fd '\.github/workflows' -t f || echo "No .github directory found"
echo -e "\n== Searching for any CI/build scripts mentioning loadgen =="
rg -l 'loadgen|aeload' --type sh --type py 2>/dev/null | head -10
echo -e "\n== Checking if there's a build script or Makefile =="
fd -H '(Makefile|build.*\.sh|.*docker.*build)' -t f | xargs grep -l 'loadgen\|aeload' 2>/dev/null || echo "No build scripts found for loadgen"Repository: k8sstormcenter/pixie
Length of output: 1718
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Content of build_gen_image.sh =="
cat -n src/e2e_test/adaptive_export_loadtest/harness/build_gen_image.sh
echo -e "\n== Checking for go.work file (workspace toolchain pinning) =="
fd 'go\.work' -t f
echo -e "\n== Checking loadgen directory structure =="
find src/e2e_test/adaptive_export_loadtest/tools/loadgen -type f | head -20
echo -e "\n== Checking if any other go.mod in repo pins patch level =="
fd 'go\.mod' -t f -x grep -H 'toolchain go' 2>/dev/null | head -5Repository: k8sstormcenter/pixie
Length of output: 2092
Pin the Go 1.22 patch level in the loadgen Dockerfile.
The build uses unpinned golang:1.22-bookworm, which pulls whatever the latest 1.22 patch happens to be available at build time. Pin to a specific patch by adding a digest or use a toolchain go1.22.x directive in go.mod to ensure consistent patch levels across builds and mitigate exposure to any stdlib advisories in older 1.22 patch releases.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/e2e_test/adaptive_export_loadtest/tools/loadgen/go.mod` at line 3, The
go.mod file currently specifies an unpinned version with "go 1.22", which allows
any patch release to be used during builds. Add a toolchain directive in go.mod
with a specific patch version such as "toolchain go1.22.x" (replacing x with the
actual patch number) to pin the exact Go patch level and ensure consistent,
reproducible builds while mitigating exposure to any stdlib advisories in older
1.22 patch releases.
Source: Linters/SAST tools
|
Proper AE release shipped — GH release: https://github.com/k8sstormcenter/pixie/releases/tag/release/vizier/v0.14.19-aeprod-clean4 kubectl -n pl set image deployment/adaptive-export \
adaptive-export=ghcr.io/k8sstormcenter/vizier-adaptive_export_image:0.14.19-aeprod-clean4What clean4 contains (on top of clean3
Trivy SBOM uploaded; helm chart skipped (expected for pre-release tag). Drop |
Author
|
Build request: fresh Note: the AE binary is unchanged since |
Author
Build request —
|
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/vizier/services/adaptive_export/internal/clickhouse/apply_test.go (1)
60-68:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix stale “last DDL” expectation in
TestApply_ExecutesEveryOperatorOwnedTable.
OperatorOwnedTablesnow ends withae_reconcile, but this test still expects the final DDL to targettrigger_watermark. That makes the test contract inconsistent with the current apply order.Proposed patch
- // and that the LAST call is for trigger_watermark (the newest - // operator-owned table, registered after adaptive_attribution). + // and that the LAST call is for ae_reconcile (registered after + // trigger_watermark). @@ - if !strings.Contains(bodies[len(bodies)-1], "forensic_db.trigger_watermark") { - t.Fatalf("last DDL not for trigger_watermark; got: %s", bodies[len(bodies)-1]) + if !strings.Contains(bodies[len(bodies)-1], "forensic_db.ae_reconcile") { + t.Fatalf("last DDL not for ae_reconcile; got: %s", bodies[len(bodies)-1]) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vizier/services/adaptive_export/internal/clickhouse/apply_test.go` around lines 60 - 68, The test TestApply_ExecutesEveryOperatorOwnedTable has a stale expectation for the last DDL call. Since OperatorOwnedTables now ends with ae_reconcile instead of trigger_watermark, update the final assertion that checks bodies[len(bodies)-1] to dynamically reference the last element of OperatorOwnedTables using OperatorOwnedTables[len(OperatorOwnedTables)-1] instead of the hard-coded string trigger_watermark, making the test contract consistent with the actual apply order.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/e2e_test/adaptive_export_loadtest/harness/ae_vs_all.sh`:
- Around line 11-14: The script retrieves the frontend service cluster IP into
the FE variable on line 11 but does not validate that it was successfully
resolved before using it in the subsequent loader pod commands. Add a validation
check after the FE variable assignment to ensure FE is not empty; if it is
empty, the script should exit with an appropriate error message. This prevents
the for loops that create the cl-1 through cl-5 pods from spawning with
malformed URLs containing an empty FE value, which would invalidate the test
measurement window.
- Around line 6-19: The script uses set -uo pipefail but is missing the -e flag,
which means it will continue execution even when commands fail (especially when
errors are suppressed with >/dev/null 2>&1 redirections). This causes the script
to proceed to the next arm iteration after a failed rollout in the run_arm
function, producing misleading results. Add the -e flag to the set command at
the beginning of the script (change set -uo pipefail to set -ueo pipefail) to
enable immediate exit on any command failure and ensure the script fails fast on
control-plane command failures.
In `@src/e2e_test/adaptive_export_loadtest/harness/exp_datavolume_extreme.sh`:
- Around line 16-33: The script sets `set -uo pipefail` but omits the `-e` flag,
which means failures in setup commands, queries, or later integrity checks can
be silently masked while the script continues executing and produces output. Add
the `-e` flag to the set command (the line currently reading `set -uo pipefail`)
to enable exit-on-error behavior, ensuring the script terminates immediately
when any command fails and prevents false "pass" signals from masked failures.
In `@src/vizier/services/adaptive_export/internal/clickhouse/schema.sql`:
- Around line 471-485: The ae_reconcile table definition lacks a TTL (Time To
Live) retention policy, which causes unbounded storage growth for this
append-only table during extended reconcile operations. Add a TTL clause to the
CREATE TABLE statement for ae_reconcile that automatically expires and deletes
old records based on the ts column timestamp, using an appropriate retention
interval (e.g., INTERVAL 7 DAY or similar) to balance operational needs with
storage management.
In `@src/vizier/services/adaptive_export/internal/sink/clickhouse.go`:
- Around line 382-433: The Record method in ClickHouseHTTP performs a
synchronous HTTP POST request that blocks the calling hot path
(scanner/passthrough/controller) on ClickHouse latency or failures. Since the
reconciliation is best-effort and errors are already swallowed, move the HTTP
request execution off the critical path by launching it asynchronously in a
background goroutine. The Record method should return immediately after queuing
the work to be sent asynchronously, ensuring that ClickHouse outages cannot
stall the pull loops.
---
Outside diff comments:
In `@src/vizier/services/adaptive_export/internal/clickhouse/apply_test.go`:
- Around line 60-68: The test TestApply_ExecutesEveryOperatorOwnedTable has a
stale expectation for the last DDL call. Since OperatorOwnedTables now ends with
ae_reconcile instead of trigger_watermark, update the final assertion that
checks bodies[len(bodies)-1] to dynamically reference the last element of
OperatorOwnedTables using OperatorOwnedTables[len(OperatorOwnedTables)-1]
instead of the hard-coded string trigger_watermark, making the test contract
consistent with the actual apply order.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: adfd8211-0bb2-4a6d-94fd-0d0297b8a4ab
📒 Files selected for processing (15)
src/e2e_test/adaptive_export_loadtest/harness/ae_vs_all.shsrc/e2e_test/adaptive_export_loadtest/harness/exp_datavolume_extreme.shsrc/e2e_test/adaptive_export_loadtest/harness/exp_pipeline_reconcile.shsrc/e2e_test/adaptive_export_loadtest/harness/exp_row_reconcile.shsrc/vizier/services/adaptive_export/cmd/main.gosrc/vizier/services/adaptive_export/internal/clickhouse/apply.gosrc/vizier/services/adaptive_export/internal/clickhouse/apply_test.gosrc/vizier/services/adaptive_export/internal/clickhouse/ddl.gosrc/vizier/services/adaptive_export/internal/clickhouse/schema.sqlsrc/vizier/services/adaptive_export/internal/controller/controller.gosrc/vizier/services/adaptive_export/internal/passthrough/passthrough.gosrc/vizier/services/adaptive_export/internal/passthrough/reconcile_test.gosrc/vizier/services/adaptive_export/internal/reconcile/reconcile.gosrc/vizier/services/adaptive_export/internal/sink/clickhouse.gosrc/vizier/services/adaptive_export/internal/streaming/scanner.go
Comment on lines
+471
to
+485
| CREATE TABLE IF NOT EXISTS forensic_db.ae_reconcile ( | ||
| ts DateTime64(9, 'UTC'), | ||
| mode String, | ||
| table_name String, | ||
| namespace String, | ||
| pod String, | ||
| win_start DateTime64(9, 'UTC'), | ||
| win_end DateTime64(9, 'UTC'), | ||
| read_count Int64, | ||
| wrote_count Int64, | ||
| write_err String, | ||
| hostname String | ||
| ) ENGINE = MergeTree | ||
| PARTITION BY toYYYYMMDD(ts) | ||
| ORDER BY (table_name, ts); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add retention to ae_reconcile to prevent unbounded growth.
ae_reconcile is append-only and currently has no TTL. During extended reconcile runs this can accumulate quickly and create avoidable storage pressure.
Suggested DDL adjustment
) ENGINE = MergeTree
PARTITION BY toYYYYMMDD(ts)
- ORDER BY (table_name, ts);
+ ORDER BY (table_name, ts)
+ TTL ts + INTERVAL 14 DAY DELETE
+ SETTINGS ttl_only_drop_parts = 1;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| CREATE TABLE IF NOT EXISTS forensic_db.ae_reconcile ( | |
| ts DateTime64(9, 'UTC'), | |
| mode String, | |
| table_name String, | |
| namespace String, | |
| pod String, | |
| win_start DateTime64(9, 'UTC'), | |
| win_end DateTime64(9, 'UTC'), | |
| read_count Int64, | |
| wrote_count Int64, | |
| write_err String, | |
| hostname String | |
| ) ENGINE = MergeTree | |
| PARTITION BY toYYYYMMDD(ts) | |
| ORDER BY (table_name, ts); | |
| CREATE TABLE IF NOT EXISTS forensic_db.ae_reconcile ( | |
| ts DateTime64(9, 'UTC'), | |
| mode String, | |
| table_name String, | |
| namespace String, | |
| pod String, | |
| win_start DateTime64(9, 'UTC'), | |
| win_end DateTime64(9, 'UTC'), | |
| read_count Int64, | |
| wrote_count Int64, | |
| write_err String, | |
| hostname String | |
| ) ENGINE = MergeTree | |
| PARTITION BY toYYYYMMDD(ts) | |
| ORDER BY (table_name, ts) | |
| TTL ts + INTERVAL 14 DAY DELETE | |
| SETTINGS ttl_only_drop_parts = 1; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/vizier/services/adaptive_export/internal/clickhouse/schema.sql` around
lines 471 - 485, The ae_reconcile table definition lacks a TTL (Time To Live)
retention policy, which causes unbounded storage growth for this append-only
table during extended reconcile operations. Add a TTL clause to the CREATE TABLE
statement for ae_reconcile that automatically expires and deletes old records
based on the ts column timestamp, using an appropriate retention interval (e.g.,
INTERVAL 7 DAY or similar) to balance operational needs with storage management.
Author
F1 (10k query-cap) fix — live-verified on aeprod11 ✅The
RCA + earlier confounds (placeholder-secret/auth, not a write bug) documented; |
entlein
commented
Jun 20, 2026
|
|
||
| load("@io_bazel_rules_go//go:def.bzl", "go_library") | ||
| load("@px//bazel:pl_build_system.bzl", "pl_go_binary") | ||
| load("//bazel:pl_build_system.bzl", "pl_go_binary") |
Comment on lines
+116
to
+154
| if !decode(r, &req) || req.Pod == "" { | ||
| w.WriteHeader(http.StatusBadRequest) | ||
| return | ||
| } | ||
| s.set.Upsert(req.key(), time.Unix(req.TEnd, 0)) | ||
| w.WriteHeader(http.StatusAccepted) | ||
| } | ||
|
|
||
| func (s *Server) handleStop(w http.ResponseWriter, r *http.Request) { | ||
| if r.Method != http.MethodPost { | ||
| w.WriteHeader(http.StatusMethodNotAllowed) | ||
| return | ||
| } | ||
| var req targetReq | ||
| if !decode(r, &req) || req.Pod == "" { | ||
| w.WriteHeader(http.StatusBadRequest) | ||
| return | ||
| } | ||
| s.set.Remove(req.key()) | ||
| w.WriteHeader(http.StatusAccepted) | ||
| } | ||
|
|
||
| func (s *Server) handleQuery(w http.ResponseWriter, r *http.Request) { | ||
| if r.Method != http.MethodPost { | ||
| w.WriteHeader(http.StatusMethodNotAllowed) | ||
| return | ||
| } | ||
| if s.runner == nil { | ||
| w.WriteHeader(http.StatusNotImplemented) | ||
| return | ||
| } | ||
| var req queryReq | ||
| if !decode(r, &req) || req.Pod == "" || req.Table == "" || req.QueryID == "" { | ||
| w.WriteHeader(http.StatusBadRequest) | ||
| return | ||
| } | ||
| err := s.runner.OrderQuery(req.target(), req.Table, | ||
| time.Unix(req.Window[0], 0), time.Unix(req.Window[1], 0), req.QueryID) | ||
| if err != nil { |
Comment on lines
+110
to
+159
| func (s *Server) handleStart(w http.ResponseWriter, r *http.Request) { | ||
| if r.Method != http.MethodPost { | ||
| w.WriteHeader(http.StatusMethodNotAllowed) | ||
| return | ||
| } | ||
| var req startReq | ||
| if !decode(r, &req) || req.Pod == "" { | ||
| w.WriteHeader(http.StatusBadRequest) | ||
| return | ||
| } | ||
| s.set.Upsert(req.key(), time.Unix(req.TEnd, 0)) | ||
| w.WriteHeader(http.StatusAccepted) | ||
| } | ||
|
|
||
| func (s *Server) handleStop(w http.ResponseWriter, r *http.Request) { | ||
| if r.Method != http.MethodPost { | ||
| w.WriteHeader(http.StatusMethodNotAllowed) | ||
| return | ||
| } | ||
| var req targetReq | ||
| if !decode(r, &req) || req.Pod == "" { | ||
| w.WriteHeader(http.StatusBadRequest) | ||
| return | ||
| } | ||
| s.set.Remove(req.key()) | ||
| w.WriteHeader(http.StatusAccepted) | ||
| } | ||
|
|
||
| func (s *Server) handleQuery(w http.ResponseWriter, r *http.Request) { | ||
| if r.Method != http.MethodPost { | ||
| w.WriteHeader(http.StatusMethodNotAllowed) | ||
| return | ||
| } | ||
| if s.runner == nil { | ||
| w.WriteHeader(http.StatusNotImplemented) | ||
| return | ||
| } | ||
| var req queryReq | ||
| if !decode(r, &req) || req.Pod == "" || req.Table == "" || req.QueryID == "" { | ||
| w.WriteHeader(http.StatusBadRequest) | ||
| return | ||
| } | ||
| err := s.runner.OrderQuery(req.target(), req.Table, | ||
| time.Unix(req.Window[0], 0), time.Unix(req.Window[1], 0), req.QueryID) | ||
| if err != nil { | ||
| w.WriteHeader(http.StatusBadGateway) | ||
| return | ||
| } | ||
| w.WriteHeader(http.StatusAccepted) | ||
| } |
| func TestTick_ReconcileRecordsReadVsWrote(t *testing.T) { | ||
| rec := &capRec{} | ||
| loop := New( | ||
| tableQuerier{n: map[string]int{ |
|
|
||
| // NewClient dials the Pixie cloud and authenticates with apiKey via | ||
| // the per-call metadata header. | ||
| func NewClient(ctx context.Context, apiKey string, cloudAddr string) (*Client, error) { |
Author
There was a problem hiding this comment.
why exactly do we need the px-client? is that only to toggle the plugin system?
|
|
||
| c := &Client{ | ||
| cloudAddr: cloudAddr, | ||
| ctx: metadata.AppendToOutgoingContext(ctx, "pixie-api-key", apiKey), |
Author
There was a problem hiding this comment.
do we really need the key itself and not a valid JWT? check how other pixie services authenticate and use the identical method
| // behaviour of fmt.Sprintf-style string builders is what the bench | ||
| // quantifies — informs whether sync.Pool'd strings.Builder would pay | ||
| // off if QueryFor turns up in CPU profiles. | ||
|
|
Author
There was a problem hiding this comment.
check that this test for query injections, that would falsify any exports
This was referenced Jun 20, 2026
Author
AE ExtremeC data-volume NFR (aeprod18)Test cluster, 12-replica benign loadgen (User SBoB → not in the active set) + log4shell, 240 s/arm. AE passthrough (writes all pods) vs streaming (writes only the active-set). The active set held only the attack pods (attacker/backend/observer/postgres).
|
entlein
added a commit
that referenced
this pull request
Jun 21, 2026
The dx image build pipeline lives in entlein/dx itself (PR #53, branch feat/bazel-release): bazel-based with @px external pin to pixie's ae-prod tip, pushes to docker.io/entlein/dx-daemon on a release/dx/v* tag in the dx repo. The pixie-side buildx workflow this reverts duplicated that intent in the wrong repo + the wrong build system (docker buildx instead of bazel + pl_go_image macros) + the wrong registry (ghcr.io/k8sstormcenter instead of docker.io/entlein).
entlein
pushed a commit
that referenced
this pull request
Jun 21, 2026
Finish the harness consolidation #53 started: adopt exp_matrix.sh (canonical ALL-vs-DX reduction matrix) + nfr.sh (throughput/mem/verdict-latency) as the single runners; cut the overlapping variants (ae_vs_all, exp_datavolume_extreme, exp_dx_steering_reduction, exp_ae_nfr_benchmark, exp_pipeline_reconcile) and the superseded standalone setup (deploy_ae, build_gen_image). README rewritten as the single 'how to run' source: two families — fixture-isolation (run.sh + E-series) and live-attack e2e (log4shell_fire → exp_matrix → nfr → exp_row_reconcile). Add e2e_log4shell_soc.yaml: k3s + full SOC stack (Pixie/kubescape/ClickHouse/AE/dx via k8sstormcenter/soc) on the oracle runner; asserts every canonical harness script runs + dx rules in; profiles dx in real life (pprof CPU/heap + verdict latency, uploaded). Uses existing repo secrets.
entlein
pushed a commit
that referenced
this pull request
Jun 21, 2026
…ntainer_images BUILD This file is unrelated to adaptive_export — the only change was buildifier alphabetizing container_type/bazel_sdk_versions (no functional change). Restore main's version to keep #53's diff to real AE changes.
Eviction-RCA finding (PR #63 NFR campaign): AE had NO memory limit (only cpu 300m) and was CPU-pinned at 300m under concurrent passthrough. AE measured tiny (16-38Mi steady), but the raised 1M-row passthrough cap can spike, so cap at 1Gi so AE can never memory-pressure a node; raise cpu limit 300m->1 core (was throttling). NOTE node evictions were NOT AE/OOM — node-01 went NotReady (network/heartbeat); the memory consumer is PEM (1365Mi, OOMs at the 2Gi default). Signed-off-by: entlein <einentlein@gmail.com>
Root cause of the recurring "AE unauthenticated / writes 0 / crashloop" reverts: kustomization.yaml bundled adaptive_export_secrets.yaml (placeholder pixie-api-key) with the role+deployment, so EVERY infra re-apply (make log4j) clobbered the real key that ae-auth had written. Separation of concerns: remove the secret from the kustomization — infra (role+deployment) stays re-appliable; the secret holds real creds and is owned solely by `make ae-auth`, created once, never touched by infra re-applies. Secret manifest kept as a hand-applied seed-only template (documented). Signed-off-by: entlein <einentlein@gmail.com>
The DX-steered arm was failing because the harness only fired stage-1 (JNDI/LDAP). That generates ldap-egress but NO kubescape R0001 → backend never flagged → DX no case → indeterminate → AE steers wrong/no pods. R0001 comes from stage-2 (post- exploitation exec). fire() now does stage-1 (JNDI) + stage-2 (whoami/shadow/token/ getent in the backend) → kubescape R0001+R0006 → DX rules backend MALIGNANT → backend enters AE activeSet → reduction is measurable. Verified live: DX evidence unexpected-spawn+sensitive-file-read → verdict ruled_in generic=MALIGNANT. Signed-off-by: entlein <einentlein@gmail.com>
…dd DX-steering diagnostics
Standing terminology rule: allowlist/blocklist, never whitelist/blacklist.
Pure rename (no behavior change) of the rev-3 streaming filter:
FilterModeWhitelist → FilterModeAllowlist
MaxWhitelistSize → MaxAllowlistSize
ADAPTIVE_STREAM_MAX_WHITELIST → ADAPTIVE_STREAM_MAX_ALLOWLIST (env)
mode=whitelist log string → mode=allowlist
plus all comments/identifiers/tests in streaming, activeset, cmd/main.
DX-steering diagnostics (the reason DX-arm-writes-0 has been hard to RCA —
we could not tell "empty ActiveSet" from "broker returned 0 rows"):
- scanner: log the empty-allowlist short-circuit (was silent) so an
empty ActiveSet is visible in logs, distinct from "query completed rows=0".
- FilterUpdater: emitted-filter log Debug→Info so the steered pod count
per ActiveSet change is visible without debug logging.
NOTE: ADAPTIVE_STREAM_MAX_WHITELIST env renamed → tooling that sets the old
name must switch to ADAPTIVE_STREAM_MAX_ALLOWLIST.
Signed-off-by: entlein <einentlein@gmail.com>
The Pixie dx_evidence_graph UI reads dx_attack_graph via px.DataFrame clickhouse_dsn, whose query template hardcodes event_time + hostname and ORDER BY event_time. A table without those columns fails 'Unknown identifier event_time'; a table created by hand (local, not via the operator) isn't globally registered. Fix: make AE own it like the other forensic tables. - schema.sql: dx_attack_graph DDL with event_time(UInt64 nanos) + hostname, edge columns, fromUnixTimestamp64Nano partition/TTL (nanos-correct). - KnownTables + OperatorOwnedTables: register it so Apply creates it at boot. - apply_test: assert last-applied DDL == last OperatorOwnedTables entry (robust to appended operator tables) instead of hardcoding trigger_watermark. go test ./.../clickhouse green. Signed-off-by: entlein <einentlein@gmail.com>
Pixie's clickhouse_dsn type mapper reads UInt8 as BOOLEAN and does not handle UInt16/UInt32/Float32 -> px fails with 'Column[N] given incorrect type' rendering the dx_evidence_graph. weight/max_severity/num_findings -> Int64, confidence -> Float64 (map cleanly to INT64/FLOAT64). Verified live: px run returns all 6 edges with every column. event_time stays UInt64 (matches kubescape_logs, which px reads). Signed-off-by: entlein <einentlein@gmail.com>
…canner buildPxL The DX/streaming arm silently capped each per-table pull at Pixie's default 10000-row limit while the passthrough/ALL arm (pxl.CompilePassthrough / QueryFor) already raises it to 1,000,000 via the broker's #px:set query flag. Validated live on 6a33dac0: a single streaming http_events pull returned exactly rows=10000 (the cap). Left unfixed this UNDER-counts the DX arm and OVERSTATES the DX-vs-ALL volume reduction. Prepend the same #px:set directive to the streaming scanner's PxL so both arms are uncapped and comparable. Signed-off-by: entlein <einentlein@gmail.com>
Adds the rule-ins-only view (condition != '') to the canonical schema.sql, registers it in KnownTables + OperatorOwnedTables (after dx_attack_graph), and teaches DDL() to extract CREATE VIEW headers. AE now creates it on boot so the dx_evidence_graph UI's default malicious-only read is standard, not a per-rig manual step. Tests updated. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: entlein <einentlein@gmail.com>
dx POSTs a JSON array of edges to /dx/attack_graph; AE writes them to forensic_db.dx_attack_graph via JSONEachRow (Applier.WriteAttackGraph). Wired in main.go when CONTROL_ADDR is set; 501 if the sink is unset. This is the AE half of the dx->AE->CH attack-graph write path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: entlein <einentlein@gmail.com>
…e, fix stale tests
PR-53 review follow-ups (see review summary in conversation):
1. License headers added to 17 src/e2e_test/adaptive_export_loadtest/
harness/*.{sh,py} scripts. Matches the convention every other
src/e2e_test/*/sh in pixie already follows.
2. Dead code removed (was unreachable per golang.org/x/tools/cmd/deadcode):
- internal/script/script.go: IsClickHouseScript, IsScriptForCluster,
GetActions, getScriptName, getInterval, templateScript, plus the
ScriptConfig and ScriptActions types. The cron-script sync flow they
served was replaced by the streaming model; only the Script and
ScriptDefinition types remain.
- internal/pixie/pixie.go: Client.GetPresetScripts (replaced by
builtinPresetScripts() inline in cmd/main.go).
- internal/streaming/{supervisor,writer}.go: SupervisorStats,
TableStats, Stats types + Supervisor.Stats and BatchWriter.Stats
methods (no production reader). Atomic counters dropped; the
existing flush log preserves the per-flush summary.
3. Stale passthrough tests fixed.
TestLoop_DefaultsTablesToPixieTables and TestNew_AppliesDefaults
asserted len(clickhouse.PixieTables()) == 13 but passthrough.New
strips excludedTables (http2_messages.beta), yielding 12. Tests now
compare against filterExcluded(clickhouse.PixieTables()) and add an
extra assertion that excluded tables were not written.
4. ClickHouse HTTP client consolidation: new internal/chhttp/ package
collapses three near-identical HTTP CH clients
(clickhouse.Applier, sink.ClickHouseHTTP, trigger.ClickHouseWatermarkStore)
into one. Centralises endpoint validation, basic-auth header,
30s default timeout, fail-loud INSERT settings (4 CH input_format
knobs), and the X-ClickHouse-Summary read path. The 4 INSERT
call sites in the three callers all route through chhttp.Client.Insert
now; SELECT through Query or QueryStream (the latter preserves the
QueryActive streaming behaviour). Net code: -200 LOC across the
three callers plus a 200-LOC chhttp package with its own tests.
5. Pixie service scaffold wired into cmd/main.go:
services.SetupService("adaptive-export", 50900) +
services.SetupSSLClientFlags() + services.PostFlagSetupAndParse() +
services.SetupServiceLogging(). Matches the pattern every other
pixie Go service uses. CheckServiceFlags() is deliberately skipped
(AE does not run a TLS gRPC server). Existing AE env-var reads
(ADAPTIVE_*) are untouched and still authoritative for tuning knobs.
All 14 adaptive_export internal packages pass go test (1 new chhttp,
13 unchanged), including the 3 differential oracle tests in
pxl/compile_test.go. arc lint OKAY for everything except 3 pre-existing
findings unrelated to this commit (loadgen has its own go.mod; one
QF1001 De Morgan's-law nit in reconcile_test.go from c9f19b6).
Signed-off-by: entlein <einentlein@gmail.com>
…der) Signed-off-by: entlein <einentlein@gmail.com>
User flagged on review 4536971862: - cmd/BUILD.bazel:18 dropped the '@px' external-workspace prefix on pl_build_system.bzl load. Restored — the standalone AE build pulls pixie as @px and needs the qualifier. Lint cleanup (PR-53 scope, no production code touched outside renames): - chhttp/chhttp_test.go: errcheck on two w.Write calls + gofumpt on the gotSettings declaration. - passthrough/reconcile_test.go: staticcheck QF1001 (De Morgan's law) on the conn_stats sink-drop assertion. - script/script.go: rename ScriptId -> ScriptID (ST1003). Propagated to pixie/pixie.go and cmd/main.go callsites. - internal/e2e/BUILD.bazel and internal/trigger/BUILD.bazel: gazelle drift — adding loadtest_test.go and clickhouse_internal_test.go to srcs. - k8s/00-sinks.yaml + gen-pod.tmpl.yaml: yamllint compliance — document-start marker, dedent sequence items per .yamllint indent-sequences=false, tighten flow-mapping spaces, collapse multi-space after commas. YAML semantics unchanged. - harness/stats.py: flake8 E501 — split a long line into two. Final arc lint state on PR-53 file scope: 0 Errors, 14 Warnings (SHELLCHECK SC2155/SC2086 in pre-existing harness scripts from ae7b86f, not introduced or modified by this commit). Pre-existing loadgen typecheck failures (separate go.mod) are unaffected. Signed-off-by: entlein <einentlein@gmail.com>
…bit items User review 4536971862: #2 passthrough/reconcile_test.go — strengthened with two new tests that exercise the FULL chain (loop → real sink.ClickHouseHTTP → httptest CH endpoint → reconcile recorder). TestTick_ReconcileCatchesCHSilentDrop mimics CH's X-ClickHouse-Summary silent-drop shape (200 OK, written_rows=0) and asserts the loop records WroteCount=0 with a silent-drop attribution — the exact R6 (sink-layer loss) regression the instrument exists to detect. TestTick_ReconcileAttributesCHFailureCorrectly covers the 500-response branch. The pre-existing in-process-fake test stays as the wiring check. #4 pixie/pixie.go — added a long comment justifying the API-key auth choice. pixie.Client targets the Pixie CLOUD (cloudpb's PluginService), whose auth interceptor accepts pixie-api-key and rejects JWT service tokens (those are for INSIDE-cluster vizier services). The pixieapi.Adapter that talks to vizier directly already uses JWT via jwtutils.GenerateJWTForService — same pattern as cloud_connector/vizhealth/checker.go:111. So the split is intentional; flipping pixie.go to JWT would break cloud auth, not improve it. #6 pxl/queryfor — hardened escapePxL so a raw \n, \r, \t or NUL byte in Target.Pod/Target.Namespace can't terminate the PxL string literal and inject a new statement. Added TestQueryFor_RejectsInjectionInTargetFields driving QueryFor with 7 adversarial pod/namespace shapes (newline, single-quote-only, CR, backslash-escape-of-escape, NUL, tab) plus the regex_match fallback path, asserting the output line count and every statement's leading token. Extends existing TestEscapePxL_TableDriven with the new escape mappings. CodeRabbit oversights (verified each against current code): CR r3379377432 (main.go) — wrapped the control-surface listener in http.Server with Read/ReadHeader/Write/Idle timeouts so a slow client can't pin a goroutine indefinitely. CR r3379377607 (pixieapi.go) — switched direct-mode dial from pxapi.WithDisableTLSVerification (env + addr-gated, brittle) to pxapi.WithDirectTLSSkipVerify (added in PR #49 b523ce3 for the same node-IP-dial scenario). Removed the cluster.local + PX_DISABLE_TLS precondition in NewDirect; the always-skip semantics match the AE deployment shape. Refactored the obsolete env-gate test. CR r3379377645 (streaming/filter.go) — the deltaCh-close path returned without calling disarm(), leaking the timer's goroutine on shutdown. Now calls disarm() before return on chan-close. CR r3426923299 (sink/clickhouse.go Record) — capped Record at a 2s per-call context timeout. The 30s chhttp default was too long for the scanner/passthrough/controller hot paths that call Record inline; reconcile is best-effort by contract, so a stalled CH must not pin the pull loop. All 14 AE packages pass go test. arc lint: 0 Errors on PR-53 file scope. Signed-off-by: entlein <einentlein@gmail.com>
Finish the harness consolidation #53 started: adopt exp_matrix.sh (canonical ALL-vs-DX reduction matrix) + nfr.sh (throughput/mem/verdict-latency) as the single runners; cut the overlapping variants (ae_vs_all, exp_datavolume_extreme, exp_dx_steering_reduction, exp_ae_nfr_benchmark, exp_pipeline_reconcile) and the superseded standalone setup (deploy_ae, build_gen_image). README rewritten as the single 'how to run' source: two families — fixture-isolation (run.sh + E-series) and live-attack e2e (log4shell_fire → exp_matrix → nfr → exp_row_reconcile). Add e2e_log4shell_soc.yaml: k3s + full SOC stack (Pixie/kubescape/ClickHouse/AE/dx via k8sstormcenter/soc) on the oracle runner; asserts every canonical harness script runs + dx rules in; profiles dx in real life (pprof CPU/heap + verdict latency, uploaded). Uses existing repo secrets. Signed-off-by: entlein <einentlein@gmail.com>
…ntainer_images BUILD This file is unrelated to adaptive_export — the only change was buildifier alphabetizing container_type/bazel_sdk_versions (no functional change). Restore main's version to keep #53's diff to real AE changes. Signed-off-by: entlein <einentlein@gmail.com>
run-genfiles: the PR had reordered the kwargs in stirling/.../container_images/
BUILD.bazel alphabetically (bazel_sdk_versions before container_type) — a
local buildifier or gazelle drift from when the file was first committed.
CI gazelle wants the original order (container_type first), so the diff
loop fails. Restored to origin/main's ordering. Local gazelle disagrees
with CI's expected output (tooling-version drift); CI is authoritative.
run-container-lint: two findings.
1. staticcheck QF1001 (De Morgan's law) in
pxl/queryfor_test.go:318. Rewrote the !(a || b || c || d) negated
disjunction as the equivalent !a && !b && !c && !d. Test behaviour
unchanged; verified locally with TestQueryFor_RejectsInjection.
2. golangci-lint typechecking failed on
src/e2e_test/adaptive_export_loadtest/tools/loadgen/cmd/{cleanloadgen,
httpsink}/main.go because that subtree carries its own go.mod and
does not resolve as a package under the root px.dev/pixie module.
Added the loadgen subtree to .arclint's exclude list. Same fix the
existing entries for other-module subtrees apply.
Local 'arc lint' on the PR-53 file scope: 0 Errors, 14 SHELLCHECK
Warnings + 26 SHELLCHECK Advice (all pre-existing in ae7b86f
harness scripts; not introduced by this PR).
Signed-off-by: entlein <einentlein@gmail.com>
run-genfiles CI re-failed after f244ffc reverted this file to main's ordering — turns out gazelle on this repo IS alphabetizing the kwargs (bazel_sdk_versions before container_type), and CI runs 'gazelle fix' then 'git diff' to catch any drift. So main's ordering is no longer gazelle-stable; the file has to match gazelle's preference, not main's. Verified locally with 'bazel run //:gazelle -- fix'; the file is now idempotent (a second gazelle run produces no diff). Signed-off-by: entlein <einentlein@gmail.com>
run-container-lint re-failed after the run-genfiles fix because two files added on this branch (a03aa15) had unfixed lint errors: .github/workflows/e2e_log4shell_soc.yaml — 5 yamllint Errors: - 1 indentation: list items under steps: must be parent-aligned per the repo's .yamllint config (indent-sequences: false), not 2-indented. Dedented every step item + its run: block by 2 spaces. - 4 line-length (>120 chars): split the long kubectl-set-image, the long grep-detection gate, the curl pprof URL, and the verdict-latency grep across continuation lines. Semantics unchanged. src/e2e_test/adaptive_export_loadtest/harness/{exp_matrix.sh, nfr.sh} — missing Apache headers. Applied via arc lint --apply-patches; exec bits restored. Local arc lint on PR-53 file scope: 0 Errors, 14 SHELLCHECK Warnings + 26 Advice (all pre-existing in harness scripts, unchanged). Signed-off-by: entlein <einentlein@gmail.com>
arc lint --apply-patches exits non-zero on Warning level too. Resolved each: replaced unused 'for i/t in ...' loop vars with '_', split a SC2155 declare-and-assign, dropped two never-referenced hip/pip assignments. Signed-off-by: entlein <einentlein@gmail.com>
10 tasks
|
Approved if the linter finally goes green, all significant backlog items (such as auth) will come in the followup PR, this image has been proven to be stable for now and have reasonable NFRs |
entlein
pushed a commit
that referenced
this pull request
Jun 21, 2026
- controller: don't fan out Pixie rows when attribution Sink.Write fails (avoids orphaned rows; release in-flight slot, non-fatal). (controller.go:347) - controller.Rehydrate: re-arm rev-1 pushPixieRows for restored windows so a restart doesn't silently miss post-restart Pixie data. (controller.go:255) - passthrough.pull: per-table timeout context so a hung dependency can't stall the sweep / delay shutdown (covers serial+concurrent ticks). (passthrough.go:147) - schema: TTL (30d) on ae_reconcile to cap append-only growth. (schema.sql:485) Verified no-change-needed: adaptive_attribution ORDER BY (hostname, anomaly_hash) is safe — anomaly_hash already encodes namespace+pod (anomaly/hash.go), so rows never collapse across ns/pod. (schema.sql:430) Already on ae-prod (no-op): filter timer leak, stats.py EXACT guard, sink async, watermark paging, pixieapi WithDirectTLSSkipVerify, http.Server timeouts.
11 tasks
This was referenced Jul 15, 2026
Closed
entlein
added a commit
that referenced
this pull request
Jul 17, 2026
* adaptive_export: production AE — streaming export + write-integrity
Signed-off-by: entlein <einentlein@gmail.com>
* adaptive_export: ADAPTIVE_PASSTHROUGH firehose loop
New env-gated background loop that runs the same PxL shape AE's
anomaly-gated path uses, but with an empty Target (no ns/pod predicate)
and over a configurable rolling window. Writes via the existing sink so
the byte-shape of forensic_db rows is comparable between the
PASSTHROUGH=1 phase (EVERYTHING) and the PASSTHROUGH=0 phase
(AE-FILTER). One-shot A/B that yields the per-table capture fraction of
the adaptive write path.
- internal/passthrough/passthrough.go — Loop + Config; defaults to
30s window / 30s refresh / clickhouse.PixieTables() table list.
- internal/passthrough/passthrough_test.go — 6 tests; the load-bearing
one is TestLoop_EmitsEmptyTargetPxL (asserts neither df.namespace nor
df.pod predicates appear in the emitted PxL).
- cmd/main.go: ADAPTIVE_PASSTHROUGH + _WINDOW_SEC + _REFRESH_SEC env
knobs. Adapter is constructed unconditionally when passthrough is on
(joins the existing PushPixie / streaming construction path so the
same pxapi grpc stream is reused). Loop is registered with the
shutdown WaitGroup so SIGTERM waits for the in-flight tick.
- cmd/BUILD.bazel: drop @px// load (other AE BUILD.bazel files use
//bazel — sticking out as the only one with @px is a leftover from a
prior gazelle run; align). Add passthrough dep.
Signed-off-by: entlein <einentlein@gmail.com>
* ci: dx-image workflow — build + publish dx-daemon to ghcr
Stand-alone workflow that builds entlein/dx (private Active-Diagnosis
Framework) into ghcr.io/k8sstormcenter/dx-daemon. Separates the dx image
publish from the bazel-based vizier_release pipeline; the dx repo ships
its own Dockerfile.dxd (Go cross-compile + distroless final stage) so it
doesn't need to live as a submodule inside src/vizier/services/dx.
Triggers:
- tag push 'release/dx/v*' on this repo cuts a release build, image tag
derived from the tag suffix (release/dx/v0.1.0 -> image tag 0.1.0).
- workflow_dispatch lets us build any dx ref on demand with a custom tag
(default: short sha of the resolved dx commit).
Pulls dx via DX_ENTLEIN_PAT (already configured on the repo). Multi-arch
build (linux/amd64, linux/arm64); Dockerfile.dxd cross-compiles in the
native BUILDPLATFORM stage and the final stage is COPY-only, so target
emulation isn't required.
Signed-off-by: entlein <einentlein@gmail.com>
* Revert ci: dx-image workflow — wrong repo
The dx image build pipeline lives in entlein/dx itself (PR #53,
branch feat/bazel-release): bazel-based with @px external pin to
pixie's ae-prod tip, pushes to docker.io/entlein/dx-daemon on a
release/dx/v* tag in the dx repo. The pixie-side buildx workflow
this reverts duplicated that intent in the wrong repo + the wrong
build system (docker buildx instead of bazel + pl_go_image macros)
+ the wrong registry (ghcr.io/k8sstormcenter instead of
docker.io/entlein).
Signed-off-by: entlein <einentlein@gmail.com>
* adaptive_export: unit-normalize trigger watermark cursor + load-test affordances
Fixes the silent-halt bug: the trigger gated on a RAW event_time high-water-mark,
so a single anomaly in a larger unit (ms/ns) drove the watermark past all real
seconds rows and AE stopped processing forever (data still on Pixie). Normalize
event_time to canonical nanoseconds in the poll SELECT filter+order and in the
in-memory/persisted cursor, boundary-dedup, and maxSeen (normalizeEventTimeNanos
+ chNormEventTimeNanos). Validated at the data layer: vs a poisoned watermark the
raw filter returns 0 rows, the normalized filter recovers all 60.
Also adds ADAPTIVE_PUSH_REFRESH_SEC (negative = single-shot pull) for the
reproducible load-test harness, an in-package trigger unit test, and an e2e
hermetic load test (mock PixieQuerier, exact rows+bytes).
Signed-off-by: entlein <einentlein@gmail.com>
* e2e_test/adaptive_export_loadtest: AE fixture-isolation load-test harness
Consolidate the adaptive_export load-test harness under src/e2e_test/ (matching
vzconn_loadtest / px_cluster conventions). Control-plane experiments (E1-E4, E6,
E8 sustained) are proven exactly-reproducible on a live rig; the data-plane
experiments (E5, E8 data-mode) are authored and pending live validation on a
vizier-registered rig (status documented in README + FINDINGS_AND_BACKLOG).
- harness/: shell + python (inject, exp_control, exp_e8, stats, ...) + lib helpers
- fixtures/EXPERIMENTS.md: curated kubescape_logs data-set catalog + expected outputs
- k8s/: isolated sinks + per-rep generator pod (no probes)
- tools/loadgen/: cleanloadgen + httpsink (docker-built test tool; .bazelignore'd
pending a bazel target — lib/pq is already vendored in the module)
- FINDINGS_AND_BACKLOG.md: F8 watermark-poison bug + the fix + AE backlog
The AE Go unit/e2e tests live with the service (internal/{trigger,e2e}).
Signed-off-by: entlein <einentlein@gmail.com>
* e2e_test/adaptive_export_loadtest: document AE implied contracts (C1-C14) + diagrams
Signed-off-by: entlein <einentlein@gmail.com>
* adaptive_export_loadtest: C15 write-duration contract + DX-steering diagram; gen sustained-DNS mode
C15 = AE must keep re-pulling+writing an active pod until t_end or DX stop (the
contract DX steers on; last week's 'wrote then stopped' is its violation). Add
DX-steering sequence diagram. Generator gains SUSTAIN_SEC (distinct-DNS trickle,
a Pixie-traced protocol) + configurable SETTLE_PRE_MS warm-up for fresh-pod
capture; harness wires GEN_SUSTAIN_SEC/GEN_SETTLE_MS.
Signed-off-by: entlein <einentlein@gmail.com>
* adaptive_export/trigger: update test SQL substrings for multiIf normalisation
The 700821d3b trigger unit-normalisation wrapped event_time in a
multiIf(...) inside both the WHERE filter and the ORDER BY. Three
existing tests in watermark_test.go + one in clickhouse_test.go pinned
the raw 'event_time >= N' substring and broke at HEAD.
Update each test's expected substring to match the new normalized form
(') >= <ns-scaled N>' — the closing paren of multiIf, then the value
in canonical nanoseconds). Per-test ns-scaling:
watermark_test.go:94 1744000000000000000 already ns -> unchanged
watermark_test.go:125 InitialWatermark=42 < 1e10 sec -> * 1e9
watermark_test.go:156 InitialWatermark=7 < 1e10 sec -> * 1e9
watermark_test.go:297 event_time='5000' < 1e10 sec -> * 1e9
clickhouse_test.go:82 ref.T=1744..303e9 ns already ns -> unchanged
go test ./src/vizier/services/adaptive_export/... all green.
Signed-off-by: entlein <einentlein@gmail.com>
* adaptive_export_loadtest: exp_control uses real now_s event_time (no future-stamp watermark poison)
Signed-off-by: entlein <einentlein@gmail.com>
* adaptive_export: ADAPTIVE_RECONCILE per-pull write-fidelity instrument
Records one forensic_db.ae_reconcile row per data-plane pull (read_count vs
wrote_count, window, ns/pod) across ALL three write paths — controller fan-out
(filter), passthrough firehose, and streaming scanner — so a reconcile run
localizes loss to query (R5: read<PEM) vs sink (R6: wrote<read) and quantifies
re-pull dup (C8). Counts alone (write >= read) were proven insufficient.
- new internal/reconcile leaf package (Row, Recorder, Nop) — no import cycle
- sink.Record: CH-backed recorder (INSERT INTO forensic_db.ae_reconcile)
- ae_reconcile table: schema.sql + KnownTables + OperatorOwnedTables (synced);
not a pixie table (absent from PixieTables, so VerifyPixieSchema ignores it)
- wired: passthrough.tick, controller.pushPixieRows (deferred, all return
paths), streaming.scanner.Run; gated by ADAPTIVE_RECONCILE=true, else Nop
- unit test proves read/wrote capture incl. the sink-drop read>wrote shape
- fixed apply_test trailing-tables guard for the new operator table
- harness: exp_row_reconcile.sh (row-level PEM<->CH), ae_vs_all.sh, exp_datavolume_extreme.sh
Signed-off-by: entlein <einentlein@gmail.com>
* harness: exp_pipeline_reconcile — skip empty-key rows (0 rows != LOSS 1)
px -o json empty result previously printed one blank line → counted as a phantom
LOSS=1. Guard: empty set → 0-byte keys file; drop all-empty-field keys. Confirmed
against the controlled log4j run (backend http 14/14 exact, conn 66>=12, no loss).
Signed-off-by: entlein <einentlein@gmail.com>
* harness: log4shell_fire.sh — reliably fire + restart the log4j-chain log4shell
Reliable BY CONSTRUCTION against bob#140 (stateful/unreliable exploit on re-fire):
fresh-JVM backend (delete pod) + attacker-before-backend + the WORKING resolvable FQDN
attacker.<ns>.svc.cluster.local:1389 (NOT the bare attacker-ns.svc which NXDOMAINs and
gets dropped), then VERIFY the actual backend->:1389 LDAP egress in forensic_db.conn_stats
and RETRY until confirmed (the validity gate). Never assumes the exploit fired. Node-side.
Signed-off-by: entlein <einentlein@gmail.com>
* harness: log4shell_fire.sh — detection-signal framing (Cyber Verification)
Reword from offensive 'exploit' to detection-signal-generation language: this validates
the kubescape->DX->AE detection chain. No logic change.
Signed-off-by: entlein <einentlein@gmail.com>
* adaptive_export(passthrough): precompiled + concurrent firehose, drop http2
- pxl.CompilePassthrough/Render: precompile per-table PxL once (fixed
window => constant relative start_time), only the two time_ bounds are
stamped per tick. Rendered output is byte-identical to QueryFor with an
empty Target (TestCompilePassthrough_MatchesQueryFor), so this is a
structural change, not a capture change. upid->pod/ns stays in PxL.
- passthrough: tickConcurrent fans every table out at once (was a serial
loop); shared pull() helper. Sink/recorder are pool/HTTP-backed and
already called concurrently elsewhere.
- drop http2_messages.beta from the firehose set (not materialised on
every cluster => ""Table not found"" every tick); shared PixieTables/DDL
lists untouched.
- toggle ADAPTIVE_PASSTHROUGH_COMPILED (default on; =false reverts to the
legacy serial QueryFor path).
Signed-off-by: entlein <einentlein@gmail.com>
* adaptive_export: bazel BUILD deps for internal/reconcile + pxl compile.go
Fixes "missing strict dependencies: import of .../internal/reconcile" that
broke the AE image build for passthrough, sink, streaming, controller, cmd
(pre-existing since the ADAPTIVE_RECONCILE commit added the package + imports
without bazel deps; never CI-built). Also wires the new pxl/compile.go srcs
+ passthrough/pxl test srcs (compiled_test.go, reconcile_test.go, compile_test.go).
- new internal/reconcile/BUILD.bazel (go_library, stdlib-only)
- +//internal/reconcile dep: passthrough, sink, streaming, controller, cmd
- pxl go_library +compile.go; pxl_test +compile_test.go; passthrough_test +compiled_test.go,reconcile_test.go (+reconcile dep)
Signed-off-by: entlein <einentlein@gmail.com>
* adaptive_export(pxl): raise Pixie 10k result cap via #px:set query flag
F1 RCA: Pixie caps every px.display at max_output_rows_per_table (default
10000, query_flags.go) — the planner add_limit_to_batch_result_sink_rule
silently truncates wide firehose windows / busy pods at the READ (write path
is clean: ae_reconcile shows read==wrote). Fix uses Pixie own native knob —
prepend `#px:set max_output_rows_per_table=1000000` to every generated PxL
(QueryFor + CompilePassthrough) so all pull paths are uncapped. Validated on
rig: a 14208-row window returned 10000 (capped) vs 14298 (with flag). No
pagination loop, no extra round-trips. See memory project-ae-passthrough-10k-cap.
Signed-off-by: entlein <einentlein@gmail.com>
* adaptive_export/sink: content_type silent-drop contract suite
Consolidates the recurring content_type silent-drop incident class
into one default-suite test gate (6 tests, ~15ms):
I1 TestContract_ContentTypeIsInt64InSchema
I2 TestContract_FastEncodeContentTypeAsInt
I3 TestContract_SilentDropDetected
I3.b TestContract_SilentDropNotTriggeredOnSuccess
I3.c TestContract_SilentDropToleratesMissingSummaryHeader
I4 TestContract_HTTPEventsRoundTrip
Top-of-file docstring chronicles the incident timeline so future
operators can grep their way to the contract.
Signed-off-by: entlein <einentlein@gmail.com>
* adaptive_export_loadtest: DX-steered-vs-ALL datavolume reduction harness
Measures the AdaptiveExport value prop: datavolume REDUCTION of DX-steered AE
(rev-3 streaming, AE writes only DX-steered activeSet pods over the control
surface) vs saving ALL data (passthrough firehose). Two arms, same fixed load,
forensic_db active-part deltas (rows+bytes) per table; reduction = 1 - DX/ALL.
Uses the canonical resolvable JNDI FQDN (attacker.attacker-ns.svc.cluster.local)
so the chain fires + DX can classify (a malformed host → NXDOMAIN → no steer).
Successor to ae_vs_all.sh, whose AE arm used the rev-2 controller gate + stale JNDI.
Signed-off-by: entlein <einentlein@gmail.com>
* adaptive_export_loadtest: deep AE NFR benchmark harness
Measures all AE non-functional requirements under steady load on the rig:
throughput (rows+bytes/sec), capture completeness (AE read vs broker count =
F1 cap proof), write fidelity (read==wrote + write-error count), end-to-end
freshness latency (now - max(time_) in CH), resource footprint (AE pod cpu/mem
idle vs loaded), per-cycle cadence. Emits a structured report; companion to
exp_dx_steering_reduction.sh. Real-data only.
Signed-off-by: entlein <einentlein@gmail.com>
* adaptive_export_loadtest: fix DX-reduction dead-arm (clear stale steering + live-pod guard)
Run-1 reported false 100% reduction because stale adaptive_attribution windows
rehydrated DEAD pods (deleted loaders) into the activeSet → AE streamed dead pods
→ 0 rows. Clear adaptive_attribution before the DX arm so the activeSet only gets
freshly-steered LIVE pods; add a guard that prints the steered pods + marshalsec
fire count + live log4j-poc pods so a dead-arm result is caught, not reported as
a reduction.
Signed-off-by: entlein <einentlein@gmail.com>
* nfr harness: fix lag (dateDiff) + drop racy broker-pct completeness
lag query used now()-DateTime64 (type error -> na); use dateDiff(second,...).
Capture-completeness vs broker was window-misaligned (623% artifact) -> drop it;
report tot_read vs tot_wrote (read==wrote) + errs instead. The 10k-cap/completeness
proof is the dedicated F1 test (max_read>10000 vs broker for the SAME window).
Signed-off-by: entlein <einentlein@gmail.com>
* dx-reduction harness: report ROWS reduction (primary) + bytes (secondary)
Run-2 byte-delta reduction came out negative because system.parts byte delta is
compaction-noisy (merges land mid-window). Report rows reduction as primary
(actual captured-row count, noise-free); keep bytes as secondary context.
Signed-off-by: entlein <einentlein@gmail.com>
* ae deployment: add memory limit (1Gi) + raise cpu limit to 1 core
Eviction-RCA finding (PR #63 NFR campaign): AE had NO memory limit (only cpu
300m) and was CPU-pinned at 300m under concurrent passthrough. AE measured tiny
(16-38Mi steady), but the raised 1M-row passthrough cap can spike, so cap at 1Gi
so AE can never memory-pressure a node; raise cpu limit 300m->1 core (was
throttling). NOTE node evictions were NOT AE/OOM — node-01 went NotReady
(network/heartbeat); the memory consumer is PEM (1365Mi, OOMs at the 2Gi default).
Signed-off-by: entlein <einentlein@gmail.com>
* ae bootstrap: separate the secret from the re-applied infra bundle
Root cause of the recurring "AE unauthenticated / writes 0 / crashloop" reverts:
kustomization.yaml bundled adaptive_export_secrets.yaml (placeholder pixie-api-key)
with the role+deployment, so EVERY infra re-apply (make log4j) clobbered the real
key that ae-auth had written. Separation of concerns: remove the secret from the
kustomization — infra (role+deployment) stays re-appliable; the secret holds real
creds and is owned solely by `make ae-auth`, created once, never touched by infra
re-applies. Secret manifest kept as a hand-applied seed-only template (documented).
Signed-off-by: entlein <einentlein@gmail.com>
* dx-reduction harness: fire BOTH attack stages so DX steers the backend
The DX-steered arm was failing because the harness only fired stage-1 (JNDI/LDAP).
That generates ldap-egress but NO kubescape R0001 → backend never flagged → DX no
case → indeterminate → AE steers wrong/no pods. R0001 comes from stage-2 (post-
exploitation exec). fire() now does stage-1 (JNDI) + stage-2 (whoami/shadow/token/
getent in the backend) → kubescape R0001+R0006 → DX rules backend MALIGNANT →
backend enters AE activeSet → reduction is measurable. Verified live: DX evidence
unexpected-spawn+sensitive-file-read → verdict ruled_in generic=MALIGNANT.
Signed-off-by: entlein <einentlein@gmail.com>
* adaptive_export: rename whitelist→allowlist across streaming path + add DX-steering diagnostics
Standing terminology rule: allowlist/blocklist, never whitelist/blacklist.
Pure rename (no behavior change) of the rev-3 streaming filter:
FilterModeWhitelist → FilterModeAllowlist
MaxWhitelistSize → MaxAllowlistSize
ADAPTIVE_STREAM_MAX_WHITELIST → ADAPTIVE_STREAM_MAX_ALLOWLIST (env)
mode=whitelist log string → mode=allowlist
plus all comments/identifiers/tests in streaming, activeset, cmd/main.
DX-steering diagnostics (the reason DX-arm-writes-0 has been hard to RCA —
we could not tell "empty ActiveSet" from "broker returned 0 rows"):
- scanner: log the empty-allowlist short-circuit (was silent) so an
empty ActiveSet is visible in logs, distinct from "query completed rows=0".
- FilterUpdater: emitted-filter log Debug→Info so the steered pod count
per ActiveSet change is visible without debug logging.
NOTE: ADAPTIVE_STREAM_MAX_WHITELIST env renamed → tooling that sets the old
name must switch to ADAPTIVE_STREAM_MAX_ALLOWLIST.
Signed-off-by: entlein <einentlein@gmail.com>
* ae(clickhouse): create forensic_db.dx_attack_graph at boot
The Pixie dx_evidence_graph UI reads dx_attack_graph via px.DataFrame
clickhouse_dsn, whose query template hardcodes event_time + hostname and
ORDER BY event_time. A table without those columns fails 'Unknown
identifier event_time'; a table created by hand (local, not via the
operator) isn't globally registered. Fix: make AE own it like the other
forensic tables.
- schema.sql: dx_attack_graph DDL with event_time(UInt64 nanos) + hostname,
edge columns, fromUnixTimestamp64Nano partition/TTL (nanos-correct).
- KnownTables + OperatorOwnedTables: register it so Apply creates it at boot.
- apply_test: assert last-applied DDL == last OperatorOwnedTables entry
(robust to appended operator tables) instead of hardcoding trigger_watermark.
go test ./.../clickhouse green.
Signed-off-by: entlein <einentlein@gmail.com>
* ae(clickhouse): dx_attack_graph numeric cols Int64/Float64 (px-readable)
Pixie's clickhouse_dsn type mapper reads UInt8 as BOOLEAN and does not
handle UInt16/UInt32/Float32 -> px fails with 'Column[N] given incorrect
type' rendering the dx_evidence_graph. weight/max_severity/num_findings
-> Int64, confidence -> Float64 (map cleanly to INT64/FLOAT64). Verified
live: px run returns all 6 edges with every column. event_time stays
UInt64 (matches kubescape_logs, which px reads).
Signed-off-by: entlein <einentlein@gmail.com>
* adaptive_export(streaming): add #px:set max_output_rows cap flag to scanner buildPxL
The DX/streaming arm silently capped each per-table pull at Pixie's default
10000-row limit while the passthrough/ALL arm (pxl.CompilePassthrough /
QueryFor) already raises it to 1,000,000 via the broker's #px:set query flag.
Validated live on 6a33dac0: a single streaming http_events pull returned
exactly rows=10000 (the cap). Left unfixed this UNDER-counts the DX arm and
OVERSTATES the DX-vs-ALL volume reduction. Prepend the same #px:set directive
to the streaming scanner's PxL so both arms are uncapped and comparable.
Signed-off-by: entlein <einentlein@gmail.com>
* ae(clickhouse): create dx_attack_graph_malicious view at boot
Adds the rule-ins-only view (condition != '') to the canonical schema.sql,
registers it in KnownTables + OperatorOwnedTables (after dx_attack_graph), and
teaches DDL() to extract CREATE VIEW headers. AE now creates it on boot so the
dx_evidence_graph UI's default malicious-only read is standard, not a per-rig
manual step. Tests updated.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: entlein <einentlein@gmail.com>
* ae(control): /dx/attack_graph ingest endpoint -> ClickHouse
dx POSTs a JSON array of edges to /dx/attack_graph; AE writes them to
forensic_db.dx_attack_graph via JSONEachRow (Applier.WriteAttackGraph). Wired in
main.go when CONTROL_ADDR is set; 501 if the sink is unset. This is the AE half
of the dx->AE->CH attack-graph write path.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: entlein <einentlein@gmail.com>
* adaptive_export: convention pass — consolidate CH HTTP, drop dead code, fix stale tests
PR-53 review follow-ups (see review summary in conversation):
1. License headers added to 17 src/e2e_test/adaptive_export_loadtest/
harness/*.{sh,py} scripts. Matches the convention every other
src/e2e_test/*/sh in pixie already follows.
2. Dead code removed (was unreachable per golang.org/x/tools/cmd/deadcode):
- internal/script/script.go: IsClickHouseScript, IsScriptForCluster,
GetActions, getScriptName, getInterval, templateScript, plus the
ScriptConfig and ScriptActions types. The cron-script sync flow they
served was replaced by the streaming model; only the Script and
ScriptDefinition types remain.
- internal/pixie/pixie.go: Client.GetPresetScripts (replaced by
builtinPresetScripts() inline in cmd/main.go).
- internal/streaming/{supervisor,writer}.go: SupervisorStats,
TableStats, Stats types + Supervisor.Stats and BatchWriter.Stats
methods (no production reader). Atomic counters dropped; the
existing flush log preserves the per-flush summary.
3. Stale passthrough tests fixed.
TestLoop_DefaultsTablesToPixieTables and TestNew_AppliesDefaults
asserted len(clickhouse.PixieTables()) == 13 but passthrough.New
strips excludedTables (http2_messages.beta), yielding 12. Tests now
compare against filterExcluded(clickhouse.PixieTables()) and add an
extra assertion that excluded tables were not written.
4. ClickHouse HTTP client consolidation: new internal/chhttp/ package
collapses three near-identical HTTP CH clients
(clickhouse.Applier, sink.ClickHouseHTTP, trigger.ClickHouseWatermarkStore)
into one. Centralises endpoint validation, basic-auth header,
30s default timeout, fail-loud INSERT settings (4 CH input_format
knobs), and the X-ClickHouse-Summary read path. The 4 INSERT
call sites in the three callers all route through chhttp.Client.Insert
now; SELECT through Query or QueryStream (the latter preserves the
QueryActive streaming behaviour). Net code: -200 LOC across the
three callers plus a 200-LOC chhttp package with its own tests.
5. Pixie service scaffold wired into cmd/main.go:
services.SetupService("adaptive-export", 50900) +
services.SetupSSLClientFlags() + services.PostFlagSetupAndParse() +
services.SetupServiceLogging(). Matches the pattern every other
pixie Go service uses. CheckServiceFlags() is deliberately skipped
(AE does not run a TLS gRPC server). Existing AE env-var reads
(ADAPTIVE_*) are untouched and still authoritative for tuning knobs.
All 14 adaptive_export internal packages pass go test (1 new chhttp,
13 unchanged), including the 3 differential oracle tests in
pxl/compile_test.go. arc lint OKAY for everything except 3 pre-existing
findings unrelated to this commit (loadgen has its own go.mod; one
QF1001 De Morgan's-law nit in reconcile_test.go from c9f19b6b1).
Signed-off-by: entlein <einentlein@gmail.com>
* adaptive_export: restore executable bits on harness scripts (post-header)
Signed-off-by: entlein <einentlein@gmail.com>
* adaptive_export: lint pass + restore @px load prefix in cmd/BUILD.bazel
User flagged on review 4536971862:
- cmd/BUILD.bazel:18 dropped the '@px' external-workspace prefix on
pl_build_system.bzl load. Restored — the standalone AE build pulls
pixie as @px and needs the qualifier.
Lint cleanup (PR-53 scope, no production code touched outside renames):
- chhttp/chhttp_test.go: errcheck on two w.Write calls + gofumpt on
the gotSettings declaration.
- passthrough/reconcile_test.go: staticcheck QF1001 (De Morgan's law)
on the conn_stats sink-drop assertion.
- script/script.go: rename ScriptId -> ScriptID (ST1003). Propagated
to pixie/pixie.go and cmd/main.go callsites.
- internal/e2e/BUILD.bazel and internal/trigger/BUILD.bazel: gazelle
drift — adding loadtest_test.go and clickhouse_internal_test.go to
srcs.
- k8s/00-sinks.yaml + gen-pod.tmpl.yaml: yamllint compliance —
document-start marker, dedent sequence items per .yamllint
indent-sequences=false, tighten flow-mapping spaces, collapse
multi-space after commas. YAML semantics unchanged.
- harness/stats.py: flake8 E501 — split a long line into two.
Final arc lint state on PR-53 file scope: 0 Errors, 14 Warnings
(SHELLCHECK SC2155/SC2086 in pre-existing harness scripts from
ae7b86f64, not introduced or modified by this commit). Pre-existing
loadgen typecheck failures (separate go.mod) are unaffected.
Signed-off-by: entlein <einentlein@gmail.com>
* adaptive_export: address user review #2 #4 #6 + 4 outstanding CodeRabbit items
User review 4536971862:
#2 passthrough/reconcile_test.go — strengthened with two new tests that
exercise the FULL chain (loop → real sink.ClickHouseHTTP → httptest
CH endpoint → reconcile recorder). TestTick_ReconcileCatchesCHSilentDrop
mimics CH's X-ClickHouse-Summary silent-drop shape (200 OK,
written_rows=0) and asserts the loop records WroteCount=0 with a
silent-drop attribution — the exact R6 (sink-layer loss) regression
the instrument exists to detect. TestTick_ReconcileAttributesCHFailureCorrectly
covers the 500-response branch. The pre-existing in-process-fake test
stays as the wiring check.
#4 pixie/pixie.go — added a long comment justifying the API-key auth
choice. pixie.Client targets the Pixie CLOUD (cloudpb's PluginService),
whose auth interceptor accepts pixie-api-key and rejects JWT service
tokens (those are for INSIDE-cluster vizier services). The
pixieapi.Adapter that talks to vizier directly already uses JWT via
jwtutils.GenerateJWTForService — same pattern as
cloud_connector/vizhealth/checker.go:111. So the split is intentional;
flipping pixie.go to JWT would break cloud auth, not improve it.
#6 pxl/queryfor — hardened escapePxL so a raw \n, \r, \t or NUL byte
in Target.Pod/Target.Namespace can't terminate the PxL string literal
and inject a new statement. Added TestQueryFor_RejectsInjectionInTargetFields
driving QueryFor with 7 adversarial pod/namespace shapes (newline,
single-quote-only, CR, backslash-escape-of-escape, NUL, tab) plus
the regex_match fallback path, asserting the output line count and
every statement's leading token. Extends existing
TestEscapePxL_TableDriven with the new escape mappings.
CodeRabbit oversights (verified each against current code):
CR r3379377432 (main.go) — wrapped the control-surface listener in
http.Server with Read/ReadHeader/Write/Idle timeouts so a slow
client can't pin a goroutine indefinitely.
CR r3379377607 (pixieapi.go) — switched direct-mode dial from
pxapi.WithDisableTLSVerification (env + addr-gated, brittle) to
pxapi.WithDirectTLSSkipVerify (added in PR #49 b523ce362 for the
same node-IP-dial scenario). Removed the cluster.local + PX_DISABLE_TLS
precondition in NewDirect; the always-skip semantics match the AE
deployment shape. Refactored the obsolete env-gate test.
CR r3379377645 (streaming/filter.go) — the deltaCh-close path returned
without calling disarm(), leaking the timer's goroutine on shutdown.
Now calls disarm() before return on chan-close.
CR r3426923299 (sink/clickhouse.go Record) — capped Record at a 2s
per-call context timeout. The 30s chhttp default was too long for
the scanner/passthrough/controller hot paths that call Record inline;
reconcile is best-effort by contract, so a stalled CH must not pin
the pull loop.
All 14 AE packages pass go test. arc lint: 0 Errors on PR-53 file
scope.
Signed-off-by: entlein <einentlein@gmail.com>
* test(harness): consolidate to one run-picture + e2e CI workflow
Finish the harness consolidation #53 started: adopt exp_matrix.sh (canonical
ALL-vs-DX reduction matrix) + nfr.sh (throughput/mem/verdict-latency) as the
single runners; cut the overlapping variants (ae_vs_all, exp_datavolume_extreme,
exp_dx_steering_reduction, exp_ae_nfr_benchmark, exp_pipeline_reconcile) and the
superseded standalone setup (deploy_ae, build_gen_image). README rewritten as the
single 'how to run' source: two families — fixture-isolation (run.sh + E-series)
and live-attack e2e (log4shell_fire → exp_matrix → nfr → exp_row_reconcile).
Add e2e_log4shell_soc.yaml: k3s + full SOC stack (Pixie/kubescape/ClickHouse/AE/dx
via k8sstormcenter/soc) on the oracle runner; asserts every canonical harness
script runs + dx rules in; profiles dx in real life (pprof CPU/heap + verdict
latency, uploaded). Uses existing repo secrets.
Signed-off-by: entlein <einentlein@gmail.com>
* revert(bazel): drop stray buildifier attribute-reorder in stirling container_images BUILD
This file is unrelated to adaptive_export — the only change was buildifier
alphabetizing container_type/bazel_sdk_versions (no functional change). Restore
main's version to keep #53's diff to real AE changes.
Signed-off-by: entlein <einentlein@gmail.com>
* ci: fix run-genfiles + run-container-lint on PR 53
run-genfiles: the PR had reordered the kwargs in stirling/.../container_images/
BUILD.bazel alphabetically (bazel_sdk_versions before container_type) — a
local buildifier or gazelle drift from when the file was first committed.
CI gazelle wants the original order (container_type first), so the diff
loop fails. Restored to origin/main's ordering. Local gazelle disagrees
with CI's expected output (tooling-version drift); CI is authoritative.
run-container-lint: two findings.
1. staticcheck QF1001 (De Morgan's law) in
pxl/queryfor_test.go:318. Rewrote the !(a || b || c || d) negated
disjunction as the equivalent !a && !b && !c && !d. Test behaviour
unchanged; verified locally with TestQueryFor_RejectsInjection.
2. golangci-lint typechecking failed on
src/e2e_test/adaptive_export_loadtest/tools/loadgen/cmd/{cleanloadgen,
httpsink}/main.go because that subtree carries its own go.mod and
does not resolve as a package under the root px.dev/pixie module.
Added the loadgen subtree to .arclint's exclude list. Same fix the
existing entries for other-module subtrees apply.
Local 'arc lint' on the PR-53 file scope: 0 Errors, 14 SHELLCHECK
Warnings + 26 SHELLCHECK Advice (all pre-existing in ae7b86f64
harness scripts; not introduced by this PR).
Signed-off-by: entlein <einentlein@gmail.com>
* ci: apply gazelle's actual kwarg order to stirling container_images
run-genfiles CI re-failed after f244ffc6c reverted this file to main's
ordering — turns out gazelle on this repo IS alphabetizing the kwargs
(bazel_sdk_versions before container_type), and CI runs 'gazelle fix'
then 'git diff' to catch any drift. So main's ordering is no longer
gazelle-stable; the file has to match gazelle's preference, not main's.
Verified locally with 'bazel run //:gazelle -- fix'; the file is now
idempotent (a second gazelle run produces no diff).
Signed-off-by: entlein <einentlein@gmail.com>
* ci: fix container-lint Errors on e2e workflow + new harness scripts
run-container-lint re-failed after the run-genfiles fix because two
files added on this branch (a03aa1570) had unfixed lint errors:
.github/workflows/e2e_log4shell_soc.yaml — 5 yamllint Errors:
- 1 indentation: list items under steps: must be parent-aligned per
the repo's .yamllint config (indent-sequences: false), not 2-indented.
Dedented every step item + its run: block by 2 spaces.
- 4 line-length (>120 chars): split the long kubectl-set-image, the
long grep-detection gate, the curl pprof URL, and the verdict-latency
grep across continuation lines. Semantics unchanged.
src/e2e_test/adaptive_export_loadtest/harness/{exp_matrix.sh, nfr.sh}
— missing Apache headers. Applied via arc lint --apply-patches; exec
bits restored.
Local arc lint on PR-53 file scope: 0 Errors, 14 SHELLCHECK Warnings
+ 26 Advice (all pre-existing in harness scripts, unchanged).
Signed-off-by: entlein <einentlein@gmail.com>
* ci: silence 6 SHELLCHECK Warnings to clear container-lint exit code
arc lint --apply-patches exits non-zero on Warning level too. Resolved
each: replaced unused 'for i/t in ...' loop vars with '_', split a
SC2155 declare-and-assign, dropped two never-referenced hip/pip
assignments.
Signed-off-by: entlein <einentlein@gmail.com>
* feat(ae-control): bearer-JWT auth + input validation (CodeRabbit followup)
AE control endpoints had no auth. Add it using the SAME shared lib the vizier
broker/PEM use (px.dev/pixie/src/shared/services/utils): SetAuth verifies a
bearer JWT via jwtutils.ParseToken (signature + audience), middleware on
Handler() with /healthz exempt. dx already mints this exact service JWT
(GenerateJWTForService, PL_JWT_SIGNING_KEY) — it just attaches it. No new
secret/crypto. Flag-gated (CONTROL_REQUIRE_AUTH + PL_JWT_SIGNING_KEY), default
OFF so it merges before dx sends the bearer; flip on after dx is updated.
Also: reject invalid t_end (<=0) and query windows (start>=end, non-positive).
Test mints a real JWT via the shared lib; asserts 401 on missing/bad token,
pass on valid, /healthz open.
* fix(ae): remaining CodeRabbit Majors (#53 followup)
- controller: don't fan out Pixie rows when attribution Sink.Write fails
(avoids orphaned rows; release in-flight slot, non-fatal). (controller.go:347)
- controller.Rehydrate: re-arm rev-1 pushPixieRows for restored windows so a
restart doesn't silently miss post-restart Pixie data. (controller.go:255)
- passthrough.pull: per-table timeout context so a hung dependency can't stall
the sweep / delay shutdown (covers serial+concurrent ticks). (passthrough.go:147)
- schema: TTL (30d) on ae_reconcile to cap append-only growth. (schema.sql:485)
Verified no-change-needed: adaptive_attribution ORDER BY (hostname, anomaly_hash)
is safe — anomaly_hash already encodes namespace+pod (anomaly/hash.go), so rows
never collapse across ns/pod. (schema.sql:430)
Already on ae-prod (no-op): filter timer leak, stats.py EXACT guard, sink async,
watermark paging, pixieapi WithDirectTLSSkipVerify, http.Server timeouts.
* fix(ae-trigger): escape a PollLimit-saturated watermark boundary (from #67)
ae-prod's boundary handling accumulates the seen-fingerprint set on a no-progress
tick, but if >PollLimit rows share one normalized event_time the SQL
(>= watermark ORDER BY time LIMIT N, no secondary key) returns the same N boundary
rows every poll → rows beyond N at that timestamp are never emitted (infinite
boundary). Detect all-skipped-at-capacity and advance the watermark by 1ns to make
forward progress (fingerprint dedup already tolerates the 1ns overlap).
Cherry-picked the trigger fix + its test from the stale CodeRabbit-chat PR #67
(687851d8c); dropped that PR's unrelated gen-pod.tmpl.yaml churn. #67 itself is
NOT mergeable (77 commits behind ae-prod, re-adds deleted terraform).
* feat(ae-control): TLS on the control surface (CONTROL_TLS) (#71)
Auth without TLS is half a control: tcpdump on dx→AE :9100 captured 720
cleartext `Authorization: Bearer` JWTs in 70s — the #68 token crosses the
CNI in plaintext. CONTROL_TLS=true now serves TLS with server.crt/key from
the service-tls-certs secret (broker/PEM already use it; dx skip-verifies).
Default-OFF for incremental rollout, symmetric to CONTROL_REQUIRE_AUTH.
Stacks on #68 (ae-followup-auth). dx client half: entlein/dx#88.
Co-authored-by: Entlein <eineintlein@gmail.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test(ae-trigger): differential oracle vs naive reference (incremental → 73.7% cov)
The trigger's incremental kubescape-events pump has 3 moving parts
that must agree: watermark advancement, boundary fingerprint dedup,
and PollLimit-saturated draining (PR #67 fix). Existing tests cover
each in isolation. This new file pins them TOGETHER against the
simplest possible reference ("drain everything in event_time order,
dedupe by fingerprint, advance the cursor to max(event_time)"). If
the iterative trigger and the reference disagree on the set of
emitted rows for ANY poll sequence, one of the three parts is wrong.
Four oracle tests:
TestOracle_TriggerEmitsNaiveSet_StaggeredCorpus — 50 rows scattered
across distinct event_times, PollLimit=10 forces ≥5 polls; trigger
must emit exactly the 50 rows the naive reference computes.
TestOracle_PollLimitSaturation_AtCapacity — regression guard for PR
#67 (dfdc465a9): when EXACTLY PollLimit rows share a boundary
event_time, every one of them must emit, and the cursor must clear
the boundary for the next-event_time row that follows.
TestOracle_PollLimitOverflow_DocumentsLossBound — pins PR #67's
intentional trade-off: when >PollLimit rows share a boundary
event_time, the first PollLimit emit, then the 1ns escape advances
the cursor past the surplus. Lock-in test: if a future fix recovers
the surplus (good!) this test fails loudly so both can update in
lock-step instead of regressing silently.
TestOracle_BoundaryDedup_NoDuplicates — when CH returns the same
boundary row in two consecutive polls (real production case: a new
row lands at the same event_time after a previous poll's cursor
advanced past it), the seenAtBoundary map must filter the duplicate.
Coverage: trigger 71.6% → 73.7% (statement-level). The oracle's value
isn't in statement count — it's in property-level coverage on
invariants the per-feature tests can't enforce together.
Build.bazel: adds oracle_test.go to the existing trigger_test target.
* ci: fix vizier-release Build Release runner label (oracle-16cpu → oracle-vm-16cpu)
Merge from main re-introduced the deprecated 'oracle-16cpu-64gb-x86-64'
label on the Build Release job. That label doesn't resolve in the
fork's runner pool, so aeprod22's release workflow sat queued
indefinitely. update-gh-artifacts-manifest at L143 already had the
'-vm-' variant; align Build Release at L18 to match. Same fix the
fork applied earlier in 21d536e3d for the standalone vizier_release
workflow — main keeps regressing it.
* ci: fix merge-conflict regressions from main pickup (runner labels, ui.bzl, fork cockpit)
The merge of main into ae-followup-auth (0c657514a) silently took the
upstream version on a swath of files where the fork had previously
deviated. This commit reverts the merge's regressions back to the
fork-correct state (origin/ae-prod) AND completes the runner-label
sweep so every release/mirror/perf workflow uses the same -vm-16cpu
label the fork's runner pool actually has.
1. Runner labels — main re-introduced 'oracle-16cpu-64gb-x86-64' and
'oracle-8cpu-32gb-x86-64' on 7 workflows. Fork's pool only resolves
'oracle-vm-16cpu-64gb-x86-64'; both stale labels sit queued forever.
Fixed: cli_release.yaml (L18+L212), cloud_release.yaml (L18),
mirror_demos.yaml (L12), mirror_deps.yaml (L12),
mirror_releases.yaml (L13), operator_release.yaml (L18+L143),
perf_common.yaml (L37+L60). vizier_release.yaml L18 was fixed
already in 0fd9c3f21.
2. bazel/ui.bzl — main reverted PR #64's webpack-build fixes that
broke release/cloud/v0.0.10 with 'export: `18': not a valid
identifier'. Restored: 'set -x' for action-shell tracing, PATH
that puts /opt/px_dev/tools/node/bin FIRST, 'hash -r', the
STABLE_BUILD_TAG|BUILD_TIMESTAMP allowlist sed (vs the wildcard
that word-splits FORMATTED_DATE), and use_default_shell_env=True
so --incompatible_strict_action_env doesn't strip yarn from PATH.
3. 28 fork-cloud config files — main's PR #2391 (cert-manager
migration) deleted private/cockpit/*,
terraform/kubernetes/auth0/*, terraform/kubernetes/cloud_deps/*,
.sops.yaml, private/skaffold_cloud.yaml. These are still load-
bearing for the AOCC pixie-cloud deployment; the fork hasn't
migrated to cert-manager-compatible secrets yet (PR #2391's
monitor.go fallback path is in place, so adoption is the
follow-up, not a blocker). Restored all 28 from origin/ae-prod.
Genuine main pickups that were CORRECT to keep (no fix needed): the
src/utils/shared/k8s/{apply,delete}.go import-order +
sets.New[string] generics migration,
src/operator/controllers/monitor.go's cert-manager secret fallback,
and the src/carnot/BUILD.bazel + src/carnot/exec/BUILD.bazel
additions.
* fix(ae): address 13 CodeRabbit Go-code findings on PR 68
11 TRUE + 2 PARTIAL CodeRabbit comments verified against current code;
all valid. This commit lands every one of them as a discrete change,
keeps the existing tests green, adds new tests where the contract
itself changed.
🔴 Real bugs:
controller/controller.go: handle() now SNAPSHOTS c.active[hash]
before mutation and ROLLS BACK on sink.Write failure. Without
this, a failed persist left c.active extended; an already-running
pushPixieRows would re-snapshot and fan out data based on an
attribution row that never landed in CH. Updated
TestController_SinkErrorNonFatal to pin the new rollback contract
(active==0 after sink error) and added a writeAttempts() observer
on fakeSink so the test doesn't race.
sink/fastencode.go: appendJSONValue now rejects NaN/+Inf/-Inf
floats with errFastEncodeUnsupported (triggers the encoding/json
fallback). Previously strconv.AppendFloat emitted invalid JSON
tokens that made CH reject the whole batch.
streaming/writer.go: flush() keeps the row buffer on write failure
(so the next attempt retries instead of silently dropping), and
the shutdown branch uses context.Background() for the final flush
so it isn't fast-failed by the already-cancelled parent ctx.
control/server.go: decode() now wraps the body in
http.MaxBytesReader(w, body, 4 MiB) so an oversized JSON payload
on the public(-ish) control endpoint can't OOM the operator. The
JWT auth still gates access — this hardens what a holding-an-
acceptable-JWT attacker can do.
🟠 Lower-impact:
cmd/main.go: isOperatorManagedScript matches the EXACT builtin
names (no "ch-" prefix), so a user-authored script named
"ch-something-custom" can't get deleted under
INSTALL_PRESET_SCRIPTS=true.
chhttp/chhttp.go: QueryStream now uses a separate http.Client with
no Timeout. Go's http.Client.Timeout covers body reads, so
reusing the 30s default would silently truncate a multi-MB
active-set rehydrate. Stream callers must bound via ctx.Deadline.
passthrough/passthrough.go: tickConcurrent's precompile-skip branch
now calls l.rec(...) so the compiled and legacy paths produce
identical reconcile-row counts. Previously the compiled path
silently dropped a table → invisible divergence.
trigger/oracle_test.go: COLLECT: labelled break stops the busy-spin
on deadline expiry. Bare "break" only exits the select, leaving
the for-loop to busy-spin until the count condition is reached.
🟡 Cosmetic / cleanup:
control/server_test.go: TestBadInputRejected now pins t_end<=0 on
/export/start AND inverted/zero window on /query — 4 new
assertions for validators that existed but were untested.
pixieapi/pixieapi.go: TODO comment marking the bounded pxapi.Client
leak for follow-up when direct-mode throughput crosses ~1 q/s.
sink/clickhouse.go: "sink: pixie write completed" demoted Info
→ Debug. One log per Pixie batch in fan-out paths was avoidable
log-volume pressure; the silent-drop guard below still fires
loud on the actual failure mode.
sink/integration_test.go: per-table 15s ctx (was a shared 60s for
the entire loop) so a slow early table can't starve later ones.
trigger/clickhouse.go: PollLimit docstring now matches the
0→10000 rewrite in New(); "unlimited" is no longer a documented
option.
Local gates: build exit 0, go test 14/14 packages pass,
tools/linters/lint_like_ci.sh OKAY (0 Errors, 0 Warnings).
* ci: carve out private/cockpit + terraform/credentials/cockpit from filename-linter
The fork's filename-linter (inherited from upstream pixie-io) rejects
any PR diff that touches a path containing 'private'. The restored
fork-cloud config from the main-merge fix (cb81ecd72) added 12
private/cockpit/* + 1 terraform/credentials/cockpit/* files, which
upstream-main had deleted via PR #2391 — the linter then correctly
saw them as 'new private/* additions' and failed.
These specific paths are LEGITIMATE fork-cloud deployment config that
pre-dates the upstream linter; the original 'no private/* leaks'
intent stays in place for everything else via two negative-glob
exceptions.
* Revert "ci: carve out private/cockpit + terraform/credentials/cockpit from filename-linter"
This reverts commit 7f43c514b7f007f30bf176cb774763e69140f411.
* ci: rename private/{cockpit,skaffold_cloud.yaml} so filename-linter accepts the fork-cloud config upstream
Honest fix for the merge-regression: the fork's filename-linter rejects
any PR diff touching a path containing 'private' (an upstream-pixie-io
guard against secrets leaking into OSS PRs). The fork is a CONTRIBUTOR
that upstreams, so the guard applies; the prior 'restore from ae-prod'
fix legitimately re-added private/cockpit/* (which upstream-main #2391
deleted via cert-manager migration) but tripped the linter.
Reverts the lazy carve-out fix (c579e48c4 reverted 7f43c514b) and does
the structural rename instead.
Moves:
private/cockpit/ -> cockpit/
private/skaffold_cloud.yaml -> skaffold/skaffold_cloud.yaml
(collapsed with the upstream-
resurrected skaffold/skaffold_cloud.yaml
— the fork's private/ version was
the maintained superset)
Reference updates:
cockpit/kustomization.yaml ../../k8s/cloud/... -> ../k8s/cloud/...
(depth-1 after the rename)
skaffold/skaffold_cloud.yaml:84 - private/cockpit -> - cockpit
Linter intent preserved: any future PR that touches a real 'private/*'
path still fails — the guard's correct semantics are restored.
* terraform: drop fork IaC from PR 68 — belongs on main, not this PR
cb81ecd72 re-added the terraform/ tree (16 files, 9817 lines) while
fixing regressions from the origin/main pickup, because the main tip
53d09cc8d had deleted it. That restoration is unrelated to the
adaptive_export auth work in this PR; it is now carried on its own
branch (restore-terraform-main, off origin/main) for a separate PR
into main. Removing it here keeps PR 68 scoped to the AE feature.
* fork-infra: drop cockpit, .sops.yaml, e2e workflow from PR 68
These were re-added on this branch while fixing the origin/main pickup
regression (cb81ecd72 + the d94624a1c cockpit rename), but they are fork
infra deleted from main by 53d09cc8d, not part of the adaptive_export
auth work. Carried on restore-fork-infra-main for a separate PR into
main. skaffold/skaffold_cloud.yaml is left in place — it is an upstream
file, not a clean fork-only add.
* adaptive_export: drop out-of-scope bazel/ui.bzl (fork UI-build fix belongs in UI PR #62)
* adaptive_export_loadtest: replace shell harness with a Go TDD suite (fixtures + KPIs)
Collapse the 12-script shell harness (+ stats.py) into a table-driven Go test
suite under suite/: fixtures (control-plane experiments), KPI asserts via
testify/require (reproducibility/reconcile/reduction), and one runner driving a
deployed AE image on a rig (live-gated by AELOAD_LIVE). Removes all CVE names and
adversarial verbs from the harness; kubescape/k8s wire tokens stay literal.
* adaptive_export_loadtest: drop the arbitrary reduction-threshold assert
Volume reduction is a measured outcome to report, not an invariant — asserting
'reduction >= minPct' gates on a made-up threshold. Removed RequireReductionAtLeast;
the reduction test now measures + logs the firehose->steered delta without a pass/fail gate.
* adaptive_export: standardize event_time on nanoseconds (schema + loadtest)
kubescape_logs.event_time is unix NANOSECONDS (Vector kubescape_enrich emits ns).
The DDL read it as seconds via toDateTime() -> ns overflow -> wrong partitions +
rows born already-TTL-expired (the F1/F8 unit bug). Fix, matching soc clickhouse-lab:
PARTITION BY toYYYYMM(fromUnixTimestamp64Nano(event_time))
TTL toDateTime(fromUnixTimestamp64Nano(event_time)) + INTERVAL 30 DAY
Also raise the protocol tables' derived event_time DateTime64(3) (ms) -> DateTime64(9)
(ns), matching time_. The loadtest suite now injects UnixNano and documents C1 as ns.
* adaptive_export_loadtest: add evidence.sh (stack integration + nanos + loadtest) + AELOAD_REPS knob
evidence.sh captures, on a live rig: pipeline components Running (kubescape→vector→
node-agent→ClickHouse→AE), every forensic_db table created, event_time NANOSECONDS
end-to-end (kubescape_logs UInt64/fromUnixTimestamp64Nano + protocol DateTime64(9)),
the pipeline flowing, and the AE control-plane reproducibility loadtest. Sandbox-safe
(no shell sleep; ClickHouse via kubectl exec). suite_test.go gains AELOAD_REPS for
fast runs. Verified live on rig 6a58ec21: PASS=29 FAIL=0.
* adaptive_export_loadtest/suite: commit go.sum (reproducible testify build)
* adaptive_export_loadtest: test EVERY forensic_db timestamp is nanoseconds
schema_test.go: per-(table,column) fixtures asserting every timestamp column is
nanosecond-precision — UInt64 unix-ns (kubescape_logs.event_time, watermark,
dx_attack_graph.event_time) or DateTime64(9) (all protocol time_/event_time,
adaptive_attribution t_start/t_end/last_seen, ae_reconcile, alerts). Plus
TestNoCoarserTimestamps: dynamic backstop that fails on ANY DateTime coarser than
(9) in forensic_db. evidence.sh runs both. Verified live: 37/37 columns PASS.
* adaptive_export_loadtest: drop evidence.sh (the Go schema + reproducibility tests are the suite)
* adaptive_export_loadtest: java-poc disease calibration (real e2e)
TestJavaPocCalibration assumes the full stack (kubescape+vector+ClickHouse+
adaptive_export+dx+java-poc) and induces the java-poc listeriosis disease once,
asserting each pipeline stage lights up as a before->after delta:
1 kubescape detects (R0001 spawn) 2 vector->ClickHouse carries it (fresh ns)
3 adaptive_export captures forensics (backend->pathogen:1389 LDAP egress)
4 dx rules in (non-blind verdict).
Pathogen vocabulary (pathogen/disease/Specimen/listeriosis); external wire literal.
Live+e2e gated (AELOAD_LIVE=1 AELOAD_E2E=1); sandbox-safe 3s polls, no long sleep.
Nothing mocked — unlike the control-plane suite, the signal originates from a real
workload and flows through every component.
* test(e2e): single pixie-native command to deploy the non-Pixie stack
The calibration/e2e suite assumed a hand-installed stack (kubescape + vector +
ClickHouse + the java-poc apps), which made runs non-reproducible. Add ONE
command that stands the whole non-Pixie environment up, from each component's
golden-source repo (one source of truth per component):
- skaffold.yaml (module e2e-nonpixie): requires the soc stack Skaffold
(ClickHouse -> kubescape -> vector) then the bob java-poc apps Skaffold
(SBoBs -> workloads). Pixie itself (pl) is overlaid separately.
- suite/deploy.go: EnsureE2EStack(t) invokes it (AELOAD_DEPLOY=1; AELOAD_CLEAN=1
for a from-scratch redeploy keeping pl + dx). TestJavaPocCalibration calls it,
so the test deploys its own world when asked and otherwise assumes a live rig.
Proven on a k3s rig: `skaffold deploy -m java-poc-apps -p k3s` applies
namespaces -> SBoBs -> 5 workloads and waits them Ready.
Refs k8sstormcenter/soc#231, k8sstormcenter/bob#152.
* test(e2e): confusion matrix + all-pod KPIs; robust stage4 + CH retry
- confusion_test.go (TestStackEvidence): extracts per-workload KPIs (R0001/R0010,
attribution, egress) across the whole java-poc stack and scores dx verdicts as a
confusion matrix vs ground truth (only backend is malignant, only under log4shell).
Reports always; asserts under AELOAD_MATRIX=1. Catches cross-scenario FPs — e.g.
backend ruled_in [argocd-malicious-render] (a dx-repo issue) → FP=1, precision=0.50.
- calibration_test.go stage4: poll the CURRENT backend pod's ruled_in (150s) instead
of a saturated cumulative count that missed dx's workup lag; all-dx-pod aware.
waitUntil now returns bool.
- harness.go chReq: retry transient transport errors + 5xx (a kubectl port-forward
EOF was flaking dedup-extend).
Verified live on k3s: full suite green (schema + reproducibility std=0 + calibration);
matrix report renders TP=1 FP=1 FN=0 TN=4.
---------
Signed-off-by: entlein <einentlein@gmail.com>
Co-authored-by: Entlein <eineintlein@gmail.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-authored-by: pixie-agent <noreply@local>
Co-authored-by: pixie-agent <croedig@sba-research.org>
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.
Summary: Productionise the adaptive-export (AE) service on the k8sstormcenter fork: streaming export of pixie protocol tables into forensic_db, write-integrity assertions, schema rev-2 (anomaly_hashes on every protocol table), conn_stats persistence, ADAPTIVE_PASSTHROUGH (firehose) for A/B capture-fraction measurement, and the supporting CI + release-pipeline cleanup (copybara fork-ignore, ghcr migration, perf workflows).
Test Plan: 1) go test ./src/vizier/services/adaptive_export/... — all unit + integration tests green (controller, sink, trigger, pxl, anomaly, kubescape, window, clickhouse, passthrough). 2) local-ci.sh phase 9 (perf-eval-soc-attack on local k3s + AOCC pixie-cloud) — passed 6/6, 698KB parquet, 43,944 redis_events rows, zero PEM/Kelvin/query-broker restarts. 3) Release pipeline: cut release/vizier/v0.14.19-aeprod-clean3 against ef7525c — vizier-release workflow GREEN end-to-end (Build Release 44m, Create Release on Github, SBOM upload), multi-arch ghcr.io/k8sstormcenter/vizier-adaptive_export_image:0.14.19-aeprod-clean3 published.
Type of change: /kind feature
Changes spanning this PR (33 commits on top of main):
2db5a9e62); ADAPTIVE_PASSTHROUGH firehose for A/B capture-fraction (45130be54); schema rev-2 with anomaly_hashes column and conn_stats persistence; per-table fault isolation; configurable rehydrate timeout.