main,osbuildprogress: add --progress=text,debug support (COMPOSER-2394) #743
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mvo5
force-pushed
the
progress3
branch
2 times, most recently
from
d31e3d4 to
c467ef0
Compare
ochosi
changed the title
mvo5
force-pushed
the
progress3
branch
11 times, most recently
from
503f610 to
313b4f7
Compare
|
Moved back to draft as the "pb.Pool" idea does not work with podman as it does not give us a real pty |
This comment was marked as outdated.
This comment was marked as outdated.
ondrejbudai
requested changes
Dec 12, 2024
This commit adds a progressbar interface and implementations: 1. terminalProgressBar based on github.com/cheggaaa/pb/v3 2. plainProgressBar to just pass the message but no progress 3. debugProgressBar to print the osbuild/images status The interface is very tailored to the needs of bib but it should be similar enough to be reusable by `image-builder-cli`. It contains a top level spinner that gives the current step: "Make manifest" or "Building image". The the subprogress from osbuild: the pipelines and the stages from the pipelines as progress bars and the last line is the latest status message, usually something from osbuild like "Starting pipeline qcow2". The spinenr is a bit of a creative choice, it could also be a progress bar because we know the steps (manifest+build). But the runtime is totally dominated by the build step so having a first level progress bar is odd as the UI will spend 90% of the time in it. Plus the spinner gives it more a more "dynamic" appearnce.
mvo5
force-pushed
the
progress3
branch
2 times, most recently
from
33403b4 to
125407a
Compare
mvo5
force-pushed
the
progress3
branch
2 times, most recently
from
c620cd6 to
6c1dd02
Compare
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to no response for status checks
This commit implements enough of `pb.Pool` for our needs without the need to implement raw terminal access. It turns out that by default podman does not connect the full tty, even in `--privileged` mode. This is a sensible security default but it means `pb.Pool` does not work as it wants to set the terminal into "raw" mode and will fail with an ioctl() error when not running on a terminal. However we really just need simple ANSI sequences to render the pool so this seems unnecessary. The initial idea was to just use `--log-driver=passthrough-tty` but that is not available on podman 4.x. (which is part of Ubuntu 24.04 LTS and the GH actions). So this commit just implemnts a custom pool like renderer.
The Start/Stop methods can no longer error so remove the error from the signature. The commit also tweaks the doc strings and comments slightly.
manifestFromCobra generate an osbuild manifest from a cobra commandline and it also takes a progress.ProgressBar as input. It needs an unstarted progres bar and will start it at the right point (it cannot be started yet to avoid the "podman pull" progress and our progress fighting). The caller is responsible for stopping the progress bar (this functtion cannot know what else needs to happen after manifest generation). TODO: provide a podman progress reader to integrate the podman progress into our progress. Thanks to Ondrej for the suggestion to clarify the relationship further.
This commit adds some extra checks to ensure that the pb.ProgressBar does not contain any errors. This should not happen but we want to be paranoid (especially since the error handling of pb.ProgressBar is a bit indirect).
This commit fixes a bug in the progress rendering when no terminal width can be identified. In this case pb.ProgressBar will strip bars with dynamic elements. This is undesirable so when no size can be found we just use the pb.defaultBarWidth (which is not exported sadly). This should get fixed upstream ideally.
The upload code is currently using a plain "pb.ProgressBar" and this is tested as part of our integration test. This will collide with our own progress implementation. For now workaround this by stopping our own progress and let plain "pb" takeover. I was tempted to fix it right away but the PR with the progress is already relatively big so I opted for a followup.
mvo5
commented
Dec 18, 2024
| // *for now* just stop our own progress and let the uploadAMI | ||
| // progress take over - but we really need to fix this in a | ||
| // followup | ||
| pbar.Stop() |
There was a problem hiding this comment.
This is addressed in #763
achilleas-k
approved these changes
Dec 18, 2024
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This adds a new
progressflag that makes use of theosbuild jsonseq progress information to show progress and
hide the low-level details from the user.
Here is what it looks like:
https://asciinema.org/a/cJpB6066UoDDzAylwHVHjEEIN
(c.f. COMPOSER-2394)