diff --git a/launcher/sdw_updater_gui/Updater.py b/launcher/sdw_updater_gui/Updater.py index 46746d80..ca8913e1 100644 --- a/launcher/sdw_updater_gui/Updater.py +++ b/launcher/sdw_updater_gui/Updater.py @@ -58,12 +58,26 @@ def run_full_install(): Re-apply the entire Salt config via sdw-admin. Required to enforce VM state during major migrations, such as template consolidation. """ - sdlog.info("Running sdw-admin apply") - cmd = ["sdw-admin", "--apply"] - subprocess.check_call(cmd) + sdlog.info("Running 'sdw-admin --apply' to apply full system state") + apply_cmd = ["sdw-admin", "--apply"] + try: + subprocess.check_call(apply_cmd) + except subprocess.CalledProcessError as e: + sdlog.error("Failed to apply full system state. Please review system logs.") + sdlog.error(str(e)) + return UpdateStatus.UPDATES_FAILED # Clean up flag requesting migration. Shell out since root created it. - subprocess.check_call(["sudo", "rm", "-rf", MIGRATION_DIR]) + rm_flag_cmd = ["sudo", "rm", "-rf", MIGRATION_DIR] + try: + subprocess.check_call(rm_flag_cmd) + except subprocess.CalledProcessError as e: + sdlog.error("Failed to remove migration flag.") + sdlog.error(str(e)) + return UpdateStatus.UPDATES_FAILED + + sdlog.info("Full system state successfully applied and migration flag cleared.") + return UpdateStatus.UPDATES_OK def migration_is_required(): @@ -349,7 +363,7 @@ def apply_dom0_state(): sdlog.info("Dom0 state applied") return UpdateStatus.UPDATES_OK except subprocess.CalledProcessError as e: - sdlog.error("Failed to dom0 state") + sdlog.error("Failed to apply dom0 state") sdlog.error(str(e)) return UpdateStatus.UPDATES_FAILED diff --git a/launcher/sdw_updater_gui/UpdaterApp.py b/launcher/sdw_updater_gui/UpdaterApp.py index 1e2a4efc..172b02e1 100644 --- a/launcher/sdw_updater_gui/UpdaterApp.py +++ b/launcher/sdw_updater_gui/UpdaterApp.py @@ -185,9 +185,8 @@ def run(self): # apply dom0 state self.progress_signal.emit(10) - result = Updater.apply_dom0_state() # add to results dict, if it fails it will show error message - results["apply_dom0"] = result.value + results["apply_dom0"] = Updater.apply_dom0_state() self.progress_signal.emit(15) # rerun full config if dom0 checks determined it's required, @@ -195,7 +194,8 @@ def run(self): if Updater.migration_is_required(): # Progress bar will freeze for ~15m during full state run self.progress_signal.emit(35) - Updater.run_full_install() + # add to results dict, if it fails it will show error message + results["apply_all"] = Updater.run_full_install() self.progress_signal.emit(75) else: upgrade_generator = Updater.apply_updates(progress_start=15, progress_end=75) diff --git a/launcher/tests/test_updater.py b/launcher/tests/test_updater.py index 9c85b365..68df9e59 100644 --- a/launcher/tests/test_updater.py +++ b/launcher/tests/test_updater.py @@ -801,7 +801,7 @@ def test_apply_dom0_state_success(mocked_info, mocked_error, mocked_subprocess): def test_apply_dom0_state_failure(mocked_info, mocked_error, mocked_subprocess): updater.apply_dom0_state() log_error_calls = [ - call("Failed to dom0 state"), + call("Failed to apply dom0 state"), call("Command 'check_call' returned non-zero exit status 1."), ] mocked_subprocess.assert_called_once_with( @@ -814,23 +814,78 @@ def test_apply_dom0_state_failure(mocked_info, mocked_error, mocked_subprocess): @mock.patch("os.path.exists", return_value=True) @mock.patch("os.listdir", return_value=["apple", "banana"]) @mock.patch("Updater.sdlog.info") -def test_migration_is_required(m_info, m_listdir, m_existence): +def test_migration_is_required(mocked_info, mocked_listdir, mocked_exists): assert updater.migration_is_required() is True - assert m_info.called_once_with("Migration is required, will enforce full config during update") + assert mocked_info.called_once_with( + "Migration is required, will enforce full config during update" + ) @mock.patch("os.path.exists", return_value=False) @mock.patch("os.listdir", return_value=[]) @mock.patch("Updater.sdlog.info") -def test_migration_not_required(mock_info, mock_listdir, mock_existence): +def test_migration_not_required(mocked_info, mocked_listdir, mocked_exists): assert updater.migration_is_required() is False - assert not mock_info.called + assert not mocked_info.called @mock.patch("Updater.sdlog.info") @mock.patch("subprocess.check_call") -def test_run_full_install(mock_call, mock_info): - MIGRATION_DIR = "potato" - updater.run_full_install() - calls = [["sdw-admin", "--apply"], ["sudo", "rm", "-rf", MIGRATION_DIR]] - assert mock_call.has_calls(calls, any_order=False) +def test_run_full_install(mocked_call, mocked_info): + """ + When a full migration is requested + And the migration succeeds + Then the migration flag is cleared + And the success enum is returned + """ + # subprocess.check_call is mocked, so this directory should never be accessed + # by the test. + MIGRATION_DIR = "/tmp/potato" # nosec + with mock.patch("Updater.MIGRATION_DIR", MIGRATION_DIR): + result = updater.run_full_install() + calls = [call(["sdw-admin", "--apply"]), call(["sudo", "rm", "-rf", MIGRATION_DIR])] + assert mocked_call.call_count == 2 + assert result == UpdateStatus.UPDATES_OK + mocked_call.assert_has_calls(calls, any_order=False) + + +@mock.patch("Updater.sdlog.error") +@mock.patch("subprocess.check_call", side_effect=subprocess.CalledProcessError(1, "check_call")) +def test_run_full_install_with_error(mocked_call, mocked_error): + """ + When a full migration is requested + And the migration fails in any way + Then the migration flag is not cleared + And the error is logged + And the failure enum is returned + """ + MIGRATION_DIR = "/tmp/potato" # nosec + with mock.patch("Updater.MIGRATION_DIR", MIGRATION_DIR): + result = updater.run_full_install() + calls = [call(["sdw-admin", "--apply"])] + assert mocked_call.call_count == 1 + assert mocked_error.called + assert result == UpdateStatus.UPDATES_FAILED + mocked_call.assert_has_calls(calls, any_order=False) + + +@mock.patch("Updater.sdlog.error") +@mock.patch( + "subprocess.check_call", side_effect=[None, subprocess.CalledProcessError(1, "check_call")] +) +def test_run_full_install_with_flag_error(mocked_call, mocked_error): + """ + When a full migration is requested + And the migration succeeds + And there is a problem clearing the migration flag + Then the error is logged + And the failure enum is returned + """ + MIGRATION_DIR = "/tmp/potato" # nosec + with mock.patch("Updater.MIGRATION_DIR", MIGRATION_DIR): + result = updater.run_full_install() + calls = [call(["sdw-admin", "--apply"]), call(["sudo", "rm", "-rf", MIGRATION_DIR])] + assert mocked_call.call_count == 2 + assert mocked_error.called + assert result == UpdateStatus.UPDATES_FAILED + mocked_call.assert_has_calls(calls, any_order=False)