Support for DB Restore from Object Stores #17791
Conversation
Support restore from an object store.
lib/evm_database_ops.rb
Outdated
| @@ -110,6 +111,7 @@ def self.restore(db_opts, connect_opts = {}) | |||
| if db_opts[:local_file].nil? | |||
| if action == :restore | |||
| uri = connect_opts[:uri] | |||
| connect_opts[:remote_file_name] ||= connect_opts[:uri] | |||
There was a problem hiding this comment.
I think we can remove this and use uri (remote_file_uri) in the block, right?
There was a problem hiding this comment.
Yes! I knew there was something we changed that could handle that. Thanks. Changing now.
lib/evm_database_ops.rb
Outdated
| @@ -89,7 +89,8 @@ def self.restore(db_opts, connect_opts = {}) | |||
| # :username => 'samba_one', | |||
| # :password => 'Zug-drep5s', | |||
|
|
|||
| uri = with_mount_session(:restore, db_opts, connect_opts) do |database_opts, _session, _remote_file_uri| | |||
| uri = with_mount_session(:restore, db_opts, connect_opts) do |database_opts, session, _remote_file_uri| | |||
| database_opts[:local_file] = session.download(database_opts[:local_file], connect_opts[:remote_file_name]) if session | |||
There was a problem hiding this comment.
I think connect_opts[:remote_file_name] here can just be the yielded remote_file_uri
Instead of overloading the connect opts by adding :remote_file_name use the already yielded remote_file_uri as the argument to the session.download call for restore.
|
@carbonin fixed. good catch - thanks. |
lib/evm_database_ops.rb
Outdated
| @@ -89,7 +89,8 @@ def self.restore(db_opts, connect_opts = {}) | |||
| # :username => 'samba_one', | |||
| # :password => 'Zug-drep5s', | |||
|
|
|||
| uri = with_mount_session(:restore, db_opts, connect_opts) do |database_opts, _session, _remote_file_uri| | |||
| uri = with_mount_session(:restore, db_opts, connect_opts) do |database_opts, session, remote_file_uri| | |||
| database_opts[:local_file] = session.download(database_opts[:local_file], remote_file_uri) if session | |||
There was a problem hiding this comment.
So I'm thinking that this should only reassign database_opts[:local_file] if the local file doesn't exist already.
I'm worried that we're clobbering the value set on line 122 for mount types that actually mount.
Maybe something like this:
database_opts[:local_file] = session&.download(database_opts[:local_file], remote_file_uri) unless File.exist?(database_opts[:local_file])What do you think?
There was a problem hiding this comment.
I feel like database_opts[:local_file] will be set for the S3 case, but won't actually exist yet because we don't mount, but for the other cases we should just restore from the mounted filesystem rather than essentially re-running the download.
Only run the download if the local file doesn't exist.
|
Checked commits jerryk55/manageiq@979731f~...8ee1edc with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
Support restore from an object store. This is one of several PRs in support of the RFE for
https://bugzilla.redhat.com/show_bug.cgi?id=1513520. Another PR against the manageiq-appliance_console is also forthcoming.
@roliveri @carbonin please review and merge when appropriate.
Links [Optional]