Additional progress info when pulling images via libpod endpoint #14886
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jmguzik The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
jmguzik
force-pushed
the
progress
branch
5 times, most recently
from
3a22ea3 to
7a0a278
Compare
There was a problem hiding this comment.
That doesn't address the issue, unfortunately. There is much more needed to get the real pull progress. In it's current form the PR just prints the same lines multiple times, see
./bin/podman-remote pull nginx
Resolved "nginx" as an alias (/home/vrothberg/.cache/containers/short-name-aliases.co
Trying to pull docker.io/library/nginx:latest...
Getting image source signatures
Copying blob sha256:fe0ef4c895f5ea450aca17342e481fada37bf2a1ee85d127a4473216c3f672ea
Copying blob sha256:fe0ef4c895f5ea450aca17342e481fada37bf2a1ee85d127a4473216c3f672ea
Copying blob sha256:f4407ba1f103abb9ae05a4b2891c7ebebaecab0c262535fc6659a628db25df44
Copying blob sha256:f4407ba1f103abb9ae05a4b2891c7ebebaecab0c262535fc6659a628db25df44
Copying blob sha256:935cecace2a02d2545e0c19bd52fe9c8c728fbab2323fc274e029f5357cda689
Copying blob sha256:935cecace2a02d2545e0c19bd52fe9c8c728fbab2323fc274e029f5357cda689
Copying blob sha256:4a7307612456a7f65365e1da5c3811df49cefa5a2fd68d8e04e093d26a395d60
Copying blob sha256:4a7307612456a7f65365e1da5c3811df49cefa5a2fd68d8e04e093d26a395d60
Copying blob sha256:8f46223e4234ce76b244c074e79940b9ee0a01b42050012c8555ebc7ac59469e
Copying blob sha256:8f46223e4234ce76b244c074e79940b9ee0a01b42050012c8555ebc7ac59469e
Copying blob sha256:b85a868b505ffd0342a37e6a3b1c49f7c71878afe569a807e6238ef08252fcb7
Copying blob sha256:b85a868b505ffd0342a37e6a3b1c49f7c71878afe569a807e6238ef08252fcb7
What needs to be done is discussed in the issue, see #12341 (comment) and other comments.
|
Nevermind my comments, I think I know what you mean :) |
|
@vrothberg Is that kind of output you are looking for? Progress channel emits properties anyway, so I reconstructed the message in place. |
|
@jmguzik it does not work as you may expect it to. The progress for a given blob is printed multiple times (see your example above). As #12341 (comment) suggests, we had to do plumbing in c/image. The progress bars for the local/native client are rendered in the containers/image library. To achieve the same progress bars on the remote client, we had to refactor the progress-bar logic, make it usable for callers outside of c/image and then do the plumbing for podman's remote API/client. It's quite some work. Cc: @mtrmac |
|
@vrothberg I have no problem with some work (a matter of time only) but I have to understand it first. I have a problem with that. Do you expect curl output to be replaced in the process? Is something like this even possible after flushing? Let's forget about podman-remote because I haven't checked things there really. Let's focus how it should look like via REST. |
I pretty much shared everything I know:
I am certain Docker does not print the lines redundantly.
In order to render progress bars we need to know a number of things:
I think the Docker-compat endpoint does parts of that already, so it may be worth checking out what's happening there. I'd also have a look at what Docker does exactly. Thanks a lot for looking into this, @jmguzik, I appreciate your help! |
I already did that, before jumping into the code. And in the podman<->docker compat layer its more or less happening the same as what is happening in my PR (BTW I can add the same progress bar as it is in docker to compat, it's missing): I think
I am using
No problem @vrothberg , the only constrain is time :) |
My purely personal opinion is that as long as we don’t make any promise of output stability, we don’t quite need to insist on this part. That said, if we were going for this, it’s an interesting question whether the MPB-drawn progress bars can be streamed over the network. It might actually be easier to do that than to extend |
@mtrmac I have never seen the possibility of sending via REST API stream of TTY-like output. Docker is sending json properties and progressBar as a string. A true REST expert would need to answer this question.
I think
I started the issue with the purpose of changing the REST API endpoint because the issue was originally stated like that. That is why I did not fully understand at the time all comments about |
|
@vrothberg it is still WIP, but I think it's the best I can do at the moment: I used an implementation similar to docker-cli, which is a message reconstruction. There is no clear way to send MPB progress bar via json. I used |
|
The code is not ready yet, but before polishing I need to ask if that output will be accepted. |
|
We cannot merge it as is as it's screwing the output of |
To consult new proposed output please see attached media: https://user-images.githubusercontent.com/78746728/178688033-5c217140-b1eb-4134-973e-6530059bac32.mp4 |
| @@ -36,6 +38,7 @@ func ImagesPull(w http.ResponseWriter, r *http.Request) { | |||
| AllTags bool `schema:"allTags"` | |||
| PullPolicy string `schema:"policy"` | |||
| Quiet bool `schema:"quiet"` | |||
| Progress bool `schema:"progress"` | |||
There was a problem hiding this comment.
The new parameter needs wiring in pkg/api/server for the Swagger docs and wiring for podman-remote (and pkg/bindings).
There was a problem hiding this comment.
Wait, wait, it's old code :)
There was a problem hiding this comment.
I am just trying to consult output. Changes are mine local for now.
There was a problem hiding this comment.
I will have a new param though, so these updates will be needed.
This looks great! |
|
Still, WIP, pushed the general idea though. |
jmguzik
changed the title
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
There was a problem hiding this comment.
I think we should take a step back and define what the goal is in this PR. The design/outcome has changed quite a bit.
I see two issues we should solve, probably in separate PRs.
1) Progress data
We already chatted about whether the PR solves #12341 or not. If non podman-remote users want to be able to render progress, then this PR is not enough. In order to achieve that goal, sometthing similar to types.ProgressProperties could be part of the returned status. To remain backwards compatible, returning such data should be opt-in via a new parameter. Users are then free to interpret the data as they want to.
2) Rendering progress in podman-remote
Currently podman-remote does not render any progress but merely prints a line indicating that a a blob/config is being copied. To properly render progress, we could use the progress data of 1) OR we could explore whether it's possible to render the progress bars from c/image directly over the wire as suggest by @mtrmac in #14886 (comment).
I prefer to tackle the two issues separately.
| @@ -10,6 +10,7 @@ import ( | |||
| "github.com/containers/podman/v4/pkg/inspect" | |||
| "github.com/containers/podman/v4/pkg/trust" | |||
| "github.com/docker/docker/api/types/container" | |||
| "github.com/docker/docker/pkg/jsonmessage" | |||
There was a problem hiding this comment.
Let's copy the type over here. This way we have full control and avoid potential silent regressions.
There was a problem hiding this comment.
@vrothberg what do you mean by copy? Move it to podman, the whole file?
There was a problem hiding this comment.
Only the type(s) and methods that are needed.
There was a problem hiding this comment.
It is also used in compat API. Are you sure you want to move and leave there untouched?
There was a problem hiding this comment.
Yes, thanks for double checking. The reasoning for the compat API is to remain compatible and follow the types.
For libpod, we should control the types.
| case e := <-progress: | ||
| switch e.Event { | ||
| case types.ProgressEventNewArtifact: | ||
| report.Status = "pulling fs layer" |
There was a problem hiding this comment.
I prefer keeping the status empty to be closer to how the local client looks like.
| // Progress contains detailed information about download progress | ||
| Progress *jsonmessage.JSONProgress `json:"progressDetail,omitempty"` | ||
| // Status contains a short progress description | ||
| Status string `json:"status,omitempty"` |
There was a problem hiding this comment.
In it's current form, I consider this to be a breaking change. Previously, all data was part of Stream. Now the data is scattered across Stream and Status. If a podman-remote prior to this change pulls from the new service the output looks as follows:
^Cpodman (main) $ ./bin/podman-remote rmi -af && ./bin/podman-remote pull nginx
Resolved "nginx" as an alias (/home/vrothberg/.cache/containers/short-name-aliases.conf)
Trying to pull docker.io/library/nginx:latest...
Getting image source signatures
Copying blobCopying blobCopying blobCopying blobCopying blobCopying blobCopying blobCopying blobCopying blobCopying blobCopying blobCopying blob
Copying blobCopying blobCopying configCopying configWriting manifest to image destination
Storing signatures
41b0e86104ba681811bf60b4d6970ed24dd59e282b36c352b8a55823bbb5e14a
That means, unless we find a backwards compatible way, rendering the progress must be opt-in. Older clients must continue working with newer servers (unless the major versions differ).
That can be easily done. I can do that in this PR. I considered the previous approach to be a bug and did not take into account old clients.
I do not get the second comment. I implemented the progress bar in |
Awesome, thanks! Let's discuss which data is needed. Shall we just include the entire ProgressProperty from c/image? I think all of the data in the struct can be of use for clients. @mtrmac @jwhonce @rhatdan WDYT?
Can you elaborate? I think that it's possible when hijacking the connection, similar to what we're doing in attach. |
So, I don't know anything about the hacky method of hijacking connection (is it still REST?). How then present the output to pure REST API users? I can elaborate on what is actually implemented right now and I think it adds nicely to the data gathered from
Just as a side note @vrothberg I started this feature as only targeting REST API, I did not take into account |
It's implemented in this PR as described above. Have you tried compiling this branch to see and used associated |
Yes, I tried. It looks much better than the status quo but I am not yet sure that's what we should do. Once we explored the idea of hijacking the connection and let c/image render, we can check again. Focusing on 1) only in this PR will be a huge leap forward. |
@vrothberg maybe it's not an argument for you, but docker also has a hijacking connection for attaching and it's still using the presented method to do the push/pull data manipulation. I also had the feeling that we applied OR condition here so I have chosen something I believe it's better suited for this use case. I consulted the output and thought it was OK. Due to a lack of time, I won't be able to pursue a "hijacking" connection. It is too big a feature for me to handle and it may result in returning to this PR (docker as an example). I can leave it for somebody else to explore. My idea for image push was the same so I would also unassign myself from the issue. From my point of view I would need some information from maintainers/approvers because I am confused about what to do next:
|
|
|
I think this PR should implement 1) but 1) and 2) are/can be conceptually different things.
If we stick with 1) However, 2) is something a user would face. I totally understand the desire to do what Docker does and render the progress bar client side. Docker had a client-server architecture from the very beginning that Podman had not for a long while. Hence, the progress bars are rendered server side. We could for sure go hybrid and have both but that comes at the cost of code duplication which is painful long term when adding new features or fixing bugs. Hence, I think we have the choice between a) consolidating the progress-bar code in c/image/copy to something for outside callers OR b) hijacking the connection. In this case, b) seems much more attractive to me. |
@vrothberg no problem, so I would leave the code here and close this PR. I leave a comment in the issue for somebody else to continue so that he/she knows the progress was made but options a), b) is needed to be explored. Regarding 1) if you make a decision with @mtrmac @jwhonce @rhatdan I can open a new PR and do this work, but I am skeptical about the structure. I think there is no problem with doing something similar to |
This will ultimately lead to this solution since you still need to reconstruct progress from the structs. Going this way there is no different outcome. |
Assuming “hijacking” refers to fundamentally changing the nature of the TCP socket, is that actually necessary? The start of the issue quotes a sequence of JSON objects in the response. So, couldn’t we just add one more type, e.g. freely intermixed with the existing output? Is that a compatibility problem because old clients couldn’t handle that, or something? (That said, streaming TTY escape sequences is not trivial either. One has to worry about $TERM, window sizes, and so on. And Cockpit and the like would definitely benefit from well-structured data much more than about old vt100-like fixed-column output.) |
Right now, c/image is producing the typed data and the progress bars using somewhat unrelated code paths. Changing that seems conceptually attractive, but it’s not something that exists and can just be made public for |
That's how attach deals with the terminal issues among other things. Maybe there is an alternative that I don't know.
Would that render the progress bars?
Yes, that would break compatibility in the sense that it'll screw the output for older clients. That's why I think it should be opt-in - which is an easy and cheap "get free out of fail" card.
I concur. |
I still wonder why docker people did not follow that. They also have attached, yet they are not using the method for streaming the progress bar. Unfortunately, tracing the rationale would take some time, which I can't spend 😞 |
I assume because the initial architecture of Docker has always been a client-service model. I think it's much more attractive to do it the Docker-way from a purity/design point of view. But we have to balance the cost-benefit ratio where "hijacking" seems more attractive to me than a potentially long rewrite of a number of code paths. |
I found a few moments to look at attach. IMHO adding it here to support progress bar use case seems like over engineering. The presented change will not result in rewriting many paths and it will keep backwards compatibility with no additional arguments, while hijacking seems to not be the same in that regards, adding additional complexity only to get progress bars.
Is it really worth to merge them since there is possibility to handle them both? If the statement is to support hijacking than I do not understand how it would change the situation. If it's a general comment I don't think it's worth investing the effort. Again, it's my opinion and i may not know everything 🙂 |
|
(I find hijacking unattractive. We can just extend the existing JSON, for any kind of data, AFAICS. We are not contemplating an interactive TTY emulation where we would need to also support passing input to the server.)
That rather depends on how strongly Podman insists on keeping exactly the same progress bar output for local and remote. I don’t think the consistency is actually that valuable (the network might be slow and high-latency, so the remote case should. IMHO, optimize of less frequent updates over elegant animations [the default is 6.6 Hz]), but if the desire is to stay consistent, the only truly maintainable way to do that is to only have one implementation that draws the progress bars shared between the local and remote cases, and one shared implementation implies one shared format of data input. |
|
FWIW, as a requester of this feature, I don't think it's necessary to keep the exact same progress bar output. From my staindpoint, the progress output is for interactive, human-readable sessions, and I just want to see the general progress and make sure things aren't stuck. I don't see why anyone would write scripts around the progress bars. Scripting should be done via return codes and formatted output like JSON. |
+1
I actually looked from my user perspective. I want probably progress for bigger blobs. Whether the progress bar will fly slowly or quickly (in a matter of frequency) who cares. I know I am using I just think either merging two signals into one in c/image and/or doing hijacking for a simple progress bar seems overengineered approach. It's my personal opinion after spending some time around that feature as sniffing around what is implemented in docker. I also do not see the argument of hijacking being maintainable. I looked at the code and it's not easy to digest at first glance, while simple output manipulation is (unit testing is actually implemented there to check consistency and can be generalized to serve both push/pull). |
|
@mtrmac @vrothberg I just want to ask after some time if you had some internal talks to design doc the approach or if any other progress was made. |
|
Thanks for reaching out, @jmguzik. I'd feel more comfortable if we had a design doc first. |
|
@vrothberg just found again this issue while searching for something else. I thought I just ping to ask if there is any recent development around version choosing or design doc for this one. |
Thanks for reaching out. I fear that there is no development on this issue yet. |
github-actions
bot
added
the
locked - please file new issue/PR
/fixes #12341
Does this PR introduce a user-facing change?