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

fix: make BuildError serialisable by Celery in case of retry #641

Merged
merged 6 commits into from
May 2, 2023

Conversation

SdgJlbl
Copy link
Contributor

@SdgJlbl SdgJlbl commented Apr 27, 2023

Description

Workaround for this Celery bug

How has this been tested?

Checklist

  • changelog was updated with notable changes
  • documentation was updated

@SdgJlbl
Copy link
Contributor Author

SdgJlbl commented Apr 27, 2023

/e2e

@Owlfred
Copy link

Owlfred commented Apr 27, 2023

End to end tests: ✔️ SUCCESS

@SdgJlbl
Copy link
Contributor Author

SdgJlbl commented Apr 27, 2023

/e2e

@Owlfred
Copy link

Owlfred commented Apr 27, 2023

End to end tests: ❌ FAILURE

Jobs status:

Oh noes.

@SdgJlbl SdgJlbl marked this pull request as ready for review April 27, 2023 10:37
@SdgJlbl SdgJlbl requested a review from a team as a code owner April 27, 2023 10:37
@SdgJlbl
Copy link
Contributor Author

SdgJlbl commented Apr 27, 2023

/e2e

@Owlfred
Copy link

Owlfred commented Apr 27, 2023

End to end tests: ✔️ SUCCESS

Crushed it!

@SdgJlbl SdgJlbl force-pushed the fix/patch-celery-retry-error-regression branch from 73d83c7 to 51fabaf Compare April 27, 2023 15:03
@SdgJlbl
Copy link
Contributor Author

SdgJlbl commented Apr 27, 2023

/e2e

@Owlfred
Copy link

Owlfred commented Apr 27, 2023

End to end tests: ✔️ SUCCESS

Congratulations!

logs = get_pod_logs(k8s_client, pod_name, KANIKO_CONTAINER_NAME, ignore_pod_not_found=True)
delete_pod(k8s_client, pod_name)
for line in (logs or "").split("\n"):
logger.info(line, pod_name=pod_name)
Copy link
Contributor

@oleobal oleobal Apr 28, 2023

Choose a reason for hiding this comment

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

Do we want to log this line by line, rather than as a single JSON line?

(I know the backend logs are supposed to be JSONified but IDK about this)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well spotted, that might be the very reason why we have so many lines of log coming from Kaniko.
I'd be in favour to try adding all in the same log, what do you think @guilhem-barthes ?

Copy link
Contributor

@oleobal oleobal left a comment

Choose a reason for hiding this comment

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

I don't have much more than a surface-level understanding of substrapp, so feel free to wait for Guilhem's review, but to me this looks very good, many thanks!

(#burncelery?)

@SdgJlbl SdgJlbl merged commit 90a7511 into main May 2, 2023
@SdgJlbl SdgJlbl deleted the fix/patch-celery-retry-error-regression branch May 2, 2023 12:51
@Milouu Milouu mentioned this pull request May 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants