Skip to content

Commit

Permalink
[EvmDatabaseOps] Fix .validate_free_space target
Browse files Browse the repository at this point in the history
Currently, `db_opts[:local_file]` will always be a FIFO with the new
file_storage mechanisms, so `.validate_free_space` will only check
against the tmp dir, which usually isn't very large and not the target
destination for the DB backup/dump.

This fix moves the check to a place were we can get access to the
file_storage object directly and target the directory of the mounted
filesystem that will receive the backup/dump.

To that end, the check now only applies to file storage classes that are
"mountable", since checking against non-mountable storages don't make
sense (s3/swift don't have this capability, and they are assumed to be
"infinite" anyway, and I don't think you can query FTP for available
storage as well so I think the same assumption is also fair to apply)

* * *

Quick note:

In a couple of spots in the tests, the following is added:

    allow(file_storage).to receive(:class).and_return(MiqSmbSession)

Despite `file_storage` being defined above with:

    let(:file_storage) { double("MiqSmbSession", :disconnect => nil) }

This is now required after moving validate_free_space to inside the
with_file_storage method, and wrapping it in a class check of:

    if file_storage.class <= MiqGenericMountSession

This "stubs the stub" to have it act like a MiqSmbSession, when in fact,
it isn't.  Unfortunately, this can't be used globally since it causes a
bunch of other tests failures.
  • Loading branch information
NickLaMuro committed May 17, 2019
1 parent 88f99d7 commit cb13d74
Show file tree
Hide file tree
Showing 2 changed files with 15 additions and 6 deletions.
16 changes: 10 additions & 6 deletions lib/evm_database_ops.rb
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,6 @@ def self.backup(db_opts, connect_opts = {})
# :remote_file_name => "backup_1", - Provide a base file name for the uploaded file

uri = with_file_storage(:backup, db_opts, connect_opts) do |database_opts|
validate_free_space(database_opts)
backup_result = PostgresAdmin.backup(database_opts)
backup_result
end
Expand All @@ -67,11 +66,6 @@ def self.dump(db_opts, connect_opts = {})
# db_opts and connect_opts similar to .backup

uri = with_file_storage(:dump, db_opts, connect_opts) do |database_opts|
# For database dumps, this isn't going to be as accurate (since the dump
# size will probably be larger than the calculated BD size), but it still
# won't hurt to do as a generic way to get a rough idea if we have enough
# disk space or the appliance for the task.
validate_free_space(database_opts)
PostgresAdmin.backup_pg_dump(database_opts)
end
_log.info("[#{merged_db_opts(db_opts)[:dbname]}] database has been dumped up to file: [#{uri}]")
Expand Down Expand Up @@ -168,6 +162,16 @@ def self.restore(db_opts, connect_opts = {})
if action == :restore
yield(db_opts, backup_type)
else
if file_storage.class <= MiqGenericMountSession
# Only check free space on "mountable" storages
#
# For database dumps, this isn't going to be as accurate (since the
# dump size will probably be larger than the calculated DB size), but
# it still won't hurt to do as a generic way to get a rough idea if
# we have enough disk space on the appliance for the task.
free_space_opts = db_opts.merge(:local_file => file_storage.uri_to_local_path(uri))
validate_free_space(free_space_opts)
end
yield(db_opts)
end
end
Expand Down
5 changes: 5 additions & 0 deletions spec/lib/evm_database_ops_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@

it "without enough free space" do
EvmSpecHelper.create_guid_miq_server_zone
allow(file_storage).to receive(:class).and_return(MiqSmbSession)
allow(EvmDatabaseOps).to receive(:backup_destination_free_space).and_return(100.megabytes)
allow(EvmDatabaseOps).to receive(:database_size).and_return(200.megabytes)
expect { EvmDatabaseOps.backup(@db_opts, @connect_opts) }.to raise_error(MiqException::MiqDatabaseBackupInsufficientSpace)
Expand Down Expand Up @@ -74,6 +75,8 @@
allow(described_class).to receive(:backup_file_name).and_return("miq_backup")
expect(PostgresAdmin).to receive(:backup).with(run_db_ops)

allow(file_storage).to receive(:class).and_return(MiqSmbSession)

log_stub = instance_double("_log")
expect(described_class).to receive(:_log).twice.and_return(log_stub)
expect(log_stub).to receive(:info).with(any_args)
Expand All @@ -89,6 +92,7 @@
@connect_opts = {:username => 'blah', :password => 'blahblah', :uri => "smb://myserver.com/share"}
@db_opts = {:dbname => 'vmdb_production', :username => 'root'}
allow(file_storage).to receive(:settings_mount_point).and_return(tmpdir)
allow(file_storage).to receive(:uri_to_local_path).and_return(tmpdir.join("share").to_s)
allow(file_storage).to receive(:add).and_yield(input_path)

allow(MiqUtil).to receive(:runcmd)
Expand Down Expand Up @@ -121,6 +125,7 @@

it "without enough free space" do
EvmSpecHelper.create_guid_miq_server_zone
allow(file_storage).to receive(:class).and_return(MiqSmbSession)
allow(EvmDatabaseOps).to receive(:backup_destination_free_space).and_return(100.megabytes)
allow(EvmDatabaseOps).to receive(:database_size).and_return(200.megabytes)
expect(PostgresAdmin).to receive(:backup_pg_dump).never
Expand Down

0 comments on commit cb13d74

Please sign in to comment.