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

Avoid calling $evm.backtrace over DRb to prevent DRb-level mutex locks #14239

Merged
merged 2 commits into from
Mar 10, 2017

Conversation

Fryguy
Copy link
Member

@Fryguy Fryguy commented Mar 8, 2017

Moves dynamic method building into DrbRemoteInvoker since it isn't
needed at all in MiqAeService. Once in DrbRemoteInvoker, we can use
the dynamic preamble to inform the generated ruby method body of things
like the number of preamble lines and the name of the method for
backtrace filtering purposes.

Note, moving this method into DrbRemoteInvoker allows us to run it in the launched automate ruby process directly and not over DRb. The latter was causing random DRb thread issues when the ruby method exited or raised an exception.

@mkanoor Please review
Paired with @jrafanie on this one, which he originally noticed in #13104 .

@Fryguy
Copy link
Member Author

Fryguy commented Mar 8, 2017

Found a bug in the actual line count...need to write a test for it :)

@Fryguy Fryguy force-pushed the remove_backtrace_over_drb branch 3 times, most recently from bb63a49 to 3a8e356 Compare March 9, 2017 16:50
@Fryguy Fryguy changed the title Avoid calling backtrace over $evm to prevent DRb-level mutex locks Avoid calling $evm.backtrace over DRb to prevent DRb-level mutex locks Mar 9, 2017
Copy link
Member

@jrafanie jrafanie left a comment

Choose a reason for hiding this comment

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

LGTM

@Fryguy
Copy link
Member Author

Fryguy commented Mar 9, 2017

In review with @mkanoor we found that nested methods will only have the first method's name because of where I put the method_name hook. So marking as WIP til I fix that.

@Fryguy Fryguy changed the title Avoid calling $evm.backtrace over DRb to prevent DRb-level mutex locks [WIP] Avoid calling $evm.backtrace over DRb to prevent DRb-level mutex locks Mar 9, 2017
@miq-bot miq-bot added the wip label Mar 9, 2017
@Fryguy Fryguy force-pushed the remove_backtrace_over_drb branch from 3a8e356 to 7325485 Compare March 9, 2017 20:59
@Fryguy Fryguy changed the title [WIP] Avoid calling $evm.backtrace over DRb to prevent DRb-level mutex locks Avoid calling $evm.backtrace over DRb to prevent DRb-level mutex locks Mar 9, 2017
@Fryguy
Copy link
Member Author

Fryguy commented Mar 9, 2017

@jrafanie @mkanoor Updated with a ton of specs.

@miq-bot miq-bot removed the wip label Mar 9, 2017
Moves dynamic method building into DrbRemoteInvoker since it isn't
needed at all in MiqAeService.  Once in DrbRemoteInvoker, we can use
the dynamic preamble to inform the generated ruby method body of things
like the number of preamble lines and the name of the method for
backtrace filtering purposes.
@Fryguy Fryguy force-pushed the remove_backtrace_over_drb branch from 7325485 to 083117c Compare March 9, 2017 21:04
@miq-bot
Copy link
Member

miq-bot commented Mar 9, 2017

Some comments on commits Fryguy/manageiq@1e30d48~...083117c

spec/lib/miq_automation_engine/engine/miq_ae_method_spec.rb

  • ⚠️ - 108 - Detected puts. Remove all debugging statements.
  • ⚠️ - 125 - Detected puts. Remove all debugging statements.
  • ⚠️ - 142 - Detected puts. Remove all debugging statements.
  • ⚠️ - 157 - Detected puts. Remove all debugging statements.
  • ⚠️ - 172 - Detected puts. Remove all debugging statements.
  • ⚠️ - 173 - Detected puts. Remove all debugging statements.
  • ⚠️ - 174 - Detected puts. Remove all debugging statements.
  • ⚠️ - 175 - Detected puts. Remove all debugging statements.
  • ⚠️ - 176 - Detected puts. Remove all debugging statements.
  • ⚠️ - 187 - Detected puts. Remove all debugging statements.
  • ⚠️ - 188 - Detected puts. Remove all debugging statements.
  • ⚠️ - 189 - Detected puts. Remove all debugging statements.
  • ⚠️ - 190 - Detected puts. Remove all debugging statements.
  • ⚠️ - 191 - Detected puts. Remove all debugging statements.
  • ⚠️ - 203 - Detected puts. Remove all debugging statements.
  • ⚠️ - 23 - Detected puts. Remove all debugging statements.
  • ⚠️ - 39 - Detected puts. Remove all debugging statements.
  • ⚠️ - 59 - Detected puts. Remove all debugging statements.
  • ⚠️ - 76 - Detected puts. Remove all debugging statements.
  • ⚠️ - 93 - Detected puts. Remove all debugging statements.

@miq-bot
Copy link
Member

miq-bot commented Mar 9, 2017

Checked commits Fryguy/manageiq@1e30d48~...083117c with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
5 files checked, 0 offenses detected
Everything looks good. 🍪

@jrafanie
Copy link
Member

Merging, @mkanoor gave verbal approval to this change yesterday 👍

@jrafanie jrafanie merged commit 529a0dc into ManageIQ:master Mar 10, 2017
@jrafanie jrafanie assigned jrafanie and unassigned mkanoor Mar 10, 2017
@jrafanie jrafanie added this to the Sprint 56 Ending Mar 13, 2017 milestone Mar 10, 2017
@jrafanie
Copy link
Member

@mkanoor @Fryguy should this go back to euwe? I don't think this is happening there but what do you think?

@mkanoor
Copy link
Contributor

mkanoor commented Mar 10, 2017

👍
@jrafanie lets wait a while and make sure QA doesn't find anything and then we can back port it

@Fryguy Fryguy deleted the remove_backtrace_over_drb branch March 10, 2017 18:29
jrafanie added a commit to jrafanie/manageiq that referenced this pull request Mar 29, 2018
We previously tried to upgrade to 2.3.6 on euwe by backporting:
ManageIQ#15993

This was causing a hang in the automation test suite on euwe and we
reverted it in:
ManageIQ#16087

https://bugzilla.redhat.com/show_bug.cgi?id=1560578

The prior 2 commits contain a manual backport of
ManageIQ#14239
which solves the 100% cpu hang of the automate ruby method with ruby 2.3.2+.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants