Conversation
Host labels were always re-read from the compiled-in CONFIG_DIR/netdata.conf, so with `netdata -c FILE` the labels came from the wrong file, both at startup and on `netdatacli reload-labels`. netdata_conf_load() now remembers the files it read (the -c file made absolute, or the user file with the stock fallback), and the labels reload uses them. A targeted inicfg_load() clears its section once before reading, so removing the whole [host labels] section also removes the labels.
Contributor
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 🧰 Additional context used📚 Code guidelines (1)No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (6)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe change adds targeted INI section replacement and a daemon API for reloading a section from retained configuration paths. Host-label loading uses the new API, and the CLI description specifies which startup configuration file supplies labels. ChangesConfiguration section reload
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant HostLabels as rrdhost_load_config_labels
participant Config as netdata_conf_reload_section
participant INI as inicfg_load
HostLabels->>Config: reload host-label section
Config->>INI: load section from primary configuration
alt primary has not loaded and fallback exists
Config->>INI: load section from stock configuration
end
Config-->>HostLabels: reload result
Merge Risk: ⚪ Minimal · up to Reloading labels uses the startup configuration and removes labels deleted from it. No actionable issue remains from the reviewed change. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change keeps label reloads tied to the selected configuration file and does not demonstrate a new remotely controllable entrypoint. The main concern is failure containment: a read failure after opening the file can discard previously valid label configuration. Exposure appears limited, but exceptional path handling and all reload callers were not fully verified. Retained concerns
Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 21.43% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 14 functions across 5 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 5 files
Architecture diagram
sequenceDiagram
participant CLI as netdata CLI
participant Agent as Netdata Agent
participant Config as Config Loader
participant File as Config Files
participant Labels as Host Labels
Note over CLI,Labels: Host Labels Configuration Flow
Agent->>Config: Start with config file spec (-c or default)
Config->>File: Read primary config file
alt Custom config (-c FILE)
Config->>Config: Store absolute path as primary
File-->>Config: Primary config content
else Default config
Config->>File: Try user netdata.conf
alt User config not found
Config->>File: Load stock netdata.conf
Config->>Config: Store fallback path
File-->>Config: Stock config content
else User config found
File-->>Config: User config content
end
end
Config->>Config: Track active config file paths
Config-->>Agent: Config loaded
Note over Agent,Labels: Runtime label reload (reload-labels command)
CLI->>Agent: reload-labels command
Agent->>Labels: Trigger label reload
Labels->>Config: Request section reload
Config->>File: Re-read [host labels] section from primary config
alt Section found in primary
File-->>Config: Section data
Config->>Config: Replace existing section options
Config-->>Labels: Updated section data
else Section not in primary, fallback exists
Config->>File: Re-read from fallback config
alt Found in fallback
File-->>Config: Section data
Config->>Config: Replace existing section options
Config-->>Labels: Updated section data
else Not found anywhere
Config-->>Config: Log warning, keep in-memory values
Config-->>Labels: Existing data retained
end
end
Labels->>Labels: Process section options
Labels-->>Agent: Labels updated (including removed ones)
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
test_inicfg_section_reload() covers a dropped option, repeated section headers, a missing file and a removed section; master fails the repeated and removed cases.
After the user netdata.conf has been read once, the reload no longer falls back to the stock file: it normally has no [host labels], so it wiped the labels. The stock fallback stays for agents that never read a user file.
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
The fixture used a fixed /tmp name, which is predictable and missing on Windows, and a write failure called fatal(). Use TMPDIR (or P_tmpdir) with mkstemp(), and fail the test instead when the fixture cannot be written.
The helper reopened the fixture with fopen("w"), which would create it
with mode 0666 if it went missing. Open the mkstemp() file with O_TRUNC
and no O_CREAT instead.
|
stelfrag
marked this pull request as ready for review
October 2, 2026 12:28
stelfrag
requested review from
Ancairon,
thiagoftsm and
vkalintiris
as code owners
October 2, 2026 12:28
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
PR Summary by QodoReload host labels from the Agent’s active config file
AI Description
Diagram
High-Level Assessment
Files changed (6)
|
Code Review by Qodo
1. A read error can erase active host labels
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+193
to
+195
| SECTION_LOCK(sect); | ||
| inicfg_option_remove_and_delete_all(sect, true); | ||
| SECTION_UNLOCK(sect); |
There was a problem hiding this comment.
1. A read error can erase active host labels 📜 Skill insight ☼ Reliability
inicfg_load() now deletes the existing section before reading the replacement, but it does not check ferror(fp) when fgets() stops and returns success even after a read error. If the config file opens but a later read fails, reload-labels treats the incomplete section as authoritative and prunes labels that were not read.
Agent Prompt
## Issue description
A targeted section reload deletes existing options before confirming that the replacement file was read successfully.
## Fix Focus Areas
- src/libnetdata/inicfg/inicfg_conf_file.c[188-200]
- src/libnetdata/inicfg/inicfg_conf_file.c[329-331]
## Recommended Fix
Check for stream read errors and only replace the existing section after a complete, successful read. Return failure without changing the section when reading fails.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+85
to
+88
| if(try_fallback) | ||
| nd_log(NDLS_DAEMON, NDLP_WARNING, | ||
| "CONFIG: cannot reload section [%s] from '%s' or '%s', using the values in memory", | ||
| section, netdata_conf_files.primary, netdata_conf_files.fallback); |
There was a problem hiding this comment.
2. Reload warnings hide the file error 📜 Skill insight ◔ Observability
netdata_conf_reload_section() warns that it cannot reload a section but omits the observed file error for both the primary and fallback attempts. When a file is missing or unreadable, the warning names the paths but does not distinguish the cause, leaving operators unable to tell which access failed and why.
Agent Prompt
## Issue description
Section-reload warnings identify the attempted files but omit the observed errors.
## Fix Focus Areas
- src/daemon/config/netdata-conf.c[73-92]
## Recommended Fix
Capture each load failure's cause before another attempt changes it, and include the relevant observed error alongside the expected reload source in the warning.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| return strdupz(path); | ||
| } | ||
|
|
||
| return filename_from_path_entry_strdupz(cwd, path); |
There was a problem hiding this comment.
3. Windows drive-relative labels do not reload 🐞 Bug ≡ Correctness
netdata_conf_path_absolute_strdupz() treats a path such as C:netdata.conf as relative and joins it to getcwd(), even though it is relative to drive C’s current directory. Startup reads the original -c path, but reload-labels reads the differently constructed path after the Agent changes directory.
Agent Prompt
## Issue description
Windows drive-relative `-c` paths are joined to the process working directory, producing a different path for reload.
## Fix Focus Areas
- src/daemon/config/netdata-conf.c[13-33]
## Recommended Fix
Resolve drive-relative paths using Windows path semantics before storing the reload path, and test that startup and reload address the same file.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+28
to
+30
| if(!getcwd(cwd, sizeof(cwd))) { | ||
| netdata_log_error("CONFIG: cannot get the current directory, '%s' will be reloaded as given.", path); | ||
| return strdupz(path); |
There was a problem hiding this comment.
4. Labels stay stale when the directory is unknown 🐞 Bug ☼ Reliability
netdata_conf_path_absolute_strdupz() stores a relative -c path unchanged when getcwd() fails. The initial load can succeed from the startup directory, but after the Agent changes directory, reload-labels tries that path elsewhere and retains the old labels.
Agent Prompt
## Issue description
A failed `getcwd()` leaves a relative config path that resolves differently after startup changes directory.
## Fix Focus Areas
- src/daemon/config/netdata-conf.c[23-33]
- src/daemon/config/netdata-conf.c[43-47]
## Recommended Fix
Do not proceed with an unresolvable relative reload path. Resolve and retain the startup file location before changing directory, or fail loading that path with a clear error.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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
reload-labelsto read host labels from the config file the Agent loaded, including-c FILE, and remove labels deleted from that file.Test Plan
/tmp/netdata-repro.conf:netdata -c /tmp/netdata-repro.conf.http://localhost:19999/api/v1/infoforhost_labels.repro_label. Before the fix, the label is absent because the Agent reads labels from the default config path.firsttosecondin that file and runnetdatacli reload-labels. Before the fix, the new value is not loaded. After the fix,host_labels.repro_labelissecond.[host labels]section and runnetdatacli reload-labelsagain. After the fix,repro_labeldisappears.Summary by CodeRabbit
New Features
-c.Bug Fixes