tests: integration: Adopt to trace for memory issues for macOS and optimize executions#12134
Open
cosmo0920 wants to merge 11 commits into
Open
tests: integration: Adopt to trace for memory issues for macOS and optimize executions#12134cosmo0920 wants to merge 11 commits into
cosmo0920 wants to merge 11 commits into
Conversation
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
… tests Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
📝 WalkthroughWalkthroughThe integration suite adds macOS ChangesIntegration suite changes
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant TestRunner
participant FluentBitManager
participant LeaksSupervisor
participant FluentBit
TestRunner->>FluentBitManager: enable LEAKS or LEAKS_STRICT
FluentBitManager->>LeaksSupervisor: launch leaks --atExit
LeaksSupervisor->>FluentBit: start monitored target
FluentBitManager->>FluentBit: send SIGHUP and SIGTERM
FluentBitManager->>LeaksSupervisor: wait for report and exit
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ec7be982b6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
cosmo0920
force-pushed
the
cosmo0920-adopt-to-trace-for-memory-issues-on-macos
branch
from
July 22, 2026 06:06
7451747 to
456cb0f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
tests/integration/scenarios/in_forward/tests/test_in_forward_001.py (1)
1674-1674: 🩺 Stability & Availability | 🔵 TrivialComplete the required memory-check validation before merge.
The supplied PR context says focused tests passed, but strict Valgrind verification and the final full-suite rerun remain incomplete. Run affected scenarios normally and with strict Valgrind, plus macOS
leakswhere supported, and report any blocker rather than relying on skips.As per coding guidelines, use the Python integration suite for end-to-end plugin behavior and run the focused scenario normally and with strict Valgrind, reporting blockers instead of silently skipping them.
🤖 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 `@tests/integration/scenarios/in_forward/tests/test_in_forward_001.py` at line 1674, Complete validation for the timeout behavior in the affected integration scenario: run the focused Python integration test normally and under strict Valgrind, run macOS leaks checks where supported, then rerun the full suite. Report any environmental or execution blocker explicitly instead of silently skipping the required checks.Source: Coding guidelines
tests/integration/test_macos_leaks_manager.py (1)
32-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMock
create_results_directoryto avoid writing real files during tests.
_prepare_startdoesn't stubcreate_results_directory, somanager.start()here creates real directories/log files undertests/integration/results/on disk instead of using pytest'stmp_path, unlike the equivalent test setup intest_fluent_bit_manager.py::test_start_uses_unique_valgrind_log_path.♻️ Proposed fix
monkeypatch.setattr(manager_module, "wait_for_port_to_be_free", lambda port, timeout: None) + monkeypatch.setattr( + FluentBitManager, + "create_results_directory", + lambda self, base_dir=None: str(tmp_path / "results"), + )🤖 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 `@tests/integration/test_macos_leaks_manager.py` around lines 32 - 67, Update the _prepare_start test fixture to monkeypatch manager_module.create_results_directory, directing it to create or return paths under tmp_path instead of the repository’s results directory. Keep the existing process and subprocess setup unchanged so manager.start() performs no real filesystem writes outside the temporary test directory.tests/integration/src/utils/fluent_bit_manager.py (1)
115-137: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCache the binary-content fallback like the version-info cache.
fluent_bit_binary_supports_config_propertyre-reads and lower-cases the entire Fluent Bit binary from disk on every call when--helpdoesn't mention the marker. With many scenarios now gating on capability detection (per PR objective), this repeats a full binary read per property check per test, unlike_read_binary_version_info, which is cached via_BINARY_VERSION_INFO_CACHE.♻️ Proposed caching fix
+_BINARY_CONTENTS_CACHE = {} + + def fluent_bit_binary_supports_config_property(property_name, binary_path=None): resolved_path = _resolve_binary_path(binary_path) property_marker = property_name.lower() try: result = subprocess.run( [resolved_path, "--help"], capture_output=True, text=True, check=False, ) help_output = f"{result.stdout}\n{result.stderr}".lower() if property_marker in help_output: return True except OSError: pass - try: - binary_contents = Path(resolved_path).read_bytes().lower() - except OSError: - return False + if resolved_path not in _BINARY_CONTENTS_CACHE: + try: + _BINARY_CONTENTS_CACHE[resolved_path] = Path(resolved_path).read_bytes().lower() + except OSError: + _BINARY_CONTENTS_CACHE[resolved_path] = None + + binary_contents = _BINARY_CONTENTS_CACHE[resolved_path] + if binary_contents is None: + return False return property_marker.encode("utf-8") in binary_contents🤖 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 `@tests/integration/src/utils/fluent_bit_manager.py` around lines 115 - 137, Cache the lower-cased binary contents used by the fallback in fluent_bit_binary_supports_config_property, using a module-level cache keyed by the resolved binary path analogous to _BINARY_VERSION_INFO_CACHE. Reuse the cached bytes for subsequent property checks, while preserving the existing OSError behavior and help-output check.
🤖 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
`@tests/integration/scenarios/hot_reload_watch/tests/test_hot_reload_watch_001.py`:
- Around line 73-78: Update the cleanup flow around stop_otlp_server() and
shutil.rmtree() so a failure stopping the OTLP server cannot prevent removal of
self.runtime_dir. Preserve the existing FLB shutdown behavior, but isolate or
otherwise contain the stop_otlp_server exception and ensure shutil.rmtree(...,
ignore_errors=True) always executes.
In `@tests/integration/src/utils/fluent_bit_manager.py`:
- Around line 355-377: Update _wait_for_leaks_target_pid so the timeout path
calls self._force_stop() before raising FluentBitStartupError, ensuring the
still-running leaks supervisor and Fluent Bit child are terminated and resources
are cleaned up. Preserve the existing timeout message and earlier process-exit
handling.
- Around line 307-317: Add a timeout to the requests.post call in
trigger_http_reload, matching the existing 0.5-second timeout used by
get_reload_status, while preserving the current response validation and JSON
return behavior.
- Around line 333-354: Update FluentBitManager._build_leaks_command to use the
macOS leaks tool’s single-dash flags -quiet, -fullStacks, and -atExit, while
preserving the existing -- separator before the target command and all remaining
arguments.
---
Nitpick comments:
In `@tests/integration/scenarios/in_forward/tests/test_in_forward_001.py`:
- Line 1674: Complete validation for the timeout behavior in the affected
integration scenario: run the focused Python integration test normally and under
strict Valgrind, run macOS leaks checks where supported, then rerun the full
suite. Report any environmental or execution blocker explicitly instead of
silently skipping the required checks.
In `@tests/integration/src/utils/fluent_bit_manager.py`:
- Around line 115-137: Cache the lower-cased binary contents used by the
fallback in fluent_bit_binary_supports_config_property, using a module-level
cache keyed by the resolved binary path analogous to _BINARY_VERSION_INFO_CACHE.
Reuse the cached bytes for subsequent property checks, while preserving the
existing OSError behavior and help-output check.
In `@tests/integration/test_macos_leaks_manager.py`:
- Around line 32-67: Update the _prepare_start test fixture to monkeypatch
manager_module.create_results_directory, directing it to create or return paths
under tmp_path instead of the repository’s results directory. Keep the existing
process and subprocess setup unchanged so manager.start() performs no real
filesystem writes outside the temporary test directory.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4bc4836a-0045-47b5-ba27-b7e58b418e9a
📒 Files selected for processing (28)
tests/integration/README.mdtests/integration/conftest.pytests/integration/pytest.initests/integration/requirements.txttests/integration/run_tests.pytests/integration/scenarios/hot_reload_watch/tests/test_hot_reload_watch_001.pytests/integration/scenarios/in_elasticsearch/tests/test_in_elasticsearch_001.pytests/integration/scenarios/in_forward/config/in_forward_storage_limit_multi_output.yamltests/integration/scenarios/in_forward/config/in_forward_storage_limit_shared_success_output.yamltests/integration/scenarios/in_forward/config/in_forward_storage_limit_single_output.yamltests/integration/scenarios/in_forward/tests/test_in_forward_001.pytests/integration/scenarios/in_opentelemetry/tests/test_in_opentelemetry_001.pytests/integration/scenarios/in_tail/config/tail_ignore_active_older.yamltests/integration/scenarios/in_tail/tests/test_in_tail_001.pytests/integration/scenarios/in_tcp/tests/test_in_tcp_001.pytests/integration/scenarios/in_udp/tests/test_in_udp_001.pytests/integration/scenarios/out_es/tests/test_out_es_ndjson_action_line_001.pytests/integration/scenarios/out_forward/tests/test_out_forward_metadata_001.pytests/integration/scenarios/out_forward/tests/test_out_forward_secure_001.pytests/integration/scenarios/out_http/tests/test_out_http_001.pytests/integration/scenarios/out_kafka/tests/test_out_kafka_001.pytests/integration/src/server/otlp_server.pytests/integration/src/utils/fluent_bit_manager.pytests/integration/src/utils/leaks.pytests/integration/src/utils/memory_check.pytests/integration/test_leaks_utils.pytests/integration/test_macos_leaks_manager.pytests/integration/tests/test_fluent_bit_manager.py
💤 Files with no reviewable changes (1)
- tests/integration/pytest.ini
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/integration/src/utils/fluent_bit_manager.py (2)
190-200: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winValidate incompatible memory-check modes before allocating startup state.
start()creates the results directory and mutatesENV_FLB_HTTP_MONITORING_PORTbefore rejecting simultaneousVALGRINDandLEAKS. A failed startup therefore leaves filesystem and process-wide environment state behind. Move this guard beforecreate_results_directory()andset_http_monitoring_port(), or clean up all startup side effects on failure.Proposed fix
def start(self): if not self.config_path or not os.path.exists(self.config_path): raise FileNotFoundError(f"Config file {self.config_path} does not exist") if not os.path.isfile(self.binary_absolute_path): raise FileNotFoundError( f"Fluent Bit binary {self.binary_absolute_path} does not exist. " "Set FLUENT_BIT_BINARY or build build/bin/fluent-bit." ) if not os.access(self.binary_absolute_path, os.X_OK): raise PermissionError(f"Fluent Bit binary {self.binary_absolute_path} is not executable") + if valgrind_enabled() and leaks_enabled(): + raise FluentBitStartupError("VALGRIND and LEAKS cannot be enabled together") # create temporary directory for logs out_dir = self.create_results_directory() ... - if valgrind_enabled() and leaks_enabled(): - raise FluentBitStartupError("VALGRIND and LEAKS cannot be enabled together")As per coding guidelines, “Validate both success and failure paths” and preserve shared lifecycle state.
🤖 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 `@tests/integration/src/utils/fluent_bit_manager.py` around lines 190 - 200, Move the incompatible-mode guard in start() to execute before create_results_directory(), assigning startup paths, and set_http_monitoring_port(). Preserve the existing FluentBitStartupError for simultaneous valgrind_enabled() and leaks_enabled() while ensuring failed validation leaves no filesystem or process-wide environment state behind.Source: Coding guidelines
258-270: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winResolve the leaks supervisor and target as one shutdown state.
Lines [259]-[260] call
poll()twice. If the supervisor exits between those calls,return_coderemainsNonewhilesupervisor_runningbecomes false. In leaks mode, the target is then signaled at Line [262], but Line [264] skips waiting entirely; a still-running target is not verified or cleaned up, and strict leak validation receives an invalid status.Capture
poll()once and explicitly force-stop or otherwise verify the leaks target when the supervisor has already exited. Add a regression test for that failure path.Proposed fix
- return_code = self.process.poll() - supervisor_running = self.process.poll() is None + return_code = self.process.poll() + supervisor_running = return_code is None if supervisor_running or (leaks_enabled() and self.target_pid): self.send_signal(signal.SIGTERM) if supervisor_running: try: timeout = LEAKS_EXIT_TIMEOUT if leaks_enabled() else 10 return_code = self.process.wait(timeout=timeout) except subprocess.TimeoutExpired: self._force_stop() return_code = self.process.returncode + elif leaks_enabled() and self.target_pid: + self._force_stop()As per coding guidelines, “Validate both success and failure paths” for shared lifecycle changes.
🤖 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 `@tests/integration/src/utils/fluent_bit_manager.py` around lines 258 - 270, The shutdown logic around the supervisor lifecycle must capture process status once and handle the target when the supervisor has already exited. Update the relevant manager shutdown method to reuse a single poll result, explicitly wait for or force-stop the leaks target whenever leaks mode is enabled, and ensure return_code is valid for strict leak validation. Add regression coverage for the supervisor-exits-between-polls failure path, including both successful and failure cleanup outcomes.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@tests/integration/src/utils/fluent_bit_manager.py`:
- Around line 190-200: Move the incompatible-mode guard in start() to execute
before create_results_directory(), assigning startup paths, and
set_http_monitoring_port(). Preserve the existing FluentBitStartupError for
simultaneous valgrind_enabled() and leaks_enabled() while ensuring failed
validation leaves no filesystem or process-wide environment state behind.
- Around line 258-270: The shutdown logic around the supervisor lifecycle must
capture process status once and handle the target when the supervisor has
already exited. Update the relevant manager shutdown method to reuse a single
poll result, explicitly wait for or force-stop the leaks target whenever leaks
mode is enabled, and ensure return_code is valid for strict leak validation. Add
regression coverage for the supervisor-exits-between-polls failure path,
including both successful and failure cleanup outcomes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4fbe2b4d-9724-49b4-90cb-0e89b72007d4
📒 Files selected for processing (4)
tests/integration/scenarios/hot_reload_watch/tests/test_hot_reload_watch_001.pytests/integration/src/utils/fluent_bit_manager.pytests/integration/test_macos_leaks_manager.pytests/integration/tests/test_fluent_bit_manager.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/integration/tests/test_fluent_bit_manager.py
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
1 participant
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.
Description
Problem
The integration suite supported Valgrind-based memory checking but lacked an equivalent workflow for macOS. It also needed
additional isolation and lifecycle handling to run reliably with multiple pytest workers.
Changes
Add macOS leaks --atExit support through --leaks, --leaks-strict, LEAKS, and LEAKS_STRICT.
Supervise Fluent Bit under leaks, signal the actual Fluent Bit child during reload and shutdown, and preserve separate Fluent
Bit and leak reports.
Reject incompatible memory-check configurations and require serial execution for macOS leak checks.
Add pytest-xdist and document file-level parallel execution with -n --dist loadfile.
Generate unique result directories for concurrent Fluent Bit processes.
Centralize Valgrind/macOS-leaks detection and apply memory-check-aware timeouts across scenarios.
Cache Fluent Bit version information and add helpers for detecting supported service and input-plugin properties.
Skip scenarios cleanly when the selected binary lacks required configuration properties.
Wait for OTLP gRPC servers and their threads to stop completely during teardown.
Improve macOS portability with shorter Unix socket paths, page-size-aware storage limits, and longer TLS connection timeouts.
Stabilize timing-sensitive scenarios:
Verification
Focused verification completed:
Strict Valgrind verification was started but interrupted before completion. A full-suite rerun after the final OpenTelemetry
concurrency adjustment has not yet been completed.
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
leaks, including environment options and supervision details.