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

main,osbuildprogress: add --progress=text support (COMPOSER-2394) #525

Closed
wants to merge 5 commits into from

Conversation

mvo5
Copy link
Collaborator

@mvo5 mvo5 commented Jul 4, 2024

This adds a new bootc-image-builder mode that makes use of the
osbuild json-seq progress information. It will display some
user friendly progress information. It can be tested via:

$ cd bib/cmd/bootc-image-builder
$ echo '{}' > config.json
$ go build && sudo ./bootc-image-builder build --progress=text --store /var/tmp/osbuild-store --config config.json quay.io/centos-bootc/centos-bootc:stream9

It looks like this right now:

Building manifest-qcow2.json
Building [|]                                                                    
[4 / 8] step: image [------------------------->_________________________] 50.00%
[7 / 11] module: bootc.install-to-filesystem [---------------->_________] 63.64%
last msg: Starting module org.osbuild.bootc.install-to-filesystem              

It uses chegggaaa/pb/v3 - but there are still open questions about the message output and error handling.

Draft because:

  • error handling is very sketchy, if something in the build fails the user gets a "failed" message but no context, we need to either make sure osbuild progress has a concept of an error and dump the entire output of the stage into this (as errors details tend to be inside the failed stage (this needs some osbuild work)
  • no syncronisation between cmd.Wait() and the progress reader reading output, this also needs to get fixed via e.g. a waitgroup so that both the cmd needs to finish and the stdout read gets an EOF before we continue as otherwise the last progress messages may get lost
  • tests (obviously) but that is tricky for progress, definitely some of the failure handling
  • osbuild currently prints random non-json at the end of a run, we need to fix that or deal with it gracefully
  • Naming of things like pipelines/source and stages
  • Needs main,monitor: fix total steps in progress reporting osbuild#1826 for correct reporting
  1. ... ?

@mvo5 mvo5 requested a review from schuellerf July 4, 2024 17:01
@schuellerf
Copy link
Contributor

As you already mentioned in the comment, I also think that the progress (at least the object itself) should have indefinite levels to be generic.

And the progress should definitely include "Generating manifest"
So basically the progress should start right after entering the application and should not only cover some main parts. (Those should be some sub-progress IMHO)

@schuellerf
Copy link
Contributor

The way I think it should work is similar to a "logger" it should always exist and just be handed over (directly or indirectly/globally)

ID string `json:"id"`
} `json:"pipeline"`
} `json:"context"`
Progress struct {
Copy link
Collaborator Author

@mvo5 mvo5 Aug 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to add "build_result"result here if/once osbuild/osbuild#1831 is merged

Copy link

github-actions bot commented Sep 5, 2024

This PR is stale because it has been open 30 days with no activity. Remove "Stale" label or comment or this will be closed in 7 days.

@github-actions github-actions bot added the Stale Issue or PR with no activity for extended period of time label Sep 5, 2024
@schuellerf schuellerf removed the Stale Issue or PR with no activity for extended period of time label Sep 5, 2024
Copy link

This PR is stale because it has been open 30 days with no activity. Remove "Stale" label or comment or this will be closed in 7 days.

@github-actions github-actions bot added the Stale Issue or PR with no activity for extended period of time label Oct 20, 2024
@mvo5 mvo5 removed the Stale Issue or PR with no activity for extended period of time label Oct 23, 2024
mvo5 added 5 commits November 14, 2024 10:57
This adds a new `bootc-image-builder` mode that makes use of the
osbuild json-seq progress information. It will display some
user friendly progress information.
Switch the progress output to `chegggaaa/pb/v3` and tweak the
progress. Still open questions about the message output and
error handling.
osbuild will send the build result to stdout in a non-json format.
By using the --monitor-fd this won't affect the progress handling.
@ondrejbudai ondrejbudai changed the title main,osbuildprogress: add --progress=text support main,osbuildprogress: add --progress=text support (COMPOSER-2394) Nov 20, 2024
@mvo5
Copy link
Collaborator Author

mvo5 commented Dec 4, 2024

As you already mentioned in the comment, I also think that the progress (at least the object itself) should have indefinite levels to be generic.

And the progress should definitely include "Generating manifest" So basically the progress should start right after entering the application and should not only cover some main parts. (Those should be some sub-progress IMHO)

Thanks, yeah, that is excellent feedback. This version was a draft and mostly to explore the space. I created #743 and osbuild/images#1047 now which are cleaned up versions based on this PR and address your points. I will close this one in favor of the other two.

@mvo5 mvo5 closed this Dec 4, 2024
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