Feature/ciac 15710/decrease rasterize memory usage#43729
Feature/ciac 15710/decrease rasterize memory usage#43729MosheEichler wants to merge 4 commits intomasterfrom
Conversation
MosheEichler
added
the
ready-for-ai-review
🤖 AI-Powered Code Review AvailableYou can leverage AI-powered code review to assist with this PR! Available Commands:
|
Coverage Report
|
||||||||||||||||||||||||||||||
|
|
marketplace-ai-reviewer
removed
the
ready-for-ai-review
|
🤖 Analysis started. Please wait for results... |
|
Validate summary Verdict: PR can be force merged from validate perspective? ✅ |
🤖 AI Review DisclaimerThis review was generated by an AI-powered tool and may contain inaccuracies. Please be advised, and we extend our sincere apologies for any inconvenience this may cause. |
marketplace-ai-reviewer
left a comment
marketplace-ai-reviewer
left a comment
There was a problem hiding this comment.
Thanks for your work on this rasterize update! I've reviewed the code and have a few suggestions to refine the memory handling and align with our best practices.
Mainly, please update the cgroup memory calculation to return -1 instead of 0 when files are missing (updating the related tests and docstrings to match), and use max_chromes for the ratio calculation to prevent exceeding defaults. Also, make sure to swap demisto.results() for return_results() and clear the memory log lines after flushing to prevent duplicate logs.
Let me know if you have any questions!
@DeanArbel please review and approve the results generated by the AI Reviewer by responding 👍 on this comment.
| data=content.encode("utf-8"), | ||
| file_type=entryTypes["entryInfoFile"], | ||
| ) | ||
| demisto.results(result) |
There was a problem hiding this comment.
Please use return_results() instead of demisto.results().
|
|
||
| except (FileNotFoundError, ValueError, PermissionError) as e: | ||
| _mem_log("DEBUG", f"get_container_available_memory_bytes: Could not read cgroup v2 memory.max: {e}") | ||
| return 0 |
There was a problem hiding this comment.
get_container_available_memory_bytes should return -1 instead of 0 when cgroup v2 files are not found.
| max_chromes = min(safe_count, default_chromes) | ||
| max_tabs = min(safe_count, default_tabs) | ||
| # Scale rasterizations proportionally: keep the same ratio as the original defaults. | ||
| ratio = safe_count / max(default_chromes, 1) |
There was a problem hiding this comment.
The ratio calculation should use max_chromes instead of safe_count to prevent exceeding configured defaults.
| Only called when there is at least one log line to write. | ||
| """ | ||
| with _memory_log_lock: | ||
| lines = list(_memory_log_lines) |
There was a problem hiding this comment.
Consider clearing _memory_log_lines after copying it to prevent duplicate logs if _flush_memory_log is ever called multiple times.
| from rasterize import get_container_available_memory_bytes | ||
|
|
||
| mocker.patch("builtins.open", side_effect=FileNotFoundError("no cgroup")) | ||
| result = get_container_available_memory_bytes() |
There was a problem hiding this comment.
Update this test to expect -1 instead of 0.
|
|
||
| def test_zero_available_bytes_returns_minimum(self): | ||
| """ | ||
| Given: 0 bytes available (cgroup files unreadable) |
There was a problem hiding this comment.
Update the docstring to reflect that 0 bytes available means the container is at its memory limit, not that the files are unreadable.
3 participants
Status
Related Issues
https://jira-dc.paloaltonetworks.com/browse/CIAC-15710
Description
Added memory-aware execution to Rasterize CIAC-15710. Chrome concurrency is automatically scaled down based on available container memory.
A runtime watchdog captures a partial screenshot and exits gracefully if memory drops below a configurable threshold, preventing OOM crashes on heavy websites.
Must have