Skip to content
Open
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
63 changes: 24 additions & 39 deletions backend/kernelCI_app/helpers/hardwareDetails.py
Original file line number Diff line number Diff line change
Expand Up @@ -290,8 +290,6 @@ def handle_test_history(
full_environment_misc: bool = False,
) -> None:
create_record_test_platform(record=record)
record_misc = sanitize_dict(record.get("misc"))

environment_misc_dict = get_environment_misc_value(
full_environment_misc=full_environment_misc,
parsed_environment_misc=record.get("parsed_environment_misc"),
Expand All @@ -313,11 +311,7 @@ def handle_test_history(
environment_misc=environment_misc,
tree_name=record["build__checkout__tree_name"],
git_repository_branch=record["build__checkout__git_repository_branch"],
lab=(
record_misc.get("runtime", UNKNOWN_STRING)
if record_misc
else UNKNOWN_STRING
),
lab=record.get("lab") or UNKNOWN_STRING,
)

task.append(test_history_item)
Expand Down Expand Up @@ -380,16 +374,14 @@ def handle_test_summary(
getattr(task.origins[origin], status) + 1,
)

misc = sanitize_dict(record.get("misc")) or {}
lab = misc.get("runtime", UNKNOWN_STRING)
if lab:
if task.labs.get(lab) is None:
task.labs[lab] = StatusCount()
setattr(
task.labs[lab],
status,
getattr(task.labs[lab], status) + 1,
)
lab = record.get("lab") or UNKNOWN_STRING
if task.labs.get(lab) is None:
task.labs[lab] = StatusCount()
setattr(
task.labs[lab],
status,
getattr(task.labs[lab], status) + 1,
)


def handle_build_history(
Expand Down Expand Up @@ -458,18 +450,16 @@ def handle_build_summary(
getattr(builds_summary.origins[origin], status_key) + 1,
)

misc = sanitize_dict(build.misc) or {}
lab = misc.get("lab", UNKNOWN_STRING)
if lab:
build_lab_summary = builds_summary.labs.get(lab)
if not build_lab_summary:
build_lab_summary = StatusCount()
builds_summary.labs[lab] = build_lab_summary
setattr(
builds_summary.labs[lab],
status_key,
getattr(builds_summary.labs[lab], status_key) + 1,
)
lab = record.get("build_lab") or UNKNOWN_STRING
build_lab_summary = builds_summary.labs.get(lab)
if not build_lab_summary:
build_lab_summary = StatusCount()
builds_summary.labs[lab] = build_lab_summary
setattr(
builds_summary.labs[lab],
status_key,
getattr(builds_summary.labs[lab], status_key) + 1,
)

process_issue(record=record, task_issues_dict=issue_dict, issue_from="build")

Expand Down Expand Up @@ -579,8 +569,7 @@ def decide_if_is_full_record_filtered_out(
if not is_current_tree_selected:
return True

misc = sanitize_dict(record.get("misc")) or {}
lab = misc.get("runtime", UNKNOWN_STRING)
lab = record.get("lab") or UNKNOWN_STRING

is_record_filtered_out_result = instance.filters.is_record_filtered_out(
hardwares=record["environment_compatible"],
Expand Down Expand Up @@ -730,10 +719,8 @@ def process_filters(*, instance, record: Dict) -> None:

instance.unfiltered_origins["build"].add(record["build__origin"])

build_misc = sanitize_dict(record.get("build__misc")) or {}
build_lab = build_misc.get("lab")
if build_lab:
instance.unfiltered_labs["build"].add(build_lab)
if record.get("build_lab"):
instance.unfiltered_labs["build"].add(record["build_lab"])

if record["id"] is not None:
if is_boot(record["path"]):
Expand Down Expand Up @@ -771,10 +758,8 @@ def process_filters(*, instance, record: Dict) -> None:
platform_set.add(test_platform)
origin_set.add(record["test_origin"])

test_misc = sanitize_dict(record.get("misc")) or {}
test_lab = test_misc.get("runtime")
if test_lab:
instance.unfiltered_labs[flag_tab].add(test_lab)
if record.get("lab"):
instance.unfiltered_labs[flag_tab].add(record["lab"])


def is_record_tree_selected(*, record, tree: Tree, is_all_selected: bool) -> bool:
Expand Down
63 changes: 31 additions & 32 deletions backend/kernelCI_app/helpers/treeDetails.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
create_issue_typed,
extract_error_message,
is_boot,
sanitize_dict,
)


Expand Down Expand Up @@ -87,42 +86,41 @@ def get_current_row_data(
"test_number_value": current_row[10],
"test_misc": current_row[11],
"test_environment_compatible": current_row[tmp_test_env_comp_key],
"build_id": current_row[13],
"build_origin": current_row[14],
"build_comment": current_row[15],
"build_start_time": current_row[16],
"build_duration": current_row[17],
"build_architecture": current_row[18],
"build_command": current_row[19],
"build_compiler": current_row[20],
"build_config_name": current_row[21],
"build_config_url": current_row[22],
"build_log_url": current_row[23],
"build_status": current_row[24],
"build_misc": current_row[25],
"checkout_id": current_row[26],
"checkout_git_repository_url": current_row[27],
"checkout_git_repository_branch": current_row[28],
"checkout_git_commit_tags": current_row[29],
"checkout_origin": current_row[30],
"incident_id": current_row[31],
"incident_test_id": current_row[32],
"incident_present": current_row[33],
"issue_id": current_row[34],
"issue_version": current_row[35],
"issue_comment": current_row[36],
"issue_report_url": current_row[37],
"test_lab": current_row[13],
"build_id": current_row[14],
"build_origin": current_row[15],
"build_comment": current_row[16],
"build_start_time": current_row[17],
"build_duration": current_row[18],
"build_architecture": current_row[19],
"build_command": current_row[20],
"build_compiler": current_row[21],
"build_config_name": current_row[22],
"build_config_url": current_row[23],
"build_log_url": current_row[24],
"build_status": current_row[25],
"build_misc": current_row[26],
"build_lab": current_row[27],
"checkout_id": current_row[28],
"checkout_git_repository_url": current_row[29],
"checkout_git_repository_branch": current_row[30],
"checkout_git_commit_tags": current_row[31],
"checkout_origin": current_row[32],
"incident_id": current_row[33],
"incident_test_id": current_row[34],
"incident_present": current_row[35],
"issue_id": current_row[36],
"issue_version": current_row[37],
"issue_comment": current_row[38],
"issue_report_url": current_row[39],
}

parsed_environment_misc = handle_misc(
misc_value_or_default(current_row_data["test_environment_misc"])
)
current_row_data["test_platform"] = parsed_environment_misc.get("platform")
current_row_data["parsed_environment_misc"] = parsed_environment_misc
test_misc = sanitize_dict(current_row_data["test_misc"])
test_runtime_lab = UNKNOWN_STRING
if test_misc is not None:
test_runtime_lab = test_misc.get("runtime", UNKNOWN_STRING)
test_runtime_lab = current_row_data["test_lab"] or UNKNOWN_STRING

if current_row_data["test_status"] is None:
current_row_data["test_status"] = NULL_STATUS
Expand Down Expand Up @@ -495,8 +493,9 @@ def process_filters(instance, row_data: dict, skip_build_filters: bool = False)
instance.global_architectures.add(row_data["build_architecture"])
instance.global_compilers.add(row_data["build_compiler"])
instance.unfiltered_origins["build"].add(row_data["build_origin"])
if (build_misc := row_data["build_misc"]) is not None:
instance.unfiltered_labs["build"].add(build_misc.get("lab", UNKNOWN_STRING))
instance.unfiltered_labs["build"].add(
row_data.get("build_lab") or UNKNOWN_STRING
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

here the code we talked about. Not sure if this conditional should be removed

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This one is tricky.
Post-migration we no longer read build_misc, so we can't distinguish "no misc at all" from "misc present but no lab" — we only have the lab field. And it's odd to branch on how lab is missing.
I think we should standardize: any missing lab as UNKNOWN.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If we do find a reason to treat them differently, we should include an explicit rule on this.


build_issue_id, build_issue_version, is_build_issue = (
should_increment_build_issue(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,7 @@
"log_url",
"log_excerpt",
"misc",
"lab_id",
"status",
],
"query": """
Expand All @@ -172,10 +173,11 @@
log_url,
log_excerpt,
misc,
lab_id,
status
)
VALUES (
%s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s
%s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s
)
ON CONFLICT (id)
DO UPDATE SET
Expand All @@ -193,6 +195,7 @@
log_url = COALESCE(builds.log_url, EXCLUDED.log_url),
log_excerpt = COALESCE(builds.log_excerpt, EXCLUDED.log_excerpt),
misc = COALESCE(builds.misc, EXCLUDED.misc),
lab_id = COALESCE(builds.lab_id, EXCLUDED.lab_id),
status = COALESCE(builds.status, EXCLUDED.status);
""",
},
Expand All @@ -213,6 +216,7 @@
"duration",
"output_files",
"misc",
"lab_id",
"number_value",
"environment_compatible",
"number_prefix",
Expand All @@ -236,14 +240,15 @@
duration,
output_files,
misc,
lab_id,
number_value,
environment_compatible,
number_prefix,
number_unit,
input_files
)
VALUES (
%s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s
%s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s
)
ON CONFLICT (id)
DO UPDATE SET
Expand All @@ -259,6 +264,7 @@
duration = COALESCE(tests.duration, EXCLUDED.duration),
output_files = COALESCE(tests.output_files, EXCLUDED.output_files),
misc = COALESCE(tests.misc, EXCLUDED.misc),
lab_id = COALESCE(tests.lab_id, EXCLUDED.lab_id),
number_value = COALESCE(tests.number_value, EXCLUDED.number_value),
environment_compatible = COALESCE(tests.environment_compatible, EXCLUDED.environment_compatible),
number_prefix = COALESCE(tests.number_prefix, EXCLUDED.number_prefix),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,9 @@ def _standardize_lab_field(item: dict[str, Any], field: str) -> None:
"""
lab = item.get("misc", {}).get(field)
is_automatic = lab and AUTOMATIC_LABS.match(lab)
# Real lab for the lab_id FK, captured before the origin fallback (#1752) below
# overwrites misc, which would otherwise pollute the lab dimension.
item["_real_lab"] = None if is_automatic else lab
if is_automatic:
item["misc"][AUTOMATIC_LAB_FIELD] = lab
item["misc"].pop(field, None)
Expand Down Expand Up @@ -188,6 +191,39 @@ def prepare_file_data(
}


def assign_lab_ids(builds_buf: list[Builds], tests_buf: list[Tests]) -> None:
"""Resolve each instance's real lab name to a labs.id and set its lab_id FK.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If the lab name is unique in the database, why not use that as a primary key? In that case you'd make a first query to check which labs are not in the database yet, a second query to update the table if needed, but you wouldn't need the third query to get the id of the newly added labs

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Mostly to avoid string keys, since this is an internal key, we might benefit more from integer indices.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Makes sense. I am worried that we are adding one query here, one query there and soon we will have a bloat of unnecessary queries in the ingester, but it's fine for now

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It is a valid concern. If we plain to move some operations to ingestion time, we certainly are going to end up increasing "ingestion complexity". But I believe that, as long as we keep the ingester fast enough, simplifying the analysis step should be the goal.


Select-first so we only INSERT genuinely new labs (avoids burning the id
sequence on every flush). New labs are committed outside the fact-insert
transaction (autocommit).
"""
objs = [*builds_buf, *tests_buf]
names = {name for obj in objs if (name := obj._lab_name)}

id_map: dict[str, int] = {}
if names:
with connections["default"].cursor() as cursor:
cursor.execute(
"SELECT id, name FROM labs WHERE name = ANY(%s)", [list(names)]
)
id_map = {name: lab_id for lab_id, name in cursor.fetchall()}

missing = [name for name in names if name not in id_map]
if missing:
cursor.executemany(
"INSERT INTO labs (name) VALUES (%s) ON CONFLICT (name) DO NOTHING",
[(name,) for name in missing],
)
cursor.execute(
"SELECT id, name FROM labs WHERE name = ANY(%s)", [missing]
)
id_map.update({name: lab_id for lab_id, name in cursor.fetchall()})

for obj in objs:
obj.lab_id = id_map.get(obj._lab_name) if obj._lab_name else None
Comment on lines +194 to +224

@tales-aparecida tales-aparecida Jul 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I was a bit concerned with the 3 queries, so I've asked gemini to try to fold them into a single query, please review

Suggested change
def assign_lab_ids(builds_buf: list[Builds], tests_buf: list[Tests]) -> None:
"""Resolve each instance's real lab name to a labs.id and set its lab_id FK.
Select-first so we only INSERT genuinely new labs (avoids burning the id
sequence on every flush). New labs are committed outside the fact-insert
transaction (autocommit).
"""
objs = [*builds_buf, *tests_buf]
names = {name for obj in objs if (name := obj._lab_name)}
id_map: dict[str, int] = {}
if names:
with connections["default"].cursor() as cursor:
cursor.execute(
"SELECT id, name FROM labs WHERE name = ANY(%s)", [list(names)]
)
id_map = {name: lab_id for lab_id, name in cursor.fetchall()}
missing = [name for name in names if name not in id_map]
if missing:
cursor.executemany(
"INSERT INTO labs (name) VALUES (%s) ON CONFLICT (name) DO NOTHING",
[(name,) for name in missing],
)
cursor.execute(
"SELECT id, name FROM labs WHERE name = ANY(%s)", [missing]
)
id_map.update({name: lab_id for lab_id, name in cursor.fetchall()})
for obj in objs:
obj.lab_id = id_map.get(obj._lab_name) if obj._lab_name else None
def assign_lab_ids(builds_buf: list[Builds], tests_buf: list[Tests]) -> None:
"""Resolve each instance's real lab name to a labs.id and set its lab_id FK.
Uses a single optimized CTE join to insert missing records and fetch all IDs
in 1 round-trip without sequence waste or ORM overhead.
"""
objs = [*builds_buf, *tests_buf]
names = list({obj._lab_name for obj in objs if getattr(obj, "_lab_name", None)})
if not names:
for obj in objs:
obj.lab_id = None
return
query = """
WITH input_names AS (
-- 1. Unnest and deduplicate inputs in Postgres C-memory
SELECT DISTINCT unnest(%s::text[]) AS name
),
inserted AS (
-- 2. Anti-join via LEFT JOIN / IS NULL (Faster than NOT EXISTS)
INSERT INTO labs (name)
SELECT i.name
FROM input_names i
LEFT JOIN labs l ON l.name = i.name
WHERE l.name IS NULL
RETURNING id, name
)
-- 3. Combine newly inserted IDs with existing IDs
SELECT id, name FROM inserted
UNION ALL
SELECT l.id, l.name
FROM labs l
JOIN input_names i ON l.name = i.name;
"""
with connections["default"].cursor() as cursor:
cursor.execute(query, [names])
# Unpacks (id, name) tuples into a {name: id} dictionary directly
id_map = dict(cursor.fetchall())
for obj in objs:
lab_name = getattr(obj, "_lab_name", None)
obj.lab_id = id_map.get(lab_name) if lab_name else None

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

great optimization but I think it might burn the id sequence in some cases, right? which is the concern of the original approach.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actually, it doesn't...

but it's great that you mentioned that! We still need need ON CONFLICT (name) DO NOTHING like we have in the original code to avoid the race condition.

Only when the race condition happens there will be ID burning on both the original and using the single query

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is tricky, because in the original solution, despite having tree queries, only the first simpler query would hit most of the time (since we expect to have the number of labs being significantly smaller than the number of builds or tests ).
With the unified query we are running 100% of the time a more complex query (I will take a look how this impact performance).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

On the topic of race condition (thanks for catching that), I believe this can be solved with a lock in the solution with split queries.



def consume_buffer(buffer: list[TableModels], table_name: TableNames) -> None:
"""
Consume a buffer of items and insert them into the database.
Expand Down Expand Up @@ -245,6 +281,8 @@ def flush_buffers(
if total == 0:
return

assign_lab_ids(builds_buf, tests_buf)

# Insert in dependency-safe order
flush_start = time.time()
try:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@ def make_build_instance(build: dict[str, Any]) -> Builds:
filtered_build = {key: value for key, value in build.items() if key in BUILD_FIELDS}
obj = Builds(**filtered_build)
obj.field_timestamp = timezone.now()
obj._lab_name = build.get("_real_lab")
return obj


Expand All @@ -127,6 +128,7 @@ def make_test_instance(test: dict[str, Any]) -> Tests:
}
obj = Tests(**filtered_test)
obj.field_timestamp = timezone.now()
obj._lab_name = test.get("_real_lab")
return obj


Expand Down
2 changes: 1 addition & 1 deletion backend/kernelCI_app/management/commands/notifications.py
Original file line number Diff line number Diff line change
Expand Up @@ -706,7 +706,7 @@ def generate_hardware_summary_report(
continue
hardware_id = environment_misc.get("platform")
raw["job_id"] = environment_misc.get("job_id")
raw["runtime"] = misc.get("runtime")
raw["runtime"] = raw.get("lab") or misc.get("runtime")
origin = raw.get("test_origin")
key = (hardware_id, origin)
tree = raw.get("tree_name")
Expand Down
41 changes: 41 additions & 0 deletions backend/kernelCI_app/migrations/0019_labs_builds_lab_tests_lab.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
# Generated by Django 5.2.11 on 2026-06-29 20:25

import django.db.models.deletion
from django.db import migrations, models


class Migration(migrations.Migration):
dependencies = [
("kernelCI_app", "0018_hardwareregistryplatformvendor_and_more"),
]

operations = [
migrations.CreateModel(
name="Labs",
fields=[
("id", models.AutoField(primary_key=True, serialize=False)),
("name", models.TextField(unique=True)),
],
options={
"db_table": "labs",
},
),
migrations.AddField(
model_name="builds",
name="lab",
field=models.ForeignKey(
null=True,
on_delete=django.db.models.deletion.DO_NOTHING,
to="kernelCI_app.labs",
),
),
migrations.AddField(
model_name="tests",
name="lab",
field=models.ForeignKey(
null=True,
on_delete=django.db.models.deletion.DO_NOTHING,
to="kernelCI_app.labs",
),
),
]
Loading
Loading