breakup containerImageRef.NewImageSource #6159
Conversation
openshift-ci
bot
added
the
kind/cleanup
|
Ephemeral COPR build failed. @containers/packit-build please check. |
|
@hanwen-flow thanks for the PR. Looks like your branch needs an update, and there are all kinds of failing tests. |
hanwen-flow
force-pushed
the
hanwen/saveblob
branch
from
160111b to
0b725c4
Compare
you folks need to fix your CI: This PR doesn't introduce new functionality, hence no new tests (somebody with permissions has to set a label on this PR) |
hanwen-flow
force-pushed
the
hanwen/saveblob
branch
from
0b725c4 to
9063978
Compare
|
@giuseppe , @TomSweeneyRedHat could you add the 'No New Tests' github label here? |
rhatdan
added
the
No New Tests
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: giuseppe, hanwen-flow 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 |
|
needs a second reviewer. @TomSweeneyRedHat , @mtrmac ? |
|
I’m afraid I can’t spend much time on this now, and this is probably locally not helpful: I happened to notice parallel #5743 . |
thanks for the heads-up, I'll need to update this PR, but that change is a big improvement. Thanks, @aaronlehmann ! |
|
A friendly reminder that this PR had no activity for 30 days. |
Just for ease of reviewing. No functional changes, hence no test updates. Signed-off-by: Han-Wen Nienhuys <[email protected]>
No functional changes, hence no test updates. Signed-off-by: Han-Wen Nienhuys <[email protected]>
No functional changes, hence no test updates. Signed-off-by: Han-Wen Nienhuys <[email protected]>
No functional changes, hence no test updates. Signed-off-by: Han-Wen Nienhuys <[email protected]>
No functional changes (hence, no tests). Signed-off-by: Han-Wen Nienhuys <[email protected]>
hanwen-flow
force-pushed
the
hanwen/saveblob
branch
from
9063978 to
b28d20e
Compare
No functional changes (hence, no tests). Signed-off-by: Han-Wen Nienhuys <[email protected]>
hanwen-flow
force-pushed
the
hanwen/saveblob
branch
from
b28d20e to
e590acb
Compare
|
A friendly reminder that this PR had no activity for 30 days. |
| if err := layerFile.Close(); err != nil { | ||
| return nil, fmt.Errorf("storing %s to file: on file close: %w", what, err) | ||
| } |
There was a problem hiding this comment.
there's a defer up top that should take care of this.
There was a problem hiding this comment.
the defer handles resource freeing in error paths. For ensuring success, the return value of close should still be checked, as closing the file may flush buffers, which can fail.
There was a problem hiding this comment.
ping? @danishprakash @giuseppe @mtrmac
I'd love to work further on this, but rebasing to keep up with changes is here is a lot of work.
For further context, introducing a separate routine for saving blobs enables splitting a single large layer into multiple disjunct layers. These can then be written and hashed in parallel. I've seen 2-3x speed improvement for podman commit.
There was a problem hiding this comment.
Thanks for tagging me. I didn't know this existed.
My proposed change was just to reduce the complexity of the existing code and help document existing behaviors.
The slightly updated version is here sebsoto@eaa8098
|
@giuseppe - do you know how we can get this to move? |
7 participants
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
containerImageRef.NewImageSource is hard to follow because it is large; this reduces complexity a bit
container commit is bottlenecked (among others) by running the layer blob through sha256, which is single-threaded. This refactoring is a preparation to split the blob into multiple streams which can be processed in parallel (not part of this PR).
How to verify it
Which issue(s) this PR fixes:
None
Special notes for your reviewer:
Does this PR introduce a user-facing change?
no.