Patch release 2.10.3#22249
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
No issues found across 12 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Architecture diagram
sequenceDiagram
participant Source as Command Source
participant Handler as dyncfg.Handler
participant Domain as Domain Callbacks (Collector, SD, Vnode, etc.)
participant Rules as Validation Rules
Note over Source,Rules: Dynamic Configuration Job Creation Flow
Source->>Handler: CmdAdd(fn)
Handler->>Domain: ExtractKey(fn)
Domain-->>Handler: key, name
Handler->>Domain: NEW: ValidateJobName(name)
alt Domain: Collector
Domain->>Rules: NEW: JobNameRuleStrict(name)
Note right of Rules: Rejects spaces, colons, AND dots
Rules-->>Domain: error/nil
else Domain: SD, SecretStore, or Vnode
Domain->>Rules: NEW: JobNameRuleAllowDots(name)
Note right of Rules: Rejects spaces and colons ONLY
Rules-->>Domain: error/nil
end
Domain-->>Handler: Validation Result
alt Invalid Name (Validation Failed)
Handler-->>Source: 400 Bad Request (invalid job name)
else Valid Name
Handler->>Domain: ParseAndValidate(fn, name)
alt Parse Success
Domain-->>Handler: Config object
Handler->>Domain: Start(Config)
Handler-->>Source: 200/201 Success
else Parse Failure
Domain-->>Handler: error
Handler-->>Source: 400/500 Error
end
end
…spin (netdata#22232) * ebpf.plugin: fix PID accounting shared-memory pool leak and 100% CPU spin The eBPF plugin's per-PID shared-memory pool (32,768 slots backing /dev/shm/netdata_shm_integration_ebpf) monotonically fills on any host with normal process churn, and once the pool is exhausted each module's apps-read loop spins one CPU core at 100% indefinitely. ROOT CAUSE netdata_ebpf_get_shm_pointer_unsafe() is an allocate-AND-set-bit API, but it is called from two incompatible contexts: 1. The per-module BPF map iteration (ebpf_read_*_apps_table), which is paired with netdata_ebpf_reset_shm_pointer_unsafe() via the kill(pid, 0) == ESRCH branch. This is a correct producer/consumer. 2. The apps and cgroup aggregation helpers (*_sum_pids, *_update_*_cgroup, *_resume_apps_data), which iterate live-PID lists built from /proc and cgroup snapshots and call the same allocating accessor. These paths have NO reset counterpart. Every second, for every live PID seen in /proc or a tracked cgroup, every enabled eBPF module unconditionally sets its bit in the shm slot for that PID. A dead PID's bit is only cleared by the owning module's BPF-map iteration - but the BPF map's entry was already deleted in the same cycle (by bpf_map_delete_elem inside reset_shm_pointer_unsafe), so the next iteration cannot find the PID and the bit is stuck forever. The slot therefore never reaches threads == 0 and is never compacted out. Over 15h on a workstation with ~1,300 live PIDs the pool reaches 100% with ~96% stale entries. Once the pool is full, get_shm_pointer_unsafe() returns NULL. Every apps_read loop had a bare `continue;` after the NULL check, which skipped the `key = next_key;` advance, so bpf_map_get_next_key() re-returned the same key forever and the thread spun at 100% CPU. FIXES A. Split allocation from lookup. Introduce netdata_ebpf_lookup_shm_pointer_unsafe(pid) that returns the existing slot or NULL and sets no bit. Replace the 16 aggregation call sites with the new API. Keep get_shm_pointer_unsafe() at the 8 BPF-map iteration call sites where allocation is legitimate and paired with reset. Aggregation paths additionally gate on the module bit being set so they do not consume zero-initialised per-module sub-structs. B. Replace `continue;` with `goto end_*_loop;` in all 8 apps-read loops so `key = next_key;` always runs, preventing the infinite loop even when the pool is legitimately exhausted. C. Zero freshly-allocated slots. ebpf_find_or_create_index_pid() now memset()s the slot on the create branch, so compacted tails and PID reuse cannot leak stale bits or stale per-module counters into new occupants. D. Use the correct kill() error check. Five modules used `if (kill(pid, 0))`, which treats EPERM (cross-UID processes) as "process is dead" and erroneously deletes the live process's kernel BPF map entry and zeroes its shm data. Switched all of them to `if (kill(pid, 0) == -1 && errno == ESRCH)`, matching the two modules that already did it correctly. Also applied to ebpf_parse_proc_files(). E. Collapse ebpf_reset_specific_pid_data() to its effective body. thread_collecting only ever has NETDATA_EBPF_PIDS_PROC_FILE set, so the `idx < PROC_FILE` switch was dead code. The function now just forwards to ebpf_del_pid_entry(). The only other reader of that bitmask, ebpf_release_pid_data(), had zero callers and was removed. F. Sweep the shm pool on module exit. New helper netdata_ebpf_sweep_shm_for_module_unsafe() clears this module's bit across every shm slot and lets the existing compaction path release slots that become empty. All eight per-PID modules now call it under the shm semaphore in their *_exit path, so runtime module restarts no longer leak bits. VERIFICATION - Root cause was captured on a live ebpf.plugin pinned at 64% CPU. gdb showed an EBPF_READ_FD thread permanently alternating bpf_map_get_next_key/bpf_map_lookup_elem at ebpf_fd.c:770-774, and ebpf_stat_values = {total = 32768, current = 32768} while ps showed 1,307 live PIDs. - The diagnosis was independently confirmed by a parallel review (codex, qwen, glm, minimax, opus), all converging on the alloc-without-reset asymmetry in the aggregation paths. - The modified tree builds cleanly (`cmake --build build --target ebpf.plugin`). No new warnings in the changed files; only the pre-existing vendored-Judy stringop-overflow warnings remain. * ebpf.plugin: wipe shm on init, unlink on cleanup, drop unused out-param Close two residual issues around the per-PID shared-memory integration pool, spotted post-merge: 1. The POSIX shm object `netdata_shm_integration_ebpf` was neither truncated on open nor unlinked on shutdown. `shm_open(..., O_CREAT)` on an existing object just reopens it, and `ftruncate` to the previous size is a no-op, so the 14 MB mapped region retained the previous plugin run's bytes. The slot-allocation path memsets each new slot, so this was not a correctness bug — but every slot in `[current, total)` kept stale PID numbers and per-module counters until system reboot. Now: - `netdata_integration_initialize_shm()` memsets the whole mapped region immediately after `nd_mmap`, so the next plugin instance always starts from a clean slab. - `netdata_integration_cleanup_shm()` calls `shm_unlink` so a clean shutdown removes the backing object from `/dev/shm`. 2. Drop the unused `bool *created` out-parameter on `ebpf_find_or_create_index_pid()`. It was added when the memset was factored out of the allocation path; since the memset now lives inside the function, no caller needs to know whether the slot was freshly allocated. Reported by Copilot on PR netdata#22232. * ebpf.plugin: address Copilot findings on 22232 Four residual issues spotted by Copilot after the initial round of changes. All valid. 1. get_shm_pointer_unsafe() no longer bails out when the pool is full. The caller path for an already-tracked PID must still reach its slot so module bits can be updated or cleared. The inner ebpf_find_or_create_index_pid() already returns existing slots unconditionally and only rejects *new* allocations when full, so the redundant outer guard was both dead weight and actively harmful — it made existing PIDs unreachable the moment the pool filled. 2. Semaphore lifecycle. netdata_integration_initialize_shm() now sem_unlinks NETDATA_EBPF_SHM_INTEGRATION_NAME before sem_open, and netdata_integration_cleanup_shm() sem_unlinks on the way out. If a previous plugin instance crashed while holding the semaphore, sem_open(O_CREAT) on the same name would reuse the existing semaphore at its last value (O_CREAT's initial-value argument is ignored when the semaphore already exists), and the next run would spin on sem_timedwait() timeouts forever. Unlinking guarantees the initial value of 1 is honoured. 3. Socket apps iteration: the kernel-map delete for a dead socket tuple used to run inside `if (local_pid) { ... }`. When the shm pool is full and local_pid is NULL, `deleted` sockets were left in the BPF map to be re-iterated every cycle, growing the map unbounded. Moved bpf_map_delete_elem(fd, &key) outside the local_pid branch; it is conditioned only on `deleted`, which captures the socket's own freshness signal and is independent of shm pool state. (For SOCKET_IDX the generic reset_shm_pointer_unsafe() intentionally skips the delete, since the socket map is keyed by a tuple, not by pid — comment added.) (cherry picked from commit bd38550)
(cherry picked from commit c4e0fb8)
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.


Summary
Summary by cubic
Fixes a PID shared-memory pool leak and 100% CPU spin in
ebpf.plugin, splits dynamic-config job name validation by domain, switches SNMP uptime tosnmpEngineTime, and removes an obsolete PowerStore field.Bug Fixes
shm_unlink/sem_unlinkand wiped the region on init.SNMP-FRAMEWORK-MIB::snmpEngineTimethe primarysystemUptimeingo.d/snmp; keeps the same metric name and HR-MIB as fallback.ExtraDetailsfromgo.d/powerstoreHardware to match the API and avoid decode errors.Migration
dyncfg.Callbacks, addValidateJobName(name string) error.JobNameRuleStrictfor collectors; useJobNameRuleAllowDotsfor service discovery, secret store, and vnode names.Written for commit 58b6d16. Summary will update on new commits. Review in cubic