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 #1941

Merged
merged 1 commit into from
Apr 29, 2019

Conversation

boynux
Copy link
Contributor

@boynux boynux commented Mar 15, 2019

Signed-off-by: Mohamad Arab [email protected]

Summary

Fixes #1627
Reopen: #1632
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

New tests cover the changes: yes

Description for the changelog

Configure CFS scheduler evaluation period from 8ms to 100ms

Licensing

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

@boynux boynux mentioned this pull request Mar 15, 2019
8 tasks
@boynux boynux force-pushed the enhancement/configure-cpu-period branch from cc55aaa to 593d2d1 Compare March 15, 2019 22:55
@boynux boynux changed the base branch from master to dev March 15, 2019 23:05
@boynux boynux force-pushed the enhancement/configure-cpu-period branch 2 times, most recently from f9fc74d to a2ce401 Compare March 15, 2019 23:59
@boynux
Copy link
Contributor Author

boynux commented Mar 18, 2019

@adnxn I applied the suggested changes here.

@adnxn adnxn added the bot/test label Mar 19, 2019
@boynux
Copy link
Contributor Author

boynux commented Mar 20, 2019

@adnxn looks like some tests are failing, can I get more details to see if they are related to this change. I remembered last time they all passed.

@adnxn
Copy link
Contributor

adnxn commented Mar 22, 2019

@adnxn looks like some tests are failing, can I get more details to see if they are related to this change. I remembered last time they all passed.

ill have to dig into this and try to get you some more information. in the meantime - have you looked at running the make targets for the integ and functional tests?

@adnxn adnxn added bot/test and removed bot/test labels Mar 25, 2019
@adnxn
Copy link
Contributor

adnxn commented Mar 25, 2019

@boynux: do you mind rebasing your changes on the latest changes from dev? just noticed that the version file in your branch is pointing to 1.25.3 and you're missing some fixes we've added to testing since then. the three functional tests that appear to be failing all have been fixed in dev branch.

specifically,

1048 --- FAIL: TestContainerMetadataFile (21.90s)
1259 --- FAIL: TestSSMSecretsEncryptedASMSecrets (7.18s)
1276 --- FAIL: TestASMSecretsARN (7.27s)

@boynux boynux force-pushed the enhancement/configure-cpu-period branch from a2ce401 to d10da19 Compare March 27, 2019 07:53
@boynux
Copy link
Contributor Author

boynux commented Mar 27, 2019

@adnxn just rebased the latest dev and pushed the update. Let's see.
Version is now: 1.26.0

@adnxn adnxn added bot/test and removed bot/test labels Mar 29, 2019
@boynux
Copy link
Contributor Author

boynux commented Apr 3, 2019

@adnxn is there anything I can do for this change?

@boynux
Copy link
Contributor Author

boynux commented Apr 16, 2019

@adnxn sorry to bother you again, is this still in your radar? And is there anything I can do to help getting this rolled out?

@adnxn
Copy link
Contributor

adnxn commented Apr 17, 2019

@adnxn sorry to bother you again, is this still in your radar? And is there anything I can do to help getting this rolled out?

no worries, yea it fell off my radar again. we'll have a second person go over the code as well and hopefully get this wrapped up soon. thanks for checking in again.

Copy link
Contributor

@fenxiong fenxiong left a comment

Choose a reason for hiding this comment

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

lgtm, some minor comments

agent/api/task/task.go Outdated Show resolved Hide resolved
agent/config/config.go Outdated Show resolved Hide resolved
@boynux boynux force-pushed the enhancement/configure-cpu-period branch from d10da19 to 7d0acd4 Compare April 18, 2019 07:21
@boynux
Copy link
Contributor Author

boynux commented Apr 18, 2019

@fenxiong done as suggested

@adnxn adnxn added bot/test and removed bot/test labels Apr 18, 2019
@adnxn adnxn added the bot/test label Apr 25, 2019
@adnxn
Copy link
Contributor

adnxn commented Apr 26, 2019

@boynux - just to update you, im trying to get these changes tested through our test suites and merged.

in the meantime - since this doesnt have any functional tests. would you be able to test and report the failure modes of full task runs at the boundaries of the configurable period?

@adnxn
Copy link
Contributor

adnxn commented Apr 26, 2019

okay so looks like we're getting ecs/windows/integ_test — Failed: Windows integration test suite cause your branch is missing this commit. mind rebasing off dev once more? 😀

update: my bad, that commit is for functional tests. let me dig into whats going on with the integ one

@yhlee-aws
Copy link
Contributor

integ test had failed due to flakey test (TestShutdownOrder): #1931. Test was rerun and all passed now.

@boynux
Copy link
Contributor Author

boynux commented Apr 29, 2019

Thanks for approving this change @adnxn

@adnxn adnxn merged commit a278a02 into aws:dev Apr 29, 2019
@boynux
Copy link
Contributor Author

boynux commented Apr 30, 2019

🎉

@fenxiong fenxiong mentioned this pull request May 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants