Repository navigation
Robustness, platform and performance fixes for the next Netdata vendoring #18
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
526db47
c828b0d
ca3f5be
321629a
7e9d9e2
cf61ad1
b450b40
2ac2011
2a5888b
5271dff
6609157
e38ffa7
b66b224
28dc796
84f6195
dbfa64a
2561405
26067c3
49a9ab9
f2b9585
2197bdb
ee8e32d
891faa1
7b73adc
485a881
5161da6
ed24606
e3a6bdd
48cc98b
5e6bfb7
0a6aed3
2eee40a
d54b22c
bbc55a2
292c6dc
80a56de
4261919
9568f4e
616bc81
d223d9f
6b14803
3fb1bef
278125c
d3c94a5
84444bd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -168,17 +168,64 @@ Load this skill for any task involving: | |||||||||||||||||||||||
| If any downstream vendored-source change is not understood, do not run | ||||||||||||||||||||||||
| `vendor-to-netdata.sh`. | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| 7. Record the CI/scanner preflight in the active SOW before vendoring. | ||||||||||||||||||||||||
| 7. Assess performance impact (AGENTS.md, Test Cadence). | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| Benchmarks take hours; run them only when the vendored changes can affect | ||||||||||||||||||||||||
| established-session steady state. Collect the SOW gate performance | ||||||||||||||||||||||||
| assessments for every source change since the baseline (step 5), and | ||||||||||||||||||||||||
| record in the active SOW whether any changed code runs per request on an | ||||||||||||||||||||||||
| established session, with file:line evidence. | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| - No such change (connection setup, reconnect, error mapping, idle-timeout | ||||||||||||||||||||||||
| paths, shutdown, tests, docs, build): no benchmark run; record the | ||||||||||||||||||||||||
| assessment. | ||||||||||||||||||||||||
| - Such a change exists: run the affected suites once on the candidate | ||||||||||||||||||||||||
| commit: | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| ```bash | ||||||||||||||||||||||||
| # Measure (run-*), then render the report and check floors (generate-*). | ||||||||||||||||||||||||
| bash tests/run-posix-bench.sh benchmarks-posix.csv | ||||||||||||||||||||||||
| bash tests/generate-benchmarks-posix.sh | ||||||||||||||||||||||||
| # On the Windows host: | ||||||||||||||||||||||||
| bash tests/run-windows-bench.sh benchmarks-windows.csv | ||||||||||||||||||||||||
| bash tests/generate-benchmarks-windows.sh | ||||||||||||||||||||||||
| ``` | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| Compare against the committed CSVs (`git diff`). If rows regress beyond | ||||||||||||||||||||||||
| noise, fix all regressions, then rerun once; bisect by SOW commit. | ||||||||||||||||||||||||
| - Assessment uncertain: ask the user before running anything. | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| A regression that is neither fixed nor explicitly accepted by the user | ||||||||||||||||||||||||
| blocks vendoring. | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| Extended suites (fuzz, stress, randomized chaos, lookup scale) are not | ||||||||||||||||||||||||
| part of correctness runs. Run them once on the candidate commit when the | ||||||||||||||||||||||||
| vendored changes touch what they exercise (codecs and decoders for fuzz; | ||||||||||||||||||||||||
| concurrency, session lifecycle or transports for stress), and record the | ||||||||||||||||||||||||
| result or the reason none was needed: | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| ```bash | ||||||||||||||||||||||||
| bash tests/run-extended-fuzz.sh | ||||||||||||||||||||||||
| bash tests/run-extended-stress.sh | ||||||||||||||||||||||||
| ``` | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| A failure blocks vendoring until it is fixed. | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| 8. Record the CI/scanner preflight in the active SOW before vendoring. | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| Include: | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| - source repository and commit; | ||||||||||||||||||||||||
| - GitHub Actions/check-run summary; | ||||||||||||||||||||||||
| - code-scanning summary by tool and severity; | ||||||||||||||||||||||||
| - Dependabot and secret-scanning summary; | ||||||||||||||||||||||||
| - Go version alignment: the `go` directive in the three plugin-ipc | ||||||||||||||||||||||||
| `go.mod` files equals Netdata's `src/go/go.mod`, and `GoVersionOverride` | ||||||||||||||||||||||||
| in the three `osv-scanner.toml` files equals the patch release Netdata | ||||||||||||||||||||||||
| pins in `packaging/check-for-go-toolchain.sh`; fix any mismatch first; | ||||||||||||||||||||||||
| - decision: proceed, block, or proceed with explicit risk acceptance. | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| 8. Only after the CI/scanner preflight and two-way migration plan pass, update | ||||||||||||||||||||||||
| 9. Only after the CI/scanner preflight and two-way migration plan pass, update | ||||||||||||||||||||||||
| the Netdata vendored copy using the project-local vendor workflow. | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| ```bash | ||||||||||||||||||||||||
|
|
@@ -187,14 +234,42 @@ Load this skill for any task involving: | |||||||||||||||||||||||
| bash ./diff-netdata-vendor.sh /path/to/netdata | ||||||||||||||||||||||||
| ``` | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| 9. Validate in the Netdata checkout. | ||||||||||||||||||||||||
| `vendor-to-netdata.sh` prints the Go import-path rewrite but does not run | ||||||||||||||||||||||||
| it, and `diff-netdata-vendor.sh` normalizes import paths, so it reports no | ||||||||||||||||||||||||
| difference either way. Apply the rewrite in the Netdata checkout (no `.bak` | ||||||||||||||||||||||||
| files), then confirm no upstream path is left: | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| ```bash | ||||||||||||||||||||||||
| grep -rlZ 'github.com/netdata/plugin-ipc/go' src/go/pkg/netipc --include='*.go' | | ||||||||||||||||||||||||
| xargs -0 -r sed -i 's|github.com/netdata/plugin-ipc/go|github.com/netdata/netdata/go/plugins|g' | ||||||||||||||||||||||||
| grep -rl 'github.com/netdata/plugin-ipc/go' src/go/pkg/netipc # expect no output | ||||||||||||||||||||||||
|
Comment on lines
+243
to
+245
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P1: These relative paths rewrite the plugin-ipc source tree, not the Netdata checkout, when the documented vendor commands are run from the source repository. Change into Prompt for AI agents
Suggested change
|
||||||||||||||||||||||||
| ``` | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| 10. Validate in the Netdata checkout. | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| Run the targeted Netdata build/tests required by the active SOW. At minimum, | ||||||||||||||||||||||||
| prove: | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| - the vendor diff is expected after copying; | ||||||||||||||||||||||||
| - Netdata-only wrapper files were not overwritten; | ||||||||||||||||||||||||
| - unrelated Netdata files were not staged or modified by the vendoring step. | ||||||||||||||||||||||||
| - unrelated Netdata files were not staged or modified by the vendoring step; | ||||||||||||||||||||||||
| - every vendored `.c` file is in Netdata's build lists | ||||||||||||||||||||||||
| (`src/libnetdata/SourceLists.cmake`), and every system library the new | ||||||||||||||||||||||||
| code calls is linked to the `netipc` target in | ||||||||||||||||||||||||
| `src/libnetdata/CMakeLists.txt` (the script does not edit either file); | ||||||||||||||||||||||||
| - the Netdata build reports no warning in vendored NetIPC sources. Netdata | ||||||||||||||||||||||||
| compiles them with `-O2 -D_FORTIFY_SOURCE=3`, which plugin-ipc's own | ||||||||||||||||||||||||
| builds do not enable, so check this before pushing the source commit: | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| ```bash | ||||||||||||||||||||||||
| cmake -S . -B .tmp/fortify-build -DCMAKE_BUILD_TYPE=Release \ | ||||||||||||||||||||||||
| -DCMAKE_C_FLAGS="-O2 -D_FORTIFY_SOURCE=3 -Wall -Wextra" | ||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: Prompt for AI agents
Suggested change
|
||||||||||||||||||||||||
| cmake --build .tmp/fortify-build 2>&1 | grep 'src/libnetdata.*warning:' | ||||||||||||||||||||||||
|
Comment on lines
+265
to
+267
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: Prompt for AI agents
Suggested change
|
||||||||||||||||||||||||
| ``` | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| Netdata builds `netipc` on Linux and Windows only; the Windows build is | ||||||||||||||||||||||||
| proven by the Netdata PR's "Build Windows" check, so the SOW stays open | ||||||||||||||||||||||||
| until that check passes. | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| ## Blockers | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
|
|
@@ -210,6 +285,8 @@ Stop and report if any of these are true: | |||||||||||||||||||||||
| - either side has NetIPC changes since the last vendoring that are not | ||||||||||||||||||||||||
| understood; | ||||||||||||||||||||||||
| - the active SOW does not contain a migration plan for both directions; | ||||||||||||||||||||||||
| - the performance assessment is missing, or a benchmark run it called for | ||||||||||||||||||||||||
| shows a regression that is neither fixed nor explicitly accepted by the user; | ||||||||||||||||||||||||
| - source tree has uncommitted changes that are intended for vendoring; | ||||||||||||||||||||||||
| - Netdata checkout has unrelated changes that would be hard to isolate. | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
|
|
@@ -228,5 +305,7 @@ Netdata vendoring preflight: | |||||||||||||||||||||||
| - Upstream gap: <summary of plugin-ipc changes since baseline> | ||||||||||||||||||||||||
| - Downstream gap: <summary of Netdata vendored changes since baseline> | ||||||||||||||||||||||||
| - Migration plan: <preserve/backport/overwrite/decision summary> | ||||||||||||||||||||||||
| - Performance: <assessment; benchmark run or why none was needed; regressions fixed/accepted/none> | ||||||||||||||||||||||||
| - Extended suites: <fuzz/stress run and result, or why none was needed> | ||||||||||||||||||||||||
| - Decision: proceed | blocked | proceed with user-accepted risk | ||||||||||||||||||||||||
| ``` | ||||||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.