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: deadlock and improve debugging experience #3570

Merged
merged 1 commit into from Apr 17, 2024

Conversation

jul-sh
Copy link
Contributor

@jul-sh jul-sh commented Apr 15, 2024

Summary

Fixes #3571. Fixes #3569. Shoutout to @jblebrun who wrote most of the code in this PR and debugged this issue together with me.

Testing Process

...

Checklist

  • Review the contributing guidelines
  • Add a reference to related issues in the PR description.
  • Update documentation if applicable.
  • Add unit tests if applicable.
  • Add changes to the CHANGELOG if applicable.

@jul-sh
Copy link
Contributor Author

jul-sh commented Apr 15, 2024

cc @tiziano88 also

@jul-sh jul-sh changed the title docker generator: output logs as command is running docker generator: output logs as command is running, instead of only once it concluded Apr 16, 2024
@jul-sh
Copy link
Contributor Author

jul-sh commented Apr 16, 2024

Turns out this PR also fixes another where action deadlocks if the builder outputs significant amounts of logs over stdio. This is due to the version on the main branch reading stdout and stderr sequentially, thus encountering this deadlock: golang/go#16787

@jul-sh jul-sh changed the title docker generator: output logs as command is running, instead of only once it concluded Fix slsa-framework/slsa-github-generator deadlock and improve debugging experience Apr 16, 2024
@jblebrun jblebrun force-pushed the main branch 2 times, most recently from 1667306 to 8489e8a Compare April 16, 2024 21:15
@jul-sh
Copy link
Contributor Author

jul-sh commented Apr 17, 2024

cc @timonvo & @thmsbinder also, I mentioned this PR in our sync earlier. It unblocks signed provenances for some of the remaining Oak components.

@laurentsimon laurentsimon changed the title Fix slsa-framework/slsa-github-generator deadlock and improve debugging experience fix: slsa-framework/slsa-github-generator deadlock and improve debugging experience Apr 17, 2024
@laurentsimon laurentsimon changed the title fix: slsa-framework/slsa-github-generator deadlock and improve debugging experience fix: deadlock and improve debugging experience Apr 17, 2024
jul-sh added a commit to jul-sh/slsa-github-generator that referenced this pull request Apr 17, 2024
jul-sh added a commit to jul-sh/slsa-github-generator that referenced this pull request Apr 17, 2024
@jblebrun
Copy link
Contributor

I'm going to so some final testing with our pipeline after the changes, just to make sure I haven't made any mistakes with the changes.

@jblebrun
Copy link
Contributor

@laurentsimon Thanks for the review! BTW, it looks like the workflows need to be approved again.

@jul-sh
Copy link
Contributor Author

jul-sh commented Apr 17, 2024

@laurentsimon fixed the linter failure caught in the checks that ran after approval. Sorry, but I think you need to approve again.

…olang/go#16787

Also improve developer experience by outputting logs incrementally.

Signed-off-by: Juliette Pretot <julsh@google.com>
Co-authored-by: Jason LeBrun <jibbl@google.com>
@laurentsimon laurentsimon enabled auto-merge (squash) April 17, 2024 18:12
@laurentsimon laurentsimon merged commit 02fc78b into slsa-framework:main Apr 17, 2024
74 checks passed
@jblebrun
Copy link
Contributor

@laurentsimon Sorry, one more question!

We were trying to use a version including this fix... referencing a specific digest didn't work (detect-env fails), but even using a named ref like main results in a failure (the build completes but the artifact can't be found later).

After some digging I found https://github.com/slsa-framework/slsa-github-generator/blob/main/README.md#referencing-slsa-builders-and-generators which seems to indicate that everything only works if we use a specific tag formed like vX.Y.Z?

Is our only solution to wait for a properly tagged version to include this fix? Is there any other workaround?

@laurentsimon
Copy link
Collaborator

You're correct, we need to release with the patch

@jblebrun
Copy link
Contributor

You're correct, we need to release with the patch

Is there any specific time table that the org prefers to follow for releases? We could follow the process in RELEASES.md to get the next release moving, if that's invited.

@laurentsimon
Copy link
Collaborator

laurentsimon commented Apr 18, 2024

@ramonpetgrave64 is looking into releasing. Appreciate the offer, but one needs to be a maintainer to go thru process REALEASE (need to have certain permissions, etc)

@ramonpetgrave64
Copy link
Collaborator

ramonpetgrave64 commented Apr 18, 2024

We have a pre-release that you can start using to build and generate provenances, but verifying won't yet work. https://github.com/slsa-framework/slsa-github-generator/releases/tag/v2.0.0-rc.0

tracking in
#3576

jul-sh added a commit to jul-sh/oak that referenced this pull request Apr 18, 2024
Per the SLSA  team this should unlock provenance generation, though verification won't work yet: slsa-framework/slsa-github-generator#3570 (comment)

Change-Id: Ia572af830c11e2733a1c0e96906a5264c9f7c62d
jul-sh added a commit to jul-sh/oak that referenced this pull request Apr 18, 2024
Per the SLSA  team this should unlock provenance generation, though verification won't work yet: slsa-framework/slsa-github-generator#3570 (comment)

Change-Id: Ia572af830c11e2733a1c0e96906a5264c9f7c62d
@ramonpetgrave64
Copy link
Collaborator

@jul-sh
Copy link
Contributor Author

jul-sh commented Apr 23, 2024

We've released v2.0.0 today. https://github.com/slsa-framework/slsa-github-generator/releases/tag/v2.0.0

thank you so much for the quick release! we updated our repo to v2 and it's working beautifully. cc @tiziano88 @jblebrun @tiziano88

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants