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

Don't run the broker for ems_inventory if update_driven_refresh is set #15579

Merged

Conversation

agrare
Copy link
Member

@agrare agrare commented Jul 17, 2017

If using WaitForUpdates inventory collection then disable MiqEmsRefreshCoreWorker and don't run the MiqVimBrokerWorker for the ems_inventory role.

@agrare agrare added the wip label Jul 17, 2017
@chessbyte chessbyte requested a review from Fryguy July 19, 2017 14:48
@agrare agrare force-pushed the disable_broker_if_using_rbvmomi_refresh branch 4 times, most recently from 74d5448 to 70f7d17 Compare July 24, 2017 14:26
@agrare agrare changed the title [WIP] Don't run the broker for ems_inventory if update_driven_refresh is set Don't run the broker for ems_inventory if update_driven_refresh is set Jul 24, 2017
@agrare agrare removed the wip label Jul 24, 2017
@agrare agrare force-pushed the disable_broker_if_using_rbvmomi_refresh branch from 70f7d17 to cc78b66 Compare July 25, 2017 17:31
@Fryguy
Copy link
Member

Fryguy commented Jul 26, 2017

These failures in Travis look legit


roles << 'ems_inventory' unless Settings.prototype.ems_vmware.update_driven_refresh

roles
Copy link
Member

Choose a reason for hiding this comment

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

A visually nicer pattern might be

    %w(
      ems_metrics_collector
      ems_operations
      smartproxy
      smartstate
    ).tap do |roles|
      roles << 'ems_inventory' unless Settings.prototype.ems_vmware.update_driven_refresh
    end

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 will do

@agrare
Copy link
Member Author

agrare commented Jul 26, 2017

These failures in Travis look legit

Yeah that's because the setting doesn't exist until ManageIQ/manageiq-providers-vmware#76 adds it

@agrare agrare force-pushed the disable_broker_if_using_rbvmomi_refresh branch from cc78b66 to 94d85ed Compare July 26, 2017 20:19
@miq-bot
Copy link
Member

miq-bot commented Jul 26, 2017

Checked commits agrare/manageiq@153cf09~...94d85ed with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
6 files checked, 5 offenses detected

app/models/miq_ems_refresh_core_worker.rb

app/models/miq_vim_broker_worker.rb

app/models/miq_vim_broker_worker/runner.rb

smartproxy
smartstate
).tap do |roles|
roles << 'ems_inventory' unless Settings.prototype.try(:ems_vmware).try(:update_driven_refresh)
Copy link
Member

Choose a reason for hiding this comment

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

I'm confused why you have to do this as well as check the setting and role in the has_ems_inventory_role? method in the runner.

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 point of this is to control if the broker should start or not, the check in the runner is to control the cache_scope once it has started. Some examples would probably help.

Assuming you're using update_driven_refresh:

  1. The only active role on this appliance is ems_inventory
    In this case the broker shouldn't even start, that's what the above block handles

  2. There are e.g. ems_operations and ems_inventory roles active
    In this case we do want to start the broker to handle the ems_operations role but without checking the setting in the runner it would use cache_scope_ems_refresh which uses a lot more memory and defeat the purpose. This is what the check in the runner handles.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks!

@blomquisg blomquisg merged commit 7b7f375 into ManageIQ:master Jul 27, 2017
@blomquisg blomquisg added this to the Sprint 66 Ending Aug 7, 2017 milestone Jul 27, 2017
@agrare agrare deleted the disable_broker_if_using_rbvmomi_refresh branch August 14, 2017 18:50
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.

5 participants