Mcp get log job fallback - #314
Conversation
|
please rebase |
f2ba72c to
2b8625d
Compare
| raise KciDevError("Log gzip stream is incomplete or malformed") | ||
|
|
||
|
|
||
| def _bare_node_id(node_id): |
There was a problem hiding this comment.
Fallback can return a Maestro log for a non-Maestro ID as any Dashboard "not found" result triggers _job_callback_log()
If the Dashboard does not contain other-origin:abcdef, but Maestro happens to contain node abcdef, get_log() could return an unrelated Maestro job's log instead of reporting that the original test was not found.
| raw = bytes(body) | ||
| if raw[:2] == b"\x1f\x8b": | ||
| expanded = bytearray() | ||
| for piece in _gunzip_iter(iter([raw])): |
There was a problem hiding this comment.
malformed gzip can leak zlib.error
| "kind": "job", | ||
| "result": "incomplete", | ||
| "data": {"error_code": "Infrastructure", "error_msg": "Unable to flash"}, | ||
| "artifacts": {"callback_data": callback_url} if callback_url else {}, |
There was a problem hiding this comment.
The current KernelCI pipeline says
# Temporarily we dont remove log field
# data.pop('log', None)
Maybe consider lava_log -> lava_logs -> callback_data["log"] or otherwise explain/document why depending on the temporary callback log field is intentional.
|
kcidev/api.py now imports gzip, but the production implementation does not use it. The tests import gzip locally instead. |
|
Please restrict the Maestro fallback to bare IDs or IDs explicitly prefixed with maestro:. Currently _bare_node_id() strips any origin prefix, so a missing other-origin: Dashboard test could accidentally resolve and return an unrelated Maestro log. Please preserve the Dashboard error for non-Maestro origins and add a regression test for this case. |
|
_job_callback_log() can call _gunzip_iter(), but unlike the normal Dashboard log path it does not catch zlib.error. A malformed gzip callback can therefore leak a raw exception from KernelCIClient.get_log() instead of KciDevError. Please catch zlib.error here and add a malformed-gzip regression test. |
A job that fails before producing results never reaches the dashboard, so get_log could not fetch its log at all: get_log and get_build both return "not found", while Maestro holds it as an incomplete job with its logs attached. Read those instead when the dashboard has no test for the id. The lava_log artifact is a plain gzipped log, so the existing streaming path reads it with the bounds and deadline it already applies. Where there is no lava_log the LAVA callback carries the log as a JSON field, which has to be read whole: that path is capped on both the download and the decompressed size. Sampling incomplete jobs, the two together cover every one that has any log artifact at all, and the pipeline treats the callback's log field as temporary, so it is the fallback rather than the first choice. Only bare or maestro-prefixed ids are looked up. Stripping any origin would let a missing other-origin:<id> resolve to an unrelated Maestro <id> and return the wrong log. Reported-by: Yogesh Lal <yogesh.lal@oss.qualcomm.com> Signed-off-by: Ben Copeland <ben.copeland@linaro.org>
|
All four done thank you, awesome feedback :) |
2b8625d to
b296fca
Compare
2 participants
A job that fails before producing results never reaches the dashboard, so get_log could not fetch its log at all. Reported by @yogeshlal while testing #306 against a triage agent: a node whose device failed to flash returns "not found" from both get_log and get_build, while Maestro holds it as an incomplete job with the LAVA callback attached.
get_log now falls back to that callback when the dashboard has no test for the id, and accepts the id in either form since Maestro takes the bare hex and the dashboard takes it origin-prefixed. The callback is JSON rather than a log file, so it cannot be streamed into a bounded buffer; it is capped on both the download and the decompressed size, with the same deadline as the dashboard path. The response reports which source was used.
get_node already returns the infra diagnosis for these nodes and is usually the better answer; this is for when the raw log is wanted.
Based on #306, which needs to merge first.