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.

Sign up for GitHub

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

Jump to bottom

[POC][WIP] Slim evm rake tasks (alternative to #14916) #15342

Closed
wants to merge 9 commits into from
Closed

[POC][WIP] Slim evm rake tasks (alternative to #14916) #15342

wants to merge 9 commits into from

Conversation

NickLaMuro
Copy link
Member

An alternative approach to #14916, which had a lot of model changes and mixin splitting.

Changes

This approach does to major things differently:

  • Uses the EvmRakeHelper to define helper methods for limiting what gets loaded (I prefer this method to what was done in [WIP][POC] Slim (mini) evm rake tasks #14916)
  • Uses pure Arel for the queries made for the evm:status calls. While gross, I admit, it completely removes the need to include the MiqServer at all

Currently, the tasks besides evm:status and evm:status_full are not updated, and that is a work in progress still.

Benchmarks

Before:

$ bundle exec miqperf profile -mt --rake evm:status_full
** Using session_store: ActionDispatch::Session::MemCacheStore
Checking EVM status...
 Zone    | Server | Status  | ID |   PID |  SPID | URL                     | Started On           | Last Heartbeat       | Master? | Active Roles
---------+--------+---------+----+-------+-------+-------------------------+----------------------+----------------------+---------+----------------
 default | EVM    | stopped |  1 | 50381 | 50479 | druby://127.0.0.1:60539 | 2017-06-07T21:20:56Z | 2017-06-07T21:22:46Z | false   | database_owner


TOTAL_MEMORY_USED: 164.75390625MiB

Timings
-------
real    0m5.196s
user    0m4.000s
sys     0m1.160s
cuser   0m0.000s
csys    0m0.000s

After:

$ bundle exec miqperf profile -mt --rake evm:status_full
Checking EVM status...
 Zone    | Server | Status  | ID |   PID |  SPID | URL                     | Started On                | Last Heartbeat            | Master? | Active Roles
---------+--------+---------+----+-------+-------+-------------------------+---------------------------+---------------------------+---------+--------------
 default | EVM    | stopped |  1 | 50381 | 50479 | druby://127.0.0.1:60539 | 2017-06-07T21:20:56-05:00 | 2017-06-07T21:22:46-05:00 | false   |


TOTAL_MEMORY_USED: 58.84375MiB

Timings
-------
real    0m1.131s
user    0m0.740s
sys     0m0.380s
cuser   0m0.000s
csys    0m0.000s

Links

Steps for Testing/QA

Mostly confirm the rake evm: based tasks still work as expected (depending on how many were converted in this PR).

@NickLaMuro
Use require_relative in database_configuration_patch
db99d24 
Allows the DatabaseConfigurationPatch to be loaded without haven't the
lib/ dir in the load path.
@NickLaMuro
Adds ManageIQ::ActiveRecordConnector
c6dc7e0 
This class is in charge of making a simple connection for scripts and
workers that don't go through `config/environment.rb` (like rake tasks
and anything using `bin/rails`).

Allows for checking if a connection exists, and creating a connection if
needed, optionally being able to pass a block to close the created
connection after it has been executed.

This includes a ConnectionConfig class for simply pulling the connection
information from that `config/database.yml` in the same fashion as what
would be done in a regular rails process.
@NickLaMuro
Use ManageIQ::ActiveRecordConnector in tools/fix_auth.rb
7f6018e
@NickLaMuro
Move vmdb/loggers' Dir.glob require to bottom
bbbbb86 
Each of the lib/vmdb/loggers/* files use the `Vmdb::Loggers` name space
when defining themselves, but that namespace has not been defined yet if
`vmdb/loggers` is required on it's own.

By using moving the Dir.glob to the bottom of the file (after the
namespace has been setup), we can load the files just fine and not have
to change how the namespacing is done in the individual logger files.
@NickLaMuro
Move MiqServer.my_guid code to lib/manageiq.rb
536c783 
Non of this code needs rails, and is just file system calls and a little
SecureRandom if needed, so moving this to the ManageIQ module allows it
to be used without loading Rails.

Maintained the MiqServer.my_guid method for now since it is used by
MiqServer.my_server and that is used in many places.
@NickLaMuro
Use Vmdb::Settings::Walker for decrypt_passwords in db patch
4b41b52 
The Vmdb::Settings.decrypt_passwords is just a convenience method and
just calls out to Vmdb::Settings::Walker.decrypt_passwords, and also
Vmdb::Settings::Walker doesn't require any external dependencies to
function, so using this makes the database_configuration_patch more
portable.
@NickLaMuro
Change evm:status and evm:status_full to non-Rails
971a298 
Use a new task called evm_connect as a pre-req to build an ActiveRecord
connection (if one doesn't exist) and allow the tasks to make DB calls.

Also converts the MiqServer calls to vanilla Arel (a little bit gross...
I know) to allow them to make DB calls without the model (and Rails)
being loaded.
@NickLaMuro
Skip loading Rails in rake for evm:status tasks
9a470a5 
Adds helpers to the evm_rake_helper to dynamically load portions of the
application depending of what rake tasks are being required.
Drastically speeds up the time it takes for task to be executed and the
time taken to do so.

Before
------

    $ bin/rake evm:status_full
    Checking EVM status...
     Zone    | Server | Status  | ID  ...
    ---------+--------+---------+---- ...
     default | EVM    | stopped |  1  ...

    TOTAL_MEMORY_USED: 164.75390625MiB

    Timings
    -------
    real    0m5.196s
    user    0m4.000s
    sys     0m1.160s
    cuser   0m0.000s
    csys    0m0.000s

After
------

    $ bin/rake evm:status_full
    Checking EVM status...
     Zone    | Server | Status  | ID  ...
    ---------+--------+---------+---- ...
     default | EVM    | stopped |  1  ...

    TOTAL_MEMORY_USED: 58.84375MiB

    Timings
    -------
    real    0m1.131s
    user    0m0.740s
    sys     0m0.380s
    cuser   0m0.000s
    csys    0m0.000s
@NickLaMuro
[WIP] Convert other evm tasks to "Rails-less"
f41564b
@miq-bot miq-bot changed the title [POC][WIP] Slim evm rake tasks (alternative to #14916) [WIP] [POC][WIP] Slim evm rake tasks (alternative to #14916) Jun 9, 2017
@miq-bot miq-bot added the wip label Jun 9, 2017
@miq-bot
Copy link
Member

miq-bot commented Jun 9, 2017

Some comments on commits NickLaMuro/manageiq@db99d24~...f41564b

spec/lib/manageiq/active_record_connector_spec.rb

  • ⚠️ - 35 - Detected puts. Remove all debugging statements.
  • ⚠️ - 37 - Detected puts. Remove all debugging statements.
  • ⚠️ - 79 - Detected puts. Remove all debugging statements.
  • ⚠️ - 80 - Detected puts. Remove all debugging statements.

@miq-bot
Copy link
Member

miq-bot commented Jun 9, 2017

Checked commits NickLaMuro/manageiq@db99d24~...f41564b with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
11 files checked, 26 offenses detected

app/models/miq_server.rb

lib/manageiq/active_record_connector.rb

lib/tasks/evm_application.rb

lib/tasks/evm_rake_helper.rb

spec/lib/manageiq/active_record_connector_spec.rb

@NickLaMuro NickLaMuro mentioned this pull request Jun 14, 2017
Merged
@NickLaMuro NickLaMuro changed the title [WIP] [POC][WIP] Slim evm rake tasks (alternative to #14916) [POC][WIP] Slim evm rake tasks (alternative to #14916) Jun 15, 2017
@miq-bot
Copy link
Member

miq-bot commented Jun 16, 2017

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

@NickLaMuro
Copy link
Member Author

@jrafanie coming in with #15375 and ruining my day... Geez...

Just kidding, saw this one coming :trollface:

@jrafanie
Copy link
Member

@jrafanie coming in with #15375 and ruining my day... Geez...

Just kidding, saw this one coming

Ah, yes! OOPS. 😅

@chessbyte chessbyte requested a review from jrafanie July 5, 2017 14:05
@miq-bot miq-bot closed this Feb 5, 2018
@miq-bot
Copy link
Member

miq-bot commented Feb 5, 2018

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jrafanie jrafanie Awaiting requested review from jrafanie jrafanie is a code owner

Assignees

@bdunne bdunne

Labels
unmergeable wip
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@NickLaMuro @miq-bot @jrafanie @bdunne