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

Remove EventMachine from Health Monitor and the Director #2502

Merged
merged 3 commits into from Mar 14, 2024

Conversation

ystros
Copy link
Contributor

@ystros ystros commented Mar 4, 2024

What is this change about?

The eventmachine gem is no longer receiving updates. This commit moves
asynchronous functionality of bosh-monitor / Health Monitor to use the
async gem (and associated ecosystem) instead.

  • em-http-request gem is replaced by async-http gem
  • EventMachine connections are replaced with async-io gem
  • EventMachine's SMTP functionality has been replaced with net-smtp gem
    wrapped in async blocks.
  • EventMachine.popen is replaced with Open3 wrapped in async blocks
  • The em_threadpool_size config has been removed as there does not
    appear to be an analog in the async gem

Please provide contextual information.

Previous PRs removing other instances of EventMachine: #2497, #2499, #2500

What tests have you run against this PR?

Overall tests:

  • ✅ Unit and functional specs
  • ✅ Manually deployed a BOSH Director using a dev release created from these changes, and observed the Health Monitor logs
  • ✅ BRATS (tests JSON plugin)

Plugins:

  • ❓ Consul (this appears to be undocumented and perhaps should be removed?)
  • ❓ Datadog
  • ✅ Email
  • ✅ Event logger
  • ✅ Graphite (confirmed that the Graphite server still receives events, not familiar enough to go beyond that)
  • ✅ JSON (through unit tests, local testing, and BRATS)
  • ❓ PagerDuty
  • ✅ Resurrector
  • ❓ Riemann
  • ❓ TSDB

Very notably, several individual Health Monitor plugins have not been verified to work. The biggest risk plugins seem to be Email, Graphite, and TSDB, as they required the most rewrites due to high coupling with EventMachine. Email and Graphite have received some very basic manual tests.

Does this PR introduce a breaking change?

Not necessarily a breaking change, but the em_threadpool_size config option for Health Monitor has been removed with no analogous configuration option added. EventMachine seems to have created a fixed-size thread pool to run its event loop whereas the async gem uses Ruby fibers.

This does mean there may be a slight risk in certain places in the code of spawning many async tasks simultaneously that would have been previously limited by the size of the EventMachine thread pool; in those places, I've added a semaphore to limit the number of simultaneous tasks.

@ystros ystros force-pushed the remove-em-from-bosh-monitor-#185264086 branch from 2a32603 to e81d4f7 Compare March 8, 2024 18:51
The eventmachine gem is no longer receiving updates. This commit moves
asynchronous functionality of bosh-monitor / Health Monitor to use the
async gem (and associated ecosystem) instead.

* em-http-request gem is replaced by async-http gem
* EventMachine connections are replaced with async-io gem
* EventMachine's SMTP functionality has been replaced with net-smtp gem
  wrapped in async blocks.
* EventMachine.popen is replaced with Open3 wrapped in async blocks
* The em_threadpool_size config has been removed as there does not
  appear to be an analog in the async gem
With the remove from bosh-monitor, no subcomponents in the BOSH Director
rely on the eventmachine gem.

🌋
@ystros ystros force-pushed the remove-em-from-bosh-monitor-#185264086 branch from e81d4f7 to e1ebd69 Compare March 11, 2024 16:03
@ystros ystros marked this pull request as ready for review March 11, 2024 21:21
Copy link
Member

@aramprice aramprice left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@lnguyen lnguyen left a comment

Choose a reason for hiding this comment

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

LGTM

@lnguyen lnguyen merged commit 1eb742d into main Mar 14, 2024
4 checks passed
@lnguyen lnguyen deleted the remove-em-from-bosh-monitor-#185264086 branch March 14, 2024 11:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants