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

Set resource limits and requests for HA #62

Merged
merged 11 commits into from
Aug 30, 2024
Merged

Set resource limits and requests for HA #62

merged 11 commits into from
Aug 30, 2024

Conversation

michaeldmitry
Copy link
Contributor

@michaeldmitry michaeldmitry commented Aug 26, 2024

Solution

A generic implementation for HA charms to provide resources_requests and a container_name to patch it with those k8s resource requests.

Context

  • Setting limits portion would be a charm's config option (e.g: cpu_limits, memory_limits)
    • If the charm (e.g: tempo) have different config option names for cpu and memory limits, you can pass those names to the constructor's resources_limit_options.
  • Setting requests portion would be hardcoded in the charm code.
    • Constructor's take a callable resources_requests and a container_name to get the dictionary of cpu and memory requests. The charm can pass a callable that sets dynamic requests values if needed.
  • Setting unit status based on the outcome of the patch operation using Transform KubernetesComputeResourcesPatch "Push" statuses into "Pull" statuses observability-libs#107

Copy link
Contributor

@PietroPasotti PietroPasotti left a comment

Choose a reason for hiding this comment

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

I need a little bit more documentation/context to know what I'm reviewing

src/cosl/coordinated_workers/coordinator.py Show resolved Hide resolved
src/cosl/coordinated_workers/coordinator.py Outdated Show resolved Hide resolved
src/cosl/coordinated_workers/coordinator.py Outdated Show resolved Hide resolved
src/cosl/coordinated_workers/coordinator.py Show resolved Hide resolved
src/cosl/coordinated_workers/worker.py Show resolved Hide resolved
Copy link
Contributor

@PietroPasotti PietroPasotti left a comment

Choose a reason for hiding this comment

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

Some more thoughts

src/cosl/coordinated_workers/worker.py Show resolved Hide resolved
tests/test_coordinated_workers/test_coordinator_status.py Outdated Show resolved Hide resolved
Copy link
Contributor

@PietroPasotti PietroPasotti left a comment

Choose a reason for hiding this comment

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

some things I missed in my previous review, sorry about that!

src/cosl/coordinated_workers/coordinator.py Outdated Show resolved Hide resolved
src/cosl/coordinated_workers/coordinator.py Outdated Show resolved Hide resolved
src/cosl/coordinated_workers/coordinator.py Outdated Show resolved Hide resolved
src/cosl/coordinated_workers/coordinator.py Outdated Show resolved Hide resolved
src/cosl/coordinated_workers/coordinator.py Outdated Show resolved Hide resolved
Copy link
Contributor

@PietroPasotti PietroPasotti left a comment

Choose a reason for hiding this comment

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

nice job!

@michaeldmitry michaeldmitry merged commit 8ea433b into main Aug 30, 2024
5 checks passed
@michaeldmitry michaeldmitry deleted the OPENG-2205 branch August 30, 2024 09:19
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