memory default is 10, not 300
#3520
Conversation
Verified by creating sample job and removing memory from resources ``` Task "redis" is "running" Task Resources CPU Memory Disk IOPS Addresses 3/100 MHz 976 KiB/10 MiB 300 MiB 0 db: **.**.**.**:20824 ```
| @@ -49,7 +49,7 @@ job "docs" { | |||
| - `iops` `(int: 0)` - Specifies the number of IOPS required given as a weight | |||
| between 0-1000. | |||
|
|
|||
| - `memory` `(int: 300)` - Specifies the memory required in MB | |||
| - `memory` `(int: 10)` - Specifies the memory required in MB | |||
There was a problem hiding this comment.
Can we actually change this to be 300 instead for changing the docs: https://github.com/hashicorp/nomad/blob/master/api/resources.go#L20
The current code would merge the job with the output of MinResources causing the nil to be replaced with MemoryMB=helper.IntToPtr(10). When later `Canonicalize`d, the 10 would cause the default of 300 not to be applied. Running `Canonicalize` on the task itself guarantees that CPU, MemoryMB, and IOPS is set, so we can remove the Merge withMinResources.
…a at all In testing realized that Resources night not be present at all. Testing this case caused panic. Added in a means to collect clean defaults in that case.
|
Had to make some additional changes because of what seems to be a logic error in Tasks.Canonicalize() which was causing the minimum memory value to dominate the nil before we got to the canonicalize step to provide the 300 mb default. |
api/tasks.go
Outdated
| min.Merge(t.Resources) | ||
| min.Canonicalize() | ||
| t.Resources = min | ||
| if t.Resources == nil { |
There was a problem hiding this comment.
Hm, this makes MinResources unused, but I think that's ok. Users of the api package still may find it useful. The old code was pretty weird anyway since MinResources and Canonicalize set the same fields so calling MinResources was already worthless.
You could simplify this to:
if t.Resources == nil {
t.Resources = &Resources{}
}
t.Resources.Canonicalize()
|
Ran into this today, and it sparked a question of curiosity: In docker, if the memory isn't specified the container basically gets "unlimited", in other words first come, first serve. Is there a reason Nomad's behavior is different when calling the docker driver? 10mb is obviously way too low (many containers flat out don't run properly) but even 300 is a far cry "unlimited". To me, I would expect for the allocated memory to reflect the behavior docker itself uses, especially when using the docker driver (can't speak for other container models). |
|
Hey @Justin-DynamicD, Nomad currently requires all tasks specify their resources as the schedulers use this to bin-pack the tasks onto nodes that have capacity for the task. If the task didn't have limits imposed on to it, it could negatively impact the performance of other allocations on the node. Docker locally doesn't have to worry about this case since it isn't a cluster manager (when not run in swarm mode). In the future Nomad will have over-subscription which will let the client monitor resources and allow a task to use the claimed but unused resources allowing this type of behavior. Hope that helps. |
|
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
Verified by creating sample job and removing memory from resources