feat(upgrade): Add total progress bar and JSON output #921
Conversation
|
Here are previews of the outputs
|
|
Well there are spurious prints on both stderr and stdout, so there is that. Deployment spinner and GLib errors are also stderr. |
|
Thanks for working on this! I've been oscillating a lot on API/RPC frameworks in #522 "jsonl" is yet another option there that is more targeted just for progress reporting (which is indeed mainly what we need now). But - but - if we do that I'd really like to survey the field and see whether there are any other similar progress APIs. For example, I think this API right now would constrain us to fetching a single layer at a time ; we could add a layer identifier to the metadata for example? Basically what we include in jsonl should be sufficient to render a "multi-progress" bar. |
|
This PR has some issues right now with spurious prints. I will try to fix them today and add different stages for deploy steps. Incl. Fixing the failed test As far as multilayer reporting goes, the way I did this PR is for overall progress so it does not make a difference how many layers are being downloaded at a time. The syntax could be extended with a layers dictionary in the future though. |
|
Ok, tried to fix spurious prints. Seems to be impossible. Since println! and eprintln! are used liberally, the functions cannot be dual purposed to json output. Other than piping stdout to stderr with |
|
Right, we should have e.g. |
|
Before we go too much farther on this can you give a read and comment on #522 too please? Specifically WRT "jsonl" vs varlink vs gRPC and your use case. |
lib/src/deploy.rs
Outdated
| @@ -45,6 +46,17 @@ pub(crate) struct ImageState { | |||
| pub(crate) ostree_commit: String, | |||
| } | |||
|
|
|||
| /// Download information | |||
| #[derive(Debug, serde::Serialize)] | |||
| pub struct JsonProgress { | |||
There was a problem hiding this comment.
In combination with my suggestion of having a mod jsonl I think this struct would go there. Just to future proof things a bit more I'd like to try to make this a little bit more abstract so that it could handle slightly more generic tasks.
For example, a fsck like operation wouldn't have "done_bytes" and "download_bytes" at least in that same sense, but it'd be good to have support for a similar progress bar for it.
In rpm-ostree we have such a generic progress API (over DBus), it's a bit clunky but see https://github.com/coreos/rpm-ostree/blob/f4fa2c5fe78774775ef3ce4f27d4199fac48f7a9/rust/src/console_progress.rs#L20
Basically I think we can structure this so clients can view progress data in a progressively more complex fashion; if they don't care about the details for an operation then can just take a simple "n/total" numbers and that could be pretty easily reused for an operation like fsck.
There was a problem hiding this comment.
I am usually using interpreted languages, so I can distinguish between different JSON objects based on "stage". However, if that is not the case for something like rust, then this struct would need to go into another struct as a union on the bottom.
There was a problem hiding this comment.
In Rust serde has good support for most JSON enum conventions: https://serde.rs/enum-representations.html
|
Go ahead and push |
|
OK, I pushed a minor cleanup - stepping out for the day and I am in theory out the rest of this week, so releasing my mutex on this one. Overall...I think my disposition is to just get this jsonl stuff in and we can sit on the larger IPC question for later. One thing I'd still really love to know though: is anyone else out there doing something like this (specifically jsonl for progress bars?). Oh hum, there's already a crate for this and it has a fair number of reverse dependencies. That's encouraging. Only briefly skimming the source to one of them, this code looks quite related. Like ideally we share a "schema" with an existing project here or at least something similar. (Maybe a good way to think of it is: could the schema we invent here be sufficiently generic that one could use it for e.g. |
|
Neither the format or the concept behind this are particularly novel. The combination perhaps is. Jsonlines is widely used in machine learning because it is dead simple, to the point it is not worth using a library for (unless performance is required), and supports streaming and compression due to it having separators very often. Binding JSON to a UI is pretty standard data-binding too (e.g., look at React, and especially backends such as Redux). I think there is a bit of a rough edge due to the fact that cli commands are used together with json, so those two words are mixed. If you want some ideas for IPC, when making hhd, I took the json idea a bit to the extreme, and did server side rendering over json, using a set of pre-defined components (the spec is here). Essentially, the That document was last updated in Dec. I think one month into making hhd and it is almost a year later now. There have been no breaking changes to that API since then, so I'd say the idea works pretty well. There are around 20x more settings now, and the project is 5x the size. The downside of that API is that it decides how the UI should be structured, and it makes it hard to interact with it over cli. But since the UI works so well now nobody asks for that. As for the UI, the only updates it has are to add stylized components (here). It is actually quite nice to be able to add features without having to update the UI(s). I will try to work through this PR this week and I think we will try to deploy this PR soon too. Just the progress bar itself is a nice QOL. |
lib/src/progress_jsonl.rs
Outdated
| pub downloaded: u64, | ||
| pub size: u64, | ||
| /// "cached", "waiting", "downloading", "done" | ||
| pub status: String, |
There was a problem hiding this comment.
This should be an enum
|
Turns out the mutex was needed after all. Progress reader spins a thread so it consumed the writer Added deployment stages as well: Inbetween these two lines there is a very suspicious 10-40s gap. So something needs to be done there progress wise. |
Looks a lot more granular and nicer now. Corruption bug still there. |
|
Hmm, I think "add update --check json" is scope creeping this; it relates to #472 (comment) Let's keep this one to what isn't in |
I guess I was not aware of the following: Sure, we can strip that commit |
|
Let me take this one back for a bit |
I'm not getting that one here...that's quite worrying. Can you try to get a core dump + stack via e.g. |
|
In this stateless model, how would this look when we transition from e.g. pull to commit to deploy type operations? I guess a renderer would detect that Now we have an enum with one element; I guess that gives us some future extensibility but not obvious to me what we'd add there.
Reference? The python subprocess
Hmm. OK, so we should definitely have a small demo python parser for this executed as part of the test suite. |
Task and id would change. Yes, the caller would assume they completed. Then, the caller could hardcode a ratio between the pull task and the deploy task to show a single progress bar. Or the caller could show two different progress bars.
As for deploy, we can add a But I did finish the rest of the updater. It includes checking for updates, updating, canceling updates, rollbacks, rebase to different branches and for each branch pinning one of the last 5 versions, undoing the pin, and detecting when rpm-ostree incompatibilities exist and offering to do rpm-ostree reset
Pass_fds is optional anyway. https://docs.python.org/3/library/pipes.html Yeah,
I did not use python. I was trying to use bash with it just to see the output by eye. The previous implementation with |
So it's possible now that an entire "task" could be skipped in theory if it happened fast enough. I guess it's a bit of a corner case: do we care about showing an 100% complete bar if the task happened fast enough? I feel like this would matter a little bit more if we changed the default TTY renderer to consume this stream? But...eh I can't claim this is a serious problem either, I just still find myself leaning towards the previous schema. |
I guess so, well, i can partially undo the change and fire the last message as required. This seems like a sane choice. |
Yeah that'd help. Actually I started to write tests for this and that's where the lossiness could definitely show up as a flake. Always emitting a message when changing tasks seems good enough probably. |
|
I still cannot get it to work still. I thought it was inheritability, indeed
import subprocess
import os
proc = None
try:
r, w = os.pipe2(0)
proc = subprocess.Popen(
[
"sudo",
"/tmp/bootc",
"switch",
"ghcr.io/ublue-os/bazzite:unstable",
f"--json-fd={w}",
],
)
with os.fdopen(r) as f:
while True:
line = f.readline()
if not line:
break
print(line, end="")
proc.wait()
except KeyboardInterrupt:
if proc:
proc.terminate() |
|
You need to provide the |
|
I tried both individually. You are right, it needed both inheritability and pass_fds together |
|
I readded staging and import as separate stages. Then fixed up the script and made it close correctly after the image is merged. import subprocess
import os
proc = None
try:
r, w = os.pipe2(0)
os.set_inheritable(r, True)
proc = subprocess.Popen(
[
"/tmp/bootc",
"switch",
"ghcr.io/ublue-os/bazzite:testing",
f"--json-fd",
str(w),
],
pass_fds=[w],
stderr=subprocess.DEVNULL,
)
# Close fd in parent process
os.close(w)
with os.fdopen(r) as f:
while proc.poll() is None:
line = f.readline()
if not line:
break
print(line, end="")
proc.wait()
except KeyboardInterrupt:
if proc:
proc.terminate() |
|
I fixed the style to match And I think its ready to fork and use for my usecase/it's getting close to be mergeable. |
|
And here it is bootc_update.webm |
|
Juggling a whole lot here but I should be able to circle back to this one by end of the week. One thing that would be helpful (that I was planning to do, to be clear) is add integration testing for this in our tmt-based tests (the current unit test is too synthetic). If you or someone else wants to jump on that it'd help, but again since the current test suite is a bit bespoke and a bit of an investment to learn I was planning to do it myself (also to help get familiar with the code). The other thing we probably should do is have a basic doc for this in |
|
No rush from my side, i pinned the current hash on a different branch so that we can deploy that. I can try to work a bit on the docs later in the week but I am also busy this week so I do not think I can jump on the tests. |
|
Just a quick drive-by/sidenote - in osbuild we had a similar discussion and have a similar json based progress/status format, we went with jsonseq though mostly because there is https://datatracker.ietf.org/doc/html/rfc7464 which defines it. But it seems there is no clear "standard" in the json streaming world so jsonn is a fine choice too. |
|
@mvo5 so essentially it is the same here but we prepend with 0x1E? It is something to consider. I will leave the choice up to Colin. |
Yes, that is my understanding too.
Yeah, I have no strong opinion either way, they are all so similar, we were on the fence flipped a coin (well, picked it because ofthe rfc) |
|
The low level protocol aspect ties into the higher level question of whether there's any way we could share a schema for the actual JSON, I'd love to be able to do that. I mean if we land this as is in bootc, surely at some point we'd end up bridging the bootc json progress into an osbuild progress? Is the osbuild progress API stable or could we change it? On the question of jsonl vs jsonseq, well...you know what's kind of maddening honestly is that varlink chose to terminate with a NUL byte instead. Honestly none of these are really difficult to handle... jsonl seems maybe more widely used because it's just so simple. But I would say IMO varlink's NUL terminator is actually the most obvious here and even though it's not a RFC, it is at least used. So umm...I dunno, I guess if I had to pick between all 3 of these things I'd say let's go with the varlink style NUL. But ultimately again it's a less interesting question than trying to share a progress schema right? |
|
I'd rather stay with jsonseq or jsonl. Both are separated with \n so they can be readlined Then jsonseq has the extra delimiter at the start that can be skipped NUL would make a lot of file handlers close |
I don't think that's true, do you have a reference? Go and Rust both have nice high level APIs for this. I am sure there is something in Python but if it exists in the stdlib I couldn't find it in a quick search (or query with a LLM)
Anyways though, yeah I don't think this matters hugely, I am also totally fine with jsonl or jsonseq. Though AFAICS nothing in the serde_json docs actually guarantees it won't emit a literal newline so maybe that's an argument for jsonseq (or trying to add that guarantee to serde_json). |
I got confused, EOL is different. But in any case, sticking jsonseq still appends |
|
@mvo5 I'm looking at osbuild/osbuild@a1eaf3d#diff-0d30cb1d123e195a69ca1c376d6493f22171312db57f78f468d1481b3bfbaa06R93 and thinking about this. It looks like in that schema there's no explicit separation like we have now between "byte downloads" vs "steps"...that seems desirable here. Something like an optional "disposition": "bytes" maybe? It also looks like there's no "total number of bytes/steps"...one could hack that by I guess just creating progress entries for them too that stay at zero, and encourage UI renderers to not show progress bars for steps that are still at zero? |
This adds a generic "progress" infrastructure for granular incremental notifications of downloading in particular, but we may extend this to other generic tasks in the future too. Signed-off-by: Antheas Kapenekakis <[email protected]> Signed-off-by: Colin Walters <[email protected]>
|
Just rebased 🏄 (to get CI fixes from git main) and squashed ⏬ (because I tend to prefer a relatively lean "git log" even though renovate ruins it). |
Adds a total progress bar to the progress output of switch and upgrade that also shows time remaining.
Then, uses the total progress bar to allow sending a jsonl output to stderr that can be read by userspace software. Has to be stderr, as bootc has some spurious prints on stdout. Progressbar is hidden as it writes to stderr.
Progress bar styling needs some cleanup. Jsonl output needs a type arg to identify the type of output, so that there can be other jsonl jsons for e.g., deployment.