-
Notifications
You must be signed in to change notification settings - Fork 63
Add environment variables to control container resource constraints (COMPLEMENT_CONTAINER_CPU_CORES, COMPLEMENT_CONTAINER_MEMORY)
#827
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
Changes from 5 commits
1360e4d
5274dd5
44cbb37
3aae727
e4396c5
0b341dc
6034e6c
2423d24
0262873
5cf419c
3baf19c
ea793c3
2061f06
75b6f5d
d8f0ee7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -11,6 +11,7 @@ import ( | |||||
| "math/big" | ||||||
| "os" | ||||||
| "regexp" | ||||||
| "sort" | ||||||
| "strconv" | ||||||
| "strings" | ||||||
| "time" | ||||||
|
|
@@ -52,6 +53,20 @@ type Complement struct { | |||||
| // starting the container. Responsiveness is detected by `HEALTHCHECK` being healthy *and* | ||||||
| // the `/versions` endpoint returning 200 OK. | ||||||
| SpawnHSTimeout time.Duration | ||||||
| // Name: COMPLEMENT_CONTAINER_CPUS | ||||||
MadLittleMods marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||
| // Default: 0 | ||||||
| // Description: The number of CPU cores available for the container to use (can be | ||||||
| // fractional like 0.5). This is passed to Docker as the `--cpus`/`NanoCPUs` argument. | ||||||
| // If 0, no limit is set and the container can use all available host CPUs. This is | ||||||
| // useful to mimic a resource-constrained environment, like a CI environment. | ||||||
| ContainerCPUCores float64 | ||||||
| // Name: COMPLEMENT_CONTAINER_MEMORY | ||||||
| // Default: 0 | ||||||
| // Description: The maximum amount of memory the container can use. This is passed to | ||||||
| // Docker as the `--memory`/`Memory` argument. If 0, no limit is set and the container | ||||||
| // can use all available host memory. This is useful to mimic a resource-constrained | ||||||
| // environment, like a CI environment. | ||||||
| ContainerMemoryBytes int64 | ||||||
| // Name: COMPLEMENT_KEEP_BLUEPRINTS | ||||||
| // Description: A list of space separated blueprint names to not clean up after running. For example, | ||||||
| // `one_to_one_room alice` would not delete the homeserver images for the blueprints `alice` and | ||||||
|
|
@@ -145,8 +160,13 @@ func NewConfigFromEnvVars(pkgNamespace, baseImageURI string) *Complement { | |||||
| // each iteration had a 50ms sleep between tries so the timeout is 50 * iteration ms | ||||||
| cfg.SpawnHSTimeout = time.Duration(50*parseEnvWithDefault("COMPLEMENT_VERSION_CHECK_ITERATIONS", 100)) * time.Millisecond | ||||||
| } | ||||||
| cfg.ContainerCPUCores, _ = strconv.ParseFloat(os.Getenv("COMPLEMENT_CONTAINER_CPUS"), 64) | ||||||
|
||||||
| cfg.ContainerCPUCores, _ = strconv.ParseFloat(os.Getenv("COMPLEMENT_CONTAINER_CPUS"), 64) | |
| cfg.ContainerCPUCores, _ = strconv.ParseFloat(os.Getenv("COMPLEMENT_CONTAINER_CPUS"), 0) |
so it is also unlimited?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
64 is the bitsize of the float:
https://pkg.go.dev/strconv#ParseFloat
// ParseFloat converts the string s to a floating-point number
// with the precision specified by bitSize: 32 for float32, or 64 for float64.
// When bitSize=32, the result still has type float64, but it will be
// convertible to float32 without changing its value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just based off glancing a few implementations and trying to make it as simple as possible for our usage here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With some hindsight, we could go even simpler and only support the same units that the Docker CLI has:
Most of these options take a positive integer, followed by a suffix of
b,k,m,g, to indicate bytes, kilobytes, megabytes, or gigabytes.-- https://docs.docker.com/engine/containers/resource_constraints/
I built this the other way around though. I wanted to be able to pass in COMPLEMENT_CONTAINER_MEMORY=1GB and wrote this out, then only later added in the Docker CLI unit variants to come to this realization 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it'll be nice dev UX to be this flexible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add this to
.gitignore?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤷 nahh, we can actually notice this way. I've added some example usage to the top of
cmd/gendoc/main.gobut ideally, the script should just do the right thing without all of the options and piping.