feat/labs migration#1968
Conversation
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>
7ee4011 to
80d7ce6
Compare
* 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>
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.
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>
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>
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)