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

feat: MaxAlloc improvements #721

Merged
merged 3 commits into from
Jun 20, 2023
Merged

feat: MaxAlloc improvements #721

merged 3 commits into from
Jun 20, 2023

Conversation

kentquirk
Copy link
Contributor

Dependent on #719; can be reviewed before merging that.

Which problem is this PR solving?

In v1, Refinery allowed setting a MaxAlloc value that controlled the maximum amount of memory it would try to use. This is hard to keep configured properly, because the guidance is to set it to some fraction of the memory allocated to the pod. This PR implements two new values:

  • the full allocation amount, called AvailableMemory
  • the percentage refinery should use, called MaxMemory

The existing MaxAlloc value is still supported. If the first two are nonzero, they are multiplied to get the maximum allocation; otherwise, MaxAlloc is used. As in v1, if all of them are 0, refinery doesn't attempt to limit its own use.

I spent a while trying to find a way to allow the app to read the max allocation from the system, but it's pretty platform-dependent. It seemed better to just make it so that the value can be put into the environment in the Helm chart.

Short description of the changes

  • Add AvailableMemory and make it settable in the environment and on the command line
  • Implement the logic for calculating the value refinery should use and use that in the allocation check
  • Fix the names of the appropriate function calls and mocks
  • Regenerate files

@kentquirk kentquirk requested a review from a team as a code owner June 17, 2023 20:29
@kentquirk kentquirk added this to the v2.0 milestone Jun 17, 2023
config/file_config.go Outdated Show resolved Hide resolved
Base automatically changed from kent.memory_size to main June 20, 2023 16:47
@kentquirk kentquirk requested a review from TylerHelmuth June 20, 2023 18:43
@kentquirk kentquirk merged commit 62b67f5 into main Jun 20, 2023
@kentquirk kentquirk deleted the kent.max_alloc branch June 20, 2023 18:49
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