many: deal with incorrect status from osbuild better #827
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
This commit changes the progress parser (again) to deal with errors from the osbuild json progress scanner. On errors we will now exit right away and potentially kill osbuild but provide an error message that hints how to workaround the issue. The original code assumed we get transient errors like json decode issues. However it turns out that this is not always the case, some errors from a bufio.Scanner are unrecoverable (like ErrTooBig) and trying again just leads to an endless loop. We can also not "break" wait for the build to finish because that would appear like the progress is broken forever and we would still have to report an error (just much later).
This commit ensures that we do restrict the memory of the bib test conatiner to catch excessive memory usage. This is prompted by a memory leak when dealing with unrecoverable status messages that lead to failures in konflux.
mvo5
commented
Feb 7, 2025
| defer func() { | ||
| // ensure osbuild is stopped even if we exit early | ||
| if cmd.Process != nil { | ||
| cmd.Process.Kill() |
There was a problem hiding this comment.
This may need a bit more care, maybe only do it in hte error case (as the happy case does a cmd.Wait already so no killing needed)
There was a problem hiding this comment.
This seems fine as is but I would strongly recommend using PR_SET_PDEATHSIG if it's not already in use. That's a kernel-enforced way to lifecycle bind the child to the parent, such that if e.g. the parent dies due to SIGSEGV or whatever, the child gets killed too.
This commit changes the centos-9 test to run with `podman -t` so that we have a test-case that uses the `terminal` progress. This is prompted by: a) we have no integration test currently that uses the terminal progress for the full build b) a konflux failure/memory leak that showed because there the test is run with `-t`
cgwalters
reviewed
Feb 7, 2025
| // can also not (in the general case) recover as | ||
| // the underlying osbuildStatus.scanner maybe in | ||
| // an unrecoverable state (like ErrTooBig). | ||
| return fmt.Errorf(`error parsing osbuild status, please eport a bug and try with "--progress=verbose": %w`, err) |
There was a problem hiding this comment.
Just eporting a typo
s/eport/report/
This commit updates the images library to pull in the fix [0] for the overly long messages from osbuild. This should test the (previously) failing test that runs the centos-9 ISO with an attached terminal. [0] osbuild/images#1200
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.
go.mod: upadate to latest
imagesto get PR#1200This commit updates the images library to pull in the fix [0] for
the overly long messages from osbuild. This should test the
(previously) failing test that runs the centos-9 ISO with an
attached terminal.
[0] osbuild/images#1200
test: run the centos-9 test with an attached terminal
This commit changes the centos-9 test to run with
podman -tsothat we have a test-case that uses the
terminalprogress. Thisis prompted by:
a) we have no integration test currently that uses the terminal
progress for the full build
b) a konflux failure/memory leak that showed because there the
test is run with
-tThis commit ensures that we do restrict the memory of the bib
test conatiner to catch excessive memory usage. This is prompted
by a memory leak when dealing with unrecoverable status messages
that lead to failures in konflux.
This commit changes the progress parser (again) to deal with
errors from the osbuild json progress scanner. On errors we
will now exit right away and potentially kill osbuild but
provide an error message that hints how to workaround the
issue.
The original code assumed we get transient errors like json
decode issues. However it turns out that this is not always
the case, some errors from a bufio.Scanner are unrecoverable
(like ErrTooBig) and trying again just leads to an endless
loop. We can also not "break" wait for the build to finish
because that would appear like the progress is broken
forever and we would still have to report an error (just
much later).