Allocate docker limits based on server size. #6879
Conversation
This assumes 1-builder per server. It is still able to be overwritten by settings, so to have this take effect we need to set: ``` DOCKER_LIMITS = None ```
| # Set docker limits dynamically based on system memory | ||
| # This assumes 1-builder per server | ||
| try: | ||
| total_memory = int(subprocess.check_output("free -m | awk '/^Mem:/{print $2}'", shell=True)) |
There was a problem hiding this comment.
We could use psutil here: https://pypi.org/project/psutil/ -- but it would add a dependency. Not sure the tradeoff.
There was a problem hiding this comment.
I'd worry a little about this call failing for some reason, then falling through to the except because it's a generic Exception, and then we could be in a place where we are setting the default amount on servers without noticing.
psutil might give us more confidence in this check, or the except should be much tighter. The failure case is likely MacOS doing something different, or MacOS free/awk doing something different than Linux free/gnu awk, so we could also put this default set operation in a if DEBUG inside a @property setting. We likely never want to settle on the defaults in prod
From a code correctness standpoint, I verified that this works. It will fall back to defaults on Mac/Windows as they typically don't have these commands.
So this leaves 1GB of memory to the rest of the system and the rest of the builder code and allocates the rest to the docker builders?
Will need to be set to |
Why? It seems useful for anyone running RTD. |
Just asking because we are changing the default from 200m to be ~75% of the total memory in the computer. Only that. |
This logic might fit better inside a settings property method inside global level in constants
| # Set docker limits dynamically based on system memory | ||
| # This assumes 1-builder per server | ||
| try: | ||
| total_memory = int(subprocess.check_output("free -m | awk '/^Mem:/{print $2}'", shell=True)) |
There was a problem hiding this comment.
I'd worry a little about this call failing for some reason, then falling through to the except because it's a generic Exception, and then we could be in a place where we are setting the default amount on servers without noticing.
psutil might give us more confidence in this check, or the except should be much tighter. The failure case is likely MacOS doing something different, or MacOS free/awk doing something different than Linux free/gnu awk, so we could also put this default set operation in a if DEBUG inside a @property setting. We likely never want to settle on the defaults in prod
Yea, I just wanted to allow users to override it still with a setting if needed. We could have two settings for this, but that just seems confusing? |
|
I will merge this now, and see how it goes in prod. We can always adjust, but I'd like to test building one deploy builder image today. |
We are settings this limits dynamically in #6879
We are settings this limits dynamically in #6879
We are settings this limits dynamically in #6879
This assumes 1-builder per server.
It is still able to be overwritten by settings,
so to have this take effect we need to set: