Fail on rpki-client errors and surface validation problems#147
Fail on rpki-client errors and surface validation problems#147jorisstrakeljahn wants to merge 4 commits into
Conversation
fjahr
left a comment
fjahr
left a comment
There was a problem hiding this comment.
Concept ACK for the batch failures/return code stuff.
The verbose logging stuff isn't really adding value IMO. We spent quite a bit of time trimming the default logs and these don't seem to be providing enough value to make the cut.
In general, when we throw away stuff like the metrics file or we use stuff only in the debug context currently, it would be better to first ask why that is. If there is a good reason that the info there is more valuable than we initially thought, then these files should be first made not throwaway/debug only anymore before using their contents in the default logs.
|
|
||
| # Healthy runs see well under 1% invalid ROAs; a failing majority | ||
| # means the local trust chain is broken, not the data. | ||
| valid_count = len(data) - invalids - incompletes - not_roas |
There was a problem hiding this comment.
I think this check seems pretty arbitrary, did we see any actual cases where this would have triggered?
| if not counts: | ||
| return | ||
| print("Valid ROAs per RIR: " | ||
| + ", ".join(f"{tal}: {n}" for tal, n in counts)) |
There was a problem hiding this comment.
tal is not the right variable name here, it's just the name of an RIR
|
|
||
| parse_ccr_hashes(result.stdout.decode() if result.stdout else "") | ||
|
|
||
| metrics_file = Path(tmpdir) / "metrics" |
There was a problem hiding this comment.
This file path handling doesn't belong here. We write the metrics file to tmpdir for a reason, you shouldn't copy it back here. If we think it has valuable info we shouldn't write it to temp to begin with. This layer should be generally handled in the context file.
I think this metric is nice to have but I think it doesn't help with debugging. The more realistic failure case is that a certain repository was not available but that doesn't mean that any RIR has no entries here then because repositories can have ROAs of different RIRs. This is data that could be in debugging but really I think it's something that can be analyzed after the fact if there are large mismatches.
I also don't know much about this metrics file other than that it was just very recently introduced. Not sure if we should rely on it generally. Overall I tend to think this should be dropped.
| be global, affecting all participants equally. | ||
| """ | ||
| pattern = re.compile( | ||
| r'rpki_client_ta_objects\{type="roa",state="valid",name="(\w+)"\} (\d+)') |
There was a problem hiding this comment.
It's interesting that rpki-client does the validation right away with the download step but we can't really rely on this because this validation is not fixed to timestamp of the run like we do for the actual validation later. So there will likely be minor differences between the counts which makes them less interesting to show IMO. It's also confusing to the readers of the logs because we would be reporting counts for valid ROAs only to the do the actual RPKI validation step afterwards.
| sys.exit(1) | ||
|
|
||
| if error_counter: | ||
| print(f"Validation reported {sum(error_counter.values())} problems, " |
There was a problem hiding this comment.
I don't think this is valuable. It is known that rpki-client reports lots of errors on every run: https://console.rpki-client.org/ If we think that this is valuable information we allow users to run with debug and then errors can be inspected directly.
|
|
||
| if result.stderr and context.debug_log: | ||
| if result.returncode != 0: | ||
| print("rpki-client validation batch failed " |
There was a problem hiding this comment.
This is different from the error counting and should be in a separate commit. This is worth keeping.
2 participants
Follow-up to the discussion in bitcoin-core/asmap-data#60.
Kartograf currently ignores rpki-client's exit code and drops its stderr unless --debug is set. All the failures we saw degraded silently into plausible looking but wrong maps: an empty cache (69MB result), an all-invalid validation (sipa's 72MB result) and a partial fetch (nkaretnikov's 62MB result in bitcoin-core/asmap-data#51).
This basically just uses more of what rpki-client already gives us: fail when it exits with an error, fail when validation results get lost or the majority of ROAs is invalid, summarize the error reasons from stderr and keep the metrics file with the per-RIR ROA counts (that would have made the partial-fetch case from bitcoin-core/asmap-data#51 visible right away).
A reproduction run over the 1783008000 data with these changes produces the identical result hash (35a4b28f), so the checks are invisible on healthy runs. Complements #146, no overlapping lines.