diff --git a/packit_service/worker/handlers/copr.py b/packit_service/worker/handlers/copr.py index a4d649c67..ce3956a3a 100644 --- a/packit_service/worker/handlers/copr.py +++ b/packit_service/worker/handlers/copr.py @@ -7,6 +7,7 @@ from typing import Optional from celery import Task, signature +from ogr.exceptions import GithubAPIException, GitlabAPIException, PagureAPIException from ogr.services.github import GithubProject from ogr.services.gitlab import GitlabProject from packit.config import ( @@ -160,6 +161,11 @@ def get_checkers() -> tuple[type[Checker], ...]: BuildNotAlreadyStarted, ) + def set_status_reporter_reraise_transient_errors(self, reraise: bool) -> None: + """Set whether to re-raise transient GitHub errors or fall back to comments.""" + # CoprBuildStartHandler doesn't use status reporting with transient error handling, + # but needs this method for babysit compatibility + def set_start_time(self): start_time = ( datetime.utcfromtimestamp(self.copr_event.timestamp) @@ -240,6 +246,23 @@ class CoprBuildEndHandler(AbstractCoprBuildReportHandler): topic = "org.fedoraproject.prod.copr.build.end" task_name = TaskName.copr_build_end + def __init__( + self, + package_config: PackageConfig, + job_config: JobConfig, + event: dict, + ): + super().__init__( + package_config=package_config, + job_config=job_config, + event=event, + ) + self._status_reporter_reraise_transient_errors = True + + def set_status_reporter_reraise_transient_errors(self, reraise: bool) -> None: + """Set whether to re-raise transient GitHub errors or fall back to comments.""" + self._status_reporter_reraise_transient_errors = reraise + def set_srpm_url(self) -> None: # TODO how to do better srpm_build = ( @@ -306,6 +329,9 @@ def _run(self) -> TaskResults: f"chroot={self.copr_event.chroot} " f"at {run_start_time.isoformat()}" ) + self.copr_build_helper.status_reporter.reraise_transient_errors = ( + self._status_reporter_reraise_transient_errors + ) if not self.build: # TODO: how could this happen? model = "SRPMBuildDB" if self.copr_event.chroot == COPR_SRPM_CHROOT else "CoprBuildDB" @@ -330,28 +356,32 @@ def _run(self) -> TaskResults: if self.copr_event.chroot == COPR_SRPM_CHROOT: return self.handle_srpm_end() - self.pushgateway.copr_builds_finished.inc() - - # if the build is needed only for test, it doesn't have the task_accepted_time - if self.build.task_accepted_time: - copr_build_time = elapsed_seconds( - begin=self.build.task_accepted_time, - end=datetime.now(timezone.utc), - ) - self.pushgateway.copr_build_finished_time.observe(copr_build_time) - # https://pagure.io/copr/copr/blob/master/f/common/copr_common/enums.py#_42 if self.copr_event.status != COPR_API_SUCC_STATE: failed_msg = "RPMs failed to be built." packit_dashboard_url = get_copr_build_info_url(self.build.id) # if SRPM build failed it has been reported already so skip reporting if self.build.get_srpm_build().status != BuildStatus.failure: - self.copr_build_helper.report_status_to_all_for_chroot( - state=BaseCommitStatus.failure, - description=failed_msg, - url=packit_dashboard_url, - chroot=self.copr_event.chroot, - ) + try: + self.copr_build_helper.report_status_to_all_for_chroot( + state=BaseCommitStatus.failure, + description=failed_msg, + url=packit_dashboard_url, + chroot=self.copr_event.chroot, + ) + except (GithubAPIException, GitlabAPIException, PagureAPIException): + # Transient error - return early before setting the state + return TaskResults(success=False, details={"msg": "Status reporting failed"}) + + # Only execute the following if GitHub reporting succeeded + self.pushgateway.copr_builds_finished.inc() + if self.build.task_accepted_time: + copr_build_time = elapsed_seconds( + begin=self.build.task_accepted_time, + end=datetime.now(timezone.utc), + ) + self.pushgateway.copr_build_finished_time.observe(copr_build_time) + self.measure_time_after_reporting() self.copr_build_helper.notify_about_failure_if_configured( packit_dashboard_url=packit_dashboard_url, @@ -362,9 +392,22 @@ def _run(self) -> TaskResults: report_long_runtime("Copr build failed end", 120, run_start_time) return TaskResults(success=False, details={"msg": failed_msg}) - self.report_successful_build() - self.measure_time_after_reporting() + try: + self.report_successful_build() + except (GithubAPIException, GitlabAPIException, PagureAPIException): + # Transient error - return early before setting the state + return TaskResults(success=False, details={"msg": "Status reporting failed"}) + # Only execute the following if GitHub reporting succeeded + self.pushgateway.copr_builds_finished.inc() + if self.build.task_accepted_time: + copr_build_time = elapsed_seconds( + begin=self.build.task_accepted_time, + end=datetime.now(timezone.utc), + ) + self.pushgateway.copr_build_finished_time.observe(copr_build_time) + + self.measure_time_after_reporting() self.set_built_packages() self.build.set_status(BuildStatus.success) self.handle_testing_farm() @@ -432,11 +475,16 @@ def handle_srpm_end(self): if self.copr_event.status != COPR_API_SUCC_STATE: failed_msg = "SRPM build failed, check the logs for details." - self.copr_build_helper.report_status_to_all( - state=BaseCommitStatus.failure, - description=failed_msg, - url=url, - ) + try: + self.copr_build_helper.report_status_to_all( + state=BaseCommitStatus.failure, + description=failed_msg, + url=url, + ) + except (GithubAPIException, GitlabAPIException, PagureAPIException): + # Transient error - return early before setting the state + return TaskResults(success=False, details={"msg": "Status reporting failed"}) + self.copr_build_helper.notify_about_failure_if_configured( packit_dashboard_url=url, external_dashboard_url=self.build.copr_web_url, @@ -449,6 +497,22 @@ def handle_srpm_end(self): ) return TaskResults(success=False, details={"msg": failed_msg}) + report_status = ( + self.copr_build_helper.report_status_to_all + if self.job_config.sync_test_job_statuses_with_builds + else self.copr_build_helper.report_status_to_build + ) + try: + report_status( + state=BaseCommitStatus.running, + description="SRPM build succeeded. Waiting for RPM build to start...", + url=url, + ) + except (GithubAPIException, GitlabAPIException, PagureAPIException): + # Transient error - return early before setting the state + return TaskResults(success=False, details={"msg": "Status reporting failed"}) + + # Set DB status after successful GitHub reporting for build in CoprBuildTargetModel.get_all_by_build_id( str(self.copr_event.build_id), ): @@ -456,16 +520,6 @@ def handle_srpm_end(self): build.set_status(BuildStatus.pending) self.build.set_status(BuildStatus.success) - report_status = ( - self.copr_build_helper.report_status_to_all - if self.job_config.sync_test_job_statuses_with_builds - else self.copr_build_helper.report_status_to_build - ) - report_status( - state=BaseCommitStatus.running, - description="SRPM build succeeded. Waiting for RPM build to start...", - url=url, - ) msg = "SRPM build in Copr has finished." logger.debug(msg) return TaskResults(success=True, details={"msg": msg}) diff --git a/packit_service/worker/handlers/testing_farm.py b/packit_service/worker/handlers/testing_farm.py index 596d881b6..8867946d4 100644 --- a/packit_service/worker/handlers/testing_farm.py +++ b/packit_service/worker/handlers/testing_farm.py @@ -12,6 +12,7 @@ from celery import Task from ogr.abstract import GitProject +from ogr.exceptions import GithubAPIException, GitlabAPIException, PagureAPIException from packit.config import JobConfig, JobType, aliases from packit.config.package_config import PackageConfig @@ -533,6 +534,11 @@ def __init__( self.log_url = event.get("log_url") self.summary = event.get("summary") self.created = event.get("created") + self._status_reporter_reraise_transient_errors = True + + def set_status_reporter_reraise_transient_errors(self, reraise: bool) -> None: + """Set whether to re-raise transient GitHub errors or fall back to comments.""" + self._status_reporter_reraise_transient_errors = reraise @staticmethod def get_checkers() -> tuple[type[Checker], ...]: @@ -550,6 +556,9 @@ def db_project_event(self) -> Optional[ProjectEventModel]: def _run(self) -> TaskResults: logger.debug(f"Testing farm {self.pipeline_id} result:\n{self.result}") + self.testing_farm_job_helper.status_reporter.reraise_transient_errors = ( + self._status_reporter_reraise_transient_errors + ) test_run_model = TFTTestRunTargetModel.get_by_pipeline_id( pipeline_id=self.pipeline_id, @@ -587,6 +596,20 @@ def _run(self) -> TaskResults: status = BaseCommitStatus.error summary = self.summary or "Error ..." + url = get_testing_farm_info_url(test_run_model.id) if test_run_model else None + try: + self.testing_farm_job_helper.report_status_to_tests_for_test_target( + state=status, + description=summary, + target=test_run_model.target, + url=url if url else self.log_url, + links_to_external_services={"Testing Farm": self.log_url}, + ) + except (GithubAPIException, GitlabAPIException, PagureAPIException): + # Transient error - return early before setting the state + return TaskResults(success=False, details={"msg": "Status reporting failed"}) + + # Record metrics - only after successful GitHub reporting to avoid double-counting on retry if self.result == TestingFarmResult.running: self.pushgateway.test_runs_started.inc() else: @@ -598,14 +621,6 @@ def _run(self) -> TaskResults: self.pushgateway.test_run_finished_time.observe(test_run_time) test_run_model.set_web_url(self.log_url) - url = get_testing_farm_info_url(test_run_model.id) if test_run_model else None - self.testing_farm_job_helper.report_status_to_tests_for_test_target( - state=status, - description=summary, - target=test_run_model.target, - url=url if url else self.log_url, - links_to_external_services={"Testing Farm": self.log_url}, - ) if failure: self.testing_farm_job_helper.notify_about_failure_if_configured( packit_dashboard_url=url, diff --git a/packit_service/worker/helpers/build/babysit.py b/packit_service/worker/helpers/build/babysit.py index 28374c209..4e99dda1f 100644 --- a/packit_service/worker/helpers/build/babysit.py +++ b/packit_service/worker/helpers/build/babysit.py @@ -197,6 +197,8 @@ def update_testing_farm_run(event: testing_farm.Result, run: TFTTestRunTargetMod job_config=job_config, event=event_dict, ) + # TODO: Consider time-based heuristic instead of always False + upstream_handler.set_status_reporter_reraise_transient_errors(False) # Check if handler should process this test if upstream_handler.pre_check(package_config, job_config, event_dict): signatures.append(upstream_handler.get_signature(event=event, job=job_config)) @@ -487,6 +489,8 @@ def update_copr_build_state( job_config=job_config, event=event_dict, ) + # TODO: Consider time-based heuristic instead of always False + handler.set_status_reporter_reraise_transient_errors(False) if handler.pre_check(package_config, job_config, event_dict): signatures.append(handler.get_signature(event=event, job=job_config)) diff --git a/packit_service/worker/helpers/job_helper.py b/packit_service/worker/helpers/job_helper.py index 1448ee559..ceb5fde63 100644 --- a/packit_service/worker/helpers/job_helper.py +++ b/packit_service/worker/helpers/job_helper.py @@ -185,6 +185,7 @@ def status_reporter(self) -> StatusReporter: packit_user=self.service_config.get_github_account_name(), project_event_id=(self.db_project_event.id if self.db_project_event else None), pr_id=self.metadata.pr_id, + reraise_transient_errors=False, ) return self._status_reporter diff --git a/packit_service/worker/reporting/reporters/base.py b/packit_service/worker/reporting/reporters/base.py index 7c9905c92..e1ae7d005 100644 --- a/packit_service/worker/reporting/reporters/base.py +++ b/packit_service/worker/reporting/reporters/base.py @@ -6,6 +6,7 @@ from typing import Callable, Optional, Union from ogr.abstract import GitProject, PullRequest +from ogr.exceptions import GithubAPIException, GitlabAPIException, PagureAPIException from ogr.services.github import GithubProject from ogr.services.gitlab import GitlabProject from ogr.services.pagure import PagureProject @@ -29,6 +30,7 @@ def __init__( packit_user: str, project_event_id: Optional[int] = None, pr_id: Optional[int] = None, + reraise_transient_errors: bool = False, ): logger.debug( f"Status reporter will report for {project}, commit={commit_sha}, pr={pr_id}", @@ -41,6 +43,7 @@ def __init__( self.project_event_id: int = project_event_id self.pr_id: Optional[int] = pr_id self._pull_request_object: Optional[PullRequest] = None + self.reraise_transient_errors: bool = reraise_transient_errors @classmethod def get_instance( @@ -50,6 +53,7 @@ def get_instance( packit_user: str, project_event_id: Optional[int] = None, pr_id: Optional[int] = None, + reraise_transient_errors: bool = False, ) -> "StatusReporter": """ Get the StatusReporter instance. @@ -67,7 +71,9 @@ def get_instance( reporter = StatusReporterGitlab elif isinstance(project, PagureProject): reporter = StatusReporterPagure - return reporter(project, commit_sha, packit_user, project_event_id, pr_id) + return reporter( + project, commit_sha, packit_user, project_event_id, pr_id, reraise_transient_errors + ) @property def project_with_commit(self) -> GitProject: @@ -97,6 +103,32 @@ def get_commit_status(state: BaseCommitStatus): def get_check_run(state: BaseCommitStatus): return MAP_TO_CHECK_RUN[state] + @staticmethod + def is_transient_error( + exception: Union[GithubAPIException, GitlabAPIException, PagureAPIException], + ) -> bool: + """ + Check if an API exception represents a transient error that should be retried. + + Transient errors include: + - Network errors (no response_code attribute) + - Rate limiting (HTTP 429) + - Server errors (HTTP 5xx) + + Args: + exception: An API exception from ogr + + Returns: + True if the error is transient and should be retried, False otherwise + """ + response_code = getattr(exception, "response_code", None) + + if response_code is None: + # Network errors (no response code) are transient + return True + + return response_code == 429 or (500 <= response_code < 600) + def set_status( self, state: BaseCommitStatus, diff --git a/packit_service/worker/reporting/reporters/github.py b/packit_service/worker/reporting/reporters/github.py index 18d725f39..915c7588c 100644 --- a/packit_service/worker/reporting/reporters/github.py +++ b/packit_service/worker/reporting/reporters/github.py @@ -58,6 +58,12 @@ def set_status( trim=True, ) except GithubAPIException as e: + if self.is_transient_error(e) and self.reraise_transient_errors: + logger.debug( + f"Re-raising transient GitHub API error when setting " + f"status for '{check_name}': {e}." + ) + raise self._comment_as_set_status_fallback(e, state, description, check_name, url) @@ -137,6 +143,12 @@ def set_status( output=create_github_check_run_output(description, summary), ) except GithubAPIException as e: + if self.is_transient_error(e) and self.reraise_transient_errors: + logger.debug( + f"Re-raising transient GitHub API error when setting " + f"status for '{check_name}': {e}." + ) + raise logger.debug( f"Failed to set status check, setting status as a fallback: {e!s}", ) diff --git a/packit_service/worker/reporting/reporters/gitlab.py b/packit_service/worker/reporting/reporters/gitlab.py index 908977342..e8e3baa27 100644 --- a/packit_service/worker/reporting/reporters/gitlab.py +++ b/packit_service/worker/reporting/reporters/gitlab.py @@ -61,11 +61,15 @@ def set_status( ) except GitlabAPIException as e: logger.debug(f"Failed to set the status: {e}. Response code: {e.response_code}") - # Ignoring Gitlab error regarding reporting a status of the same state + + # Special case: Ignore "Cannot transition status" errors # https://github.com/packit-service/packit-service/issues/741 - if e.response_code != 400 or "Cannot transition status" not in str(e): - # 403: No permissions to set status, falling back to comment - # 404: Commit has not been found, e.g. used target project on GitLab - self._comment_as_set_status_fallback(e, state, description, check_name, url) - if e.response_code not in {400, 403, 404}: + if e.response_code == 400 and "Cannot transition status" in str(e): + return + + # Check if error is transient and reraise is enabled + if self.is_transient_error(e) and self.reraise_transient_errors: raise + + # Fall back to comment for all other errors + self._comment_as_set_status_fallback(e, state, description, check_name, url) diff --git a/tests/unit/test_copr_build.py b/tests/unit/test_copr_build.py index a4c805208..c5fb63a49 100644 --- a/tests/unit/test_copr_build.py +++ b/tests/unit/test_copr_build.py @@ -57,6 +57,7 @@ from packit_service.worker.celery_task import CeleryTask from packit_service.worker.checker.copr import IsGitForgeProjectAndEventOk from packit_service.worker.handlers import CoprBuildHandler +from packit_service.worker.handlers.copr import CoprBuildEndHandler from packit_service.worker.helpers.build.copr_build import ( BaseBuildJobHelper, CoprBuildJobHelper, @@ -1039,3 +1040,26 @@ def test_check_if_actor_can_run_job_and_report(jobs, should_pass): ) == should_pass ) + + +def test_copr_build_end_handler_default_reraise_flag(): + """Verify CoprBuildEndHandler defaults to reraise_transient_errors=True.""" + + handler = CoprBuildEndHandler( + package_config=flexmock(), + job_config=flexmock(), + event={"build_id": 123, "chroot": "fedora-rawhide-x86_64"}, + ) + assert handler._status_reporter_reraise_transient_errors is True + + +def test_copr_build_end_handler_set_reraise_flag(): + """Test set_status_reporter_reraise_transient_errors() method.""" + + handler = CoprBuildEndHandler( + package_config=flexmock(), + job_config=flexmock(), + event={"build_id": 123, "chroot": "fedora-rawhide-x86_64"}, + ) + handler.set_status_reporter_reraise_transient_errors(False) + assert handler._status_reporter_reraise_transient_errors is False diff --git a/tests/unit/test_reporting.py b/tests/unit/test_reporting.py index ef213a1c7..ad79afc3a 100644 --- a/tests/unit/test_reporting.py +++ b/tests/unit/test_reporting.py @@ -1,6 +1,8 @@ # Copyright Contributors to the Packit project. # SPDX-License-Identifier: MIT +import contextlib + import pytest from flexmock import flexmock from gitlab.exceptions import GitlabError @@ -806,3 +808,141 @@ def test_update_message_with_configured_failure_comment_message( ), ) assert update_message_with_configured_failure_comment_message(comment, job_config) == result + + +@pytest.mark.parametrize( + "response_code,is_transient", + [ + (None, True), # Network error, response_code not set + (429, True), # Rate limiting + (500, True), # Server error + (502, True), # Bad gateway + (400, False), # Bad request + (403, False), # Forbidden + (404, False), # Not found + ], +) +def test_is_transient_error(response_code, is_transient): + """Test classification of API errors as transient across all platforms.""" + exception = flexmock(response_code=response_code) + + assert StatusReporter.is_transient_error(exception) == is_transient + + +@pytest.mark.parametrize( + "reraise_transient_errors,response_code", + [ + (True, 500), # Transient error, reraise enabled -> should reraise + (False, 500), # Transient error, reraise disabled -> should fallback + (True, 403), # Non-transient error, reraise enabled -> should fallback + (False, 403), # Non-transient error, reraise disabled -> should fallback + ], +) +def test_github_checks_error_handling(reraise_transient_errors, response_code): + """Test error handling in StatusReporterGithubChecks.""" + project = GithubProject(None, None, None) + reporter = StatusReporter.get_instance( + project=project, + commit_sha="abc123", + pr_id=1, + project_event_id=1, + packit_user="packit", + reraise_transient_errors=reraise_transient_errors, + ) + + exception = flexmock(GithubAPIException(), response_code=response_code) + + flexmock(GithubProject).should_receive("create_check_run").and_raise( + exception, + ).once() + + is_transient = reporter.is_transient_error(exception) + + if reraise_transient_errors and is_transient: + # Should NOT fall back to commit status + flexmock(GithubProject).should_receive("set_commit_status").never() + # Should re-raise the exception + expectation = pytest.raises(GithubAPIException) + else: + # Should fall back to commit status + flexmock(GithubProject).should_receive("set_commit_status").with_args( + "abc123", + CommitStatus.success, + "https://example.com", + "Test completed", + "packit/test", + trim=True, + ).once() + # Should NOT raise + expectation = contextlib.nullcontext() + + with expectation: + reporter.set_status( + BaseCommitStatus.success, + "Test completed", + "packit/test", + "https://example.com", + ) + + +@pytest.mark.parametrize( + "reporter_class,exception_class", + [ + (StatusReporterGithubStatuses, GithubAPIException), + (StatusReporterGitlab, GitlabAPIException), + ], +) +@pytest.mark.parametrize( + "reraise_transient_errors,response_code", + [ + (True, 502), # Transient error, reraise enabled -> should reraise + (False, 500), # Transient error, reraise disabled -> should fallback + (True, 404), # Non-transient error, reraise enabled -> should fallback + (False, 404), # Non-transient error, reraise disabled -> should fallback + ], +) +def test_commit_status_error_handling( + reporter_class, exception_class, reraise_transient_errors, response_code +): + """Test error handling in commit status reporters (GitHub and GitLab).""" + project = flexmock() + reporter = flexmock( + reporter_class( + project=project, + commit_sha="abc123", + pr_id=1, + packit_user="packit", + reraise_transient_errors=reraise_transient_errors, + ) + ) + + exception = flexmock(exception_class(), response_code=response_code) + + project.should_receive("set_commit_status").and_raise(exception).once() + + is_transient = reporter.is_transient_error(exception) + + if reraise_transient_errors and is_transient: + # Should NOT fall back to comment + reporter.should_receive("_comment_as_set_status_fallback").never() + # Should re-raise the exception + expectation = pytest.raises(exception_class) + else: + # When commit_sha is present, it uses commit_comment + reporter.should_receive("_comment_as_set_status_fallback").with_args( + exception, + BaseCommitStatus.success, + "Build completed", + "packit/build", + "https://example.com", + ).once() + # Should NOT raise + expectation = contextlib.nullcontext() + + with expectation: + reporter.set_status( + BaseCommitStatus.success, + "Build completed", + "packit/build", + "https://example.com", + ) diff --git a/tests/unit/test_testing_farm.py b/tests/unit/test_testing_farm.py index 083a6fce0..12be893ab 100644 --- a/tests/unit/test_testing_farm.py +++ b/tests/unit/test_testing_farm.py @@ -2344,3 +2344,24 @@ def test_parse_comment_arguments( assert helper.comment_arguments.identifier == expected_identifier assert helper.comment_arguments.labels == expected_labels assert helper.comment_arguments.envs == expected_envs + + +def test_testing_farm_handler_default_reraise_flag(): + """Verify TestingFarmResultsHandler defaults to reraise_transient_errors=True.""" + handler = TFResultsHandler( + package_config=flexmock(), + job_config=flexmock(), + event={}, + ) + assert handler._status_reporter_reraise_transient_errors is True + + +def test_testing_farm_handler_set_reraise_flag(): + """Test set_status_reporter_reraise_transient_errors() method.""" + handler = TFResultsHandler( + package_config=flexmock(), + job_config=flexmock(), + event={}, + ) + handler.set_status_reporter_reraise_transient_errors(False) + assert handler._status_reporter_reraise_transient_errors is False