Remove use of pkg/pools in archive#49117
Conversation
9670770 to
89996ba
Compare
|
Update the DecompressStream logic to match what is done in containerd, it has the buffer pool now and cleans up a case where zstd decompression was not getting |
|
The previous test failures were both related to the tests erroneously relying on the bufio to fill up the read buffer while now the pipe reader may return smaller chunks. Switching to |
| return 0, io.EOF | ||
| } | ||
| n, err = r.buf.Read(p) | ||
| if err == io.EOF { |
There was a problem hiding this comment.
Won't this leak the buffer if read fails with other errors? Couldn't this just be err != nil?
There was a problem hiding this comment.
Guess this should have a closer to return the buffer
There was a problem hiding this comment.
In that case it will just get GC'ed. It is pointless to return to the pool on other errors since the higher level operation will end. Am I missing something else which will hold other a reference?
This implementation is intentionally the same as containerd's. We can probably unify the two, the only difference is we don't (intentionally) support xz.
|
Silly question; the existing pkg/pools package had some test-coverage on those pools (although I had a quick peek at some of those, and it may be very limited). With some of these being moved internal it looks like we don't have separate coverage for their replacements, other than existing tests covering them as part of where they're used. Will we be reducing coverage with this, or are we confident that the existing tests were already covering the functionality enough? |
We can be confident. The pools tests weren't testing much other than just adding coverage to the pools package. From coverage perspective, everything added is used by archive tests. |
|
Error looks legit. |
Signed-off-by: Derek McGowan <derek@mcg.dev>
After the untar errors, the reader must complete in order to fill the buffer used by the subsequent check. Signed-off-by: Derek McGowan <derek@mcg.dev>
Cleanup decompress logic and add a pool. The close logic should be custom defined for each compression type since they have different close interfaces. Signed-off-by: Derek McGowan <derek@mcg.dev>
The use of bufio for writing without flushing can lead to an incomplete writing of the tar and subsequent unexpected EOF when importing. Signed-off-by: Derek McGowan <derek@mcg.dev>
ac5badd to
be4eac7
Compare
|
I updated the docker/cli PR I had in draft to test this branch, and seeing a failure that looks related; |
|
It also looks like docker/cli still has a direct dependency on |
I can look at the tests there, both the failures I found in tests here were bugs in the tests which only worked because of the large bufio not requiring flushing or discarding unread data. The |
|
@thaJeztah Opened a fix for the test, definitely looks like a bad test docker/cli#5715 |
|
Merged that fix, and tested this PR in cli, buildx and buildkit with these draft PRs, and looks like CI is happy on all of them 🎉 ; |
Removes unnecessary use of pooling and prefers use of buffer pool on copy rather than pooling bufio. In some cases the bufio was used and the pool copy was still used which unnecessarily got a buffer which would not be used.