Introduce bundler groups #15459

jrafanie · 2017-06-27T20:52:09Z

What is a bundler group?

gem label/tag to group related dependencies
can tag with multiple bundler groups:

gem "good_migrations", :group => :development, :test

OR:

group :development, :test do
  gem "good_migrations"
  gem "parallel_tests"
  gem "rspec-rails",      "~>3.6.0"
end

Gem with no group are put in the :default bundler group.

How are they used?

the default config/application.rb does:

Bundler.require(*Rails.groups)

What this does:

groups get expanded to:
default bundler group + the Rails.env group: (test, development, or production)
populates the load path and optionally requires the different groups

Why?

We need to isolate dependencies but splitting them all out is hard as some are shared dependencies and the "core" manageiq features (models, basic libs) is not in a gemspec file that each worker could include in a Gemfile.

First step solution, use bundler groups to group dependencies and requires

The idea: "tag" our gems with a "specific feature" AND a manageiq_default group:

group :smartstate, :manageiq_default do
  gem "manageiq-smartstate",            "~>0.1.1",       :require => false
end

the smartstate feature is enabled by requiring either the smartstate or manageiq_default group.

How?

config/application.rb can now do:

ENV['BUNDLER_GROUPS'] ||= "manageiq_default"

if defined?(Bundler)
  groups = ENV['BUNDLER_GROUPS'].split(",").collect(&:to_sym)
  ...
  Bundler.require(*Rails.groups, *groups)
end

So, you can inject your dependencies at command line and for each process type by injecting them into an environment variable.

Workers specify their dependencies OR you can do BUNDLER_GROUPS="rest_api" bin/rails server

Is this the long term solution?

No. The proper way to do this is to move all dependencies where they belong and have a Gemfile for each process type. But, that's HARD and risky.

Bundler groups are safer and can be done simultaneously for ~~4.6~~ gaprindashvili and a bit more aggressively for master/~~5.0~~ h-release. EDIT @Fryguy

Summary:

memory and boot improvements now
organization and feature "seams"
eventual extraction of dependencies to code providing that feature (less disk space)

Loading all dependencies via the manageiq_default bundle group:

2311 requires
119 MB
real	0m5.918s
user	0m4.268s
sys	0m1.602s

With none of the manageiq_default gems:

1949 requires
86 MB

real	0m4.279s
user	0m2.866s
sys	0m1.358s

Not all workers will exclude all of the manageiq_default gems but it can pick and choose which ones it needs.

NickLaMuro

Looks good, just a few minor things I noticed since seeing this last. So I guess this whole effort only took you like... say... a couple hours to put together, right?

NickLaMuro · 2017-06-27T21:22:01Z

config/application.rb

-  Bundler.require(*Rails.groups, :ui_dependencies)
+  groups = ENV['BUNDLER_GROUPS'].split(",").collect(&:to_sym)
+  puts "loading: #{Rails.groups.inspect}"
+  puts "loading: #{groups.inspect}"


You should remove these.

I like them. Maybe I can disable them by default and put a bundler group nerd flag to enable them.

I support the concept of "Nerd Flags" with this. Seems like only a few people would actually be interested in this info (I would probably fall in that category at some point ).

NickLaMuro · 2017-06-27T21:22:14Z

app/models/mixins/miq_web_server_runner_mixin.rb

@@ -17,6 +17,7 @@ def do_before_work_loop

  module ClassMethods
    def start_worker(*args)
+      $log.info("#{self.name} $LOADED_FEATURES: #{$LOADED_FEATURES.length}")


Do we want to keep this?

More nerd flags here. I guess it can go

NickLaMuro · 2017-06-27T21:22:23Z

app/models/miq_worker/runner.rb

@@ -20,6 +20,7 @@ class TemporaryFailure < RuntimeError
  SAFE_SLEEP_SECONDS = 60

  def self.start_worker(*args)
+    $log.info("#{self.name} $LOADED_FEATURES: #{$LOADED_FEATURES.length}")


Do we want to keep this?

Same as above... 🤓

NickLaMuro · 2017-06-27T21:23:04Z

app/models/miq_worker.rb

@@ -370,7 +370,7 @@ def self.runner_script
  end

  def start_runner_via_spawn
-    pid = Kernel.spawn(self.class.build_command_line(guid), :out => "/dev/null", :err => [Rails.root.join("log", "evm.log"), "a"])
+    pid = Kernel.spawn(self.class.build_command_line(guid), [:out, :err] => [Rails.root.join("log", "evm.log"), "a"])


Huh... was this always like this? Oh well, 👍

It was until I tried to puts above ... 😉

NickLaMuro · 2017-06-27T21:25:32Z

lib/workers/miq_worker_types.rb

+  "MiqVimBrokerWorker"                                                        => [:manageiq_default],
+  "MiqWebServiceWorker"                                                       => %i(rest_api ui_dependencies web_server),
+  "MiqWebsocketWorker"                                                        => %i(ui_dependencies web_server web_socket)
+}.freeze


Ruh-roh... someone went "full @bdunne " on sortingthat array...

On a slightly more serious note, should we just always use %i() for these, just to be consistent?

haha, yeah... In order to mass rename some of the "conservative by default: manageiq_default" to more aggressive values... I had to sort workers by name, sort groups by name... Clearly, @bdunne would be proud.

Yeah, I can make even the single element arrays %i if that's helpful.

Yeah, I can make even the single element arrays %i if that's helpful.

Whatever you think makes sense. I think swapping between the two makes it a bit noisy to look at, but it is a pedantic concern. It's not like I am @bdunne ... #sickBurn #atTenThirtyAtNight #goToBedNick

👍 I approve of the sorting and agree with being consistent. (Although I prefer [] over %i())

Ok, I made the decision to not make changes... We can always change this if there are compelling reasons later. I like the lack of commas and feel that %i(...) is not required for a single element arrays.

I'm on the fence on this so do speak up if your OCD radar wants it one way or the other.

@jrafanie Well, at least fix your rubocop errors for the values being mis-aligned

jrafanie · 2017-06-28T02:12:14Z

@NickLaMuro did you like my 📖 in the PR description? 😉

NickLaMuro · 2017-06-28T03:30:52Z

@NickLaMuro did you like my 📖 in the PR description? 😉

It was a very cute "short story" you put together (though, I feel like I have seen the contents in some kind of "markdown-based-vim-like-presentation" or something...). I eagerly await your future releases/novels/epics.

bdunne · 2017-06-28T15:00:09Z

Gemfile

+  gem "responders",                     "~>2.0"
+  gem "ruby-dbus" # For external auth
+  gem "thin",                           "~>1.7.0",       :require => false
+  gem "secure_headers",                 "~>3.0.0"


This should be above thin

bdunne · 2017-06-28T15:03:08Z

Gemfile

+
+### shared dependencies
+group :google, :openshift, :manageiq_default do
+  gem "sshkey",                         "~>1.8.0",       :require => false


Should this be moved to the provider gems instead? Does it need to be listed in the :google group here since I only see app/models/container_deployment.rb:239: require 'sshkey'

Yes, for reference, the goal was to create groups and move dependencies into the groups to make it obvious where things belong. The next step is to push these dependencies to the correct place. Putting these in groups makes this very obvious.

bdunne · 2017-06-28T15:03:34Z

Gemfile

+end
+
+group :automate, :cockpit, :manageiq_default do
+  gem "open4",                          "~>1.3.0",       :require => false


Same concern here

bdunne · 2017-06-28T15:10:22Z

lib/workers/miq_worker_types.rb

+  "MiqVimBrokerWorker"                                                        => [:manageiq_default],
+  "MiqWebServiceWorker"                                                       => %i(rest_api ui_dependencies web_server),
+  "MiqWebsocketWorker"                                                        => %i(ui_dependencies web_server web_socket)
+}.freeze


👍 I approve of the sorting and agree with being consistent. (Although I prefer [] over %i())

Use an environment variable for now to default which groups are loaded. In workers, override this variable with what individual workers need. Delay requires or move them to where they are needed. Please enter the commit message for your changes. Lines starting

Defines a variable for defining the default bundler groups in the lib/workers/miq_worker_types.rb file that is reusable.

Converts MIQ_WORKER_TYPES into a hash to then be usable to set the bundler groups for a particular worker. Also allows the run_single_worker script function with the changes to the MiqWorker and MiqWorker::Runner classes to work with the new worker spawning workflow. Using this method removes duplicate code in the lib/workers/bin directory and makes things simpler to extend in the future.

This manageiq_default group functions like the :default group that all ungrouped gems are put in. We require this new group for all processes by default and specifici worker processes can bypass it via the BUNDLER_GROUPS env variable or updating the list of groups in the worker class list.

jrafanie · 2017-06-28T15:55:34Z

Gemfile

 gem "simple-rss",                     "~>1.3.1",       :require => false
 gem "snmp",                           "~>1.2.0",       :require => false
-gem "sshkey",                         "~>1.8.0",       :require => false
-gem "thin",                           "~>1.7.0",       :require => false


Very important: thin is completely gone since it doesn't support our async notifications using websockets.

…ations.

jrafanie · 2017-06-28T16:03:35Z

Ok, I think I addressed all the things. Throw more 🍅 .

Is everyone ok with being aggressive in minimizing dependencies in workers and fixing any bugs that come out from that? We can always change specific workers to [:manageiq_default] if we need to.

chrisarcand · 2017-06-28T16:13:23Z

Looks good!

Is everyone ok with being aggressive in minimizing dependencies in workers and fixing any bugs that come out from that?

👍

I'm guessing from the split you do that multiple groups can be set on the command line by comma-delimiting them?

NickLaMuro · 2017-06-28T16:16:03Z

Is everyone ok with being aggressive in minimizing dependencies in workers and fixing any bugs that come out from that? We can always change specific workers to [:manageiq_default] if we need to.

Yes, because by default, we will be using the default anyway, so we will only see this when using spawn.

jrafanie · 2017-06-28T18:20:52Z

I'm guessing from the split you do that multiple groups can be set on the command line by comma-delimiting them?

Yes, BUNDLER_GROUPS="ui_dependencies,seed" MIQ_SPAWN_WORKERS=true be rake evm:start

jrafanie · 2017-06-29T20:20:23Z

Ok, @bdunne I think all the feedback has been addressed. I pushed a minor fix I found while doing the follow up to this PR... The fix ensures we check for the ui classic engine constant since that is what would setup the autoload paths.

miq-bot · 2017-06-29T20:21:55Z

Some comments on commits jrafanie/manageiq@fdc3b4c~...dc40ab2

config/application.rb

⚠️ - 27 - Detected puts. Remove all debugging statements.
⚠️ - 28 - Detected puts. Remove all debugging statements.

lib/workers/bin/run_single_worker.rb

⚠️ - 41 - Detected puts. Remove all debugging statements.

miq-bot · 2017-06-29T20:22:56Z

Checked commits jrafanie/manageiq@fdc3b4c~...dc40ab2 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
11 files checked, 2 offenses detected

config/application.rb

❗ - Line 27, Col 5 - Rails/Output - Do not write to stdout. Use Rails's logger if you want to log.
❗ - Line 28, Col 5 - Rails/Output - Do not write to stdout. Use Rails's logger if you want to log.

Fryguy · 2017-06-30T20:41:40Z

Gemfile

+  manageiq_plugin "manageiq-providers-amazon"
+end
+
+group :ansible, :manageiq_default do


Minor, but does this group mean ansible_tower, embedded_ansible or both? If the latter, why not just have two group names. Having it as ansible-only feels confusing

group, :ansible_tower, :embedded_ansible, :manageiq_default do

From a high level, it's an ansible feature, to talk to ansible. Most of the providers give a way to talk to the provider and this is the same. So, it's both.

Fryguy · 2017-06-30T20:42:18Z

Gemfile

+
+group :foreman, :manageiq_default do
+  manageiq_plugin "manageiq-providers-foreman"
+  gem "foreman_api_client",             ">=0.1.0",   :require => false, :git => "https://github.com/ManageIQ/foreman_api_client.git", :branch => "master"


Don't we own this gem? We should just cut a release.

On this note, we used to have the savon gem somewhere for automate model purposes. However, I can't find it anymore. If we still need to provide it I think that it should now live in either manageiq-content's gemspec, or even better in manageiq-appliance's gem listing.

Sure... that's outside the scope of this PR. I'm not sure why we're riding master here. Lots of the "bundler groups" have these developer friendly things that hopefully are easier to see with bundler groups. We can fix them one by one.

Agreed that it's outside the PR scope...just wanted to get the comment out there :)

Fryguy · 2017-06-30T20:43:26Z

Gemfile

+  gem "sshkey",                         "~>1.8.0",       :require => false
+end
+
+group :automate, :cockpit, :manageiq_default do


Does automate here mean automation_engine or the automate model (i.e. manageiq-content)?

We can rename to automate_engine if you want... I tried to line these up with basic features than to try to be very specific.

I tried to line these up with basic features than to try to be very specific.

I'm concerned that this will actually make things more confusing (as exemplified here and with the ansible comment above) (NOTE: not pushing my agenda...feel free to say "maybe later" 😉 )

Fryguy · 2017-06-30T20:44:04Z

Gemfile

+
+### end of provider bundler groups
+
+group :automate, :seed, :manageiq_default do


I think the :automate group should be named :automation_engine to distinguish it from the automate model.

Fryguy · 2017-06-30T20:48:31Z

app/models/mixins/miq_web_server_worker_mixin.rb

@@ -24,9 +24,6 @@ def preload_for_console
    end

    def preload_for_worker_role
-      # Make these constants globally available
-      ::UiConstants
-


@jrafanie looks like this is still needed.....

17:04:06 bdunne@bdunne-t460p:~/projects/redhat/manageiq (unclaimed_deletions)$ rails s => Booting Puma => Rails 5.0.4 application starting in development on http://localhost:3000 => Run `rails server -h` for more startup options ** ManageIQ master; Database: adapter=postgresql, name=vmdb_development, host= ** Using session_store: ActionDispatch::Session::MemCacheStore I, [2017-07-05T17:07:40.017774 #3074] INFO -- : Initializing websocket worker! Puma starting in single mode... * Version 3.3.0 (ruby 2.3.3-p222), codename: Jovial Platypus * Min threads: 5, max threads: 5 * Environment: development * Listening on tcp://localhost:3000 Use Ctrl-C to stop W, [2017-07-05T17:07:49.288776 #3074] WARN -- : DEPRECATION WARNING: You didn't set `secret_key_base`. Read the upgrade documentation to learn more about this new config option. (called from env_config at /home/bdunne/.gem/ruby/2.3.3/gems/railties-5.0.4/lib/rails/application.rb:246) I, [2017-07-05T17:07:49.289844 #3074] INFO -- : Started GET "/" for ::1 at 2017-07-05 17:07:49 -0400 I, [2017-07-05T17:07:51.393252 #3074] INFO -- : Processing by DashboardController#login as HTML F, [2017-07-05T17:07:51.649379 #3074] FATAL -- : Error caught: [NameError] uninitialized constant ApplicationController::PPCHOICES /home/bdunne/.gem/ruby/2.3.3/bundler/gems/manageiq-ui-classic-937e70b2d07b/app/controllers/application_controller.rb:1874:in `get_global_session_data' /home/bdunne/.gem/ruby/2.3.3/gems/activesupport-5.0.4/lib/active_support/callbacks.rb:382:in `block in make_lambda' /home/bdunne/.gem/ruby/2.3.3/gems/activesupport-5.0.4/lib/active_support/callbacks.rb:150:in `block (2 levels) in halting_and_conditional' /home/bdunne/.gem/ruby/2.3.3/gems/actionpack-5.0.4/lib/abstract_controller/callbacks.rb:12:in `block (2 levels) in <module:Callbacks>' /home/bdunne/.gem/ruby/2.3.3/gems/activesupport-5.0.4/lib/active_support/callbacks.rb:151:in `block in halting_and_conditional' /home/bdunne/.gem/ruby/2.3.3/gems/activesupport-5.0.4/lib/active_support/callbacks.rb:454:in `block in call' /home/bdunne/.gem/ruby/2.3.3/gems/activesupport-5.0.4/lib/active_support/callbacks.rb:454:in `each' /home/bdunne/.gem/ruby/2.3.3/gems/activesupport-5.0.4/lib/active_support/callbacks.rb:454:in `call' /home/bdunne/.gem/ruby/2.3.3/gems/activesupport-5.0.4/lib/active_support/callbacks.rb:101:in `__run_callbacks__' /home/bdunne/.gem/ruby/2.3.3/gems/activesupport-5.0.4/lib/active_support/callbacks.rb:750:in `_run_process_action_callbacks' /home/bdunne/.gem/ruby/2.3.3/gems/activesupport-5.0.4/lib/active_support/callbacks.rb:90:in `run_callbacks' /home/bdunne/.gem/ruby/2.3.3/gems/actionpack-5.0.4/lib/abstract_controller/callbacks.rb:19:in `process_action' /home/bdunne/.gem/ruby/2.3.3/gems/actionpack-5.0.4/lib/action_controller/metal/rescue.rb:20:in `process_action' /home/bdunne/.gem/ruby/2.3.3/gems/actionpack-5.0.4/lib/action_controller/metal/instrumentation.rb:32:in `block in process_action' /home/bdunne/.gem/ruby/2.3.3/gems/activesupport-5.0.4/lib/active_support/notifications.rb:164:in `block in instrument'

Reverted by #15518

Fryguy · 2017-06-30T20:49:39Z

config/application.rb

 if defined?(Bundler)
-  Bundler.require(*Rails.groups, :ui_dependencies)
+  groups = ENV['BUNDLER_GROUPS'].split(",").collect(&:to_sym)


You might want to tack in a .map(&:strip), just in case someone does a comma separated list and instinctively adds in commas

BUNDLER_GROUPS="rest_api, ui_dependencies"

Hmm, are you saying they'd put spaces after each comma?

Yes...it's possible...I do it all the time, instinctively.

Super proud of @Fryguy for not using collect here

Believe it or not, I actually prefer map 😄

Fryguy · 2017-06-30T20:58:52Z

You want to explicitly disable the rubocop for Rails/Output since these are intentional.

This file pollutes the global namespace for legacy bad reasons. Followup to bundler groups PR: ManageIQ/manageiq#15459 The autoloading of ui constants in manageiq was reverted back here: ManageIQ/manageiq#15518

See: ManageIQ/manageiq-ui-classic#1658 Followup to bundler groups PR: ManageIQ#15459 The autoloading of ui constants in manageiq was reverted back here: ManageIQ#15518

jrafanie added core dependencies labels Jun 27, 2017

jrafanie requested review from Fryguy and NickLaMuro June 27, 2017 20:52

jrafanie changed the title ~~[WIP] Introduce bundler groups~~ Introduce bundler groups Jun 27, 2017

NickLaMuro reviewed Jun 27, 2017

View reviewed changes

bdunne requested changes Jun 28, 2017

View reviewed changes

jrafanie and others added 8 commits June 28, 2017 11:43

Add MIQ_DEFAULT_BUNDLER_GROUPS to global constants

a63b4d5

Defines a variable for defining the default bundler groups in the lib/workers/miq_worker_types.rb file that is reusable.

Add rest_api, ssa, nuage, automate, and open4

a1e4a95

Keep Runner.new interface as a hash for now

2ae3e30

Align all the things for readability.

8907d78

Sort by the worker class keys so similar providers stick together.

59b2b3a

jrafanie force-pushed the bundler_groups_with_runner_script branch from 1904ad0 to 2c295b4 Compare June 28, 2017 15:52

jrafanie commented Jun 28, 2017

View reviewed changes

jrafanie added 3 commits June 28, 2017 12:02

Aggressively specify bundler groups for each worker.

c59ca01

Add scheduler bundler group, do automate in seeding.

6d5270d

Remove thin web server since it doesn't support our websocket notific…

fa71aa5

…ations.

jrafanie force-pushed the bundler_groups_with_runner_script branch from 2c295b4 to fa71aa5 Compare June 28, 2017 16:02

NickLaMuro approved these changes Jun 28, 2017

View reviewed changes

Look if the Engine is loaded since that sets up autoload path

dc40ab2

Fryguy reviewed Jun 30, 2017

View reviewed changes

Fryguy approved these changes Jun 30, 2017

View reviewed changes

chessbyte assigned bdunne Jul 5, 2017

bdunne approved these changes Jul 5, 2017

View reviewed changes

bdunne merged commit 8dc77d3 into ManageIQ:master Jul 5, 2017

bdunne added the fine/no label Jul 5, 2017

bdunne added this to the Sprint 64 Ending Jul 10, 2017 milestone Jul 5, 2017

jrafanie mentioned this pull request Jul 6, 2017

Autoload UiConstants so other repos don't need to do this ManageIQ/manageiq-ui-classic#1658

Merged

jrafanie mentioned this pull request Jul 6, 2017

UiConstants are now autoloaded in manageiq-ui-classic #15520

Merged

1 task

martinpovolny mentioned this pull request Jul 7, 2017

Remove UiConstants ManageIQ/manageiq-ui-classic#1661

Closed

jrafanie deleted the bundler_groups_with_runner_script branch August 15, 2017 19:27


		### end of provider bundler groups

		group :automate, :seed, :manageiq_default do

Introduce bundler groups #15459

Introduce bundler groups #15459

Conversation

jrafanie commented Jun 27, 2017 • edited

What is a bundler group?

Why?

First step solution, use bundler groups to group dependencies and requires

How?

Is this the long term solution?

Summary:

NickLaMuro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrafanie commented Jun 28, 2017

NickLaMuro commented Jun 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrafanie commented Jun 28, 2017

chrisarcand commented Jun 28, 2017

NickLaMuro commented Jun 28, 2017

jrafanie commented Jun 28, 2017

jrafanie commented Jun 29, 2017

miq-bot commented Jun 29, 2017

miq-bot commented Jun 29, 2017

Fryguy Jun 30, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fryguy Jun 30, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fryguy Jun 30, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fryguy commented Jun 30, 2017

jrafanie commented Jun 27, 2017 •

edited

Fryguy Jun 30, 2017 •

edited

Fryguy Jun 30, 2017 •

edited

Fryguy Jun 30, 2017 •

edited