diff --git a/cg/constants/__init__.py b/cg/constants/__init__.py index f6baa67e39..8b3bcf0f2c 100644 --- a/cg/constants/__init__.py +++ b/cg/constants/__init__.py @@ -29,7 +29,7 @@ ) from cg.constants.paths import TMP_DIR from cg.constants.priority import Priority -from cg.constants.process import EXIT_FAIL, EXIT_SUCCESS, RETURN_SUCCESS +from cg.constants.process import EXIT_FAIL, EXIT_SUCCESS from cg.constants.report import * from cg.constants.sample_sources import ANALYSIS_SOURCES, METAGENOME_SOURCES from cg.constants.sequencing import FLOWCELL_Q30_THRESHOLD diff --git a/cg/constants/pdc.py b/cg/constants/pdc.py index b9f60a56e5..b5fe921c15 100644 --- a/cg/constants/pdc.py +++ b/cg/constants/pdc.py @@ -7,10 +7,3 @@ class DSMCParameters(ListEnum): ARCHIVE_COMMAND: list = ["archive"] QUERY_COMMAND: list = ["q", "archive"] RETRIEVE_COMMAND: list = ["retrieve", "-replace=yes"] - - -class PDCExitCodes(IntEnum): - """Exit codes for PDC commands""" - - SUCCESS: int = 0 - NO_FILES_FOUND: int = 8 diff --git a/cg/constants/process.py b/cg/constants/process.py index ae0fd07d4a..58c17ddecd 100644 --- a/cg/constants/process.py +++ b/cg/constants/process.py @@ -1,6 +1,5 @@ """Constants for Processes""" -RETURN_SUCCESS = 0 -RETURN_WARNING = 8 EXIT_SUCCESS = 0 +EXIT_WARNING = 8 EXIT_FAIL = 1 diff --git a/cg/meta/backup/backup.py b/cg/meta/backup/backup.py index b8217012a1..28bc16b093 100644 --- a/cg/meta/backup/backup.py +++ b/cg/meta/backup/backup.py @@ -12,10 +12,8 @@ from cg.constants.constants import FileExtensions, FlowCellStatus from cg.constants.demultiplexing import DemultiplexingDirsAndFiles from cg.constants.indexes import ListIndexes -from cg.constants.pdc import PDCExitCodes -from cg.constants.process import RETURN_WARNING from cg.constants.symbols import NEW_LINE -from cg.exc import ChecksumFailedError, PdcNoFilesMatchingSearchError +from cg.exc import ChecksumFailedError, PdcError, PdcNoFilesMatchingSearchError from cg.meta.backup.pdc import PdcAPI from cg.meta.encryption.encryption import EncryptionAPI, SpringEncryptionAPI from cg.meta.tar.tar import TarAPI @@ -86,12 +84,7 @@ def fetch_flow_cell(self, flow_cell: Optional[Flowcell] = None) -> Optional[floa self.status.session.commit() LOG.info(f"{flow_cell.name}: retrieving from PDC") - try: - dsmc_output: list[str] = self.query_pdc_for_flow_cell(flow_cell.name) - - except PdcNoFilesMatchingSearchError as error: - LOG.error(f"PDC query failed: {error}") - raise error + dsmc_output: list[str] = self.query_pdc_for_flow_cell(flow_cell.name) archived_key: Path = self.get_archived_encryption_key_path(dsmc_output=dsmc_output) archived_flow_cell: Path = self.get_archived_flow_cell_path(dsmc_output=dsmc_output) @@ -209,18 +202,12 @@ def retrieve_archived_key(self, archived_key: Path, flow_cell: Flowcell, run_dir archived_file=archived_key, run_dir=run_dir, ) - except subprocess.CalledProcessError as error: - if error.returncode == RETURN_WARNING: - LOG.warning( - f"WARNING for retrieval of encryption key of flow cell {flow_cell.name}, please check " - "dsmerror.log" - ) - else: - LOG.error(f"{flow_cell.name}: key retrieval failed") - if not self.dry_run: - flow_cell.status = FlowCellStatus.REQUESTED - self.status.session.commit() - raise error + except PdcError as error: + LOG.error(f"{flow_cell.name}: key retrieval failed") + if not self.dry_run: + flow_cell.status = FlowCellStatus.REQUESTED + self.status.session.commit() + raise error def retrieve_archived_flow_cell( self, archived_flow_cell: Path, flow_cell: Flowcell, run_dir: Path @@ -231,23 +218,14 @@ def retrieve_archived_flow_cell( archived_file=archived_flow_cell, run_dir=run_dir, ) - except subprocess.CalledProcessError as error: - if error.returncode == RETURN_WARNING: - LOG.warning( - f"WARNING for retrieval of flow cell {flow_cell.name}, please check dsmerror.log" - ) - else: - LOG.error(f"{flow_cell.name}: run directory retrieval failed") - if not self.dry_run: - flow_cell.status = FlowCellStatus.REQUESTED - self.status.session.commit() - raise error - if not self.dry_run: - try: + if not self.dry_run: self._set_flow_cell_status_to_retrieved(flow_cell) - except OperationalError as error: - LOG.error(f"Could not set status for flow cell {flow_cell.name}: {error}") - raise error + except PdcError as error: + LOG.error(f"{flow_cell.name}: run directory retrieval failed") + if not self.dry_run: + flow_cell.status = FlowCellStatus.REQUESTED + self.status.session.commit() + raise error def _set_flow_cell_status_to_retrieved(self, flow_cell: Flowcell): flow_cell.status = FlowCellStatus.RETRIEVED @@ -257,21 +235,19 @@ def _set_flow_cell_status_to_retrieved(self, flow_cell: Flowcell): def query_pdc_for_flow_cell(self, flow_cell_id: str) -> list[str]: """Query PDC for a given flow cell id. Raise: - CalledProcessError if an error OTHER THAN no files found is raised. + PdcNoFilesMatchingSearchError if no files are found. """ - dsmc_output: list[str] = [] for _, encryption_directory in self.encryption_directories: search_pattern = f"{encryption_directory}*{flow_cell_id}*{FileExtensions.GPG}" - try: - self.pdc.query_pdc(search_pattern) - dsmc_output: list[str] = self.pdc.process.stdout.split(NEW_LINE) - except subprocess.CalledProcessError as error: - if error.returncode != PDCExitCodes.NO_FILES_FOUND: - raise error - LOG.debug(f"No archived files found for PDC query: {search_pattern}") - continue - LOG.info(f"Found archived files for PDC query: {search_pattern}") - return dsmc_output + self.pdc.query_pdc(search_pattern) + if self.pdc.was_file_found(self.pdc.process.stderr): + LOG.info(f"Found archived files for PDC query: {search_pattern}") + return self.pdc.process.stdout.split(NEW_LINE) + LOG.debug(f"No archived files found for PDC query: {search_pattern}") + + raise PdcNoFilesMatchingSearchError( + message=f"No flow cell files found at PDC for {flow_cell_id}" + ) def retrieve_archived_file(self, archived_file: Path, run_dir: Path) -> None: """Retrieve the archived file from PDC to a flow cell runs directory.""" diff --git a/cg/meta/backup/pdc.py b/cg/meta/backup/pdc.py index 731b020083..7f63745498 100644 --- a/cg/meta/backup/pdc.py +++ b/cg/meta/backup/pdc.py @@ -2,10 +2,12 @@ import logging from pathlib import Path +from subprocess import CalledProcessError import psutil from cg.constants.pdc import DSMCParameters +from cg.constants.process import EXIT_WARNING from cg.exc import ( DsmcAlreadyRunningError, FlowCellAlreadyBackedUpError, @@ -20,6 +22,7 @@ LOG = logging.getLogger(__name__) SERVER = "hasta" +NO_FILE_FOUND_ANSWER = "ANS1092W" class PdcAPI: @@ -55,9 +58,7 @@ def query_pdc(self, search_pattern: str) -> None: """Query PDC based on a given search pattern.""" command: list = DSMCParameters.QUERY_COMMAND.copy() command.append(search_pattern) - LOG.debug("Starting DSMC command:") - LOG.debug(f"{self.process.binary} {' '.join(command)}") - self.process.run_command(parameters=command) + self.run_dsmc_command(command=command) def retrieve_file_from_pdc(self, file_path: str, target_path: str = None) -> None: """Retrieve a file from PDC""" @@ -76,8 +77,11 @@ def run_dsmc_command(self, command: list) -> None: LOG.debug(f"{self.process.binary} {' '.join(command)}") try: self.process.run_command(parameters=command, dry_run=self.dry_run) - except Exception as error: - raise PdcError(f"{error}") from error + except CalledProcessError as error: + if error.returncode == EXIT_WARNING: + LOG.warning(f"{error}") + return + raise PdcError(message=f"{error}") from error def validate_is_flow_cell_backup_possible( self, db_flow_cell: Flowcell, flow_cell_encryption_api: FlowCellEncryptionAPI @@ -104,14 +108,9 @@ def backup_flow_cell( self, files_to_archive: list[Path], store: Store, db_flow_cell: Flowcell ) -> None: """Back-up flow cell files.""" - archived_file_count: int = 0 for encrypted_file in files_to_archive: - try: + if not self.dry_run: self.archive_file_to_pdc(file_path=encrypted_file.as_posix()) - archived_file_count += 1 - except PdcError: - LOG.warning(f"{encrypted_file.as_posix()} cannot be archived") - if archived_file_count == len(files_to_archive) and not self.dry_run: store.update_flow_cell_has_backup(flow_cell=db_flow_cell, has_backup=True) LOG.info(f"Flow cell: {db_flow_cell.name} has been backed up") @@ -133,3 +132,8 @@ def start_flow_cell_backup( store=status_db, db_flow_cell=db_flow_cell, ) + + @staticmethod + def was_file_found(dsmc_output: str) -> bool: + """Check if file was found in PDC.""" + return NO_FILE_FOUND_ANSWER not in dsmc_output diff --git a/cg/utils/commands.py b/cg/utils/commands.py index 2bf470c88c..25115d4f34 100644 --- a/cg/utils/commands.py +++ b/cg/utils/commands.py @@ -7,7 +7,7 @@ import subprocess from subprocess import CalledProcessError -from cg.constants.process import RETURN_SUCCESS +from cg.constants.process import EXIT_SUCCESS LOG = logging.getLogger(__name__) @@ -83,7 +83,7 @@ def run_command(self, parameters: list = None, dry_run: bool = False) -> int: LOG.info("Running command %s", " ".join(command)) if dry_run: LOG.info("Dry run: process call will not be executed!!") - return RETURN_SUCCESS + return EXIT_SUCCESS if self.environment: res = subprocess.run( @@ -100,7 +100,7 @@ def run_command(self, parameters: list = None, dry_run: bool = False) -> int: self.stdout = res.stdout.decode("utf-8").rstrip() self.stderr = res.stderr.decode("utf-8").rstrip() - if res.returncode != RETURN_SUCCESS: + if res.returncode != EXIT_SUCCESS: LOG.critical("Call %s exit with a non zero exit code", command) LOG.critical(self.stderr) raise CalledProcessError(res.returncode, command) diff --git a/tests/cli/get/test_cli_get_analysis.py b/tests/cli/get/test_cli_get_analysis.py index e57c072521..555d783c37 100644 --- a/tests/cli/get/test_cli_get_analysis.py +++ b/tests/cli/get/test_cli_get_analysis.py @@ -3,7 +3,7 @@ from click.testing import CliRunner from cg.cli.get import get -from cg.constants import RETURN_SUCCESS +from cg.constants import EXIT_SUCCESS from cg.models.cg_config import CGConfig from cg.store import Store from cg.store.models import Analysis @@ -19,7 +19,7 @@ def test_get_analysis_bad_case(cli_runner: CliRunner, base_context: CGConfig): result = cli_runner.invoke(get, ["analysis", name], obj=base_context) # THEN it should error about missing case instead of getting a analysis - assert result.exit_code != RETURN_SUCCESS + assert result.exit_code != EXIT_SUCCESS def test_get_analysis_required( @@ -35,7 +35,7 @@ def test_get_analysis_required( result = cli_runner.invoke(get, ["analysis", internal_id], obj=base_context) # THEN it should have been gotten - assert result.exit_code == RETURN_SUCCESS + assert result.exit_code == EXIT_SUCCESS assert str(analysis.started_at) in result.output assert analysis.pipeline in result.output assert analysis.pipeline_version in result.output diff --git a/tests/meta/backup/conftest.py b/tests/meta/backup/conftest.py index d3374f2064..ae1799b300 100644 --- a/tests/meta/backup/conftest.py +++ b/tests/meta/backup/conftest.py @@ -1,12 +1,11 @@ import fnmatch -import subprocess from pathlib import Path +from subprocess import CompletedProcess from typing import Callable import pytest from cg.constants import FileExtensions -from cg.constants.pdc import PDCExitCodes from cg.models.cg_config import EncryptionDirectories @@ -14,13 +13,9 @@ def mock_pdc_query_method(archived_flow_cells: list[str]) -> Callable: """Returns a mock method mimicking the pattern search made by the dsmc q archive command.""" - def mock_method(search_pattern: str) -> list[str]: - match = fnmatch.filter(archived_flow_cells, search_pattern) - if not match: - raise subprocess.CalledProcessError( - cmd="dummy_method", returncode=PDCExitCodes.NO_FILES_FOUND - ) - return match[0] + def mock_method(search_pattern: str) -> list[str] | None: + if match := fnmatch.filter(archived_flow_cells, search_pattern): + return match return mock_method @@ -78,3 +73,14 @@ def archived_flow_cell() -> Path: def archived_key() -> Path: """Path of archived key""" return Path("/path/to/archived/encryption_key.key.gpg") + + +def create_process_response( + return_code: int, args: str = "", std_out: str = "", std_err: str = "" +) -> CompletedProcess: + return CompletedProcess( + args=args, + returncode=return_code, + stderr=std_err.encode("utf-8"), + stdout=std_out.encode("utf-8"), + ) diff --git a/tests/meta/backup/test_meta_backup.py b/tests/meta/backup/test_meta_backup.py index 049e9eb29b..98965525c2 100644 --- a/tests/meta/backup/test_meta_backup.py +++ b/tests/meta/backup/test_meta_backup.py @@ -52,16 +52,6 @@ def test_query_pdc_for_flow_cell( assert fnmatch.filter( names=caplog.messages, pat=f"Found archived files for PDC query:*{flow_cell_name}*.gpg" ) - # THEN the flow cell is logged as not found for two of the search patterns - assert ( - len( - fnmatch.filter( - names=caplog.messages, - pat=f"No archived files found for PDC query: *{flow_cell_name}*{FileExtensions.GPG}", - ) - ) - == 2 - ) def test_get_archived_encryption_key_path(dsmc_q_archive_output: list[str], flow_cell_name: str): @@ -393,59 +383,6 @@ def test_fetch_flow_cell_retrieve_specified_flow_cell( assert result > 0 -@mock.patch("cg.meta.backup.backup.BackupAPI.unlink_files") -@mock.patch("cg.meta.backup.backup.BackupAPI.create_rta_complete") -@mock.patch("cg.meta.backup.backup.BackupAPI.get_archived_flow_cell_path") -@mock.patch("cg.meta.backup.backup.BackupAPI.get_archived_encryption_key_path") -@mock.patch("cg.meta.backup.backup.BackupAPI.check_processing") -@mock.patch("cg.meta.backup.backup.BackupAPI.get_first_flow_cell") -@mock.patch("cg.meta.tar.tar.TarAPI") -@mock.patch("cg.store.models.Flowcell") -@mock.patch("cg.meta.backup.pdc.PdcAPI") -@mock.patch("cg.store") -def test_fetch_flow_cell_pdc_retrieval_failed( - mock_store, - mock_pdc, - mock_flow_cell, - mock_tar, - mock_get_first_flow_cell, - mock_check_processing, - mock_get_archived_key, - mock_get_archived_flow_cell, - archived_key, - archived_flow_cell, - cg_context, - caplog, -): - """Tests the fetch_flow_cell method of the backup API when PDC retrieval failed""" - - caplog.set_level(logging.INFO) - - # GIVEN we are going to retrieve a flow cell from PDC - backup_api = BackupAPI( - encryption_api=mock.Mock(), - encryption_directories=cg_context.backup.encryption_directories, - status=mock_store, - tar_api=mock_tar, - pdc_api=mock_pdc, - flow_cells_dir="cg_context.flow_cells_dir", - ) - mock_flow_cell.status = FlowCellStatus.REQUESTED - mock_flow_cell.sequencer_type = Sequencers.NOVASEQ - backup_api.check_processing.return_value = True - backup_api.get_archived_encryption_key_path.return_value = archived_key - backup_api.get_archived_flow_cell_path.return_value = archived_flow_cell - backup_api.tar_api.run_tar_command.return_value = None - - # WHEN the retrieval process fails - mock_pdc.retrieve_file_from_pdc.side_effect = subprocess.CalledProcessError(1, "echo") - with pytest.raises(subprocess.CalledProcessError): - backup_api.fetch_flow_cell(flow_cell=mock_flow_cell) - - # THEN the failure to retrieve is logged - assert "retrieval failed" in caplog.text - - @mock.patch("cg.meta.backup.backup.BackupAPI.unlink_files") @mock.patch("cg.meta.backup.backup.BackupAPI.create_rta_complete") @mock.patch("cg.meta.backup.backup.BackupAPI.query_pdc_for_flow_cell") diff --git a/tests/meta/backup/test_meta_pdc.py b/tests/meta/backup/test_meta_pdc.py index 06cd8176cd..0ea260c7a0 100644 --- a/tests/meta/backup/test_meta_pdc.py +++ b/tests/meta/backup/test_meta_pdc.py @@ -4,16 +4,20 @@ import pytest +from cg.constants import EXIT_FAIL +from cg.constants.process import EXIT_WARNING from cg.exc import ( DsmcAlreadyRunningError, FlowCellAlreadyBackedUpError, FlowCellEncryptionError, + PdcError, ) from cg.meta.backup.pdc import PdcAPI from cg.meta.encryption.encryption import FlowCellEncryptionAPI from cg.models.cg_config import CGConfig from cg.store import Store from cg.store.models import Flowcell +from tests.meta.backup.conftest import create_process_response from tests.store_helpers import StoreHelpers @@ -202,21 +206,23 @@ def test_backup_flow_cell_when_unable_to_archive( store=base_store, ) - # WHEN backing up flow cell - pdc_api.backup_flow_cell( - files_to_archive=[ - flow_cell_encryption_api.final_passphrase_file_path, - flow_cell_encryption_api.encrypted_gpg_file_path, - ], - store=base_store, - db_flow_cell=db_flow_cell, - ) - - # THEN log unable to archive - assert ( - f"{flow_cell_encryption_api.encrypted_gpg_file_path.as_posix()} cannot be archived" - in caplog.text - ) + # GIVEN that archiving fails + with mock.patch( + "cg.utils.commands.subprocess.run", + return_value=create_process_response(return_code=EXIT_FAIL), + ): + # WHEN backing up flow cell + + # THEN the appropriate error should have been raised + with pytest.raises(PdcError): + pdc_api.backup_flow_cell( + files_to_archive=[ + flow_cell_encryption_api.final_passphrase_file_path, + flow_cell_encryption_api.encrypted_gpg_file_path, + ], + store=base_store, + db_flow_cell=db_flow_cell, + ) @mock.patch("cg.meta.backup.pdc.Process") @@ -246,7 +252,9 @@ def test_query_pdc(mock_process, cg_context: CGConfig, binary_path, backup_file_ pdc_api.query_pdc(backup_file_path) # THEN a dsmc process should be started with parameters 'q archive' - mock_process.run_command.assert_called_once_with(parameters=["q", "archive", backup_file_path]) + mock_process.run_command.assert_called_once_with( + parameters=["q", "archive", backup_file_path], dry_run=False + ) @mock.patch("cg.meta.backup.pdc.Process") @@ -263,3 +271,36 @@ def test_retrieve_file_from_pdc(mock_process, cg_context: CGConfig, binary_path, mock_process.run_command.assert_called_once_with( parameters=["retrieve", "-replace=yes", backup_file_path], dry_run=False ) + + +def test_run_dsmc_command_fail(cg_context: CGConfig): + """Test that non-zero, non-warning exit-codes raise an error.""" + # GIVEN an instance of the PDC API + pdc_api = cg_context.pdc_api + + # GIVEN an exit code signifying failure + with pytest.raises(PdcError), mock.patch( + "cg.utils.commands.subprocess.run", + return_value=create_process_response(return_code=EXIT_FAIL), + ): + # WHEN running a dsmc command + pdc_api.run_dsmc_command(["archive", "something"]) + + # THEN the appropriate error should have been raised + + +def test_run_dsmc_command_warning(cg_context: CGConfig, caplog): + """Test that warning exit-codes do not raise an error.""" + # GIVEN an instance of the PDC API + pdc_api = cg_context.pdc_api + + # GIVEN an exit code signifying a warning + with mock.patch( + "cg.utils.commands.subprocess.run", + return_value=create_process_response(return_code=EXIT_WARNING), + ): + # WHEN running a dsmc command + pdc_api.run_dsmc_command(["archive", "something"]) + + # THEN the warning should have been logged + assert "WARNING" in caplog.text diff --git a/tests/mocks/process_mock.py b/tests/mocks/process_mock.py index 6f948b15ab..6f1a27aa57 100644 --- a/tests/mocks/process_mock.py +++ b/tests/mocks/process_mock.py @@ -4,7 +4,7 @@ import logging from subprocess import CalledProcessError -from cg.constants import RETURN_SUCCESS +from cg.constants import EXIT_SUCCESS from cg.utils.commands import Process LOG = logging.getLogger(__name__) @@ -41,7 +41,7 @@ def __init__( if self.config: self.base_call.extend([self.config_parameter, self.config]) LOG.debug("Use base call %s", self.base_call) - self._exit_code: int = RETURN_SUCCESS + self._exit_code: int = EXIT_SUCCESS if error: self._exit_code = 1 @@ -62,14 +62,14 @@ def run_command(self, parameters: list = None, dry_run: bool = False) -> int: LOG.info("Running command %s", " ".join(command)) if dry_run: - return RETURN_SUCCESS + return EXIT_SUCCESS - if self._exit_code != RETURN_SUCCESS: + if self._exit_code != EXIT_SUCCESS: LOG.critical("Call %s exit with a non zero exit code", command) LOG.critical(self.stderr) raise CalledProcessError(self._exit_code, command) - return RETURN_SUCCESS + return EXIT_SUCCESS def set_stdout(self, text: str): """Mock the stdout"""