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

Use total_memory to calculate "time" Docker limit #7203

Merged
merged 2 commits into from
Jun 22, 2020

Conversation

humitos
Copy link
Member

@humitos humitos commented Jun 17, 2020

4Gb servers:

  • total memory: 3417
  • memory limit: 2700
  • time limit (using memory limit): 700
  • time limit (using total memory): 900 (15 minutes)

8Gb servers:

  • total memory: 7935
  • memory limit: 7200
  • time limit (using memory limit): 1800
  • time limit (using total memory): 2000 (33 minutes)

These numbers are more similar to what we communicate in
https://docs.readthedocs.io/en/stable/builds.html. We are giving an increase of
3 extra minutes in Read the Docs for Business instead of 4 minutes less in Read
the Docs Community.

>>> round(3417 - 750, -2)
2700
>>> round(2700 * 0.25, -2)
700.0
>>> round(3417 * 0.25, -2)
900.0
>>> round(7935 - 750, -2)
7200
>>> round(7200 * 0.25, -2)
1800.0
>>> round(7935 * 0.25, -2)
2000.0

4Gb servers:
 - total memory: 3417
 - memory limit: 2700
 - time limit (using memory limit): 700
 - time limit (using total memory): 900 (15 minutes)

8Gb servers:
 - total memory: 7935
 - memory limit: 7200
 - time limit (using memory limit): 1800
 - time limit (using total memory): 2000 (33 minutes)

These numbers are more similar to what we communicate in
https://docs.readthedocs.io/en/stable/builds.html. We are giving an increase of
3 extra minutes in Read the Docs for Business instead of 4 minutes less in Read
the Docs Community.

```
>>> round(3417 - 750, -2)
2700
>>> round(2700 * 0.25, -2)
700.0
>>> round(3417 * 0.25, -2)
900.0
>>> round(7935 - 750, -2)
7200
>>> round(7200 * 0.25, -2)
1800.0
>>> round(7935 * 0.25, -2)
2000.0
```
@humitos humitos requested review from agjohnson and a team June 17, 2020 12:14
Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

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

I'm still not convinced that we shouldn't just be setting this explicitly, instead of doing a bunch of magic to try and have it line up with our intended goal of an obvious set of values for specific servers. This seems fine I guess, but still over-engineered to me.

Copy link
Contributor

@agjohnson agjohnson left a comment

Choose a reason for hiding this comment

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

This makes more sense 👍

@agjohnson
Copy link
Contributor

I don't think it helps to spend more time on reverting this logic. The original approach using static values set the wrong limits on multiple occasions -- the approach we have now has been working fine.

@agjohnson agjohnson merged commit cf90cc3 into master Jun 22, 2020
@agjohnson agjohnson deleted the humitos/use-total-memory-for-docker-limits branch June 22, 2020 21:37
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.

3 participants