feat(bug-finding): warn users if resource config conflicts with machi…#159
Open
feat(bug-finding): warn users if resource config conflicts with machi…#159
Conversation
azchin
requested changes
Mar 30, 2026
Collaborator
azchin
left a comment
azchin
left a comment
There was a problem hiding this comment.
Thanks for the PR and sorry for the late response! I have a few requests detailed below. We also use a few linters and formatters in our CI workflow, so I recommend running them manually before pushing.
oss_crs/src/utils.py
Outdated
| except(OSError): | ||
| pass | ||
|
|
||
| memeinfo = Path("/proc/meminfo") |
Collaborator
There was a problem hiding this comment.
meminfo typo (caught by ruff)
oss_crs/src/crs_compose.py
Outdated
| machine_cpu_count = os.cpu_count() | ||
| machine_memory = get_host_memory() | ||
|
|
||
| if machine_cpu_count is None or machine_memory == 0: |
Collaborator
There was a problem hiding this comment.
(C1) Let's get machine_memory to return None on failure
oss_crs/src/utils.py
Outdated
| except subprocess.CalledProcessError as e: | ||
| raise RuntimeError(f"Error removing {path} with Docker: {e}") | ||
|
|
||
| def get_host_memory() -> int: |
Collaborator
There was a problem hiding this comment.
Return val as int | None, see (C1)
oss_crs/src/utils.py
Outdated
| for line in meminfo.read_text().splitlines(): | ||
| if line.startswith("MemTotal:"): | ||
| return int(line.split()[1])*1024 | ||
| return 0 |
oss_crs/src/crs_compose.py
Outdated
| except ValueError as e: | ||
| log_warning(f"Failed to parse memory for {name}: {e}") | ||
|
|
||
| cpu_check = max_cpus_required <= machine_cpu_count |
Collaborator
There was a problem hiding this comment.
I think this is an off-by-one, since os.cpu_count() returns number of CPUs but cpuset is indexed by zero, so we'd get range of 0-3 on 4-core machine. max([0, 1, 2, 3]) == 3 < 4
max_cpus_required < machine_cpu_count would be the correct check. But please check/test for this edge case dynamically.
Author
There was a problem hiding this comment.
You're absolutely right ^^'... I will also add dynamic testing.
…ne resources Signed-off-by: tkqdldk <samia.20.bouchaal@gmail.com>
tkqdldk
force-pushed
the
feat/resource-config-warn
branch
from
c274218 to
867fac0
Compare
… memory output Signed-off-by: tkqdldk <samia.20.bouchaal@gmail.com>
Signed-off-by: tkqdldk <samia.20.bouchaal@gmail.com>
tkqdldk
force-pushed
the
feat/resource-config-warn
branch
from
867fac0 to
c55125d
Compare
Collaborator
|
Hm I'm not seeing the error messages implemented in your PR in my output. The build does indeed abort, but that's because of Docker's error handling and happens after I can take a deeper look at what we're missing after the weekend, if needed. |
Author
Hi, sorry for the late response, so the validation only runs before run currently, as I understood that would be the only need. I'll extend the check to build-target so the warning shows here too. |
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
2 participants
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.
Closes #49
Description
Adds resources check and a warning when the compose file resource configurations (max CPU and total memory) exceed machine resources. The check runs in "Validate Configuration for Running" phase, before Docker attempts to start containers.
The function emits a warning but does not stop the execution.
Changes
get_host_total_memory()inutils.py_validate_machine_resources()incrs_compose.py__validate_before_runValidation
All existing tests pass.
P.S. : Can't assign a reviewer directly, cc @azchin for review