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

NickLaMuro · 2018-08-22T20:43:16Z

This is a reworking of #17652

By integrating the MiqFileStorage::Interface from ManageIQ/manageiq-gems-pending#361, we can introduce file splitting to all file storages that conform to that interface.

The PR simply reworks EvmDatabaseOps to use that new interface, and introduces a --byte-count flag to the dump/backup tasks to tell the File Storage to split the files on creation.

Links

Integration test suite: https://gist.github.com/NickLaMuro/8438015363d90a2d2456be650a2b9bc6
Depends on:
- MiqFileStorage interface and subclassing (with file splitting) manageiq-gems-pending#361
- Probably another PR or two from gems-pending split out from the above
Prerequisite for Adds file splitting support to dump/backup commands manageiq-appliance_console#50 (still works)

Steps for Testing/QA

You can use my test suite... if you can understand what it is doing and how to set it up...

miq-bot · 2018-08-30T18:27:22Z

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

NickLaMuro · 2018-09-17T15:10:14Z

@carbonin This is the last PR that is necessary to get things in the appliance console working again for all file storage types. Should hopefully be a straight forward review.

cc @jerryk55

carbonin

Mostly just asking for some clarification so I can understand what's going on a bit better. But looks good from a high level.

carbonin · 2018-09-17T14:29:17Z

lib/evm_database_ops.rb

+  STORAGE_ACTIONS_TO_METHODS = { :backup => :add, :dump => :add, :restore => :download }.freeze
+  private_class_method def self.with_file_storage(action, db_opts, connect_opts)
+    db_opts      = merged_db_opts(db_opts)
+    connect_opts = connect_opts.reverse_merge(:uri => "file://")


Where will this put the resulting backup file? Is this the same behavior as before or should we choose a better default?

Okay, so this is a bit of a hack to force the MiqLocalMountSession to come out. Basically, all of the evm:db:remote tasks should come with a :uri by default, but evm:db:local does not, and just passes a :local_file option.

This "HACK" basically forces the MiqLocalMountSession to be initialized when building the MiqFileStorage object below, the :uri is never used in that case, but just forces the scheme to the proper class.

Everything should work as it has before, and this just handles the case of local files in the same way.

Hm, okay it looks like this all could be simplified because it sounds like we're actually removing special cases by handling them all the same, but I won't ask (and don't want) a refactoring of all of this in this PR.

lib/evm_database_ops.rb

carbonin · 2018-09-17T15:28:44Z

lib/evm_database_ops.rb

-      db_opts[:local_file] = session.uri_to_local_path(uri)
+    MiqFileStorage.with_interface_class(connect_opts) do |file_storage|
+      send_args = [uri, db_opts[:byte_count]].compact
+      file_storage.send(STORAGE_ACTIONS_TO_METHODS[action], *send_args) do |input_path|


input_path here is the IO stream that the actual backup command will either write to or read from, correct?

Maybe it's worth noting here in a comment that this can either be a stream or a file on a mounted filesystem. What do you think?

Yeah, that seems reasonable. I will add some docs.

lib/evm_database_ops.rb

NickLaMuro · 2018-09-17T21:48:14Z

lib/evm_database_ops.rb

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

Okay @miq-bot, you're drunk... this is inside a comment....

carbonin

Just some comments on the comments, then this should be good to go.

carbonin · 2018-09-17T21:48:14Z

lib/evm_database_ops.rb

+      #
+      # This just puts the bare minimum necessary for URI.parse to recognize
+      # the :uri option as "file" scheme, and allows MiqFileStorage to then
+      # instanciate MiqLocalMountSession below in the `.with_interface_class`


typo s/instanciate/instantiate/

Would you believe me if I even made sure to :Spellcheck prior to pushing... and still didn't catch this...

carbonin · 2018-09-17T21:49:53Z

lib/evm_database_ops.rb

+      # `uri` will always be the final destination, but `input_path` below will
+      # be an intermediary fifo that will take the input from `pg_dump`,
+      # `pg_restore`, or `pg_basebackup`, and stream the results (in ruby) it
+      # to whatever endpoint it might be the resulting `uri`.


and stream the results (in ruby) it to whatever endpoint it might be the resulting uri.

This end bit of the sentence here just doesn't make sense.

carbonin · 2018-09-17T21:51:17Z

lib/evm_database_ops.rb

+      # HACK(ish): Since everything is flowing through MiqFileStorage so that
+      # we can handle the file splitting logic all in one place, this means
+      # that local dumps/backups also need to now have a "uri" in the
+      # connection opts so that MiqLocalMountSession will be used.


I know I asked for this comment, but I think just the second paragraph gets the point across.

Sometimes I write books, and sometimes it is just "word vomit"... you got the latter here.

MiqFileStorage is the new wrapper class around Object Stores and mounts, will a consistent interface for interacting with both. It's job is to handle streaming command output to the source destination, without letting the contents of the commands touch the local disk (unless the target is the local disk). A call to either `.add` or `.download` is now required for the proper IO redirection to work, but the interface is consistent between all providers, and the IO is expected to be handled in the same way for each.

Adds the `--byte-count` flag to evm:db:dump:* and evm:db:backup:* rake tasks which passes the `:byte_count` option to the MiqFileStorage for handling file splitting. Works with all classes that conform to the MiqFileStorage interface.

miq-bot · 2018-09-17T22:11:25Z

Some comments on commits NickLaMuro/manageiq@6902abc~...64f7f10

lib/evm_database_ops.rb

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

miq-bot · 2018-09-17T22:12:07Z

Checked commits NickLaMuro/manageiq@6902abc~...64f7f10 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 3 offenses detected

lib/evm_database_ops.rb

❗ - Line 132, Col 1 - Layout/EmptyLinesAroundArguments - Empty line detected around arguments.
❗ - Line 151, Col 1 - Layout/EmptyLinesAroundArguments - Empty line detected around arguments.

lib/tasks/evm_dba.rake

❗ - Line 53, Col 17 - Style/SymbolArray - Use %i or %I for an array of symbols.

miq-bot added the wip label Aug 22, 2018

This was referenced Aug 22, 2018

Use STDOUT for pg_basebackup ManageIQ/manageiq-gems-pending#365

Merged

[WIP] Add file splitting to evm:db tasks #17652

Closed

miq-bot added the unmergeable label Aug 30, 2018

NickLaMuro force-pushed the integrate_file_storage_changes_into_evm_database_ops branch from b087e1e to d5d4eca Compare August 30, 2018 21:09

miq-bot removed the unmergeable label Aug 31, 2018

NickLaMuro mentioned this pull request Sep 10, 2018

DB Backups to Openstack Swift ManageIQ/manageiq-gems-pending#371

Merged

NickLaMuro changed the title ~~[WIP] Add file splitting to evm:db tasks (V2)~~ Add file splitting to evm:db tasks (V2) Sep 13, 2018

miq-bot removed the wip label Sep 13, 2018

carbonin self-assigned this Sep 13, 2018

NickLaMuro force-pushed the integrate_file_storage_changes_into_evm_database_ops branch from d5d4eca to 462721d Compare September 13, 2018 15:48

carbonin reviewed Sep 17, 2018

View reviewed changes

lib/evm_database_ops.rb Show resolved Hide resolved

NickLaMuro mentioned this pull request Sep 17, 2018

Openstack Swift DB Backups #17967

Merged

NickLaMuro force-pushed the integrate_file_storage_changes_into_evm_database_ops branch from 462721d to f12f99c Compare September 17, 2018 21:45

carbonin reviewed Sep 17, 2018

View reviewed changes

NickLaMuro added 3 commits September 17, 2018 17:05

Add s3 support to evm:db:dump:* tasks

8881472

Add file splitting support to evm_dba tasks

64f7f10

Adds the `--byte-count` flag to evm:db:dump:* and evm:db:backup:* rake tasks which passes the `:byte_count` option to the MiqFileStorage for handling file splitting. Works with all classes that conform to the MiqFileStorage interface.

NickLaMuro force-pushed the integrate_file_storage_changes_into_evm_database_ops branch from f12f99c to 64f7f10 Compare September 17, 2018 22:06

carbonin approved these changes Sep 17, 2018

View reviewed changes

carbonin added enhancement core gaprindashvili/no labels Sep 17, 2018

carbonin merged commit ea94ae7 into ManageIQ:master Sep 17, 2018

carbonin added this to the Sprint 95 Ending Sep 24, 2018 milestone Sep 17, 2018

NickLaMuro mentioned this pull request Sep 19, 2018

Adds file splitting support to dump/backup commands ManageIQ/manageiq-appliance_console#50

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

NickLaMuro commented Aug 22, 2018 •

edited

Loading

miq-bot commented Aug 30, 2018

NickLaMuro commented Sep 17, 2018 •

edited

Loading

carbonin left a comment

carbonin Sep 17, 2018

NickLaMuro Sep 17, 2018

carbonin Sep 17, 2018

carbonin Sep 17, 2018

NickLaMuro Sep 17, 2018

carbonin Sep 17, 2018

NickLaMuro Sep 17, 2018

NickLaMuro commented Sep 17, 2018

carbonin left a comment

carbonin Sep 17, 2018

NickLaMuro Sep 17, 2018

carbonin Sep 17, 2018

carbonin Sep 17, 2018

NickLaMuro Sep 17, 2018

miq-bot commented Sep 17, 2018

miq-bot commented Sep 17, 2018

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

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

Conversation

NickLaMuro commented Aug 22, 2018 • edited Loading

Links

Steps for Testing/QA

miq-bot commented Aug 30, 2018

NickLaMuro commented Sep 17, 2018 • edited Loading

carbonin 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

NickLaMuro commented Sep 17, 2018

carbonin 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

miq-bot commented Sep 17, 2018

miq-bot commented Sep 17, 2018

NickLaMuro commented Aug 22, 2018 •

edited

Loading

NickLaMuro commented Sep 17, 2018 •

edited

Loading