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

Update -Cprocessors on scale compute #681

Merged
merged 1 commit into from
Nov 28, 2024

Conversation

WalBeh
Copy link
Contributor

@WalBeh WalBeh commented Nov 26, 2024

Summary of changes

Improve Resource Requests and Limits Handling for start_cluster function. And also change the jvm -CProcessor setting which should reliably report the number of CPUs available in a docker environment.

  • Updated start_cluster Function:

    • Modified to accept both resource_requests and resource_limits as separate parameters.
    • Ensured proper handling and application of these resources when initializing clusters.
  • Refactored utils.py:

    • Corrected conditional checks to accurately apply resource limits and requests.
    • Enhanced error handling for missing resource specifications.
  • Enhanced Test Cases in test_change_compute.py:

    • Fixed asynchronous calls by properly awaiting the start_cluster coroutine, as the test now needs to pull in the crate container commands from the Statefulset.
    • Updated tests to verify that CPU and memory limits, requests and -Cprocessors are set correctly on scale compute.
    • Added assertions to ensure that resource configurations are applied as intended.

Thanks a lot to @Taliik for the help with the changes in the tests.

Checklist

  • Link to issue this PR refers to: https://github.com/crate/cloud/issues/2277
  • Relevant changes are reflected in CHANGES.rst
  • Added or changed code is covered by tests
  • Documentation has been updated if necessary
  • Changed code does not contain any breaking changes (or this is a major version change)

@WalBeh WalBeh requested review from juanpardo and tomach November 26, 2024 16:18
Copy link
Contributor

@tomach tomach left a comment

Choose a reason for hiding this comment

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

💯 LGTM! just one minor suggestion, if you feel like it. and pls add a short note to CHANGES.rst

crate/operator/change_compute.py Show resolved Hide resolved
@WalBeh WalBeh force-pushed the bw/update-cprocessor-onscale-compute branch from 0bea2aa to f0669be Compare November 27, 2024 13:57
@WalBeh WalBeh merged commit 97c3ed1 into master Nov 28, 2024
5 checks passed
@WalBeh WalBeh deleted the bw/update-cprocessor-onscale-compute branch November 28, 2024 08:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants