From ca92710159b27df7271a4d540b9603a6724da930 Mon Sep 17 00:00:00 2001 From: Nick LaMuro Date: Mon, 5 Nov 2018 16:45:07 -0600 Subject: [PATCH] Add support for magic check in EvmDatabaseOps Calls the newly created method `MiqFileStorage.magic_number_for` to pre-fetch the magic from the file that will be downloaded, and send that to `PostgresAdmin` as a `database_opts` to avoid checking it on it's end. Note: Shortened a comment in here, but since it is part of the git history, I am fine with it. Those willing to dig deep can find it. --- lib/evm_database_ops.rb | 32 +++++++++++++++++++------------ spec/lib/evm_database_ops_spec.rb | 23 ++++++++++++++++++++-- 2 files changed, 41 insertions(+), 14 deletions(-) diff --git a/lib/evm_database_ops.rb b/lib/evm_database_ops.rb index ee5391ce32b..0d73d12f667 100644 --- a/lib/evm_database_ops.rb +++ b/lib/evm_database_ops.rb @@ -89,12 +89,12 @@ def self.restore(db_opts, connect_opts = {}) # :username => 'samba_one', # :password => 'Zug-drep5s', - uri = with_file_storage(:restore, db_opts, connect_opts) do |database_opts| - prepare_for_restore(database_opts[:local_file]) + uri = with_file_storage(:restore, db_opts, connect_opts) do |database_opts, backup_type| + prepare_for_restore(database_opts[:local_file], backup_type) # remove all the connections before we restore; AR will reconnect on the next query ActiveRecord::Base.connection_pool.disconnect! - PostgresAdmin.restore(database_opts) + PostgresAdmin.restore(database_opts.merge(:backup_type => backup_type)) end _log.info("[#{merged_db_opts(db_opts)[:dbname]}] database has been restored from file: [#{uri}]") uri @@ -139,12 +139,16 @@ def self.restore(db_opts, connect_opts = {}) MiqFileStorage.with_interface_class(connect_opts) do |file_storage| send_args = [uri, db_opts[:byte_count]].compact - # Okay, this probably seems a bit silly seeing that we just went to the - # trouble of doing a `.compact)`. The intent is that with a restore, we - # are doing a `MiqFileStorage.download`, and the interface for that - # method is to pass a `nil` for the block form since we are streaming the - # data from the command that we are writting as part of the block. - send_args.unshift(nil) if action == :restore + if action == :restore + # `MiqFileStorage#download` requires a `nil` passed to the block form + # to accommodate streaming + send_args.unshift(nil) + magic_numbers = { + :pgdump => PostgresAdmin::PG_DUMP_MAGIC, + :basebackup => PostgresAdmin::BASE_BACKUP_MAGIC + } + backup_type = file_storage.magic_number_for(uri, :accepted => magic_numbers) + end # Note: `input_path` will always be a fifo stream (input coming from # PostgresAdmin, and the output going to the `uri`), since we want to @@ -161,15 +165,19 @@ def self.restore(db_opts, connect_opts = {}) # set `db_opts` local file to that stream. file_storage.send(STORAGE_ACTIONS_TO_METHODS[action], *send_args) do |input_path| db_opts[:local_file] = input_path - yield(db_opts) + if action == :restore + yield(db_opts, backup_type) + else + yield(db_opts) + end end end uri end - private_class_method def self.prepare_for_restore(filename) - backup_type = validate_backup_file_type(filename) + private_class_method def self.prepare_for_restore(filename, backup_type = nil) + backup_type ||= validate_backup_file_type(filename) if application_connections? message = "Database restore failed. Shut down all evmserverd processes before attempting a database restore" diff --git a/spec/lib/evm_database_ops_spec.rb b/spec/lib/evm_database_ops_spec.rb index 8f93f16c8ed..8d34ff81e10 100644 --- a/spec/lib/evm_database_ops_spec.rb +++ b/spec/lib/evm_database_ops_spec.rb @@ -152,11 +152,10 @@ allow(MiqSmbSession).to receive(:runcmd) allow(MiqSmbSession).to receive(:raw_disconnect) allow(file_storage).to receive(:settings_mount_point).and_return(tmpdir) + allow(file_storage).to receive(:magic_number_for).and_return(:pgdump) allow(file_storage).to receive(:download).and_yield(input_path) allow(PostgresAdmin).to receive(:runcmd_with_logging) - allow(PostgresAdmin).to receive(:pg_dump_file?).and_return(true) - allow(PostgresAdmin).to receive(:base_backup_file?).and_return(false) allow(VmdbDatabaseConnection).to receive(:count).and_return(1) end @@ -283,9 +282,18 @@ def execute_with_file_storage(action = :backup) it "returns calculated uri" do expect(execute_with_file_storage { "block result" }).to eq("smb://tmp/foo/db_backup/baz") end + + it "yields `db_opt`s only" do + allow(file_storage).to receive(:download) { |&block| block.call(input_path) } + expect do |rspec_probe| + described_class.send(:with_file_storage, :backup, db_opts, connect_opts, &rspec_probe) + end.to yield_with_args(:dbname => "vmdb_production", :local_file => input_path) + end end context "for a restore action" do + before { expect(file_storage).to receive(:magic_number_for).and_return(:pgdump) } + it "updates db_opts[:local_file] in the method context" do expect(file_storage).to receive(:send).with(:download, nil, "smb://tmp/foo") execute_with_file_storage(:restore) @@ -303,6 +311,17 @@ def execute_with_file_storage(action = :backup) expect(execute_with_file_storage(:restore) { "block result" }).to eq("smb://tmp/foo") end + it "yields `backup_type` along with `db_opt`s" do + allow(file_storage).to receive(:download) { |&block| block.call(input_path) } + expected_yield_args = [ + { :dbname => "vmdb_production", :local_file => input_path }, + :pgdump + ] + expect do |rspec_probe| + described_class.send(:with_file_storage, :restore, db_opts, connect_opts, &rspec_probe) + end.to yield_with_args(*expected_yield_args) + end + context "with query_params in the URI" do let(:connect_opts) { { :uri => "swift://container/foo.gz?2plus2=5" } }