Refactor remote pull to provide progress#7647
Refactor remote pull to provide progress#7647openshift-merge-robot merged 1 commit intocontainers:masterfrom
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jwhonce The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@vrothberg This moves remote closer to abi, but the progress bars are still off. Do you have experience with why they would be different? |
|
@jwhonce you need to rebase on master to fix CI. My fault, sorry. |
cmd/podman/images/pull.go
Outdated
There was a problem hiding this comment.
Unrelated to this PR: I believe pull can/should support --authfile.
I'll fetch the PR and have a look |
Got it. We have a check in containers/image to not render the progress bars if the provided writer is not a TTY. The reasoning was to prevent verbose and ugly logs. In such cases, we only print a single line indicating an operation. I will play a bit with c/image and see what we can do. I see it as an extension to this PR that we can tackle later. |
There was a problem hiding this comment.
Just a couple of nits.
Note that rendering progress bars as the local client could be a longer journey:
- The progress bars are rendered by containers/image (i.e., the backend) and we don't have a notion of a front end.
- We could hijack the connection as we do with attach and allow to control the client terminal.
- Or we do some extensive rewrite in c/image, and allow the rendering on the client side (via progress messages from the server).
I think keeping the output as is may be the best option at the moment. Hijacking seems overly complex and I don't think we have enough resources to break the progress bars out of c/image.
podman and podman-remote do not exactly match as the lower layer code checks if the output is destined for a TTY before creating the progress bars. A future PR for containers/images could change this behavior. Fixes containers#7543 Tested with: $ (echo '# start'; podman-remote pull nginx ) 2>&1 | ts '[%Y-%m-%d %H:%M:%.S]' $ (echo '# start'; podman pull nginx ) 2>&1 | ts '[%Y-%m-%d %H:%M:%.S]' Signed-off-by: Jhon Honce <jhonce@redhat.com>
41de036 to
222cf74
Compare
|
Restarted what looked like a flake in the ubuntu job |
|
/lgtm |
- pause test: enable when rootless + cgroups v2 (was previously disabled for all rootless) - run --pull: now works with podman-remote (in containers#7647, thank you @jwhonce) - various other run/volumes tests: try reenabling It looks like containers#7195 was fixed (by containers#7451? I'm not sure if I'm reading the conversation correctly). Anyway, remove all the skip()s on 7195. Only time will tell if it's really fixed) Also: - new test for podman image tree --whatrequires (because TIL). Doesn't work with podman-remote. Signed-off-by: Ed Santiago <santiago@redhat.com>
Fixes #7543
Tested with:
$ (echo '# start'; podman-remote pull nginx ) 2>&1 | ts '[%Y-%m-%d %H:%M:%.S]'
$ (echo '# start'; podman pull nginx ) 2>&1 | ts '[%Y-%m-%d %H:%M:%.S]'
Signed-off-by: Jhon Honce jhonce@redhat.com