bin: run additional validation for --dry-run #11092
Conversation
ahayworth
requested review from
edsiper,
fujimotos and
koleini
as code owners
WalkthroughIn --dry-run mode, fluent-bit now calls flb_reload_property_check_all(config) before cleanup, stores its return value, always performs cleanup, and exits non-zero on validation failure or prints a success message and exits normally on success. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FB as "fluent-bit (--dry-run)"
participant Validator as "flb_reload_property_check_all"
User->>FB: Start fluent-bit with --dry-run
Note over FB: Parse config, init env/options/context
FB->>Validator: flb_reload_property_check_all(config)
alt Validator returns non-zero
Validator-->>FB: non-zero (validation failure)
Note over FB: Perform cleanup (destroy config/options/context)
FB-->>User: Exit non-zero (print validation failure)
else Validator returns zero
Validator-->>FB: zero (validation success)
Note over FB: Perform cleanup (destroy config/options/context)
FB-->>User: Print "configuration test succeeded" and exit zero
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/fluent-bit.c(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/fluent-bit.c (1)
src/flb_reload.c (1)
flb_reload_property_check_all(208-245)
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
ahayworth
force-pushed
the
ahayworth/sc-149733/additional-dry-run-validation
branch
from
4d255f8 to
538df01
Compare
|
Hi, could you rebase off master? |
cosmo0920
left a comment
cosmo0920
left a comment
There was a problem hiding this comment.
Patch is good but it needs to be rebased off master.
patrick-stephens
left a comment
patrick-stephens
left a comment
There was a problem hiding this comment.
I would expect some tests as well to validate this to ensure we do not regress in the future as well.
ahayworth
force-pushed
the
ahayworth/sc-149733/additional-dry-run-validation
branch
2 times, most recently
from
1a9755a to
d180b19
Compare
|
@patrick-stephens @cosmo0920 I've rebased and added a test - I'm not sure how extensive we'd like test coverage to be here, but if needed I can add additional cases. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/fluent-bit.c (1)
1438-1451: Cleanup logic now correctly handles both success and failure paths.The updated implementation properly addresses the resource leak issue raised in the previous review. Resources (
cf_optsandctx) are now cleaned up before exiting in both the success and failure cases, which prevents resource leaks during dry-run validation.The flow is clear:
- Validate configuration
- Always perform cleanup
- Exit based on validation result
Minor: Comment placement could be more precise.
The comment on line 1441 states "config test is done" but appears before the validation result is checked (lines 1446-1448). Consider moving this comment to line 1439 or rephrasing it to be more precise about the sequence of operations.
tests/runtime_shell/dry_run_invalid_property.sh (2)
11-24: Consider using unique temporary file names.The test uses a hardcoded path
/tmp/dry_run_invalid_property.yamlwhich could cause conflicts if multiple test instances run in parallel. Consider usingmktempto generate unique temporary file names:-cat > /tmp/dry_run_invalid_property.yaml << EOL +TEMP_CONFIG=$(mktemp /tmp/dry_run_invalid_property_XXXXXX.yaml) +cat > $TEMP_CONFIG << EOLThen reference
$TEMP_CONFIGthroughout the script and update the cleanup accordingly.
30-32: Same consideration applies to output file.The output file also uses a hardcoded path. If you adopt
mktempfor the config file (as suggested above), apply the same pattern here:-OUTPUT_FILE="/tmp/dry_run_invalid_property_output.txt" +OUTPUT_FILE=$(mktemp /tmp/dry_run_invalid_property_output_XXXXXX.txt)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/fluent-bit.c(1 hunks)tests/runtime_shell/dry_run_invalid_property.sh(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/fluent-bit.c (3)
src/flb_reload.c (1)
flb_reload_property_check_all(208-245)src/flb_lib.c (2)
flb_init_env(128-141)flb_destroy(228-263)src/config_format/flb_config_format.c (1)
flb_cf_destroy(151-157)
🔇 Additional comments (2)
tests/runtime_shell/dry_run_invalid_property.sh (2)
1-10: LGTM! Environment setup follows good practices.The environment variable handling with fallback defaults is robust and follows common patterns for shell test scripts. The echo statement on line 9 aids in debugging and test transparency.
42-64: LGTM! Comprehensive validation and proper cleanup.The test correctly validates all three expected behaviors:
- Non-zero exit code from dry-run
- Error message about the unknown property
- Reload validation failure message
The use of
|| trueon lines 43-44 prevents grep failures from prematurely terminating the script, and the cleanup (lines 47-49) ensures temporary files are removed before the final exit. The detailed diagnostic output on failure (lines 60-62) will aid in debugging test failures.
There was a problem hiding this comment.
Can you just confirm this actually runs in CI @ahayworth ?
There was a problem hiding this comment.
🤦 I did forget to add the test to the list to run, and added that here: 8180652
I'm not 100% sure which CI job would actually run those tests, though.
There was a problem hiding this comment.
I approved to run CI tests. Waiting for the results.
There was a problem hiding this comment.
Thanks - looks like CI has passed, but I am not sure which of the matrix jobs actually runs the test to double-check it.
ahayworth
force-pushed
the
ahayworth/sc-149733/additional-dry-run-validation
branch
from
0e3f6de to
8180652
Compare
ahayworth
force-pushed
the
ahayworth/sc-149733/additional-dry-run-validation
branch
from
8180652 to
9ef91de
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/runtime_shell/dry_run_invalid_property.sh (4)
4-7: Consider adding validation for FLB_BIN existence.The script assumes
$FLB_BINpoints to a valid executable but doesn't verify this before use (line 32). Consider adding a check after line 7:fi + +if [ ! -x "$FLB_BIN" ]; then + echo "Error: Fluent Bit binary not found or not executable at: $FLB_BIN" + exit 1 +fiThis would provide clearer error messages if the binary path is incorrect.
12-24: Consider using mktemp for safer temporary file creation.The script uses hardcoded paths in
/tmpwhich can lead to race conditions in concurrent test runs or permission issues in some environments.-cat > /tmp/dry_run_invalid_property.yaml << EOL +CONFIG_FILE=$(mktemp /tmp/dry_run_invalid_property.XXXXXX.yaml) +cat > "$CONFIG_FILE" << EOLThen update references throughout the script to use
$CONFIG_FILEinstead of the hardcoded path.
32-32: Consider using mktemp for the output file as well.Similar to the config file, the output file path is hardcoded. For line 31-32:
-OUTPUT_FILE="/tmp/dry_run_invalid_property_output.txt" +OUTPUT_FILE=$(mktemp /tmp/dry_run_invalid_property_output.XXXXXX.txt) $FLB_BIN --dry-run -c /tmp/dry_run_invalid_property.yaml > $OUTPUT_FILE 2>&1
46-49: Early cleanup may hinder debugging test failures.The temporary files are removed (lines 48-49) before the test validation completes (lines 55-64). If the test fails, developers won't be able to inspect the config or output files for debugging. Consider moving the cleanup to after line 64, or conditionally preserving files on failure:
+# Clean up only on success if [ "$EXIT_CODE" -ne 0 ] && [ "$UNKNOWN_PROPERTY" -gt 0 ] && [ "$RELOAD_ERROR" -gt 0 ]; then echo "Test passed: Fluent Bit --dry-run correctly detected invalid property and failed" + rm -f /tmp/dry_run_invalid_property.yaml + rm -f $OUTPUT_FILE exit 0 else echo "Test failed: Fluent Bit --dry-run should detect invalid properties and fail" echo "Exit code: $EXIT_CODE (expected non-zero)" echo "Unknown property message count: $UNKNOWN_PROPERTY (expected > 0)" echo "Reload error message count: $RELOAD_ERROR (expected > 0)" + echo "Config and output files preserved for debugging" exit 1 fi - -# Clean up -echo "Cleaning up..." -rm -f /tmp/dry_run_invalid_property.yaml -rm -f $OUTPUT_FILE
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/fluent-bit.c(1 hunks)tests/runtime_shell/CMakeLists.txt(1 hunks)tests/runtime_shell/dry_run_invalid_property.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/runtime_shell/CMakeLists.txt
🧰 Additional context used
🧬 Code graph analysis (1)
src/fluent-bit.c (3)
src/flb_reload.c (1)
flb_reload_property_check_all(208-245)src/flb_lib.c (2)
flb_init_env(128-141)flb_destroy(228-263)src/config_format/flb_config_format.c (1)
flb_cf_destroy(151-157)
🔇 Additional comments (2)
src/fluent-bit.c (1)
1438-1451: LGTM! The dry-run validation logic is well-structured.The implementation correctly validates properties using
flb_reload_property_check_all, performs cleanup regardless of the validation result, and exits with the appropriate status code. The cleanup sequence (init_env → destroy cf_opts → destroy ctx) ensures resources are properly released in both success and failure paths.Note: The
[reload]prefix in error messages (fromflb_reload_property_check_allat src/flb_reload.c:207-244) may be slightly misleading in a dry-run context, but this is already acknowledged in the PR description and can be addressed in a follow-up if needed.tests/runtime_shell/dry_run_invalid_property.sh (1)
55-64: Test validation logic looks solid.The test correctly validates all three expected conditions:
- Non-zero exit code (validation should fail)
- Error message about the unknown property
- Error message from reload validation
This comprehensive checking ensures the new dry-run validation behavior is working as intended.
|
@ahayworth Could you rebase off master? I fixed CI issue by no left device space. So, we need to confirm that all of CI tasks is green. |
There are certain kinds of invalid configurations that pass the
`--dry-run` check, but fail to start at run time. This commit addresses
the issue by re-using some of the hot reload validation logic.
For example, this config is trivially invalid, passes `--dry-run`, and
fails at runtime:
```
pipeline:
inputs:
- name: dummy
tag: test
invalid_property_that_does_not_exist: some_value
outputs:
- name: stdout
match: '*'
```
```
zsh ❮ fluent-bit --dry-run -c ~/tmp/fbconfig-bad-property.yaml
Fluent Bit v4.1.1
* Copyright (C) 2015-2025 The Fluent Bit Authors
* Fluent Bit is a CNCF sub-project under the umbrella of Fluentd
* https://fluentbit.io
______ _ _ ______ _ _ ___ __
| ___| | | | | ___ (_) | / | / |
| |_ | |_ _ ___ _ __ | |_ | |_/ /_| |_ __ __/ /| | `| |
| _| | | | | |/ _ \ '_ \| __| | ___ \ | __| \ \ / / /_| | | |
| | | | |_| | __/ | | | |_ | |_/ / | |_ \ V /\___ |__| |_
\_| |_|\__,_|\___|_| |_|\__| \____/|_|\__| \_/ |_(_)___/
configuration test is successful
zsh ❮ fluent-bit -c ~/tmp/fbconfig-bad-property.yaml
Fluent Bit v4.1.1
* Copyright (C) 2015-2025 The Fluent Bit Authors
* Fluent Bit is a CNCF sub-project under the umbrella of Fluentd
* https://fluentbit.io
______ _ _ ______ _ _ ___ __
| ___| | | | | ___ (_) | / | / |
| |_ | |_ _ ___ _ __ | |_ | |_/ /_| |_ __ __/ /| | `| |
| _| | | | | |/ _ \ '_ \| __| | ___ \ | __| \ \ / / /_| | | |
| | | | |_| | __/ | | | |_ | |_/ / | |_ \ V /\___ |__| |_
\_| |_|\__,_|\___|_| |_|\__| \____/|_|\__| \_/ |_(_)___/
[2025/10/31 16:05:55.268532000] [ info] [fluent bit] version=4.1.1, commit=, pid=21037
[2025/10/31 16:05:55.268808000] [ info] [storage] ver=1.5.3, type=memory, sync=normal, checksum=off, max_chunks_up=128
[2025/10/31 16:05:55.269140000] [ info] [simd ] disabled
[2025/10/31 16:05:55.269147000] [ info] [cmetrics] version=1.0.5
[2025/10/31 16:05:55.269407000] [ info] [ctraces ] version=0.6.6
[2025/10/31 16:05:55.269476000] [error] [config] dummy: unknown configuration property 'invalid_property_that_does_not_exist'. The following properties are allowed: samples, dummy, metadata, rate, interval_sec, interval_nsec, copies, start_time_sec, start_time_nsec, fixed_timestamp, flush_on_startup, and test_hang_on_exit.
[2025/10/31 16:05:55.269486000] [ help] try the command: fluent-bit -i dummy -h
[2025/10/31 16:05:55.269515000] [error] [engine] input initialization failed
```
With this commit, we can see that the additional validation from hot
reload catches the error right away:
```
zsh ❯ bin/fluent-bit --dry-run -c ~/tmp/fbconfig-bad-property.yaml
Fluent Bit v4.2.0
* Copyright (C) 2015-2025 The Fluent Bit Authors
* Fluent Bit is a CNCF sub-project under the umbrella of Fluentd
* https://fluentbit.io
______ _ _ ______ _ _ ___ __
| ___| | | | | ___ (_) | / | / |
| |_ | |_ _ ___ _ __ | |_ | |_/ /_| |_ __ __/ /| | `| |
| _| | | | | |/ _ \ '_ \| __| | ___ \ | __| \ \ / / /_| | | |
| | | | |_| | __/ | | | |_ | |_/ / | |_ \ V /\___ |__| |_
\_| |_|\__,_|\___|_| |_|\__| \____/|_|\__| \_/ |_(_)___/
[2025/10/31 16:07:50.402568000] [error] [config] dummy: unknown configuration property 'invalid_property_that_does_not_exist'. The following properties are allowed: samples, dummy, metadata, rate, interval_sec, interval_nsec, copies, start_time_sec, start_time_nsec, fixed_timestamp, flush_on_startup, and test_hang_on_exit.
[2025/10/31 16:07:50.402792000] [ help] try the command: bin/fluent-bit -i dummy -h
[2025/10/31 16:07:50.402800000] [error] [reload] check properties for input plugins is failed
```
(The logs of course now say `[reload]`, which is a little misleading...
we can clean that up, if desired).
Signed-off-by: Andrew Hayworth <[email protected]>
We need to perform the same partial initialization and releasing of resources whether or not the dry-run was successful, so this commit ensures that we call those functions before exiting either way. Signed-off-by: Andrew Hayworth <[email protected]>
This adds a runtime_shell test that runs fluent-bit in dry-run mode and ensures we are running the additional validation recently added. Specifically, we test that configuration property validation is happening, which is borrowed from the hot reload code and was not previously used for dry-run validation. Signed-off-by: Andrew Hayworth <[email protected]>
This was an oversight on my part - we actually need to list this as a test for it to be run. Signed-off-by: Andrew Hayworth <[email protected]>
ahayworth
force-pushed
the
ahayworth/sc-149733/additional-dry-run-validation
branch
from
9ef91de to
31924ee
Compare
|
@cosmo0920 Absolutely, rebased! |
4 participants
There are certain kinds of invalid configurations that pass the
--dry-runcheck, but fail to start at run time. This commit addressesthe issue by re-using some of the hot reload validation logic.
For example, this config is trivially invalid, passes
--dry-run, andfails at runtime:
With this commit, we can see that the additional validation from hot
reload catches the error right away:
(The logs of course now say
[reload], which is a little misleading...we can clean that up, if desired).
Enter
[N/A]in the box, if an item is not applicable to your change.Testing
Before we can approve your change; please submit the following in a comment:
If this is a change to packaging of containers or native binaries then please confirm it works for all targets.
ok-package-testlabel to test for all targets (requires maintainer to do).Documentation
Backporting
Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.