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.

Sign up for GitHub

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

Jump to bottom

fix: deadlock and improve debugging experience #3570

Merged
merged 1 commit into from
Apr 17, 2024
Merged

fix: deadlock and improve debugging experience #3570

merged 1 commit into from
Apr 17, 2024

Conversation

jul-sh
Copy link
Contributor

@jul-sh jul-sh commented Apr 15, 2024 •
edited
Loading

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 jul-sh force-pushed the main branch 6 times, most recently from 6413dfa to 8eb9d32 Compare April 15, 2024 20:57
@jul-sh jul-sh requested a review from laurentsimon April 15, 2024 21:19
@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 •
edited
Loading

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
address comments in slsa-framework#3570
e280a41
jul-sh added a commit to jul-sh/slsa-github-generator that referenced this pull request Apr 17, 2024
@jul-sh
address comments in slsa-framework#3570
80ee0dc
@jul-sh jul-sh mentioned this pull request Apr 17, 2024
Closed
5 tasks
@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 jul-sh requested a review from laurentsimon April 17, 2024 17:55
@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.

@jul-sh @jblebrun
Fix deadlock when the builder outputs a lot of data over stdio. Ref: g…
6c69792 
…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 •
edited
Loading

@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 •
edited
Loading

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 jul-sh mentioned this pull request Apr 18, 2024
Closed
jul-sh added a commit to jul-sh/oak that referenced this pull request Apr 18, 2024
@jul-sh
Use SLSA pre-release that includes deadlock fix
1f05e29 
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
@jul-sh
Use SLSA pre-release that includes deadlock fix
94cbd7c 
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

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

@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

@jul-sh jul-sh mentioned this pull request Apr 23, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jblebrun jblebrun jblebrun left review comments

@tiziano88 tiziano88 tiziano88 left review comments

@laurentsimon laurentsimon laurentsimon approved these changes

@joshuagl joshuagl Awaiting requested review from joshuagl joshuagl is a code owner

@kpk47 kpk47 Awaiting requested review from kpk47 kpk47 is a code owner

@ramonpetgrave64 ramonpetgrave64 Awaiting requested review from ramonpetgrave64 ramonpetgrave64 is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@jul-sh @jblebrun @laurentsimon @ramonpetgrave64 @tiziano88