Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix memory limits reading on fly.io/firecracker #5114

Merged
merged 1 commit into from
Jan 22, 2025

Conversation

HugoCasa
Copy link
Contributor

@HugoCasa HugoCasa commented Jan 22, 2025

Important

Fix memory limit reading in get_memory() for fly.io/firecracker by using /proc/meminfo for high limits.

  • Behavior:
    • Modify get_memory() in worker.rs to handle high memory limits by reading from /proc/meminfo.
    • Introduce get_memory_from_meminfo() to parse total memory from /proc/meminfo.
  • Functions:
    • get_memory() now checks if memory limit exceeds 1 PB and uses get_memory_from_meminfo() if true.
    • get_memory_from_meminfo() parses MemTotal from /proc/meminfo and returns memory in bytes.

This description was created by Ellipsis for 3b08e77. It will automatically update as commits are pushed.

@HugoCasa HugoCasa requested a review from rubenfiszel as a code owner January 22, 2025 11:49
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Reviewed everything up to 3b08e77 in 1 minute and 48 seconds

More details
  • Looked at 56 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. backend/windmill-common/src/worker.rs:515
  • Draft comment:
    Consider optimizing get_memory_from_meminfo by reading only the necessary line from /proc/meminfo instead of the entire file.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The suggestion is about optimizing memory usage and I/O by reading less data. The current implementation uses parse_file() which runs cat on the entire file. The optimization would be possible by using something like head or grep instead. However, /proc/meminfo is typically a small file (a few KB at most) and this function is not called frequently - it's only used as a fallback when cgroup memory limits are very high.
    The optimization might be premature - we don't have evidence that this is causing performance issues. The current implementation is also simpler and more maintainable.
    While the optimization is technically possible, the benefit would be minimal given the small file size and infrequent usage. The current code is more maintainable.
    Delete the comment as it suggests an optimization that would add complexity for minimal benefit in a rarely-used code path.
2. backend/windmill-common/src/worker.rs:545
  • Draft comment:
    Define a constant for the high memory limit threshold to improve code readability.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The condition to check if memory_limit is super high uses a hardcoded value which is not self-explanatory. It would be better to define a constant for this value to improve code readability.

Workflow ID: wflow_xBS4NelDTGdPW5lT


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@rubenfiszel rubenfiszel merged commit a8fa75a into main Jan 22, 2025
6 of 7 checks passed
@rubenfiszel rubenfiszel deleted the hc/fix-fly-mem-limit-reading branch January 22, 2025 11:53
@github-actions github-actions bot locked and limited conversation to collaborators Jan 22, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants