add a flag for cpu_hard_limit #3825

jaininshah9 · 2018-02-01T18:54:58Z

diptanu · 2018-02-01T21:30:25Z

client/driver/docker.go

@@ -120,6 +120,11 @@ const (
 	// https://docs.docker.com/engine/reference/run/#block-io-bandwidth-blkio-constraint
 	dockerBasicCaps = "CHOWN,DAC_OVERRIDE,FSETID,FOWNER,MKNOD,NET_RAW,SETGID," +
 		"SETUID,SETFCAP,SETPCAP,NET_BIND_SERVICE,SYS_CHROOT,KILL,AUDIT_WRITE"
+
+		// This is cpu.cfs_period_us: the length of a period. The default values is 100 microsecnds represented in nano Seconds, below is the documnentation


Wrap the line to 80 chars

jaininshah9 · 2018-02-02T18:22:58Z

Once I made the changes to the code, I created the nomad binary and started the agent in dev mode. After that I started a nomad job for docker -> stress (docker run --rm -it progrium/stress --cpu 4). In my job, I passed 1500 CPU (out of 24800 MHz), which equates to about 6.04 percent of CPU. And by setting the flag cpu_hard_limit = true, the CPU usage did not go over ~6%. Attached are 3 screenshots:

Screenshot for starting the agent

2. Screenshot for starting the job CPU usage where cpu_hard_limit = true

Screenshot for CPU usage where cpu_hard_limit = false

schmichael · 2018-02-06T01:15:01Z

I'm sorry for not responding to this PR sooner. We should support this in a cross-driver way which means adding cpu_hard_limit to the resources stanza of the job file.

I'll leave more details on the issue. Sorry again for not letting you know this requirement earlier!

jaininshah9 · 2018-02-06T01:28:47Z

@schmichael: Sure can add that in resources stanza. I will get started on that, but if you have other specific instructions then let me know whenever you can.

schmichael · 2018-02-06T01:31:23Z

@jaininshah9 That's quite a bit more work, so on second thought let's get this work in and we can deprecate it once there's a cross-driver solution.

Sorry for the confusion!

schmichael · 2018-02-06T01:33:10Z

client/driver/docker.go

+	// Below is the documnentation:
+	// https://www.kernel.org/doc/Documentation/scheduler/sched-bwc.txt
+	// https://docs.docker.com/engine/admin/resource_constraints/#cpu
+	defaultCFSPeriod = 100000


So for a task allocated 10% of the CPU it could pause for 90ms at a time? That seems awfully long, perhaps 10000 (10ms) or 1000 (1ms) increments would be a better level of granularity to ensure responsiveness for low priority interactive services?

Yes, if a task is allocated 10% of CPU and we keep the default 100 microseconds cfs_peroid then it will be paused for 90 microseconds. We can change that to 10 ms, for that we will have to pass CPUPeriod as well (which would always be 10 ms )

Side note: Sorry, I meant "ms" as in milli seconds. I know that kernel doc uses "ms" to mean microseconds, so I'm sorry for confusing the matter. Within Nomad code/comments/docs let's always use "ms" for millis and "us" or "μs" for micros.

I'd be in favor of lowering this to 10000μs (10ms) to minimize pause duration unless somebody has a strong preference.

Since it's only used for an internal calculation tweaking it in the future should be ok (and it should probably even be configurable once we make this cross-driver).

@schmichael The default for cfs period in docker is 100000, changing this would mean people's experience of using the quota flag would differ between using normal docker cli and Nomad. If you want a lower value, maybe introduce another flag to tweak that but keep the default as is?

@schmichael I am testing the changes that you recommended and running into an issue. When I specify cpu.cfs_period_us=10000 (milliseconds) and when I want to use 6% of CPU, I will have to specify cpu.cfs_quota_us=600(milliseconds). When I do that, it gives me an error: CPU cfs quota cannot be less than 1ms (i.e. 1000).

@diptanu has a good point. Matching Docker's setting is the right thing to do. Sorry for the noise!

Although now I'm confused by the error @jaininshah9 mentioned. According to Docker's API docs this setting should be in microseconds (like the kernel itself expects), not nanoseconds as the code comment implies.

The Docker docs lead me to believe this should be the value?

defaultCFSPeriod = 100

@schmichael You may be right, that got me confused as well, I am digging into the error and will let you know.

Okay, I think initially I made a mistake mentioning that it's a nanosecond. The reason I mentioned nanosecond it is because the docker docs says (see screenshot below) the default value is 100 microseconds, which is wrong, it should say 100 milliseconds (which is what kernel doc says).

I also tested and the docker API does not do any translation. Whatever value we pass in CPUPeriod is what we find under /sys/fs/cgroups...
So the value is the code is correct, I need to update the comment on it though

Sorry for the confusion I created. I will add a change which has a clear comment on what the defaultCFSPeriod refers to.

schmichael · 2018-02-06T01:38:18Z

client/driver/docker.go

@@ -1119,6 +1130,12 @@ func (d *DockerDriver) createContainerConfig(ctx *ExecContext, task *structs.Tas
 		VolumeDriver: driverConfig.VolumeDriver,
 	}

+	// Calculate CPU Quota
+	if driverConfig.CPUHardLimit {
+		percentTicks := float64(task.Resources.CPU) / shelpers.TotalTicksAvailable()


You should use d.node.Resources.CPU instead as we don't always properly detect the available CPU so some users have to override it.

diptanu · 2018-02-07T20:12:33Z

client/driver/docker.go

-	// https://docs.docker.com/engine/admin/resource_constraints/#cpu
-	defaultCFSPeriod = 100000
+	// https://docs.docker.com/engine/api/v1.35/#
+	defaultCFSPeriod_us = 100000


No _s in Go. Rename this to `defaultCFSPeriodUS.

schmichael · 2018-02-09T01:31:53Z

LGTM! Just need to fix the merge conflicts before I can pull it.

jaininshah9 · 2018-02-09T02:02:31Z

Ok, is that something I would do or you would? Happy to do it if I have to do it? On Feb 8, 2018 5:31 PM, "Michael Schurter" <[email protected]> wrote: LGTM! Just need to fix the merge conflicts before I can pull it. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3825 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGz_ULwpoGsBuRn5f8EsLYP7ZysxIBrDks5tS6AOgaJpZM4R2G-T> .

schmichael · 2018-02-09T04:41:02Z

I went and resolved the merge conflicts and updated the documentation. Thanks for sticking with this one @jaininshah9!

github-actions · 2023-03-13T02:14:26Z

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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

Jainin Shah added 2 commits February 1, 2018 10:09

add a flag for cpu_hard_limit

e6b691c

changes after running go fmt

8ef9f29

diptanu reviewed Feb 1, 2018

View reviewed changes

wrapping the line to less than 80 characters

4ff7500

schmichael closed this Feb 6, 2018

schmichael reopened this Feb 6, 2018

schmichael requested changes Feb 6, 2018

View reviewed changes

Jainin Shah added 2 commits February 6, 2018 14:52

using d.node.Resources.CPU as suggested

4a65fe6

clearing the confusion between microsecond,nanosecond and millisecond

2ebced7

diptanu suggested changes Feb 7, 2018

View reviewed changes

removing underscore in variable name

bc0256d

schmichael approved these changes Feb 9, 2018

View reviewed changes

schmichael added 2 commits February 8, 2018 20:14

Merge branch 'master' into f-cpu_hard_limit

bc1894f

docker: add cpu_hard_limit docs

aba6d13

schmichael merged commit c7c4564 into hashicorp:master Feb 9, 2018

jaininshah9 mentioned this pull request Feb 27, 2018

CPU hard limit in Nomad - Fix #3915

Closed

burdandrei mentioned this pull request Apr 16, 2018

Use soft docker memory limit instead of hard one #2771

Closed

omame mentioned this pull request Jun 28, 2018

Make cpu.cfs_period_us configurable #4456

Closed

github-actions bot locked as resolved and limited conversation to collaborators Mar 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add a flag for cpu_hard_limit #3825

add a flag for cpu_hard_limit #3825

jaininshah9 commented Feb 1, 2018 •

edited by schmichael

Loading

diptanu Feb 1, 2018

jaininshah9 commented Feb 2, 2018 •

edited

Loading

schmichael commented Feb 6, 2018

jaininshah9 commented Feb 6, 2018

schmichael commented Feb 6, 2018 •

edited

Loading

schmichael Feb 6, 2018

jaininshah9 Feb 6, 2018

schmichael Feb 6, 2018

diptanu Feb 6, 2018

jaininshah9 Feb 6, 2018

schmichael Feb 7, 2018 •

edited

Loading

schmichael Feb 7, 2018

jaininshah9 Feb 7, 2018

jaininshah9 Feb 7, 2018

schmichael Feb 6, 2018

diptanu Feb 7, 2018

schmichael commented Feb 9, 2018

jaininshah9 commented Feb 9, 2018 via email

schmichael commented Feb 9, 2018

github-actions bot commented Mar 13, 2023

add a flag for cpu_hard_limit #3825

add a flag for cpu_hard_limit #3825

Conversation

jaininshah9 commented Feb 1, 2018 • edited by schmichael Loading

Choose a reason for hiding this comment

jaininshah9 commented Feb 2, 2018 • edited Loading

schmichael commented Feb 6, 2018

jaininshah9 commented Feb 6, 2018

schmichael commented Feb 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schmichael Feb 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schmichael commented Feb 9, 2018

jaininshah9 commented Feb 9, 2018 via email

schmichael commented Feb 9, 2018

github-actions bot commented Mar 13, 2023

jaininshah9 commented Feb 1, 2018 •

edited by schmichael

Loading

jaininshah9 commented Feb 2, 2018 •

edited

Loading

schmichael commented Feb 6, 2018 •

edited

Loading

schmichael Feb 7, 2018 •

edited

Loading