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

Add setting to spawn instead of fork a worker #16943

Closed
wants to merge 1 commit into from

Conversation

agrare
Copy link
Member

@agrare agrare commented Feb 2, 2018

Add a config setting per worker type to force the worker to be spawned
instead of forked.

This will allow us to force certain provider workers to be spawned
instead of forked to work around possible shared thread object ID
issues.

Add a config setting per worker type to force the worker to be spawned
instead of forked.
@agrare agrare requested a review from jrafanie February 2, 2018 20:05
@agrare
Copy link
Member Author

agrare commented Feb 2, 2018

cc @jrafanie thinking of this for the DRb incompatible marshal file error

@miq-bot
Copy link
Member

miq-bot commented Feb 2, 2018

Checked commit agrare@32adf61 with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 🍪

end

def spawn_worker?
ENV['MIQ_SPAWN_WORKERS'] || !Process.respond_to?(:fork) || worker_settings[:spawn_worker]
Copy link
Member

Choose a reason for hiding this comment

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

@agrare Why do we need a setting for this? Does the ENV variable not work? I'm not sure the "why" here

Copy link
Member Author

Choose a reason for hiding this comment

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

I was hoping this would give us a way to control this on fine release, I wasn't aware of a way to change the env vars per worker type on that release but if it exists then great.

Copy link
Member Author

Choose a reason for hiding this comment

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

The real "why" is to spawn not fork VMware provider workers on a fine appliance to see if it solves the drb errors connecting with the broker

Copy link
Member

Choose a reason for hiding this comment

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

@agrare Pretty sure this SPAWN_WORKERS isn't available on fine:

https://github.com/ManageIQ/manageiq/blob/fine/app/models/miq_worker.rb

So you might need to do a bit of backporting if that was the end goal.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes absolutely I was planning on needing to backport some of @jrafanie 's changes to make this an option.

Copy link
Member

Choose a reason for hiding this comment

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

Mmmk, you will absolute need this then as well:

https://github.com/ManageIQ/manageiq/blob/master/lib/workers/bin/run_single_worker.rb

And there are a few PRs surrounding that as well that would need to be backported.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I have a hard time believing this would work without a significant amount of work to backport changes. The main problem is the forked workers inherit all of the booted code, $LOADED_FEATURES, so if we go spawn, we have to ensure anything that wasn't loaded in the spawned workers, we'd need to make it load as part of the boot of the spawned workers.

@agrare
Copy link
Member Author

agrare commented Feb 6, 2018

Closing in favor of #16953

@agrare agrare closed this Feb 6, 2018
@agrare agrare deleted the spawn_worker_config_setting branch May 2, 2018 16:25
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