feat/labs migration - #1968
Conversation
7ee4011 to
80d7ce6
Compare
80d7ce6 to
3404f84
Compare
|
|
||
|
|
||
| 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. |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Mostly to avoid string keys, since this is an internal key, we might benefit more from integer indices.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
| tests.number_value AS tests_number_value, | ||
| tests.misc AS tests_misc, | ||
| tests.environment_compatible AS tests_environment_compatible, | ||
| COALESCE(test_labs.name, tests.misc ->> 'runtime') AS tests_lab, |
There was a problem hiding this comment.
I think you forgot a TODO here
| ) | ||
| return list(result) | ||
| tests = [] | ||
| for test in result: |
There was a problem hiding this comment.
Why not assign directly to lab like before?
There was a problem hiding this comment.
Mostly because lab is now the column name, but thinking again. I might change the column name for lab_id, this way we keep the lab name free to use, and follow more closely the the foreign key nomenclature.
There was a problem hiding this comment.
Changed mind, to really avoid rename hacking on the ORM, it was easier to just add a proper sql query instead.
| result = [] | ||
| for row in rows: | ||
| build_misc = row[11] | ||
| sanitized_build_misc = sanitize_dict(build_misc) |
There was a problem hiding this comment.
Why does this not use the fallback?
| log_excerpt = models.CharField(max_length=16384, blank=True, null=True) | ||
| misc = models.JSONField(blank=True, null=True) | ||
| lab = models.ForeignKey( | ||
| Labs, db_constraint=False, null=True, on_delete=models.DO_NOTHING |
There was a problem hiding this comment.
Shouldn't we use db_constraint=True?
There was a problem hiding this comment.
I see all other tables are using db_constraint=False, so there must be a good reason for this, but I do not see it documented
There was a problem hiding this comment.
For the kci keys, we unfortunately have some missing keys in a few rows. But since lab has only internal keys, I believe we can safely enforce constraints on them. Good point.
| build_lab_summary = builds_summary.labs.get(lab) | ||
| if not build_lab_summary: | ||
| build_lab_summary = StatusCount() | ||
| builds_summary.labs[lab] = build_lab_summary |
There was a problem hiding this comment.
Copy pattern from previous chunk for consistence and readability
There was a problem hiding this comment.
The point is, this is the original implementation (pre-refactor)
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,
)The conditional for lab is never false, because we are assigning UNKNOWN_STRING for lab.
However, it seems that this function is no longer used, and might be a legacy function used before performance improvements on HardwareDetails endpoints.
I will confirm it, and if this is the case, remove it.
We might as well plan to perform some dead code elimination, and check for similar cases.
MarceloRobert
left a comment
MarceloRobert
left a comment
There was a problem hiding this comment.
Haven't tested yet, but looks good so far
| from typing import Optional | ||
|
|
||
| from django.db.models.expressions import F | ||
| from django.db import connection |
There was a problem hiding this comment.
very nit: prefer connections['default']
c511998 to
e0d17ed
Compare
Part of kernelci#1948 Signed-off-by: Alan Peixinho <alan.peixinho@profusion.mobi>
Part of kernelci#1948 Signed-off-by: Alan Peixinho <alan.peixinho@profusion.mobi>
* For analysis queries we are going for lab column information
first, and coalescing to json misc information.
* All COALESCE expressions are marked with TODO comments for removal
after the lab_id backfill is complete.
Closes kernelci#1948
Signed-off-by: Alan Peixinho <alan.peixinho@profusion.mobi>
Signed-off-by: Alan Peixinho <alan.peixinho@profusion.mobi>
Signed-off-by: Alan Peixinho <alan.peixinho@profusion.mobi>
e0d17ed to
3f30f62
Compare
| instance.unfiltered_labs["build"].add(build_misc.get("lab", UNKNOWN_STRING)) | ||
| instance.unfiltered_labs["build"].add( | ||
| row_data.get("build_lab") or UNKNOWN_STRING | ||
| ) |
There was a problem hiding this comment.
here the code we talked about. Not sure if this conditional should be removed
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
If we do find a reason to treat them differently, we should include an explicit rule on this.
| SELECT | ||
| t.misc->>'runtime' AS lab, | ||
| -- TODO remove misc->>'runtime' fallback after lab backfill | ||
| COALESCE(l.name, t.misc->>'runtime', t.origin) AS lab, |
There was a problem hiding this comment.
why t.origin here ?
If we have neither a laboratory nor a runtime, won't the WHERE clause you wrote will filter out the record?
There was a problem hiding this comment.
Nice catch. Missed removing some test here.
Rename Builds/Tests FK field from lab to lab_id so lab is free for annotated lab names without conflicting with the ORM relation. Signed-off-by: Alan Peixinho <alan.peixinho@profusion.mobi>
Keep lab as the standard Django FK field and resolve build test lab names via raw SQL, matching the hardware query pattern. Signed-off-by: Alan Peixinho <alan.peixinho@profusion.mobi>
Enforce the lab FK db_constraint on builds and tests, drop the unused lab_id from the build tests response, and fix the tree query lab TODO. Signed-off-by: Alan Peixinho <alan.peixinho@profusion.mobi>
Signed-off-by: Alan Peixinho <alan.peixinho@profusion.mobi>
Signed-off-by: Alan Peixinho <alan.peixinho@profusion.mobi>
ef51685 to
b4c96b2
Compare
| 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 |
There was a problem hiding this comment.
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
| 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 |
There was a problem hiding this comment.
great optimization but I think it might burn the id sequence in some cases, right? which is the concern of the original approach.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I was reviewing this PR again, and realized that we are in fact safe from race condition here.
The do nothing on conflict seems to really be enough.
There was a problem hiding this comment.
The do nothing on conflict seems to really be enough.
Oh yes, sorry if I was not clear! My point was that including the DO NOTHING in my suggestion would prevent the race condition, like it is preventing it in the original code
|
I'm concerned with how much overhead the labs will bring to the kcidb ingestion. either put it under a feature flag, so we can toggle it off quickly in case it's catastrophic, or show some metrics comparing the time to consume a few hundred payloads in a local development, before and after this MR |
|
@tales-aparecida, (checking in a local run) the new version turned out to be slightly faster (but it could be just noise). Previous implementation
New implementation
Hardware
Software
Comparison table
|
I'm confused, is "old" the main branch and "new" the feature branch? |
|
Please autosquash the fixups 😁 |
Exactly. Sorry for the bad nomenclature. |
which layer of cache is that? |
5 participants
Summary
Migrates all read paths for lab information from JSONB
miscfields (builds.misc->>'lab',tests.misc->>'runtime') to the newlabstable vialab_idforeign key, with JSONB fallback for rows not yet backfilled.Every SQL query and Python helper that previously extracted lab names by parsing JSONB now uses
COALESCE(labs.name, misc_fallback)through aLEFT JOINon thelabstable. This ensures:lab_idpopulated by the ingester) reads from the structured FKlab_id) still works via the JSONB fallbackTODOcomments)How to test
legacy database
lab_idis NULL on existing rowsMigrated database (test in a local database)