[WIP][POC] Slim (mini) evm rake tasks #14916
Conversation
| require File.expand_path('../config/application', __FILE__) | ||
| require File.expand_path('../lib/tasks/evm_rake_helper', __FILE__) | ||
| require 'bigdecimal' | ||
| puts "Mem: #{((1024 * BigDecimal.new(`ps -o rss= -p #{Process.pid}`))/BigDecimal.new(1_048_576)).to_f}MiB" |
There was a problem hiding this comment.
These two lines are tmp and can be ignored. This is just how I got the mem vals in the examples. Will be removed in the future.
|
cc @kbrock @dmetzger57 @Fryguy @jrafanie Figured you four might be interested in this. Very WIPy/POCy at the moment, but wouldn't mind your initial thoughts before investing too much more into this. |
Rakefile
Outdated
| if defined?(RSpec) | ||
| Rake::Task.tasks.select { |t| t.name =~ /^spec(:)?/ }.each(&:clear) | ||
| require File.expand_path('../lib/tasks/evm_rake_helper', __FILE__) | ||
| if ARGV.any? {|arg| arg.start_with? "evm:"} |
There was a problem hiding this comment.
Looks like the test suite proved this is a little too naive. Will look into a "smarter" version for this if.
NickLaMuro
force-pushed
the
mini-miq-server
branch
from
5f4319c
to
80fdf1f
Compare
|
😍 This was something @jrafanie and myself discussed doing a well, but it's cool to see you actually got it working. I can't really see it here, but do you have an understanding of what exactly is not loading that is contributing to the savings? Are the savings somewhat permanent over the life of the worker? |
@Fryguy So right now, the numbers are a bit misleading... the rake tasks are only short lived processes that then spawn the worker manager and such, which is done in the That said, I am planning to tackle
A lot of these savings is just not loading the entire rails application. For example, part of what got me on this kick was I noticed that the So that means that about 15MiB of our application being loaded is just our routes, and those will never be garbage collected, and those pretty much get loaded any time the But really, what needs to know about the routes in our workers? Basically the UI worker and that is it, and most of the workers as well probably just need a subset of the models in the directory, so my thought is that is that hopefully the workers can be smarter on what they load initially (specifically the main Still working out the finer details for implementing that though... Extra Infotl; dr; we can't get any smaller than 30MiB per process Finally, just for reference, if I do the following: $ ruby -rbigdecimal -e 'puts "Mem: #{((1024 * BigDecimal.new(`ps -o rss= -p #{Process.pid}`))/BigDecimal.new(1_048_576)).to_f}MiB"'
Mem: 8.13671875MiBSo that means the loading the ruby interpreter is basically 8Mb, and the other 22Mb before the first puts at the top of the |
|
|
||
| load File.expand_path("../lib/tasks/evm.rake", __FILE__) | ||
| else | ||
| require File.expand_path('../config/application', __FILE__) |
There was a problem hiding this comment.
It feels like all of the non evm:* rake tasks have an implicit application dependency. I wish we could bake this dependency into a rake task prerequisite. 🤔
|
@NickLaMuro wow, interesting 👍 ... I like the concept... let me think about it... some of the hurdles you have to leap here feel hard to maintain. I wonder how we could do this safely. |
|
@jrafanie Yeah, wouldn't mind sitting down with you and others to get an idea about how we should approach this at some point, but something we need to do immediately. But I am sure there are things we could here that I am not thinking of that could make this a bit easier, more manageable, and safer. I am sure a bunch of which I am not thinking of myself. Working on breaking some of the changes here out into their own PRs for a bit of an easier diff to deal with here, which was the intention from the get go, and might help with the above a bit. |
|
#14953 Fixes some of the test failures that we are seeing in this branch currently. The fixes should be obvious in each commit for the tests in #14953, and would have been less so in this PR. I will set up a |
I thought @imtayadeway fixed this recently, so that it wasn't doing a ton of allocations at boot. EDIT: Oh it wasn't merged yet: #14624 |
|
@NickLaMuro I had started coming up with a list of minimum things that a worker must do (I was planning on coding this exact thing, but was approaching it top-down). This list is far from complete, but I figured I'd share anyway. I expect that some things like Settings and Loggers will have to be made non-Railsy, or perhaps they can stay Railsy, but we have a different Rails environment (perhaps an entire Rails application) for them.
Things I'm not sure about
|
@Fryguy Looks like that part of my statement will still be true as there seemed to be no changes made to that PR. Will have to re-run my tests in a bit to see how much that might have changed based on Tim's changes. |
@Fryguy @NickLaMuro that's not quite right. That change can't help the time it takes the api.yml file to load into the config/options which is all that happens when you load the file above. Historically when the routes were drawn it was loading some expensive stuff that I delayed by introducing that |
|
@imtayadeway Still need to look over your PR, but is that optimizing for Memory or for CPU? Couldn't tell just by skimming or based off your comment. This PR is specifically trying to solve for memory, which if unsure if that was the intent of your PR. While the routes where one of the things I noticed, I am sure it wasn't the only thing. It was just an example of things that were leading to the large memory footprint at start up and after the application has been loaded properly, and also something that is basically only used by the UI worker (as far as I can tell), and doesn't need to be loaded by every worker booted. |
|
@NickLaMuro I think there's some confusion here between the PR that just got merged, which was merely a refactor to do away with some heavy-handedness, and this much older PR which I believe solved the problem of some expensive calls (in terms of CPU) being made at boot time, delaying their execution until needed (typically first request time). |
|
@imtayadeway ah, gotcha, that makes more sense then. Then my numbers are probably not that much different with and without these changes. Thanks for the clarification. |
This is a work in progress effort to reduce the time and memory it takes
to execute evm tasks, with the intent of eventually sliming down the
base EvmServer process and forked workers.
This commit is a blanket commit that WILL be broken up in the future,
but currently encompases the following:
* Trims what is loaded in the Rakefile when a `evm` task is loaded, so
that only the `lib/tasks/evm.rake` tasks are loaded, and the Rails
environment is skipped from being loaded
* Moves the bulk of config/environments/patches/database_configuration
to lib/miq_db_config.rb so the code can be reused (makes some changes
so Rails isn't required as well)
* Adds mini_miq_server.rb and mini_miq_queue.rb tools/utils, which are a
collection of ActiveRecord models with the bare minimum methods
necessary to run the evm tasks. Currently there is a bunch of
duplicated code, and future refactoring will share this code with the
main classes. These classes are namespaced under `Mini` (working
title) for now to avoid conflicts with the main models.
* Updates to lib/tasks/evm.rake and lib/tasks/evm_application.rb to use
the Mini::MiqServer and friends, and avoid loading the entire Rails
env.
Other minor updates and experimentation
* lib/vmdb/loggers.rb includes some code to allow it to be loaded
without autoloading
* Updates to lib/vmdb/settings.rb lib/vmdb/settings/database_source.rb
to not require Rails (unused at the moment... I think...)
With this, the time to run the updated tasks:
* evm:start
* evm:stop
* evm:status
* evm:status_full
Drops about 4-5 seconds (most only take about 2 seconds, and the memory
necessary drops in half.
Examples:
# master
$ time 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 | 74208 | 74246 | druby://127.0.0.1:58609 | 2017-04-27T02:33:17Z | 2017-04-27T02:34:10Z | false | database_owner
real 0m6.674s
user 0m4.789s
sys 0m1.530s
# this branch
$ time rake evm:status_full
Mem: 33.7109375MiB
Checking EVM status...
Zone | Server | Status | ID | PID | SPID | URL | Started On | Last Heartbeat | Master? | Active Roles
---------+--------+---------+----+-------+-------+-------------------------+----------------------+----------------------+---------+----------------
default | EVM | stopped | 1 | 74208 | 74246 | druby://127.0.0.1:58609 | 2017-04-27T02:33:17Z | 2017-04-27T02:34:10Z | false | database_owner
Mem: 61.5MiB
real 0m1.882s
user 0m1.384s
sys 0m0.468s
Because of things like this in the tools/utils/mini_miq_server.rb
existing:
# HACK: find a better way to share this code...
module MiqServer
module WorkerManagement
module Monitor
module Kill
end
end
end
end
And that this file now gets loaded whenever the
`lib/tasks/evm_application.rb` file is loaded (which is very early
because when loading rake tasks because it is part
Vmdb::Application.load_tasks, and that is usually before any of the
app/models are autoloaded), it means that this prevents the actual
autoloading of the MiqServer class from happening correctly.
Note: This commit will not live on it's in the future, and will be
merged in with some other commit, but committing and pushing this now
because it was a bit of a pain to discover the cause of errors that
resulted from this (and I want to make sure that this now passes on CI)
Because we are calling "load" under the hood, and the require statements are hitting an absolute path (I think), loading this code twice, when running a non `evm:` rake task, causes a bunch of WARNS to flood the output, and prevents travis from running properly.
These were both defined either to allow modules to be loaded in a non-load order dependent fashion, or allow the Mini:: classes to act as stubs for the main ones in other code. But this causes issues with the vmdb tests because they do in fact load the EvmApplication class to run some light tests on some of it's methods. By checking if the classes are autoloadable first, we can prevent them from being re-defined and breaking things because of that.
This is a commit to help implement the features implemented in the
following PRs:
ManageIQ#14953
ManageIQ#14967
ManageIQ#15014
ManageIQ#15018
ManageIQ#15019
ManageIQ#15020
Meant to illustrate how they will eventually be used in this PR, and
show how they will be implemented when they are merged into master.
The prior commits to this one are left in place to show before and
after, but this and those will be removed in the final iteration of this
pull request, and only the necessary changes needed to implement this in
the Rake tasks will be part of this PR.
Applying all of the changes for this PR was done as follows:
$ git apply <(curl -L ManageIQ#14953)
$ git apply <(curl -L ManageIQ#14967)
$ git apply <(curl -L ManageIQ#15014)
$ git apply <(curl -L ManageIQ#15018)
$ git apply <(curl -L ManageIQ#15019)
$ git apply <(curl -L ManageIQ#15020)
And then with some extra changes on top of that:
* Making use of `Miq.env` and `Miq.root` where relevant
* Implementing the modulularized PRs in the `tools/utils/mini_*.rb`
files where relavant
* Various bug fixes (though some seem like they shouldn't be there...)
Again, this is meant as a demonstration commit to show the before and
after while this is still a POC. Once the necessary pieces are in place
on master to make this commit irrelevant, it will be rebased out. It
may also change over time as this PR is worked on further.
NickLaMuro
force-pushed
the
mini-miq-server
branch
from
3965431
to
93e2dd4
Compare
|
Some comments on commits NickLaMuro/manageiq@113a547~...93e2dd4 Rakefile
app/models/miq_queue/put_methods.rb
lib/tasks/evm.rake
|
|
Checked commits NickLaMuro/manageiq@113a547~...93e2dd4 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 Rakefile
app/models/miq_queue.rb
app/models/miq_queue/constants.rb
app/models/miq_queue/put_methods.rb
app/models/miq_server/base_methods.rb
lib/miq_db_config.rb
lib/tasks/evm.rake
lib/tasks/evm_application.rb
tools/utils/mini_miq_queue.rb
tools/utils/mini_miq_server.rb
|
|
This pull request is not mergeable. Please rebase and repush. |
|
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! |
Purpose
This is a work in progress effort to reduce the time and memory it takes to execute evm tasks, with the intent of eventually sliming down the base EvmServer process and forked workers. All workers currently include the entire Rails environment, which includes routes, views, controllers, which is all code that is only needed by the UI process.
The end goal of this effort is to reduce the worker size by only loading what is needed by each worker, and this starts by reducing what is loaded by the main process, which requires updating
lib/worker/evm_server.rbwith similar changes.This commit is a blanket commit that WILL be broken up in the future, but currently encompasses the following:
Rakefilewhen aevmtask is executed, so that only thelib/tasks/evm.raketasks are loaded, and the Rails environment is skipped from being loaded entirelyconfig/environments/patches/database_configurationtolib/miq_db_config.rbso the code can be reused (makes some changes so Rails isn't required as well). Update: moved to a separate PR: Modularize patches/database_configuration.rb #14953mini_miq_server.rbandmini_miq_queue.rbtotools/utils, which are a collection of ActiveRecord models with the bare minimum methods necessary to run the evm tasks. Currently there is a bunch of duplicated code, and future refactoring will share this code with the main classes. These classes are namespaced underMini(working title) for now to avoid conflicts with the main models.lib/tasks/evm.rakeandlib/tasks/evm_application.rbto use theMini::MiqServerand friends to avoid loading the entire Rails env.Other minor updates and experimentation
lib/vmdb/loggers.rbincludes some code to allow it to be loaded without autoloadinglib/vmdb/settings.rb lib/vmdb/settings/database_source.rbto not require Rails (unused at the moment... I think...)With this, the time to run the updated tasks:
evm:startevm:stopevm:statusevm:status_fullDrops about 4-5 seconds (most only take about 2 seconds, and the memory necessary drops in half).
Examples Runs:
With a stopped appliance
master
this branch
With a running appliance
master
this branch
TODO
lib/tasks/evm.rakelib/workers/evm_server.rbapp/modelsandtools/utils