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

osbuildmonitor: add new package to monitor osbuild output #1047

Merged
merged 2 commits into from
Dec 5, 2024

Conversation

mvo5
Copy link
Contributor

@mvo5 mvo5 commented Nov 15, 2024

This commit adds support for scanning/parsing the jsonseq based
monitoring that osbuild provides. This is useful for machine
readable status reporting and progress information for e.g.
bootc-image-builder.

The API consists of a new osbuild.Status struct that contains
parsed details about the osbuild status/progress and a scanner
that can scan a stream of osbuild jsonseq based progress and
emit osbuild.Status structs.

Note that the status is broadly of two categories:

  1. information amining at a user interface (like
    status.Message or status.Progress)
  2. low-level details for logging/error reporting (like
    status.Trace and status.Timestamp

Note that error handline a bit weak right now. The consumer
of the API will not get an error via the monitor currently
but only know that something went wrong from osbuild exiting
and then the consumer needs to reconstruct the error log from
the trace messages. This is not a show-stopper but we could
improve this by merging osbuild/osbuild#1831
and adding Result to the statusJSON and Status (but this
needs more thinking and probably best done in followups).

See https://github.com/osbuild/bootc-image-builder/compare/main...mvo5:progress3?expand=1 how this will be used.

Note that the provided information is intentionally small right now,
we will need to go over the osbuild implementation and see if we
should improve things here too (c.f. 1831) but it's a good starting
point I think.

The two main use-cases this should cover right now:

  1. Allow to hide all low-level osbuild output when run on the terminal and show only progress information (e.g. bib and image-builder-cli)
  2. Write a log file from "Trace" lines so that the full build can be easily reviewed or in case of errors reported

@mvo5 mvo5 force-pushed the osbuildmonitor-pkg branch from 1b72324 to f09dee0 Compare November 26, 2024 11:20
@achilleas-k achilleas-k self-requested a review November 28, 2024 16:47
Copy link
Member

@achilleas-k achilleas-k left a comment

Choose a reason for hiding this comment

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

Some small comments from a quick review.

pkg/osbuildmonitor/testdata/osbuild-monitor-output.json Outdated Show resolved Hide resolved
pkg/osbuildmonitor/monitor.go Outdated Show resolved Hide resolved
pkg/osbuildmonitor/monitor.go Outdated Show resolved Hide resolved
pkg/osbuildmonitor/monitor.go Outdated Show resolved Hide resolved
pkg/osbuildmonitor/monitor.go Outdated Show resolved Hide resolved
@achilleas-k
Copy link
Member

[draft until I have integrated this into either ibuilder(image-builder-cli)/bootc-image-builder) to show that it has no gaps, but wanted to show it here for early feedback]

Why not add support for it directly here in osbuild.RunOSBuild?

Note that this function is used by osbuild-composer, so changing it will need changes when it's integrated there, but that is a central point to add support for this I think and it can then be immediately tested with cmd/build.

@achilleas-k
Copy link
Member

Also, since all interfaces to osbuild live in pkg/osbuild/ perhaps we could move this package there as well? Maybe as a subpackage (pkg/osbuild/monitor/) if you think that's cleaner.

@mvo5
Copy link
Contributor Author

mvo5 commented Dec 4, 2024

Also, since all interfaces to osbuild live in pkg/osbuild/ perhaps we could move this package there as well? Maybe as a subpackage (pkg/osbuild/monitor/) if you think that's cleaner.

I like it and it fits well into osbuild - my only small worry would be that the go doc pkg/osbuild output is already very big as it contains all he bindings to the stages. But happy to move it there.

As for the integration with RunOSBuild that is also planned of course but the final shape is not clear in my head, the (simplisitic) way to drive it right now is https://github.com/osbuild/bootc-image-builder/pull/525/files#diff-930ca2eb460ee83e152bc1133ce98074bc7b26bc23a40e66cccb1ffdc5406140R158 but I think it woudl evolve into a "RunOsbuildWithOpts" or something and then a Options struct with keyword like args, the existing interface to run osbuild can probably reduced to take less args with that.

@mvo5 mvo5 force-pushed the osbuildmonitor-pkg branch from 1dfc5d6 to 0d17683 Compare December 4, 2024 10:33
mvo5 added 2 commits December 4, 2024 12:22
This commit contains an abreviated version of an osbuild jsonseq
output log. Most of the repetitive data from the curl and rpm
stages got removed as they don't add (much) to the understanding.

The data was generated via:
```
$ sudo python3 -m osbuild --libdir . --monitor JSONSeqMonitor \
   --export image --output-directory /tmp/out \
   ./test/data/manifests/fedora-boot.json > osbuild-status-output.json
```
This commit adds support for scanning/parsing the jsonseq based
monitoring that osbuild provides. This is useful for machine
readable status reporting and progress information for e.g.
`bootc-image-builder`.

The API consists of a new `osbuild.Status` struct that contains
parsed details about the osbuild status/progress and a scanner
that can scan a stream of osbuild jsonseq based progress and
emit `osbuild.Status` structs.

Note that the status is broadly of two categories:
1. information amining at a user interface (like
   `status.Message` or `status.Progress`)
2. low-level details for logging/error reporting (like
   `status.Trace` and `status.Timestamp`

Note that error handline a bit weak right now. The consumer
of the API will not get an error via the monitor currently
but only know that something went wrong from osbuild exiting
and then the consumer needs to reconstruct the error log from
the trace messages. This is not a show-stopper but we could
improve this by merging osbuild/osbuild#1831
and adding `Result` to the `statusJSON` and `Status` (but this
needs more thinking and probably best done in followups).

See https://github.com/osbuild/bootc-image-builder/compare/main...mvo5:progress3?expand=1 how this will be used.
@achilleas-k
Copy link
Member

achilleas-k commented Dec 4, 2024

I like it and it fits well into osbuild - my only small worry would be that the go doc pkg/osbuild output is already very big as it contains all he bindings to the stages. But happy to move it there.

I've wondered once or twice if it would be cleaner for each stage to be its own subpackage, but I think that's a bit much. Maybe stages, sources, inputs, mounts could be separate though? A decision for another time.

Copy link
Member

@achilleas-k achilleas-k left a comment

Choose a reason for hiding this comment

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

Ship it!

@mvo5 mvo5 added this pull request to the merge queue Dec 5, 2024
Merged via the queue into osbuild:main with commit 5ecf4b5 Dec 5, 2024
19 checks passed
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