Skip to content
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

Make CGroups CPU period configurable #1632

Closed
wants to merge 1 commit into from

Conversation

boynux
Copy link
Contributor

@boynux boynux commented Oct 18, 2018

Summary

Fixes #1627
Make CFS CPU Period configurable

CFS keeps throttling tasks very excessively if the default period 100ms is specified, to reduce the impact we should be able to set lower CPU period when defining cgroup quotas.

Implementation details

New environment parameter added to adjust CFS period.

Testing

See here: https://gist.github.com/bobrik/2030ff040fad360327a5fab7a09c4ff1

-->

  • Builds on Linux (make release)
  • Builds on Windows (go build -out amazon-ecs-agent.exe ./agent)
  • Unit tests on Linux (make test) pass
  • Unit tests on Windows (go test -timeout=25s ./agent/...) pass
  • Integration tests on Linux (make run-integ-tests) pass
  • Integration tests on Windows (.\scripts\run-integ-tests.ps1) pass
  • Functional tests on Linux (make run-functional-tests) pass
  • Functional tests on Windows (.\scripts\run-functional-tests.ps1) pass

New tests cover the changes: yes

Licensing

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@boynux boynux force-pushed the enhancement/configure-cpu-period branch 3 times, most recently from 1488696 to ed0e31b Compare October 18, 2018 20:18
@samuelkarp
Copy link
Contributor

Hi @boynux, thanks for sending this! I wanted to give some context on why the CPU period is hard-coded to 100ms today, and what needs to be considered in changing it.

CPU period and CPU quota are the mechanisms that we employ to power the task size CPU hard limits. When you specify vCPU in your task definition, we translate that into the CPU period and CPU quota settings that apply to the cpu cgroup subsystem.

CPU quota controls the amount of CPU time granted to a cgroup during a given CPU period. Both settings are expressed in terms of microseconds; having a CPU quota that equals CPU period means that a cgroup can execute up to 100% on one vCPU (or 50% on each of two vCPUs, or any other fraction that totals to 100% of one). The CPU quota has a maximum of 1000000us and CPU period has a minimum of 1ms, so it becomes a math problem to express limits that make sense for a given CPU count and the limits that you want to set. Changing the CPU period without changing the CPU quota will cause you to have different effective limits than what you've specified in your task definition; these values need to co-vary with the vCPU task size.

The hard-coded 100ms period that we have today lets us express values for vCPUs ranging from 0.125 to 10, and this is reflected in the ECS task definition API validation (i.e., if you attempt to specify greater than 10 or less than 0.125 in your task definition, you'll get an error back). Changing the quota changes the values that would be valid (from the Linux kernel's perspective) for controlling task size, and we would potentially need to adjust the ECS API to reflect that.

This is recorded in the comments of agent/api/task/task_linux.go:

	// TODO: DefaultCPUPeriod only permits 10VCPUs.
	// Adaptive calculation of CPUPeriod required for further support
	// (samuelkarp) The largest available EC2 instance in terms of CPU count is a x1.32xlarge,
	// with 128 vCPUs. If we assume a fixed evaluation period of 100ms (100000us),
	// we'd need a quota of 12800000us, which is longer than the maximum of 1000000.
	// For 128 vCPUs, we'd probably need something like a 1ms (1000us - the minimum)
	// evaluation period, an 128000us quota in order to stay within the min/max limits.

I don't think we'll be able to take this pull request in its current form, because of how it interacts with the task definition's task size feature. In order to allow the CPU period to change, we'll need to both adjust the CPU quota in accordance with the task size and adjust the task definition validation logic.

@boynux
Copy link
Contributor Author

boynux commented Oct 18, 2018

@samuelkarp Thanks for your reply, as it is mentioned in the description we are experiencing some issues with the current 100ms default period which seems to be related to the way CFS calculate the used quota. This makes task level quota very inefficient and unusable to some extend for us and most likely other users.
I'm happy to apply changes as you may suggest to make it more relevant to ECS use case. Unfortunately API and task validation settings are not something that I can help with as I believe they are not public code bases.
As an initial measure I can limit the parameter value ranging from 1ms to 100ms (the current limit), and I think that would cover all the valid cpu range from 0.128 - 10, am I correct here?

@boynux boynux force-pushed the enhancement/configure-cpu-period branch 2 times, most recently from afb836c to 41fad33 Compare October 19, 2018 09:47
@boynux
Copy link
Contributor Author

boynux commented Oct 19, 2018

@samuelkarp now it's limited to 1ms to 100ms only 👼
What do you think? thank you.

@boynux boynux force-pushed the enhancement/configure-cpu-period branch from 41fad33 to 39b7c18 Compare December 28, 2018 08:58
@adnxn
Copy link
Contributor

adnxn commented Jan 11, 2019

now it's limited to 1ms to 100ms only

@boynux: do you mind updating your branch with the latest changes from dev and resolving the conflicts? though i'll double check with our team, the bounded range via the config seems like a reasonable compromise to me.

@boynux boynux force-pushed the enhancement/configure-cpu-period branch from 39b7c18 to 36267c2 Compare January 12, 2019 09:09
@boynux
Copy link
Contributor Author

boynux commented Jan 18, 2019

@adnxn in case you didn't get the notification, I rebased master to my PR. Should be good to go!

@adnxn adnxn added bot/test and removed bot/test labels Jan 23, 2019
Copy link
Contributor

@adnxn adnxn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so playing around with these changes some more, it seems the 1ms lower bound will keep the taskCPUPeriod AT the lower bound considered valid by linux. as per CFS docs, "The minimum quota allowed for the quota or period is 1ms."

however with a 1ms taskCPUPeriod, we'll end up with a 125us taskCPUQuota, this is below the minimum quota allowed by linux. so it seems like we'll need a lower bound of at least 8ms for cpu period to ensure we stay within the range considered valid by linux such that taskCPUQuota is at 1000us.

agent/config/parse.go Show resolved Hide resolved
agent/config/config_unix_test.go Show resolved Hide resolved
agent/api/task/task_linux_test.go Show resolved Hide resolved
agent/api/task/task_linux.go Show resolved Hide resolved
agent/config/config.go Show resolved Hide resolved
@adnxn
Copy link
Contributor

adnxn commented Mar 7, 2019

@boynux: im closing this PR for now. but if feel free to address the comments and reopen if you'd like us to take a look again. sorry about the delay on our end! 😳

@boynux
Copy link
Contributor Author

boynux commented Mar 15, 2019

@adnxn I changed the code according to your suggestions and opened a new PR #1941

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants