Add evm:db:dump:local and evm:db:dump:remote tasks #17483

NickLaMuro · 2018-05-25T22:10:12Z

Builds off of ManageIQ/manageiq-gems-pending#351 , which added pg_dump functionality back to the PostgresAdmin lib, and creates rake tasks for running a database dump to either a local file, of NFS/SMB share.

Notable changes

Refactors EvmDatabaseOps#backup so that EvmDatabaseOps#dump can leverage almost all of the same code.
Carbon copies the evm:db:backup tasks into evm:db:dump tasks.
Adds option to --exclude-table-data from the dump for specific tables

Links

Part of https://bugzilla.redhat.com/show_bug.cgi?id=1535345
Required (already merged): Add pg_dump support back to PostgresAdmin manageiq-gems-pending#351

Steps for Testing/QA

Not sure the best way to test that these work, but we probably should run some tests to confirm everything is behaving as expected. I will probably do some testing of this locally now that I have this up for review, and will report back with how I tested things for others to try as well.

Update: Here is what I did to test:

Found a db to dump on my local machine

Ran in the terminal:

$ rake evm:db:dump:local -- --local-file tmp/vmdb_db_dump --dbname my_vmdb

Checked the log/evm.log to see that the commands run made sense:

$ tail log/evm.log
...
[----] I, [2018-05-25T17:17:16.749795 #46992:3ff54d03fa0c]  INFO -- : MIQ(PostgresAdmin.runcmd_with_logging) Running command... psql --no-password --dbname my_vmdb --command SELECT\ pg_database_size\(\'my_vmdb\'\)\;
[----] I, [2018-05-25T17:17:16.769418 #46992:3ff54d03fa0c]  INFO -- : MIQ(EvmDatabaseOps.run_db_opt_for_new_uri) [my_vmdb] with database size: [100 bytes], free space at [tmp/vmdb_db_dump]: [200 bytes]
[----] I, [2018-05-25T17:17:16.769763 #46992:3ff54d03fa0c]  INFO -- : MIQ(PostgresAdmin.runcmd_with_logging) Running command... pg_dump --no-password --format c --file tmp/vmdb_db_dump my_vmdb
[----] I, [2018-05-25T17:18:25.311775 #46992:3ff54d03fa0c]  INFO -- : MIQ(EvmDatabaseOps.dump) [my_vmdb] database has been dumped up to file: [tmp/vmdb_db_dump]

Ran in the terminal:

$ rake evm:db:dump:local --           \
    --local-file tmp/vmdb_db_dump_2   \
    --dbname my_vmdb                  \
    --exclude-table-data "metrics_*" "vim_performance_states" "event_streams"

Confirmed that the dump size was smaller and commands executed made sense:

$ ls -lhtmp/vmdb_db_dump*
-rw-r--r--  1 nicklamuro  staff   452M May 25 17:18 tmp/vmdb_db_dump
-rw-r--r--  1 nicklamuro  staff   331M May 25 18:35 tmp/vmdb_db_dump_2
$ tail log/evm.log
...
[----] I, [2018-05-25T18:34:16.749795 #46993:3ff54d03fa0c]  INFO -- : MIQ(PostgresAdmin.runcmd_with_logging) Running command... psql --no-password --dbname my_vmdb --command SELECT\ pg_database_size\(\'my_vmdb\'\)\;
[----] I, [2018-05-25T18:34:16.769418 #46993:3ff54d03fa0c]  INFO -- : MIQ(EvmDatabaseOps.run_db_opt_for_new_uri) [my_vmdb] with database size: [100 bytes], free space at [tmp/vmdb_db_dump]: [200 bytes]
[----] I, [2018-05-25T18:34:16.769763 #46993:3ff54d03fa0c]  INFO -- : MIQ(PostgresAdmin.runcmd_with_logging) Running command... pg_dump --no-password --format c --file tmp/vmdb_db_dump my_vmdb --exclude-table-data metrics_\* --exclude-table-data vim_performance_states --exclude-table-data event_streams
[----] I, [2018-05-25T18:35:25.311775 #46993:3ff54d03fa0c]  INFO -- : MIQ(EvmDatabaseOps.dump) [my_vmdb] database has been dumped up to file: [tmp/vmdb_db_dump]

NickLaMuro · 2018-05-25T23:56:28Z

@miq-bot assign @carbonin
@miq-bot add_labels enhancement, gaprindashvili/no

carbonin · 2018-05-29T18:21:36Z

lib/evm_database_ops.rb

+  #   :username => 'samba_one',
+  #   :password => 'Zug-drep5s',
+  #   :remote_file_name => "backup_1",     - Provide a base file name for the uploaded file
+  private_class_method def self.run_db_opt_for_new_uri(db_opts, connect_opts = {})


This method name is not great. I would prefer we refactor this into more pieces to make the individual operations more obvious. For example, with_mount_session and check_free_space

Not sure why this was squashed .. will copy the comment into a separate one.

carbonin · 2018-05-29T18:23:35Z

lib/tasks/evm_dba.rake

+      end
+
+      desc 'Dump the local ManageIQ EVM Database (VMDB) to a remote file'
+      task :remote do


Do we need the restore for the dump side of things?

Nope, this is handled in PostgresAdmin:

https://github.com/ManageIQ/manageiq-gems-pending/blob/559540c54a32f202a60db14d6ba4a51bf01262e5/lib/gems/pending/util/postgres_admin.rb#L93-L102

Oddly enough, written by you: ManageIQ/manageiq-gems-pending@dbb78da

Lol sorry, I read this as task :restore do
🤦‍♂️

To be more clear, I was suggesting removing this task because I thought it was restoring a pg_dump which I don't really want to do unless it was an old backup (from before we removed it as a backup option).

That is fair. Yeah, I really don't plan to support pg_dump restores myself. I have a fun set of directions for importing DBs when working on performance issues and such, so me personally, I only really care about getting the pg_dump stuff working from a "backup" perspective.

The fact that it "can work" in the :restore tasks is just a nice plus.

carbonin · 2018-05-29T18:26:21Z

lib/evm_database_ops.rb

+  #   :username => 'samba_one',
+  #   :password => 'Zug-drep5s',
+  #   :remote_file_name => "backup_1",     - Provide a base file name for the uploaded file
+  private_class_method def self.run_db_opt_for_new_uri(action, db_opts, connect_opts = {})


This method name is not great. I would prefer we refactor this into more pieces to make the individual operations more obvious. For example, with_mount_session and check_free_space

Cool, yeah, I can look into doing that. I agree, the name sucks.

cc @jrafanie #namingIsHard #botherLJWheneverPossible

I like naming things "run_things_and_stuff" to force myself to name it before merge. Sometimes, I leave it like that so someone else can provide a better name.

For me this is the only thing holding this PR up. @NickLaMuro are you going to try to refactor this or are you just looking for naming suggestions?

@carbonin actively looking at this right now actually.

I think your naming suggestions are pretty solid, so probably just going to go with them, but I am noticing that half of what I would do for with_mount_session is also needed for the EvmDatabaseOps.restore method as well, so pondering how I can that in as well.

Since I am splitting things up a bit now (where before, it was just a copy-pasta for the most part), I am taking my time to do it right. This is the disadvantage of going the full refactoring route, where I original just kinda did it half way. Splitting this up via your suggestion is the overall better approach in my opinion, just has the cost of time to implement.

Sounds good @NickLaMuro, just making sure 👍

A helper method that will be used in future commits. Also adds a few specs to validate log output.

This methods is used to handle the session mount logic for the restore/backup methods in this class in a single spot. Note: merged_db_opts is used here in the log messages because db_opts is not mutated as part of with_mount_session, but we want to make sure we are logging the correct info. Putting the log statement inside the with_mount_session seemed like a bad idea, so rebuilding the calculated db_opts hash seemed like the least ugly workaround.

Moves out free_space calculation code from EvmDatabaseOps.backup into a helper method for later shared use.

Runs using the same setup as EvmDatabaseOps#backup, but runs a `pg_dump` under the hood. It uses the same method for determining if there is enough diskspace for the dump as the backup method. This isn't 100% accurate, but it should be "Good Enough™".

Effectively a carbon copy of the `:backup` namespace tasks.

Allows excluding of specific tables from the database dump that may not be necessary for the intended use.

miq-bot · 2018-05-31T01:57:34Z

Some comments on commits NickLaMuro/manageiq@bf7e19f~...051a1f7

spec/lib/evm_database_ops_spec.rb

⚠️ - 68 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.

miq-bot · 2018-05-31T01:58:51Z

Checked commits NickLaMuro/manageiq@bf7e19f~...051a1f7 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 6 offenses detected

lib/evm_database_ops.rb

❗ - Line 107, Col 1 - Layout/EmptyLinesAroundArguments - Empty line detected around arguments.
❗ - Line 117, Col 1 - Layout/EmptyLinesAroundArguments - Empty line detected around arguments.
❗ - Line 121, Col 1 - Layout/EmptyLinesAroundArguments - Empty line detected around arguments.
❗ - Line 125, Col 5 - Style/SafeNavigation - Use safe navigation (&.) instead of checking if an object exists before calling the method.

lib/tasks/evm_dba.rake

❗ - Line 232, Col 9 - Style/SymbolArray - Use %i or %I for an array of symbols.
❗ - Line 235, Col 9 - Style/SymbolArray - Use %i or %I for an array of symbols.

carbonin · 2018-05-31T13:32:48Z

Those EmptyLinesAroundArguments cops are strange, is that because of how we're using private_class_method here? Feels like a bug ...

carbonin

Looks great @NickLaMuro

Looking over all of this again I'm seeing a bunch of things that would be nice to fix up (I can't keep track of these options hashes at all), but this PR is definitely not the place for it.

NickLaMuro force-pushed the add_evm_db_dump_tasks branch 3 times, most recently from f1ed8cb to 6116691 Compare May 25, 2018 23:36

miq-bot assigned carbonin May 25, 2018

miq-bot added enhancement gaprindashvili/no labels May 25, 2018

NickLaMuro mentioned this pull request May 26, 2018

Add dbdump action ManageIQ/manageiq-appliance_console#45

Merged

carbonin reviewed May 29, 2018

View reviewed changes

ManageIQ deleted a comment from NickLaMuro May 30, 2018

Adds helper method for building calculated db_opts

bf7e19f

A helper method that will be used in future commits. Also adds a few specs to validate log output.

NickLaMuro force-pushed the add_evm_db_dump_tasks branch 3 times, most recently from a8630f2 to 56ad9f2 Compare May 31, 2018 01:29

NickLaMuro added 5 commits May 30, 2018 20:35

Add EvmDatabaseOps.validate_free_space

594e50d

Moves out free_space calculation code from EvmDatabaseOps.backup into a helper method for later shared use.

Add EvmDatabaseOps.dump

93ef4c1

Runs using the same setup as EvmDatabaseOps#backup, but runs a `pg_dump` under the hood. It uses the same method for determining if there is enough diskspace for the dump as the backup method. This isn't 100% accurate, but it should be "Good Enough™".

Add evm:db:dump:local and evm:db:dump:remote tasks

cb8524a

Effectively a carbon copy of the `:backup` namespace tasks.

Add --exclude-table-data to evm:db:dump tasks

051a1f7

Allows excluding of specific tables from the database dump that may not be necessary for the intended use.

NickLaMuro force-pushed the add_evm_db_dump_tasks branch from 56ad9f2 to 051a1f7 Compare May 31, 2018 01:36

carbonin approved these changes May 31, 2018

View reviewed changes

carbonin merged commit a09f2d1 into ManageIQ:master May 31, 2018

carbonin added this to the Sprint 87 Ending Jun 4, 2018 milestone May 31, 2018

NickLaMuro mentioned this pull request Jun 5, 2018

Allow specifying table exclusions in Database Dump action ManageIQ/manageiq-appliance_console#47

Merged

NickLaMuro mentioned this pull request Aug 9, 2018

Fix bug with EvmDatabaseOps.dump #17830

Merged

NickLaMuro mentioned this pull request Sep 17, 2018

Add file splitting to evm:db tasks (V2) #17894

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add evm:db:dump:local and evm:db:dump:remote tasks #17483

Add evm:db:dump:local and evm:db:dump:remote tasks #17483

NickLaMuro commented May 25, 2018 •

edited

Loading

NickLaMuro commented May 25, 2018

carbonin May 29, 2018

carbonin May 29, 2018

carbonin May 29, 2018

NickLaMuro May 30, 2018

carbonin May 30, 2018

carbonin May 30, 2018

NickLaMuro May 30, 2018

carbonin May 29, 2018

NickLaMuro May 30, 2018

jrafanie May 30, 2018

carbonin May 30, 2018

NickLaMuro May 30, 2018

carbonin May 30, 2018

miq-bot commented May 31, 2018

miq-bot commented May 31, 2018

carbonin commented May 31, 2018

carbonin left a comment

Add evm:db:dump:local and evm:db:dump:remote tasks #17483

Add evm:db:dump:local and evm:db:dump:remote tasks #17483

Conversation

NickLaMuro commented May 25, 2018 • edited Loading

Notable changes

Links

Steps for Testing/QA

NickLaMuro commented May 25, 2018

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

miq-bot commented May 31, 2018

miq-bot commented May 31, 2018

carbonin commented May 31, 2018

carbonin left a comment

Choose a reason for hiding this comment

NickLaMuro commented May 25, 2018 •

edited

Loading