-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Fix Nio/CPU issue and CI failures #1543
Conversation
- Reverts ea2286 that introduced a wakeup on each connection loop run. - In SSL handshake code removes delegated tasks to be run in separate threads. Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>
if (s_logger.isTraceEnabled()) { | ||
s_logger.trace("SSL: Running delegated task!"); | ||
} | ||
task.run(); |
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.
@jburwell thanks for pointing out, I've made the runnable task run in the current thread only. The delegated tasks in case of SSL handshake could be resolving domain names to address.
@rhtyd This patch seems to have fixed the cpu issue. I've got this running on a hardware lab right now. Thought you'd like to know before I hit the sack for the night! |
Thanks for the fix @rhtyd and for testing this @kiwiflyer. 👍 |
@kiwiflyer Thanks you for testing this so quickly and sharing. |
- Fixes oobm integration test to skip if known ipmitool bug is hit - Fixes ProcessTest unit test case to use sleep - Removes redundant unit test that covers code in ProcessTest Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>
LGTM based on code review. |
code LGTM. trusting @kiwiflyer I will probably test later but am a bit stuck today. |
Changes the cleanup sequence as the cleanup code causes exceptions in several Travis runs such as: https://travis-ci.org/apache/cloudstack/jobs/129925224 Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>
I've also fixed a integration/component test that has failed on Travis several time for various PRs due to cleanup order (tearDown issue):
|
Most of these things are fixes to Jenkins and travis issues, so those jobs being green is good validation of the fixes. The main functionality change has been validated by @kiwiflyer, so I will get this merged right away. Thanks for being on top of this @rhtyd, I really appreciate it. 👍 |
Fix Nio/CPU issue and CI failures- Reverts ea2286 that introduced a wakeup on each connection loop run. - In SSL handshake code removes delegated tasks to be run in separate threads. /cc @kiwiflyer @swill @jburwell and others for review @kiwiflyer please help me test this fix and share if it makes the NioConnection robust now, without having the selector consume a lot of CPU. Thanks. * pr/1543: test: fix cleanup sequence for test_acl_listvolume test CLOUDSTACK-9299: Fix test failures on CI CLOUDSTACK-9348: Make NioConnectio loop less aggressive Signed-off-by: Will Stevens <williamstevens@gmail.com>
Thanks @swill |
Thanks for the effort stabilizing Jenkins and Travis the last few days. This has been super helpful as we get close to the freeze. Very much appreciated. Most of the runs are coming back clean now. 👍 |
/cc @kiwiflyer @swill @jburwell and others for review
@kiwiflyer please help me test this fix and share if it makes the NioConnection robust now, without having the selector consume a lot of CPU. Thanks.