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.

Sign up for GitHub

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

Jump to bottom

feat: allow suffixes on memory size in config #719

Merged
merged 4 commits into from
Jun 20, 2023
Merged

feat: allow suffixes on memory size in config #719

merged 4 commits into from
Jun 20, 2023

Conversation

kentquirk
Copy link
Contributor

@kentquirk kentquirk commented Jun 17, 2023 •
edited
Loading

Based on #718, so ready to review independently.

Which problem is this PR solving?

  • Up to now, setting MaxAlloc requires typing or copy-pasting large numbers that are hard to read and visually parse. This PR allows using standard suffixes like "Gb" and "MiB". This responds to a request from Honeycomb field engineering.

Short description of the changes

  • Import the docker library for parsing memory sizes
  • Create a MemorySize type with marshal and unmarshal methods, and use it as appropriate
  • Update the metadata and templates
  • Regenerate all the things

@kentquirk kentquirk requested a review from a team as a code owner June 17, 2023 16:57
@kentquirk kentquirk changed the base branch from main to kent.sample_rate_warning June 17, 2023 16:59
@kentquirk kentquirk mentioned this pull request Jun 17, 2023
Merged
@kentquirk kentquirk force-pushed the kent.sample_rate_warning branch from cfc28d6 to 300e7bf Compare June 19, 2023 15:29
Base automatically changed from kent.sample_rate_warning to main June 20, 2023 16:41
@kentquirk kentquirk requested a review from TylerHelmuth June 20, 2023 16:41
@kentquirk kentquirk merged commit 406c4f2 into main Jun 20, 2023
@kentquirk kentquirk deleted the kent.memory_size branch June 20, 2023 16:47
kentquirk added a commit that referenced this pull request Jun 20, 2023
@kentquirk
feat: MaxAlloc improvements (#721)
62b67f5 
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TylerHelmuth TylerHelmuth TylerHelmuth approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kentquirk @TylerHelmuth