Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

don't fail when chown to postgres fails #888

Merged
merged 5 commits into from
Jun 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 17 additions & 5 deletions definitions/procedures/backup/prepare_directory.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,18 +6,29 @@ class PrepareDirectory < ForemanMaintain::Procedure
param :backup_dir, 'Directory where to backup to', :required => true
param :preserve_dir, 'Directory where to backup to', :flag => true
param :incremental_dir, 'Changes since specified backup only'
param :online_backup, 'Select for online backup', :flag => true, :default => false
end

# rubocop:disable Metrics/MethodLength
def run
puts "Creating backup folder #{@backup_dir}"

unless @preserve_dir
puts "Creating backup folder #{@backup_dir}"

FileUtils.mkdir_p @backup_dir
FileUtils.chmod_R 0o770, @backup_dir
end

if feature(:instance).postgresql_local? && !@preserve_dir
FileUtils.chown_R(nil, 'postgres', @backup_dir)
if feature(:instance).postgresql_local? && @online_backup
begin
FileUtils.chown_R(nil, 'postgres', @backup_dir)
rescue Errno::EPERM
warn_msg = <<~MSG
#{@backup_dir} could not be made readable by the 'postgres' user.
This won't affect the backup procedure, but you have to ensure that
the 'postgres' user can read the data during restore.
MSG
set_status(:warning, warn_msg)
end
end
end

FileUtils.rm(Dir.glob(File.join(@backup_dir, '.*.snar'))) if @preserve_dir
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be in the else part of unless @preserve_dir?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe. But honestly I didn't want to touch too much of the code, just the parts that I actually wanted to change.

Also, I am not really sure why this is guarded by if @preserve_dir anyway.
As when it's false, the @backup_dir will be fresh and empty and the .glob will just return [].

Expand All @@ -30,5 +41,6 @@ def run
end
end
end
# rubocop:enable Metrics/MethodLength
end
end
3 changes: 2 additions & 1 deletion definitions/scenarios/backup.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,8 @@ def compose
check_valid_strategy
safety_confirmation
add_step_with_context(Procedures::Backup::AccessibilityConfirmation) if strategy == :offline
add_step_with_context(Procedures::Backup::PrepareDirectory)
add_step_with_context(Procedures::Backup::PrepareDirectory,
:online_backup => strategy == :online)
add_step_with_context(Procedures::Backup::Metadata, :online_backup => strategy == :online)

case strategy
Expand Down
109 changes: 109 additions & 0 deletions test/definitions/procedures/backup/prepare_directory_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
require 'test_helper'

describe Procedures::Backup::PrepareDirectory do
include DefinitionsTestHelper

let(:backup_dir) { '/mnt/backup' }

context 'with default params' do
subject do
Procedures::Backup::PrepareDirectory.new(:backup_dir => backup_dir)
end

it 'creates backup directory for local DB' do
assume_feature_present(:instance, :postgresql_local? => true)

FileUtils.expects(:mkdir_p).with(backup_dir).once
FileUtils.expects(:chmod_R).with(0o770, backup_dir).once
FileUtils.expects(:chown_R).with(nil, 'postgres', backup_dir).never

result = run_procedure(subject)
assert result.success?, 'the procedure was expected to succeed'
end

it 'creates backup directory for remote DB' do
assume_feature_present(:instance, :postgresql_local? => false)

FileUtils.expects(:mkdir_p).with(backup_dir).once
FileUtils.expects(:chmod_R).with(0o770, backup_dir).once
FileUtils.expects(:chown_R).with(nil, 'postgres', backup_dir).never

result = run_procedure(subject)
assert result.success?, 'the procedure was expected to succeed'
end
end

context 'with online_backup=>true' do
subject do
Procedures::Backup::PrepareDirectory.new(:backup_dir => backup_dir, :online_backup => true)
end

it 'creates backup directory for local DB' do
assume_feature_present(:instance, :postgresql_local? => true)

FileUtils.expects(:mkdir_p).with(backup_dir).once
FileUtils.expects(:chmod_R).with(0o770, backup_dir).once
FileUtils.expects(:chown_R).with(nil, 'postgres', backup_dir).once

result = run_procedure(subject)
assert result.success?, 'the procedure was expected to succeed'
end

it 'does not fail when chown to postgres fails' do
assume_feature_present(:instance, :postgresql_local? => true)

FileUtils.expects(:mkdir_p).with(backup_dir).once
FileUtils.expects(:chmod_R).with(0o770, backup_dir).once
FileUtils.expects(:chown_R).with(nil, 'postgres', backup_dir).once.raises(Errno::EPERM)

result = run_procedure(subject)
assert result.warning?, 'the procedure was expected to warn'

warn_msg = <<~MSG
/mnt/backup could not be made readable by the 'postgres' user.
This won't affect the backup procedure, but you have to ensure that
the 'postgres' user can read the data during restore.
MSG
assert_equal warn_msg, result.output
end

it 'creates backup directory for remote DB' do
assume_feature_present(:instance, :postgresql_local? => false)

FileUtils.expects(:mkdir_p).with(backup_dir).once
FileUtils.expects(:chmod_R).with(0o770, backup_dir).once
FileUtils.expects(:chown_R).with(nil, 'postgres', backup_dir).never

result = run_procedure(subject)
assert result.success?, 'the procedure was expected to succeed'
end
end

context 'with preserve_dir=>true' do
subject do
Procedures::Backup::PrepareDirectory.new(:backup_dir => backup_dir, :preserve_dir => true)
end

it 'does not create backup directory for local DB' do
assume_feature_present(:instance, :postgresql_local? => true)

FileUtils.expects(:mkdir_p).with(backup_dir).never
FileUtils.expects(:chmod_R).with(0o770, backup_dir).never
FileUtils.expects(:chown_R).with(nil, 'postgres', backup_dir).never

result = run_procedure(subject)
assert result.success?, 'the procedure was expected to succeed'
end

it 'does not create backup directory for remote DB' do
assume_feature_present(:instance, :postgresql_local? => false)

FileUtils.expects(:mkdir_p).with(backup_dir).never
FileUtils.expects(:chmod_R).with(0o770, backup_dir).never
FileUtils.expects(:chown_R).with(nil, 'postgres', backup_dir).never

result = run_procedure(subject)
assert result.success?, 'the procedure was expected to succeed'
end
end
end
Loading