ostree-ext/filter_tar_async: flush decompressor stream

[jeckersb]

We need to read anything past the tar stream, otherwise we'll deadlock
via skopeo in FinishPipe.

Also remove the bit where we hold the pipe open longer, we shouldn't
need to do that anymore because we've ensured we've read everything
out of it by this point.

Resolves: #1204

Signed-off-by: John Eckersberg jeckersb@redhat.com

[jeckersb]

Hm should have probably run tests locally first 😆

[cgwalters]

Ahh, that definitely explains why it happens reliably with zstd:chunked.

But...hmmm...we have still seen it occasionally for non zstd:chunked layers though too, could it be that even gzip has a similar thing, just less likely?

[jeckersb]

Yeah I'm not familiar with the specifics on gzip but that could be the case.

Looking at the failing tests though, it looks like maybe when we do the filtering that causes us to not always read to the end of even the basic tar stream? I guess that could also cause the same behavior. Should be easy enough to test by writing a large enough file at the end of the stream that ends up getting filtered out.

[cgwalters]

If the last item is filtered out, then that will definitely happen.

But even there, in the tar format there's trailing zeros to indicate end-of-archive, and I wonder if tar-rs is always consuming those bytes? It probably should be, but not 100% sure.

[jeckersb]

Well I suppose the "cheater" fix here is to just trace! and ignore when we see extra data at the end of the stream since technically it's not fatal. Part of me wants to know why tar-rs doesn't seem to read all the way through, but the practical part of me just wants to move on.

[jeckersb]

Another thing re: "why does gzip work better", note that for the gzip case we explicitly create a BufReader since it's required by bufread:GzDecoder, whereas in the zstd case it's allocating it's own BufReader::with_capacity. I haven't run down what the result of zstd_safe::DCtx::in_size() ends up being, but I suspect in the zstd case we end up buffering a lot less than the gzip case, which means it would be more likely that we "miss" the end of the stream in our buffer.

[jeckersb]

This comment needs removed (If we actually remove this logic)

[cgwalters]

@jeckersb So I went to re-test #963 after this landed but I still get the same symptom. I think we should add an integration test that verifies we can fetch from a zstd:chunked image as part of fixes here.

[jeckersb]

@jeckersb So I went to re-test #963 after this landed but I still get the same symptom. I think we should add an integration test that verifies we can fetch from a zstd:chunked image as part of fixes here.

We do already have tests in ostree-ext for zstd and zstd:chunked:

bootc/ostree-ext/tests/it/main.rs

Lines 1674 to 1684 in c94cfdf

	/// Test for zstd
	#[tokio::test]
	async fn test_container_zstd() -> Result<()> {
	test_non_gzip("zstd").await
	}
	/// Test for zstd:chunked
	#[tokio::test]
	async fn test_container_zstd_chunked() -> Result<()> {
	test_non_gzip("zstd:chunked").await
	}

but obviously those were passing before this change. I was only able to get it to break with a sufficiently large container image (empty worked, 50MB worked, 100MB intermittently failed, 200MB always failed). So I'll add a test to generate a large (1GB?) zstd/zstd:chunked image and import that.

[jeckersb]

@jeckersb So I went to re-test #963 after this landed but I still get the same symptom. I think we should add an integration test that verifies we can fetch from a zstd:chunked image as part of fixes here.

I am able to reproduce this as well with bootc switch. Worth noting that it's always failing in unencapsulate_base, whereas before I was seeing it in subsequent layers. So maybe not exactly the same issue, but certainly closely related. Should be easy (famous last words...) to figure out what the difference is between the base and non-base case with such a simple and reliable reproducer.

[jeckersb]

Your image also fails using the same command I was using before (just container image pull from ostree-ext):

# ostree --repo=repo refs --delete ostree/container; ./bootc internals ostree-container image pull --quiet --insecure-skip-tls-verification repo ostree-unverified-registry:quay.io/cgwalters/ostest:zstdchunked
layers already present: 0; layers needed: 65 (378.5 MB)
ERROR Importing: Unencapsulating base: failed to invoke method FinishPipe: failed to invoke method FinishPipe: write |1: broken pipe

[jeckersb]

Followup PR should fix this