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

Move docker limits back to setting #7023

Merged
merged 4 commits into from May 6, 2020
Merged

Move docker limits back to setting #7023

merged 4 commits into from May 6, 2020

Conversation

agjohnson
Copy link
Contributor

@agjohnson agjohnson commented May 5, 2020

This setting was moved to a constant, which was harder to override and
replicated a lot of the same logic we had for settings. We hit a number
of issues around the DOCKER_LIMITS setting existing, and these constants
not being used, etc.

This hard codes a memory limit, which would be used for dev
environments outside Docker, and perhaps tests. Limits are 1g memory
and 600s build time -- up from 200m memory limit that is too small.

Production guessing of memory/time will be in ops settings now.

Refs #6983

agjohnson added 3 commits May 5, 2020
This setting was moved to a constant, which was harder to override and
replicated a lot of the same logic we had for settings. We hit a number
of issues around the DOCKER_LIMITS setting existing, and these constants
not being used, etc.

This hard codes a memory limit, which would be used for dev
environments outside Docker, and perhaps tests. Limits are 1g memory
and 600s build time -- up from 200m memory limit that is too small.

Production guessing of memory/time will be in ops settings now.
This will require two small changes for these additional settings.
@agjohnson agjohnson requested a review from May 5, 2020
'memory': '1g',
'time': 600,
}
if self.RTD_IS_PRODUCTION:
Copy link
Member

@stsewd stsewd May 5, 2020

Choose a reason for hiding this comment

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

What about just checking for DEBUG?

Copy link
Member

@ericholscher ericholscher May 6, 2020

Choose a reason for hiding this comment

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

Yea, I don't like adding a new setting for this, we already have enough settings that we should be able to figure out one that is good for this.

@ericholscher ericholscher merged commit 134d4e0 into master May 6, 2020
0 of 2 checks passed
@ericholscher ericholscher deleted the agj/move-docker-limits branch May 6, 2020
@ericholscher ericholscher added the PR: hotfix label May 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: hotfix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants