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: Read the entire reader before writing to bucket #7462

Merged
merged 1 commit into from
Jul 20, 2020
Merged

fix: Read the entire reader before writing to bucket #7462

merged 1 commit into from
Jul 20, 2020

Conversation

abayer
Copy link
Contributor

@abayer abayer commented Jul 20, 2020 •
edited
Loading

Submitter checklist

  • Change is code complete and matches issue description.
  • Change is covered by existing or new tests.

Description

For some reason, io.Copy was resulting in a "short write" error in the build controller, and the best suspicion I've got at this point is that it's a problem with the bucket writer closing early or something along those lines. So let's instead go back to earlier behavior and read the reader contents and write that directly rather than relying on io.Copy.

Special notes for the reviewer(s)

Which issue this PR fixes

fixes #7461 (hopefully!)

@abayer
fix: Read the entire reader before writing to bucket
2bd412a 
For some reason, `io.Copy` was resulting in a "short write" error in
the build controller, and the best suspicion I've got at this point is
that it's a problem with the bucket writer closing early or something
along those lines. So let's instead go back to earlier behavior and
read the reader contents and write that directly rather than relying
on `io.Copy`.

fixes jenkins-x#7461 (hopefully!)

Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>
@abayer abayer force-pushed the no-io-copy-when-writing-to-bucket branch from 870eac4 to 2bd412a Compare July 20, 2020 13:30
@mgoltzsche
Copy link

/lgtm

@ankitm123
Copy link
Member

Just a note @abayer: io.copy is the recommended way, as readAll is inefficient (reads everything to memory), for now, it's nice to get to a working solution, but may be this can be investigated later.

@abayer
Copy link
Contributor Author

abayer commented Jul 20, 2020

@ankitm123 Yeah, io.Copy errors out if number of bytes written doesn't match number of bytes read, while the ReadAll approach is more lenient, so my best guess is there's some set of bytes in the logs to write that the blob writer doesn't accept for some reason, causing io.Copy to fail. But I dunno.

@abayer
Copy link
Contributor Author

abayer commented Jul 20, 2020

/test unit

@abayer
Copy link
Contributor Author

abayer commented Jul 20, 2020

/test integration

@abayer
Copy link
Contributor Author

abayer commented Jul 20, 2020

/approve

@jenkins-x-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abayer, mgoltzsche

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@abayer
Copy link
Contributor Author

abayer commented Jul 20, 2020

/test boot-vault

@jenkins-x-bot jenkins-x-bot merged commit c47ce72 into jenkins-x:master Jul 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@mgoltzsche mgoltzsche

Labels
approved lgtm size/S
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Completed build logs not properly storing in buckets
4 participants
@abayer @mgoltzsche @ankitm123 @jenkins-x-bot