-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Basic Support for Swap #4091
Basic Support for Swap #4091
Conversation
Added a SwapMB property to the all relevant “resource” structs and to the configuration parsing logic.
api/resources.go
Outdated
@@ -66,6 +72,9 @@ func (r *Resources) Merge(other *Resources) { | |||
if other.MemoryMB != nil { | |||
r.MemoryMB = other.MemoryMB | |||
} | |||
if other.SwapMB != nil { |
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.
go fmt
command/agent/config_parse.go
Outdated
@@ -472,6 +472,7 @@ func parseReserved(result **Resources, list *ast.ObjectList) error { | |||
valid := []string{ | |||
"cpu", | |||
"memory", | |||
"swap", |
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.
go fmt
jobspec/parse.go
Outdated
@@ -1199,6 +1199,7 @@ func parseResources(result *api.Resources, list *ast.ObjectList) error { | |||
"iops", | |||
"disk", | |||
"memory", | |||
"swap", |
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.
go fmt
nomad/structs/structs.go
Outdated
@@ -1594,6 +1597,9 @@ func (r *Resources) Merge(other *Resources) { | |||
if other.MemoryMB != 0 { | |||
r.MemoryMB = other.MemoryMB | |||
} | |||
if other.SwapMB != 0 { |
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.
go fmt
@tbartelmess Great job on this PR but unfortunately this is not something we would like in Nomad. The reason being is that when a subset of allocations on a node are using swap, it can lead to degraded performance of the entire node and thus negatively impact the other allocations. In Nomad 0.9 we will be targeting fingerprinter plugins, custom resources and drivers as plugins. With these newer features you will be able to accomplish your goal of making Nomad swap aware without it being in core. Hope my explanation makes sense and that the path forward to not maintaining a fork is acceptable! Thanks again! |
@dadgar my 50c on this is to allow the feature, but as a client configuration knob there are tons of valid cases for needing this, and hiding it behind a flag would nomad team to say |
I totally agree about letting the user allow swap for dockers as well. Like @jippi says, I too has imagined that this could be a client/driver option; something like |
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. |
This PR adds basic support to make Swap space a schedulable resource. While especially in cloud environments swap usage can be tricky since the performance can vary a lot. However, for some cases that process very ununiform data (especially batch workload), swap is often needed to not over-provision Memory to a great extent that then never ends up being used. At @Marketcircle, we are using a fork of Nomad with those changes.
There are no changes to the documentation yet, and also the metric collection is not finalized.
The main change is that "Swap" is a schedulable resource and can be defined in the resources of a task.
Nodes do get fingerprinted for available swap space and the docker and lxc schedulers have been updated to enforce the limit.