use a tempdir instead of the rpki output dir for rpki-client output#143
Conversation
|
This breaks the stable repos flag because, if used, the flag gets appended after the temp dir and then it is ignored (see the download time in the logs, it doesn't download anything, thats why it is so fast). |
…tput Previously, the out/rpki folder was used only for output generated by kartograf, not rpki-client. By adding the context's rpki_out_dir to the command, rpki-client writes its output to it. This isolates all rpki-client output to a temporary directory which is cleaned up automatically after the run finishes. We also add the '-m' flag to the run args, since it limits the output written to the smallest item (metrics).
|
thanks, should have caught that. fixed with 2a983f2 by appending the |
fjahr
left a comment
fjahr
left a comment
There was a problem hiding this comment.
Logic looks good but I would like to see some more documentation here because it doesn't seem intuitive at all why this is being done the way it is. I will do some further manual testing. Then, with the added docs, should be good to go!
| # rpki-client requires a writable output directory as a positional | ||
| # argument. Without one it falls back to a compiled-in default that | ||
| # is not writable in some environments (e.g. nix), which makes it | ||
| # exit before emitting the CCR hashes. |
There was a problem hiding this comment.
Why is this comment removed? It's still valid and valuable information IMO. Maybe also add a similar comment in parse. And I think this could be even more strongly worded that there is nothing written to the folder that we care about, which is why it's ok to use the temp folder here.
There was a problem hiding this comment.
fair, brought it back with 1bf32c2. I duplicated the docs in rpki/fetch.py and rpki/parse.py.
| parse_ccr_hashes(result.stdout.decode() if result.stdout else "") | ||
| with TemporaryDirectory(prefix="rpki-out-") as tmpdir: | ||
| tal_options = [item for path in data_tals(context) for item in ('-t', path)] | ||
| run_args = ["rpki-client", "-m", "-d", context.data_dir_rpki_cache, |
There was a problem hiding this comment.
Not sure if we really need the -m as optimization since we are deleting the files anyway. If you want to keep it, please add a small comment why it's there (also below). For the other parameters it seems intuitive why they are needed but for -m I'm not sure I will remember why it's there in a year ;)
There was a problem hiding this comment.
it just reduces the amount of data we write before deleting, which is an optimization. there's no need to write data we won't need, so it makes sense imo.
| since the default writes several hundred MB of data. The metrics | ||
| data is not used. | ||
| """ | ||
| # Download TALs and persist them in the RPKI data folder |
There was a problem hiding this comment.
nit: This should have been part of the docstring
2 participants
a by-product of aee081e was the introduction of a writable directory explicitly in the rpki-client run command invocation, which now gets all rpki-client output written to it. This directory is intended for kartograf output, not rpki-client. It seems that systems with writable directories (non-Nix) write this data to a default directory managed by rpki-client, and Nix systems don't write the data at all, failing silently.
The better solution is to name an explicit, temporary directory for rpki-client output and pass it to the
runcommand. The temporary directory is cleaned up automatically after the run finishes.The
runcommand defaults to writing-joBcmoutputs, but we don't need the output. We add the-mflag to the run args, since it limits the output written to the smallest item (metrics, ~330K).To summarize: we set an explicit, temporary output directory, we write a tiny bit of data to it, and it gets cleaned up automatically.