-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Add support for specifying cpu_cfs_period in the Docker driver #4462
Conversation
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.
A test that at least asserts the validation logic would be nice. We could test the other settings by calling InspectContainer() as we do here: https://github.com/hashicorp/nomad/blob/master/client/driver/docker_test.go#L871
If you can't get to it we can probably get to it later in the 0.9 dev cycle.
Thanks! Overall looks good!
@@ -361,7 +361,9 @@ The `docker` driver supports the following configuration in the job spec. Only | |||
soft limiting is used and containers are able to burst above their CPU limit | |||
when there is idle capacity. | |||
|
|||
* `advertise_ipv6_address` - (Optional) `true` or `false` (default). Use the container's | |||
* `cpu_cfs_period` - (Optional) An integer value that specifies the duration in microseconds of the period during which the CPU usage quota is measured. The default is 100000 (0.1 second) and the maximum allowed value is 1000000 (1 second). See [here](https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/6/html/resource_management_guide/sec-cpu#sect-cfs) for more details. |
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.
Please wrap long lines in docs.
Not sure if this is enough. Feel free to ask for more. |
Hey @schmichael, did you have any time to review? |
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.
Looks fantastic @omame! Thanks!
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. |
Fixes #4456.
This adds the ability to specify
cpu.cfs_period_us
to a cgroup created by the Docker driver.There is no need to force
cpu.cfs_quota_us
as this needs to be calculated with the formula introduced in #3917.