Bodhi update fixes (packit#2596)

Bodhi update fixes

Fixes packit#2573.

Reviewed-by: Matej Focko
Reviewed-by: Nikola Forró

packit_service/models.py

@@ -7,6 +7,7 @@
 
 import enum
 import logging
+import re
 from collections import Counter
 from collections.abc import Generator, Iterable
 from contextlib import contextmanager
@@ -42,6 +43,7 @@
     null,
 )
 from sqlalchemy.dialects.postgresql import array as psql_array
+from sqlalchemy.dialects.postgresql import dialect as psql_dialect
 from sqlalchemy.ext.declarative import declarative_base
 from sqlalchemy.orm import (
     Session as SQLASession,
@@ -2484,7 +2486,7 @@ def get_all_projects(cls) -> set["GitProjectModel"]:
             return set(projects)
 
     @classmethod
-    def get_first_successful_by_sidetag(
+    def get_last_successful_by_sidetag(
         cls,
         sidetag: str,
     ) -> Optional["BodhiUpdateTargetModel"]:
@@ -2495,9 +2497,28 @@ def get_first_successful_by_sidetag(
                     BodhiUpdateTargetModel.status == "success",
                     BodhiUpdateTargetModel.sidetag == sidetag,
                 )
+                .order_by(BodhiUpdateTargetModel.update_creation_time.desc())
                 .first()
             )
 
+    @classmethod
+    def get_all_successful_or_in_progress_by_nvrs(
+        cls,
+        koji_nvrs: str,
+    ) -> set["BodhiUpdateTargetModel"]:
+        regexp = "|".join(re.escape(nvr) for nvr in set(koji_nvrs.split()))
+        with sa_session_transaction() as session:
+            return set(
+                session.query(BodhiUpdateTargetModel)
+                .filter(
+                    BodhiUpdateTargetModel.status.in_(("queued", "retry", "success")),
+                    BodhiUpdateTargetModel.koji_nvrs.regexp_match(regexp).compile(
+                        dialect=psql_dialect(),
+                    ),
+                )
+                .all(),
+            )
+
 
 class BodhiUpdateGroupModel(ProjectAndEventsConnector, GroupModel, Base):
     __tablename__ = "bodhi_update_groups"

packit_service/worker/handlers/bodhi.py

@@ -110,11 +110,33 @@ def run(self) -> TaskResults:
                 existing_alias = None
                 # get update alias from previous run(s) from the same sidetag (if any)
                 if target_model.sidetag and (
-                    model := BodhiUpdateTargetModel.get_first_successful_by_sidetag(
+                    existing_model := BodhiUpdateTargetModel.get_last_successful_by_sidetag(
                         target_model.sidetag,
                     )
                 ):
-                    existing_alias = model.alias
+                    existing_alias = existing_model.alias
+                    if set(target_model.koji_nvrs.split()) == set(
+                        existing_model.koji_nvrs.split(),
+                    ):
+                        logger.info("No changes, skipping Bodhi update edit")
+                        target_model.set_status("skipped")
+                        continue
+
+                if not existing_alias:
+                    # avoid creating another update containing the same build - Bodhi shouldn't
+                    # allow it anyway but there is a race condition that makes it possible
+                    existing_models = (
+                        BodhiUpdateTargetModel.get_all_successful_or_in_progress_by_nvrs(
+                            target_model.koji_nvrs,
+                        )
+                    )
+                    if existing_models - {target_model}:
+                        logger.info(
+                            "Bodhi update containing one or more builds from "
+                            f"{{{target_model.koji_nvrs}}} already exists, skipping",
+                        )
+                        target_model.set_status("skipped")
+                        continue
 
                 logger.debug(
                     (f"Edit update {existing_alias} " if existing_alias else "Create update ")

tests/integration/test_bodhi_update.py

@@ -118,6 +118,9 @@ def test_bodhi_update_for_unknown_koji_build(koji_build_completed_old_format):
     )
     flexmock(ProjectEventModel).should_receive("get_or_create")
     flexmock(BodhiUpdateGroupModel).should_receive("create").and_return(group_model)
+    flexmock(BodhiUpdateTargetModel).should_receive(
+        "get_all_successful_or_in_progress_by_nvrs",
+    ).with_args("packit-0.43.0-1.fc36").and_return(set())
     flexmock(BodhiUpdateTargetModel).should_receive("create").with_args(
         target="rawhide",
         koji_nvrs="packit-0.43.0-1.fc36",
@@ -221,6 +224,9 @@ def test_bodhi_update_for_unknown_koji_build_failed(koji_build_completed_old_for
         ],
     )
     flexmock(BodhiUpdateGroupModel).should_receive("create").and_return(group_model)
+    flexmock(BodhiUpdateTargetModel).should_receive(
+        "get_all_successful_or_in_progress_by_nvrs",
+    ).with_args("packit-0.43.0-1.fc36").and_return(set())
     flexmock(BodhiUpdateTargetModel).should_receive("create").with_args(
         target="rawhide",
         koji_nvrs="packit-0.43.0-1.fc36",
@@ -328,6 +334,9 @@ def test_bodhi_update_for_unknown_koji_build_failed_issue_created(
         ],
     )
     flexmock(BodhiUpdateGroupModel).should_receive("create").and_return(group_model)
+    flexmock(BodhiUpdateTargetModel).should_receive(
+        "get_all_successful_or_in_progress_by_nvrs",
+    ).with_args("packit-0.43.0-1.fc36").and_return(set())
     flexmock(BodhiUpdateTargetModel).should_receive("create").with_args(
         target="rawhide",
         koji_nvrs="packit-0.43.0-1.fc36",
@@ -448,6 +457,9 @@ def test_bodhi_update_for_unknown_koji_build_failed_issue_comment(
         ],
     )
     flexmock(BodhiUpdateGroupModel).should_receive("create").and_return(group_model)
+    flexmock(BodhiUpdateTargetModel).should_receive(
+        "get_all_successful_or_in_progress_by_nvrs",
+    ).with_args("packit-0.43.0-1.fc36").and_return(set())
     flexmock(BodhiUpdateTargetModel).should_receive("create").with_args(
         target="rawhide",
         koji_nvrs="packit-0.43.0-1.fc36",
@@ -545,6 +557,9 @@ def test_bodhi_update_build_not_tagged_yet(
         ],
     )
     flexmock(BodhiUpdateGroupModel).should_receive("create").and_return(group_model)
+    flexmock(BodhiUpdateTargetModel).should_receive(
+        "get_all_successful_or_in_progress_by_nvrs",
+    ).with_args("packit-0.43.0-1.fc36").and_return(set())
     flexmock(BodhiUpdateTargetModel).should_receive("create").with_args(
         target="rawhide",
         koji_nvrs="packit-0.43.0-1.fc36",
@@ -656,6 +671,9 @@ def test_bodhi_update_for_unknown_koji_build_not_for_unfinished(
         ],
     )
     flexmock(BodhiUpdateGroupModel).should_receive("create").and_return(group_model)
+    flexmock(BodhiUpdateTargetModel).should_receive(
+        "get_all_successful_or_in_progress_by_nvrs",
+    ).with_args("packit-0.43.0-1.fc36").and_return(set())
     flexmock(BodhiUpdateTargetModel).should_receive("create").with_args(
         target="rawhide",
         koji_nvrs="packit-0.43.0-1.fc36",
@@ -742,6 +760,9 @@ def test_bodhi_update_for_known_koji_build(koji_build_completed_old_format):
         ],
     )
     flexmock(BodhiUpdateGroupModel).should_receive("create").and_return(group_model)
+    flexmock(BodhiUpdateTargetModel).should_receive(
+        "get_all_successful_or_in_progress_by_nvrs",
+    ).with_args("packit-0.43.0-1.fc36").and_return(set())
     flexmock(BodhiUpdateTargetModel).should_receive("create").with_args(
         target="rawhide",
         koji_nvrs="packit-0.43.0-1.fc36",
@@ -893,6 +914,9 @@ def test_bodhi_update_fedora_stable_by_default(koji_build_completed_f36):
         ],
     )
     flexmock(BodhiUpdateGroupModel).should_receive("create").and_return(group_model)
+    flexmock(BodhiUpdateTargetModel).should_receive(
+        "get_all_successful_or_in_progress_by_nvrs",
+    ).with_args("python-ogr-0.34.0-1.fc36").and_return(set())
     flexmock(BodhiUpdateTargetModel).should_receive("create").with_args(
         target="f36",
         koji_nvrs="python-ogr-0.34.0-1.fc36",
@@ -929,12 +953,30 @@ def test_bodhi_update_fedora_stable_by_default(koji_build_completed_f36):
 
 
 @pytest.mark.parametrize(
-    "missing_dependency, existing_update",
-    [(False, None), (False, "FEDORA-2024-abcdef1234"), (True, None)],
+    "missing_dependency, non_unique_builds, existing_update",
+    [
+        (False, False, None),
+        (
+            False,
+            False,
+            flexmock(
+                alias="FEDORA-2024-abcdef1234",
+                koji_nvrs="python-specfile-0.31.0-1.fc40 packit-0.99.0-1.fc40",
+            ),
+        ),
+        (
+            False,
+            False,
+            flexmock(alias="FEDORA-2024-abcdef1234", koji_nvrs="packit-0.99.0-1.fc40"),
+        ),
+        (False, True, None),
+        (True, False, None),
+    ],
 )
 def test_bodhi_update_from_sidetag(
     koji_build_tagged,
     missing_dependency,
+    non_unique_builds,
     existing_update,
 ):
     """(Sidetag scenario.)"""
@@ -997,10 +1039,8 @@ def test_bodhi_update_from_sidetag(
     ).and_return(flexmock(target=dg_branch, get_project_event_model=lambda: None))
 
     flexmock(BodhiUpdateTargetModel).should_receive(
-        "get_first_successful_by_sidetag",
-    ).with_args(sidetag_name).and_return(
-        flexmock(alias=existing_update) if existing_update else None,
-    )
+        "get_last_successful_by_sidetag",
+    ).with_args(sidetag_name).and_return(existing_update)
 
     specfile_packit_yaml = (
         "{'specfile_path': 'python-specfile.spec', 'synced_files': [],"
@@ -1096,30 +1136,44 @@ def _create_update(dist_git_branch, update_type, koji_builds, sidetag, alias):
             "packit-0.99.0-1.fc40",
         }
         assert sidetag == sidetag_name
-        assert alias == (existing_update if existing_update else None)
+        assert alias == (existing_update.alias if existing_update else None)
         return "alias", "url"
 
     flexmock(PackitAPI).should_receive("create_update").replace_with(
         _create_update,
-    ).times(0 if missing_dependency else 1)
+    ).times(
+        (
+            0
+            if missing_dependency
+            or non_unique_builds
+            or existing_update
+            and " " in existing_update.koji_nvrs
+            else 1
+        ),
+    )
 
     flexmock(PipelineModel).should_receive("create").and_return(flexmock())
-    group_model = flexmock(
-        grouped_targets=[
-            flexmock(
-                target=dg_branch,
-                koji_nvrs="python-specfile-0.31.0-1.fc40 packit-0.99.0-1.fc40",
-                sidetag=sidetag_name,
-                set_status=lambda x: None,
-                set_data=lambda x: None,
-                set_web_url=lambda x: None,
-                set_alias=lambda x: None,
-                set_update_creation_time=lambda x: None,
-            ),
-        ],
-    )
+    update_model = flexmock(
+        target=dg_branch,
+        koji_nvrs="python-specfile-0.31.0-1.fc40 packit-0.99.0-1.fc40",
+        sidetag=sidetag_name,
+        set_status=lambda x: None,
+        set_data=lambda x: None,
+        set_web_url=lambda x: None,
+        set_alias=lambda x: None,
+        set_update_creation_time=lambda x: None,
+    )
+    group_model = flexmock(grouped_targets=[update_model])
     flexmock(BodhiUpdateGroupModel).should_receive("create").and_return(group_model)
 
+    flexmock(BodhiUpdateTargetModel).should_receive(
+        "get_all_successful_or_in_progress_by_nvrs",
+    ).with_args(
+        "python-specfile-0.31.0-1.fc40 packit-0.99.0-1.fc40",
+    ).and_return(
+        {flexmock(), update_model} if non_unique_builds else {update_model},
+    )
+
     def _create(target, koji_nvrs, sidetag, status, bodhi_update_group):
         assert target == dg_branch
         assert set(koji_nvrs.split()) == {

tests/integration/test_issue_comment.py

@@ -495,13 +495,19 @@ def test_issue_comment_retrigger_bodhi_update_handler(
         ],
     )
     flexmock(BodhiUpdateGroupModel).should_receive("create").and_return(group_model)
+    flexmock(BodhiUpdateTargetModel).should_receive(
+        "get_all_successful_or_in_progress_by_nvrs",
+    ).with_args("python-teamcity-messages.fc38").and_return(set())
     flexmock(BodhiUpdateTargetModel).should_receive("create").with_args(
         target="f38",
         koji_nvrs="python-teamcity-messages.fc38",
         sidetag=None,
         status="queued",
         bodhi_update_group=group_model,
     ).and_return().once()
+    flexmock(BodhiUpdateTargetModel).should_receive(
+        "get_all_successful_or_in_progress_by_nvrs",
+    ).with_args("python-teamcity-messages.fc37").and_return(set())
     flexmock(BodhiUpdateTargetModel).should_receive("create").with_args(
         target="f37",
         koji_nvrs="python-teamcity-messages.fc37",

tests/integration/test_pr_comment.py

@@ -2526,6 +2526,9 @@ def test_bodhi_update_retrigger_via_dist_git_pr_comment(pagure_pr_comment_added)
         ],
     )
     flexmock(BodhiUpdateGroupModel).should_receive("create").and_return(group_model)
+    flexmock(BodhiUpdateTargetModel).should_receive(
+        "get_all_successful_or_in_progress_by_nvrs",
+    ).with_args("123").and_return(set())
     flexmock(BodhiUpdateTargetModel).should_receive("create").with_args(
         target="the_distgit_branch",
         koji_nvrs="123",

tests/unit/test_bodhi_update_error_msgs.py

@@ -13,6 +13,7 @@
 from packit.exceptions import PackitException
 
 from packit_service.config import ServiceConfig
+from packit_service.models import BodhiUpdateTargetModel
 from packit_service.worker.celery_task import CeleryTask
 from packit_service.worker.events import (
     PullRequestCommentPagureEvent,
@@ -139,6 +140,9 @@ def test_pull_request_retrigger_bodhi_update_with_koji_data(
             task_id=123,
         ),
     )
+    flexmock(BodhiUpdateTargetModel).should_receive(
+        "get_all_successful_or_in_progress_by_nvrs",
+    ).and_return(set())
     flexmock(CeleryTask).should_receive("is_last_try").and_return(True)
     handler = RetriggerBodhiUpdateHandler(package_config, job_config, data, flexmock())
     handler.run()