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

Consider identifiers when retriggering #2658

Merged
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
31 changes: 31 additions & 0 deletions alembic/versions/d625d6c1122f_add_identifier_to_copr_build.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
"""Add identifier to copr build

Revision ID: d625d6c1122f
Revises: f69687c314c5
Create Date: 2024-11-22 13:05:49.763917

"""

import sqlalchemy as sa

from alembic import op

# revision identifiers, used by Alembic.
revision = "d625d6c1122f"
down_revision = "f69687c314c5"
branch_labels = None
depends_on = None


def upgrade():
# ### commands auto generated by Alembic - please adjust! ###
op.add_column(
"copr_build_targets", sa.Column("identifier", sa.String(), default="", nullable=True)
)
# ### end Alembic commands ###


def downgrade():
# ### commands auto generated by Alembic - please adjust! ###
op.drop_column("copr_build_targets", "identifier")
# ### end Alembic commands ###
24 changes: 15 additions & 9 deletions packit_service/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -162,10 +162,10 @@ def get_submitted_time_from_model(
) -> datetime:
# TODO: unify `submitted_name` (or better -> create for both models `task_accepted_time`)
# to delete this mess plz
if isinstance(model, CoprBuildTargetModel):
return model.build_submitted_time

return model.submitted_time
try:
return model.build_submitted_time # type: ignore[union-attr]
except AttributeError:
return model.submitted_time # type: ignore[union-attr]


@overload
Expand Down Expand Up @@ -202,11 +202,11 @@ def get_most_recent_targets(
for model in models:
submitted_time_of_current_model = get_submitted_time_from_model(model)
if (
most_recent_models.get(model.target) is None
or get_submitted_time_from_model(most_recent_models[model.target])
most_recent_models.get((model.target, model.identifier)) is None
or get_submitted_time_from_model(most_recent_models[(model.target, model.identifier)])
< submitted_time_of_current_model
):
most_recent_models[model.target] = model
most_recent_models[(model.target, model.identifier)] = model

return list(most_recent_models.values())

Expand Down Expand Up @@ -254,12 +254,14 @@ def filter_most_recent_target_names_by_status(
Iterable["TFTTestRunTargetModel"],
],
statuses_to_filter_with: list[str],
) -> Optional[set[str]]:
) -> Optional[set[tuple[str, str]]]:
filtered_models = filter_most_recent_target_models_by_status(
models,
statuses_to_filter_with,
)
return {model.target for model in filtered_models} if filtered_models else None
return (
{(model.target, model.identifier) for model in filtered_models} if filtered_models else None
)


# https://github.com/python/mypy/issues/2477#issuecomment-313984522 ^_^
Expand Down Expand Up @@ -2102,6 +2104,8 @@ class CoprBuildTargetModel(GroupAndTargetModelConnector, Base):

scan = relationship("OSHScanModel", back_populates="copr_build_target")

identifier = Column(String)

def set_built_packages(self, built_packages):
with sa_session_transaction(commit=True) as session:
self.built_packages = built_packages
Expand Down Expand Up @@ -2297,6 +2301,7 @@ def create(
status: BuildStatus,
copr_build_group: "CoprBuildGroupModel",
task_accepted_time: Optional[datetime] = None,
identifier: Optional[str] = None,
) -> "CoprBuildTargetModel":
with sa_session_transaction(commit=True) as session:
build = cls()
Expand All @@ -2307,6 +2312,7 @@ def create(
build.web_url = web_url
build.target = target
build.task_accepted_time = task_accepted_time
build.identifier = identifier or ""
session.add(build)

copr_build_group.copr_build_targets.append(build)
Expand Down
8 changes: 4 additions & 4 deletions packit_service/worker/events/comment.py
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,8 @@ def __init__(
comment_id: int,
commit_sha: str = "",
comment_object: Optional[Comment] = None,
build_targets_override: Optional[set[str]] = None,
tests_targets_override: Optional[set[str]] = None,
build_targets_override: Optional[set[tuple[str, str]]] = None,
tests_targets_override: Optional[set[tuple[str, str]]] = None,
) -> None:
super().__init__(
pr_id=pr_id,
Expand Down Expand Up @@ -93,7 +93,7 @@ def comment_object(self) -> Optional[Comment]:
return self._comment_object

@property
def build_targets_override(self) -> Optional[set[str]]:
def build_targets_override(self) -> Optional[set[tuple[str, str]]]:
if not self._build_targets_override and "rebuild-failed" in self.comment:
self._build_targets_override = (
super().get_all_build_targets_by_status(
Expand All @@ -104,7 +104,7 @@ def build_targets_override(self) -> Optional[set[str]]:
return self._build_targets_override

@property
def tests_targets_override(self) -> Optional[set[str]]:
def tests_targets_override(self) -> Optional[set[tuple[str, str]]]:
if not self._tests_targets_override and "retest-failed" in self.comment:
self._tests_targets_override = (
super().get_all_tf_targets_by_status(
Expand Down
48 changes: 34 additions & 14 deletions packit_service/worker/events/event.py
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,8 @@ def __init__(
event_dict: Optional[dict],
issue_id: Optional[int],
task_accepted_time: Optional[datetime],
build_targets_override: Optional[list[str]],
tests_targets_override: Optional[list[str]],
build_targets_override: Optional[set[tuple[str, str]]],
tests_targets_override: Optional[set[tuple[str, str]]],
branches_override: Optional[list[str]],
):
self.event_type = event_type
Expand Down Expand Up @@ -119,8 +119,16 @@ def from_event_dict(cls, event: dict):
time = event.get("task_accepted_time")
task_accepted_time = datetime.fromtimestamp(time, timezone.utc) if time else None

build_targets_override = event.get("build_targets_override")
tests_targets_override = event.get("tests_targets_override")
build_targets_override = (
{(target, identifier_) for [target, identifier_] in event.get("build_targets_override")}
if event.get("build_targets_override")
else set()
Copy link
Member

Choose a reason for hiding this comment

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

just to be sure, I also remember some discussions about the empty set vs None for *targets_override, is it ok to have empty set here instead of None?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it is, however I want to test this a bit deeper in the staging instance.

)
tests_targets_override = (
{(target, identifier_) for [target, identifier_] in event.get("tests_targets_override")}
if event.get("tests_targets_override")
else set()
)
branches_override = event.get("branches_override")

return EventData(
Expand Down Expand Up @@ -508,17 +516,19 @@ def packages_config(self):
raise NotImplementedError("Please implement me!")

@property
def build_targets_override(self) -> Optional[set[str]]:
def build_targets_override(self) -> Optional[set[tuple[str, str]]]:
"""
Return the targets to use for building of the all targets from config
Return the targets and identifiers to use for building
of the all targets from config
for the relevant events (e.g.rerunning of a single check).
"""
return None

@property
def tests_targets_override(self) -> Optional[set[str]]:
def tests_targets_override(self) -> Optional[set[tuple[str, str]]]:
"""
Return the targets to use for testing of the all targets from config
Return the targets and identifiers to use for testing
of the all targets from config
for the relevant events (e.g.rerunning of a single check).
"""
return None
Expand Down Expand Up @@ -638,34 +648,44 @@ def get_packages_config(self) -> Optional[PackageConfig]:
def get_all_tf_targets_by_status(
self,
statuses_to_filter_with: list[str],
) -> Optional[set[str]]:
) -> Optional[set[tuple[str, str]]]:
if self.commit_sha is None:
return None

logger.debug(
f"Getting failed Testing Farm targets for commit sha: {self.commit_sha}",
f"Getting Testing Farm targets for commit sha {self.commit_sha} "
f"and statuses {statuses_to_filter_with}",
)
return filter_most_recent_target_names_by_status(
found_targets = filter_most_recent_target_names_by_status(
models=TFTTestRunTargetModel.get_all_by_commit_target(
commit_sha=self.commit_sha,
),
statuses_to_filter_with=statuses_to_filter_with,
)
logger.debug(
f"Testing Farm found targets {found_targets}",
)
return found_targets

def get_all_build_targets_by_status(
self,
statuses_to_filter_with: list[str],
) -> Optional[set[str]]:
) -> Optional[set[tuple[str, str]]]:
if self.commit_sha is None or self.project.repo is None:
return None

logger.debug(
f"Getting failed COPR build targets for commit sha: {self.commit_sha}",
f"Getting COPR build targets for commit sha {self.commit_sha} "
f"and statuses {statuses_to_filter_with}",
)
return filter_most_recent_target_names_by_status(
found_targets = filter_most_recent_target_names_by_status(
models=CoprBuildTargetModel.get_all_by_commit(commit_sha=self.commit_sha),
statuses_to_filter_with=statuses_to_filter_with,
)
logger.debug(
f"Builds found targets {found_targets}",
)
return found_targets

def get_non_serializable_attributes(self):
return [
Expand Down
16 changes: 12 additions & 4 deletions packit_service/worker/events/github.py
Original file line number Diff line number Diff line change
Expand Up @@ -236,16 +236,24 @@ def __init__(
)
self.job_identifier = job_identifier

def _parse_target_and_identifier(self, target_string: str) -> tuple[str, str]:
"""Parse target and identifier from check name target string."""
if ":" in target_string:
target, identifier = target_string.split(":")
else:
target, identifier = target_string, ""
return target, identifier

@property
def build_targets_override(self) -> Optional[set[str]]:
def build_targets_override(self) -> Optional[set[tuple[str, str]]]:
if self.check_name_job in {"rpm-build", "production-build", "koji-build"}:
return {self.check_name_target}
return {self._parse_target_and_identifier(self.check_name_target)}
return None

@property
def tests_targets_override(self) -> Optional[set[str]]:
def tests_targets_override(self) -> Optional[set[tuple[str, str]]]:
if self.check_name_job == "testing-farm":
return {self.check_name_target}
return {self._parse_target_and_identifier(self.check_name_target)}
return None

@property
Expand Down
2 changes: 1 addition & 1 deletion packit_service/worker/handlers/mixin.py
Original file line number Diff line number Diff line change
Expand Up @@ -432,7 +432,7 @@ def copr_build_helper(self) -> CoprBuildJobHelper:
# when reporting state of SRPM build built in Copr
build_targets_override = (
{
build.target
(build.target, build.identifier)
for build in CoprBuildTargetModel.get_all_by_build_id(
str(self.copr_event.build_id),
)
Expand Down
45 changes: 32 additions & 13 deletions packit_service/worker/helpers/build/build_helper.py
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,8 @@ def __init__(
metadata: EventData,
db_project_event: ProjectEventModel,
job_config: JobConfig,
build_targets_override: Optional[set[str]] = None,
tests_targets_override: Optional[set[str]] = None,
build_targets_override: Optional[set[tuple[str, str]]] = None,
tests_targets_override: Optional[set[tuple[str, str]]] = None,
pushgateway: Optional[Pushgateway] = None,
):
super().__init__(
Expand All @@ -67,8 +67,8 @@ def __init__(
pushgateway=pushgateway,
)
self.run_model: Optional[PipelineModel] = None
self.build_targets_override: Optional[set[str]] = build_targets_override
self.tests_targets_override: Optional[set[str]] = tests_targets_override
self.build_targets_override: Optional[set[tuple[str, str]]] = build_targets_override
self.tests_targets_override: Optional[set[tuple[str, str]]] = tests_targets_override
self.pushgateway = pushgateway

# lazy properties
Expand Down Expand Up @@ -199,7 +199,7 @@ def build_targets(self) -> set[str]:
"""
if self.build_targets_override:
logger.debug(f"Build targets override: {self.build_targets_override}")
return self.build_targets_all & self.build_targets_override
return self.build_targets_all & {target for target, _ in self.build_targets_override}

return self.build_targets_all

Expand Down Expand Up @@ -270,13 +270,24 @@ def build_targets_for_test_job(self, test_job_config: JobConfig) -> set[str]:

if self.build_targets_override:
logger.debug(f"Build targets override: {self.build_targets_override}")
targets_override.update(self.build_targets_override)
targets_override.update(
[
target
for (target, identifier) in self.build_targets_override
if identifier == (test_job_config.identifier or "")
]
)

if self.tests_targets_override:
logger.debug(f"Test targets override: {self.tests_targets_override}")
targets_override.update(
self.test_target2build_target_for_test_job(target, test_job_config)
for target in self.tests_targets_override
self.test_target2build_target_for_test_job(t, test_job_config)
for t in [
target
for (target, identifier) in self.tests_targets_override
if identifier
== (test_job_config.identifier if test_job_config.identifier else "")
]
)

return configured_targets & targets_override if targets_override else configured_targets
Expand Down Expand Up @@ -314,14 +325,22 @@ def tests_targets_for_test_job(self, test_job_config: JobConfig) -> set[str]:

if self.build_targets_override:
logger.debug(f"Build targets override: {self.build_targets_override}")
for target in self.build_targets_override:
targets_override.update(
self.build_target2test_targets_for_test_job(target, test_job_config),
)
for target, identifier in self.build_targets_override:
if identifier == (test_job_config.identifier if test_job_config.identifier else ""):
Copy link
Member

Choose a reason for hiding this comment

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

Why do you need the ternary there? 👀

Copy link
Member Author

Choose a reason for hiding this comment

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

job_config.identifier could be None, so yes I think we need it. A non specified identifier is the default identifier "".
We can also say that the default identifier is always None but in this case I am a bit afraid of the serialization, if we go through a serialization/deserialization process of a None string value I think we will end up with an empty str ""?
So I feel this comparison more safe.

targets_override.update(
self.build_target2test_targets_for_test_job(target, test_job_config),
)

if self.tests_targets_override:
logger.debug(f"Test targets override: {self.tests_targets_override}")
targets_override.update(self.tests_targets_override)
targets_override.update(
[
target
for target, identifier in self.tests_targets_override
if identifier
== (test_job_config.identifier if test_job_config.identifier else "")
]
)

return (
configured_targets & targets_override
Expand Down
5 changes: 3 additions & 2 deletions packit_service/worker/helpers/build/copr_build.py
Original file line number Diff line number Diff line change
Expand Up @@ -80,8 +80,8 @@ def __init__(
metadata: EventData,
db_project_event: ProjectEventModel,
job_config: JobConfig,
build_targets_override: Optional[set[str]] = None,
tests_targets_override: Optional[set[str]] = None,
build_targets_override: Optional[set[tuple[str, str]]] = None,
tests_targets_override: Optional[set[tuple[str, str]]] = None,
Comment on lines +83 to +84
Copy link
Member

Choose a reason for hiding this comment

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

I have a feeling you're changing the meaning of this attribute, so the name no longer holds very well, but… I guess that's a different rabbit hole to go down…

Copy link
Member Author

Choose a reason for hiding this comment

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

I would say they are still targets override but a bit more specific. They are targets for a specific identifier.

The most efficient structure here is probably a dictionary and not a set anymore, however I would start with this simpler enhancement.

pushgateway: Optional[Pushgateway] = None,
celery_task: Optional[CeleryTask] = None,
copr_build_group_id: Optional[int] = None,
Expand Down Expand Up @@ -605,6 +605,7 @@ def _get_or_create_build_group(self) -> CoprBuildGroupModel:
status=BuildStatus.waiting_for_srpm,
copr_build_group=group,
task_accepted_time=self.metadata.task_accepted_time,
identifier=self.job_config.identifier,
)

if unprocessed_chroots:
Expand Down
4 changes: 2 additions & 2 deletions packit_service/worker/helpers/build/koji_build.py
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,8 @@ def __init__(
metadata: EventData,
db_project_event: ProjectEventModel,
job_config: JobConfig,
build_targets_override: Optional[set[str]] = None,
tests_targets_override: Optional[set[str]] = None,
build_targets_override: Optional[set[tuple[str, str]]] = None,
tests_targets_override: Optional[set[tuple[str, str]]] = None,
):
super().__init__(
service_config=service_config,
Expand Down
Loading
Loading