De-couple DescendantLoader from Rails

[NickLaMuro]

Does the following to allow DescendantLoader to be loaded without a full Rails dependency:

• Moves the patches to ActiveRecord and ActiveSupport from the bottom of the file to a dedicated .setup method.
• Moves applying the patch to ActsAsArModel into that file, which then only applies the patch when ActsAsArModel is loaded (we own this code, so no need to monkey patch it with .send).
• Requires ActiveRecord as part of DescendantLoader, which allows the .setup method to function.
• Makes use of lib/manageiq.rb instead of Rails functions, allowing other instance methods to function.

This is part of allowing Vmdb::Settings to be loaded without Rails, since Vmdb::Plugins makes use of DescendantLoader.

Verbose description

aka: "Read this section if you are not into the whole tl; dr; thing"

Say you had a metrics collector for VMWare that was mostly a pure ruby script (this is a "completely original theoretical idea" that I came with on my own... ), and really didn't need much of Rails beyond the few models it needed to put in to the database. The active record code might not even be entirely necessary if we change the queuing system.

That said, the Vmdb::Settings code would still be most likely be necessary for this process (and/or container), and others like it, for it to boot up and be configurable via the main ManageIQ application. The lib/vmdb/settings.rb file, while by default won't use Vmdb::Plugins (where DescendantLoader is used directly), but far down the stack of Vmdb::Settings::init, it does call out to Vmdb::Plugins (and eventually DescendantLoader):

https://github.com/ManageIQ/manageiq/blob/cd17a20/lib/vmdb/settings.rb#L18
https://github.com/ManageIQ/manageiq/blob/cd17a20/lib/vmdb/settings.rb#L65
https://github.com/ManageIQ/manageiq/blob/cd17a20/lib/vmdb/settings.rb#L85
https://github.com/ManageIQ/manageiq/blob/cd17a20/lib/vmdb/settings.rb#L130
https://github.com/ManageIQ/manageiq/blob/cd17a20/lib/vmdb/settings.rb#L114

Above being the lines numbers from an approximation of the callstack starting from the line of Vmdb::Settings.init down to the line in Vmdb::Settings#template_roots that calls Vmdb::Plugins.

Explanation of End Goal and why QA steps is so minimal

End goal for this would be to be able to allow the following to work:

$ bundle exec ruby -Ilib -e "require 'vmdb/settings'; Vmdb::Settings.init"

But has a bunch of other pre-requisites that aren't handled in this PR:

• Load MoreCoreExtensions::Shared::Nested to allow lib/patches/config_patches.rb to be loaded properly
• Merge [WIP] Load Vmdb::Plugins via bundler #15461 (or something with the same kind of affect) to allow plugins to be loaded without Rails being loaded
• require 'lib/manageiq' and change all instances of Rails.root and Rails.env to ManageIQ.root and ManageIQ.env respectively (see Adds MiqHelper #15020 for some background)
• Include a require of active_record and autoload ActiveRecord::Base so config works properly and require_dependency is defined.
• Require vmdb/logging in some fashion (currently used in Vmdb::Plugins, but assumes autoloading is setup)

The other option to simplify some of those steps above would be to implement our own autoloading rule set, similar to what I did/described here:

#15174 (comment)

But again, something that is out of scope for this PR. Doing the end goal would be a lengthy process of multiple PRs, and this is a small component of that.

Misc Historical Context

A few PRs were merged prior to this one (one significantly so) that made the changes here necessary:

• Register VMDB plugins: Register VMDB plugins #12221
• Allow Vmdb::Plugins to work through code reloads in development.: Allow Vmdb::Plugins to work through code reloads in development. #15057

The first was needed when we switched to a provider plugins in separate repos architecture, and the second just makes sense to avoid devs having to restart their application/console session when changing code (which is slow). Both clearly wouldn't have tried to account for this case when those PR's were conceived, but now attribute to why this PR was necessary to solve the require "vmdb/settings" issue.

Mostly included this section because I discussed (parts of) this with @Fryguy at some point, and he was un-aware of some of the changes above. Also, I didn't include this originally since I figured most people would not care for the full explanation in the steps for testing this changes "works".

Links

• https://www.pivotaltracker.com/story/show/146936263

Steps for Testing/QA [Optional]

Run the following from the root of the ManageIQ directory:

$ bundle exec ruby -Ilib -e "require 'extensions/descendant_loader'; puts DescendantLoader.descendants_paths"

Test with both this branch and master.

[miq-bot]

Checked commits NickLaMuro/manageiq@2ff8f43~...cc17416 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks fine. 🍪

[jrafanie]

Looks good @NickLaMuro. Can you update the description with what you're solving? It doesn't really say why you did this. Decoupling is nice but clearly, you tried something in a pure ruby script, it failed, and so you made it work.

[jrafanie]

I'm curious what was the use case and how much faster and small the process is using this technique vs. a fat rails process.

[NickLaMuro]

@jrafanie This was basically the gist of the rational (already in the description):

This is part of allowing Vmdb::Settings to be loaded without Rails, since Vmdb::Plugins makes use of DescendantLoader.

I guess to avoid the novel, I was assuming from there someone could have extrapolated that "and to load Vmdb::Settings, you need to have Vmdb::Plugins, which is what I am going for in the slim worker process", so I skipped adding that part. Does that make sense or did you need more clarification?

[NickLaMuro]

but clearly, you tried something in a pure ruby script, it failed, and so you made it work.

Oh... woops... that ruby script was actually taken from the bundler bugfix PR I was working on... copy pasta fail... let me update...

[chessbyte]

@NickLaMuro @jrafanie what is the status of this PR?
assigning this PR to @Fryguy

[NickLaMuro]

This PR is complete as far as I am concerned, but it sounds like @jrafanie has some questions, which I feel like I answered already in my PR description. To avoid writing a verbose explanation unless it was truly necessary, I was waiting to see if asking if my PR description was not clear and should be elaborated on.

I can explain things further if needed, and added the Pivotal story related to this effort in PR description for context.

[Fryguy]

Remove the .rb

[Fryguy]

I'm concerned that this breaks Rails autoloading, but that concern is probably minor/unwarranted.

[NickLaMuro]

I will look into this.

[NickLaMuro]

Following the precedent from my other recent comment:

• Will remove the .rb (for some reason I thought it was required... or helped in some way, but nope)
• Not sure how I will test the autoloading concern to be honest. Might take me more time to investigate then I probably should at this time, mostly because I only so deep of knowledge on the internal workings of Rails' autoloading that I probably would need to do some spelunking to properly give a "yay" or "nay". I will try and update regardless.

[Fryguy]

Technically, this should be part of the setup as well. (The at_exit I mean, if my comment here was not clear )

[jrafanie]

Maybe address this comment?

[NickLaMuro]

Agreed. Will do. Should have confirmed that I intended to do that originally. Will look into doing this tomorrow.

[Fryguy]

This is part of allowing Vmdb::Settings to be loaded without Rails, since Vmdb::Plugins makes use of DescendantLoader.

I guess to avoid the novel, I was assuming from there someone could have extrapolated that "and to load Vmdb::Settings, you need to have Vmdb::Plugins, which is what I am going for in the slim worker process", so I skipped adding that part. Does that make sense or did you need more clarification?

It sounds like you are trying to not load the Vmdb::Plugins? If so, that doesn't make sense to me. The Vmdb::Plugins provide the models that the DescendantLoader needs to see. If those models aren't loaded, then there's really no point to the DescendantLoader at all.

---

Separately, I'm disliking to the move to the non-Rails ManageIQ.* methods more as I see this progress, but I think part of my issue is that the "run in a non Rails env" code still lives in the Rails app, so it's confusing, and potentially breakable by other developers who are not aware of the distinctions, and could easily add in some Rails.root code. I know having the code living inside the Rails app was intentional, as this started as more of a POC, but I think we should more this to a core gem sooner than later to keep the distinctions clearer.

That being said, we were also waiting on the manageiq-gems-pending/lib/util directory to be moved into a new gem such that it becomes the new core gem that we can then build on top of. I see the path forward as

• Extract manageiq-gems-pending/lib/util to a manageiq-core gem ( cc @chessbyte )
• Move the "core" stuff from manageiq into this new core gem and have manageiq then depend on the gem

[NickLaMuro]

It sounds like you are trying to not load the Vmdb::Plugins? If so, that doesn't make sense to me.

Poorly constructed sentence on my part I guess, but what I was referring to when I said "so I skipped adding that part" was the omission of adding what was in the quotes to the PR description, and not trying to avoid loading Vmdb::Plugins. This will not affect Vmdb::Plugins from being loaded, as I agree that would be counter productive, and I also wouldn't have bothered with #15461

I think we should more this to a core gem sooner than later to keep the distinctions clearer.

That being said, we were also waiting on the manageiq-gems-pending/lib/util directory to be moved into a new gem such that it becomes the new core gem that we can then build on top of.

I don't disagree with this, but:

a) I myself am not in charge of this effort, or am around in person where I am sure a majority of these conversations are involved with this, and unless I would have daily conversations with you and/or @chessbyte about it's direction, me working on it would impede it's progress
b) This is a huge blocker for me getting anything else done if I wait for this to happen, and I expect this to be weeks out yet before hits, so I am basically stonewalled from doing anything else until then

So, while agree this is confusing having two ways of doing the same thing (as you have also stated), but my thought was to move forward in this repo for now until design decisions have been made and are more closely a reality. Also, hopefully when slim workers are in place, we have some tests around them such that if Rails.env and such is being used, then that would fail because it does not exist.

[NickLaMuro]

Currently on a "New Year's cleanup" of some of my older PRs, and read through this one and made some additions after I re-acclimated myself to what was done here and why.

@jrafanie (and I guess @Fryguy as well), I have updated the description to include a "more verbose" section that describes a possible use case for this stuff down the road, and should hopefully address the confusion mentioned in @jrafanie 's comments here and here. #NickLaMuroBookClub2018

That said, I know @Fryguy still probably holds his opinion from above, and which is understandable and I while don't fully disagree with it, I still think this is the simplest way to achieve the motivations behind this PR, but I will add some color thought to that below.

That all said, this is not directly related to anything being worked on by myself or anyone else at the moment (as far as I know), and if Jason's comment above is enough to warrant closing this PR, that is fine as I assume my comment below will just be extra noise (and another novel to the archives).

---

So the ManageIQ.env and ManageIQ.root stuff is a pretty core component to allow this code, and other code in our codebase to function without needing the full Rails stack. The metrics collector example in the PR description should be a decent use case of where this would be used and is necessary with a slimmer footprint than what we have today, and I firmly believe this is the type of worker we should aim form when possible to make debugging performance issues easier and decrease the overall ManageIQ footprint, among other things.

I will emphasize, though, this is not suggesting we abandon using Rails, just only use it where appropriate (in my opinion, just the UI and API workers as they are today, but that is a larger, more encompassing architectural discussion that I will hold off on having here). I still very much think ActiveRecord should be the basis for our models, but I think in a lot of cases, we can limit the number of things we are loading into memory to mainly require 'active_record' and the models needed. There are many approaches for doing this that I have explored already (see #15174 and the comments for some of those approaches), but each require some assumptions on the end goal of the future architecture of ManageIQ, which I am not probably not fully up to date on what that is and/or is not fully solidified yet.

Going back to the Rails.root/Rails.env topic, an alternative solution would be push a change upstream a to do Rails.root without needing to initialize the entire Rails.application (see https://github.com/rails/rails/blob/bce675e/railties/lib/rails.rb#L64 for that, though, Rails.env is already fine as is), but I don't know how difficult that that would be implement, and it most likely would be a pretty niche use case to do that which might come with some with some heavy resistance from the core team. I figured just using our own implementation when necessary made the most sense and would unblock us from making further progress with this as a dependency. Ideally we would want to convert everything to using it (over time), as I think we want to use the same way all the time going forward to exercise it as much as possible, and we could enforce it with a custom rubocop rule as well.

Again, I think moving towards this (or something similar) makes sense in my mind, but if not, we maybe consider undoing what has already been merged since we are sitting in a limbo state at the moment and the debate on whether this is a good idea or not is blocking PRs from being merged/closed.

[jrafanie]

I don't know about the words in this PR but I'm fine with the code changes as long as the comments have been addressed. This PR doesn't feel controversial. It's merely a Rails-less alias for the Rails constant.

[miq-bot]

This pull request is not mergeable. Please rebase and repush.

[miq-bot]

This pull request has been automatically closed because it has not been updated for at least 6 months.

Feel free to reopen this pull request if these changes are still valid.

Thank you for all your contributions!