smaller job defaults requirements #4785
Conversation
WalkthroughThe pull request introduces several changes to the Bacalhau system, including updates to the formatting of memory and disk capacity outputs in the CLI node columns by switching from the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
💤 Files with no reviewable changes (3)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/cli/node/columns.go(2 hunks)pkg/config/defaults.go(5 hunks)
🔇 Additional comments (3)
cmd/cli/node/columns.go (2)
7-7: LGTM: Good choice of package for human-readable formatting
The go-humanize package is a well-maintained library that provides consistent human-readable number formatting.
103-105: LGTM: Consistent formatting implementation
The implementation consistently uses humanize.Bytes() for both memory and disk capacity formatting, which aligns with the PR objective of unifying the library used for printing human-readable byte values.
Also applies to: 111-113
pkg/config/defaults.go (1)
54-56: LGTM: Reasonable increase in resource allocation
Increasing allocated capacity from 70% to 80% for CPU, Memory, and Disk is a good balance between:
- Better resource utilization
- Maintaining sufficient headroom for system processes
| @@ -63,7 +63,7 @@ var Default = types.Bacalhau{ | |||
| Task: types.BatchTaskDefaultConfig{ | |||
| Resources: types.ResourcesConfig{ | |||
| CPU: "500m", | |||
| Memory: "1Gb", | |||
| Memory: "512Mb", | |||
There was a problem hiding this comment.
💡 Codebase verification
Memory configuration appears to be reduced from previous defaults
Based on the search results, I can make the following observations:
- The production configuration (
pkg/config_legacy/configenv/production.go) previously used 1Gb memory - Test configurations (
pkg/test/compute/resourcelimits_test.goandpkg/config/types/bacalhau_test.go) use 1Gb memory - The new default of 512Mb is a significant reduction from the previous 1Gb setting
Since this is a significant reduction in memory allocation and there's evidence of higher memory usage in production and test configurations, this warrants attention.
pkg/config_legacy/configenv/production.go: Previously used 1Gbpkg/config/defaults.go: New default of 512Mbpkg/test/compute/resourcelimits_test.go: Tests expecting 1Gb
🔗 Analysis chain
Verify if 512Mb is sufficient for typical workloads
While reducing memory requirements to 512Mb aligns with the goal of running on smaller EC2 instances, we should verify this doesn't impact typical workloads.
Consider implementing memory auto-scaling or providing documentation for users to override these defaults when needed for memory-intensive workloads.
Also applies to: 75-75, 84-84, 93-93
Let me try a different approach to gather information about memory usage patterns in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for memory-related issues or discussions in recent PRs and issues
gh search issues "memory OOM out of memory" --repo bacalhau-project/bacalhau --state all --limit 10
# Look for memory configuration mentions in tests
rg -l "Memory.*[0-9]+[MGT]b" "test" || true
Length of output: 2860
Script:
#!/bin/bash
# Search for memory configurations in the entire codebase
rg -A 2 -B 2 'Memory.*[0-9]+[MGT]b' || true
# Look for memory-related configurations and defaults
fd -e yaml -e yml | xargs rg 'memory|Memory' || true
# Look for memory-related test configurations
ast-grep --pattern 'Memory: "$_"'
Length of output: 4163
wdbaruni
deleted the
eng-192-compute-node-fails-to-start-on-small-nodes
branch
Smaller job resource requirements so that jobs can run on smaller EC2 instances by default.
This PR also introduce the following changes:
Summary by CodeRabbit
New Features
Bug Fixes