chunked: rework GetBlobAt usage #2162
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: giuseppe 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 |
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
giuseppe
force-pushed
the
close-all-chans
branch
from
c782691 to
e2cd1d2
Compare
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
|
testing in containers/podman#24478 |
giuseppe
force-pushed
the
close-all-chans
branch
from
e2cd1d2 to
4656a32
Compare
| @@ -48,21 +48,21 @@ func readEstargzChunkedManifest(blobStream ImageSourceSeekable, blobSize int64, | |||
| Offset: uint64(blobSize - footerSize), | |||
| Length: uint64(footerSize), | |||
| } | |||
| parts, errs, err := blobStream.GetBlobAt([]ImageSourceChunk{chunk}) | |||
|
|
|||
| footer := make([]byte, footerSize) | |||
There was a problem hiding this comment.
IIRC, a lot of the code could benefit from a single “ask for one chunk, read it into memory, validate against a digest” utility.
Arguably that’s a cleanup that should not block this PR…
| if err != nil { | ||
| return nil, nil, nil, 0, err | ||
| } | ||
|
|
||
| defer func() { |
There was a problem hiding this comment.
This works.
Maybe another way to structure the consumers would be to:
defer consumeRemainingStreamsOrErrors(streamsOrErrors) // this loop, ignoring all errors. Also see elsewhere — maybe this should be a context cancellation instead.
// consume data: if there is only one chunk:
reader, err := getNextStream(streamsOrErrors) // if this contains an error, or is closed, returns an error
// Optionally, after all data is consumed: Alternatively, this can be enforced in `getBlobAt` — if we are fine with ignoring errors after all expected data is consumed, like excess chunks.
err := ensureAllDone(streamsOrErrors) // Ensure straemsOrErrors is at EOF, there are no unexpected items, there are no errors reported .There was a problem hiding this comment.
looking more into this, I don't think we should ignore errors. I'll move the code to a new function
| if err != nil { | ||
| return nil, nil, nil, 0, err | ||
| } | ||
|
|
||
| defer func() { |
There was a problem hiding this comment.
Having to consume everything is, generally, an undesired cost.
The typical handling for that is to use a cancellable context, and to have the consumer here defer cancelTheContext; and for the goroutine to observe that context and terminate on cancellation.
Sadly that is not possible in ImageSourceSeekable (no Context parameter), but maybe we could build the streamsOrErrors the idiomatic way, and have getBlobAt deal with draining the produced output, instead of doing that in every consumer.
(I didn’t try, this might be too hard to do. Notably channel sends in getBlobAt would also have to select vs. the context terminating.)
giuseppe
force-pushed
the
close-all-chans
branch
from
4656a32 to
5e80346
Compare
giuseppe
force-pushed
the
close-all-chans
branch
from
5e80346 to
ea81dba
Compare
|
@cgwalters thanks for the review. Addressed the comments and pushed a new version |
giuseppe
force-pushed
the
close-all-chans
branch
from
ea81dba to
d8f279c
Compare
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
giuseppe
force-pushed
the
close-all-chans
branch
from
d8f279c to
e1238b6
Compare
|
@mtrmac addressed your comments, pushed a new version |
giuseppe
force-pushed
the
close-all-chans
branch
from
e1238b6 to
8ae71aa
Compare
what do you think of the last version? |
|
Sure, that works. The |
rewrite how the result from GetBlobAt is used, to make sure 1) that the streams are always closed, and 2) that any error is processed. Closes: https://issues.redhat.com/browse/OCPBUGS-43968 Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
giuseppe
force-pushed
the
close-all-chans
branch
from
8ae71aa to
1f54749
Compare
|
/lgtm |
|
@giuseppe Thanks! |
rewrite how the result from GetBlobAt is used, to make sure 1) that the streams are always closed, and 2) that any error is processed.
Closes: https://issues.redhat.com/browse/OCPBUGS-43968