Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

progress: display vertex status name #4041

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jedevc
Copy link
Member

@jedevc jedevc commented Jul 20, 2023

So, it seems that VertexStatus.Name isn't actually used anywhere. Originally, I was attempting to try and update the display name for a vertex status, thinking that Name would be the way to go, but that's not plumbed through in the same way.

I'm not quite sure what the original intention of Name was, but it appears like it was probably something like determining how this would get displayed? The issue is, that over time, the ID field for vertex status has been the thing to display (as a name), and isn't entirely used purely as an identifier.

The Name field gets populated (by setting .Action) by the local file transfers, so taking a change like this could mean that progress on older clients would change, so maybe the safest option would be doing a protobuf field number bump on .Name (since it doesn't appear used anywhere client-side as far as I can tell 🤔).

@sipsma I know you worked a lot on progress things, so maybe you have some insight? I poked around to see how dagger handles this and found this similar kind of remapping: https://github.com/dagger/dagger/blob/35da30bbd9452e16f7e68f3d4fdea93a5c3ec2e2/core/bk2progrock.go#L64.

@jedevc jedevc requested review from tonistiigi and sipsma July 20, 2023 10:43
@vito
Copy link
Contributor

vito commented Jul 20, 2023

Chiming in as the one that did the remapping in Dagger/Progrock. I was also confused when I saw there was both an ID and a Name, and Name never seemed to be used, but ID seemed to be used like a Name.

Do we need both? It sounds OK in theory, for the same reason that vertices have an ID and a name, but in practice nothing ever keys on a status ID independently of a vertex, and I don't think you'd ever have duplicate names within a vertex, so having both just seems like cruft to me. But maybe I'm missing something.

@jedevc
Copy link
Member Author

jedevc commented Jul 20, 2023

We don't need both at the moment - however, if we want to be able to change the Name on a VertexStatus during the run, we'd need both. For example, say you want to change a message to say "extracting" instead of "downloading". We'd need some stable way to refer to that VertexStatus so as to change the name, we can do this already with the Vertex.Digest for vertices, but there's no corresponding way to do it for a VertexStatus (without the ID).

@vito
Copy link
Contributor

vito commented Jul 20, 2023

@jedevc Playing devil's advocate: wouldn't it be better to create a new status instead of replacing the name on an old one? That way you can distinguish between the "downloading" and "extracting" duration.

Your point definitely stands if there's still some reason to update the name though. 👍

@jedevc
Copy link
Member Author

jedevc commented Jul 20, 2023

@vito you're definitely not wrong 😄 Though then, you have to have two separate statuses ( taking up two lines), even if it's "one operation".

e.g. imagine downloading and unpacking a set of artifacts, or layers in an image, etc - having to have double of all of those is a bit tricky, and then takes up a lot more space than wanted.

@jedevc jedevc force-pushed the progress-use-status-name branch from 9f4160f to ba78154 Compare August 17, 2023 10:01
@jedevc
Copy link
Member Author

jedevc commented Aug 17, 2023

Did some more updates for this - TL;DR, I think we should completely get rid of the old progress.Status.Action property (regardless of whether we take this PR) - they're not used to display anything in the progress UI, and seem to be left over from some previous refactor. I've split this removal out into 985b247.

I've then added a new Name property in (with a different proto id), that we can use to actually be able to change the vertex display. This is what we can use to change VertexStatuses displays - for example, to change from "downloading" to "extracting" in a per-layer view.

With this change, we could change:

 => resolve image config for docker.io/docker/dockerfile:1                                                                                                   5.1s
 => docker-image://docker.io/docker/dockerfile:1@sha256:ac85f380a63b13dfcefa89046420e1781752bab202122f8f50032edf31be0021                                     0.9s
 => => resolve docker.io/docker/dockerfile:1@sha256:ac85f380a63b13dfcefa89046420e1781752bab202122f8f50032edf31be0021                                         0.0s
 => => sha256:9d9c93f4b00be908ab694a4df732570bced3b8a96b7515d70ff93402179ad232 11.80MB / 11.80MB                                                             0.7s
 => => extracting sha256:9d9c93f4b00be908ab694a4df732570bced3b8a96b7515d70ff93402179ad232                                                                    0.1s
 => [internal] load metadata for docker.io/library/ubuntu:20.04                                                                                              1.3s
 => [1/4] FROM docker.io/library/ubuntu:20.04@sha256:33a5cc25d22c45900796a1aca487ad7a7cb09f09ea00b779e3b2026b4fc2faba                                        1.8s
 => => resolve docker.io/library/ubuntu:20.04@sha256:33a5cc25d22c45900796a1aca487ad7a7cb09f09ea00b779e3b2026b4fc2faba                                        0.0s
 => => sha256:edaedc954fb53f42a7754a6e2d1b57f091bc9b11063bc445c2e325ea448f8f68 27.51MB / 27.51MB                                                             1.1s
 => => extracting sha256:edaedc954fb53f42a7754a6e2d1b57f091bc9b11063bc445c2e325ea448f8f68                                                                    0.5s

into

 => resolve image config for docker.io/docker/dockerfile:1                                                                                                   5.1s
 => docker-image://docker.io/docker/dockerfile:1@sha256:ac85f380a63b13dfcefa89046420e1781752bab202122f8f50032edf31be0021                                     0.9s
 => => resolve docker.io/docker/dockerfile:1@sha256:ac85f380a63b13dfcefa89046420e1781752bab202122f8f50032edf31be0021                                         0.0s
 => => done sha256:9d9c93f4b00be908ab694a4df732570bced3b8a96b7515d70ff93402179ad232 11.80MB / 11.80MB                                                        0.7s
 => [internal] load metadata for docker.io/library/ubuntu:20.04                                                                                              1.3s
 => [1/4] FROM docker.io/library/ubuntu:20.04@sha256:33a5cc25d22c45900796a1aca487ad7a7cb09f09ea00b779e3b2026b4fc2faba                                        1.8s
 => => resolve docker.io/library/ubuntu:20.04@sha256:33a5cc25d22c45900796a1aca487ad7a7cb09f09ea00b779e3b2026b4fc2faba                                        0.0s
 => => done sha256:edaedc954fb53f42a7754a6e2d1b57f091bc9b11063bc445c2e325ea448f8f68 27.51MB / 27.51MB                                                        1.1s

As a layer moves between "downloading", "extracting" and "done" stages, we can change the message. This halves the number of lines needed to display layer downloads, which could be incredibly useful for large builds.

@jedevc jedevc marked this pull request as ready for review August 17, 2023 10:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants