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

Handle panics in the osbuild job & fix panic when OCI authentication fails #3666

Merged
merged 3 commits into from
Sep 1, 2023

Conversation

ondrejbudai
Copy link
Member

@ondrejbudai ondrejbudai commented Aug 29, 2023

Two fixes related to each other:

  1. When an osbuild job panics, but osbuild actually succeeded and no JobError was set, we currently report success back to composer. That's wrong. I added a recover() to set an error state.
  2. Passing wrong credentials when uploading to OCI causes a panic. Seems like the OCI SDK doesn't handle this well. I worked around this by doing more checking when the error is returned.

It doesn't make sense to have them together.

Signed-off-by: Ondřej Budai <ondrej@budai.cz>
Previously, the worker would happily report success if osbuild succeeded,
there was no JobError, but the job actually panicked in the meantime.
Let's fix this by adding a recovery mechanism.

Signed-off-by: Ondřej Budai <ondrej@budai.cz>
resp.IsResumable crashes if resp.MultipartUploadResponse == nil. This happens
for instance when authentication fails. Fix this by also checking the
MultipartUploadResponse field.

Signed-off-by: Ondřej Budai <ondrej@budai.cz>
@henrywang
Copy link
Member

Tests / edge-commit-cs9 failure is due to bug https://bugzilla.redhat.com/show_bug.cgi?id=2234390. PR #3663 included a workaround for this bug.

Copy link
Contributor

@bcl bcl left a comment

Choose a reason for hiding this comment

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

Looks ok to me, other than not having a test -- which of course would be tricky to do with a panic.

@ondrejbudai ondrejbudai merged commit 23718dc into osbuild:main Sep 1, 2023
65 of 68 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.

3 participants