fix dropped pull progress output due to canceled context#2254
fix dropped pull progress output due to canceled context#2254tonistiigi merged 1 commit intomoby:masterfrom
Conversation
fixes moby#2248 Signed-off-by: coryb <cbennett@netflix.com>
|
|
||
| // progressFactory needs the outer context, the context in `p.g.Do` will | ||
| // be canceled before the progress output is complete | ||
| progressFactory := progress.FromContext(ctx) |
There was a problem hiding this comment.
looking at FromContext implementation it only uses context for the ctx.Value(contextKey) call and later uses another context coming with the call. So why does it matter if this context gets canceled. Also, it is possible that this context also gets canceled before the blobs are actually pulled.
There was a problem hiding this comment.
yeah, it was not obvious to me either.
The progress factory is not used until later when we call the controller Start
Start is eventually called here when we need to Unlazy the ref:
Line 201 in 699121c
By the time we call Start, the original context from the
p.g.Do has already been canceled, the new writer immediately gets ignored via:buildkit/util/progress/progress.go
Lines 171 to 172 in 699121c
There was a problem hiding this comment.
As discussed in slack, it seems the reader is what is getting associated with the canceled context via the flightcontrol on g.Do:
buildkit/util/flightcontrol/flightcontrol.go
Lines 105 to 110 in 699121c
So this fix is really about using the reader associated with the CacheKey ctx
tonistiigi
left a comment
There was a problem hiding this comment.
As we discussed in slack, there might be a case where progress gets lost in here if there are parallel calls to this group as previously the magic in flightcontrol would have cached it and sent to all callers while now progress from only one caller is used. When I tested it I couldn't repro on the default case and when pulling the same image both still showed progress. Might be different if only a layer is shared.
So I'm ok with this as a fix but might make sense to look into it further sometime to figure out if there is something more to change.
|
yeah, I am not sure what else we could do here. I feel like the progress system generally needs a redesign to remove the need for progress.Controllers, but at the moment I don't know what that design would look like. I will give it some thought, but unlikely to have much time in the near future. |
sipsma
left a comment
There was a problem hiding this comment.
LGTM
yeah, I am not sure what else we could do here. I feel like the progress system generally needs a redesign to remove the need for progress.Controllers, but at the moment I don't know what that design would look like. I will give it some thought, but unlikely to have much time in the near future.
I agree, progress.Controller was very much kludge piled on top of the existing progress code when I was a lot less familiar with the codebase and trying to get lazy pull working.
I think all of the progress code might be easier to follow if it was replaced by something like a singleton ProgressManager object that centralized the progress stream handling (similar to how there's a singleton CacheManager, Solver, Worker, etc). If it had sort of a pub-sub style API you could decouple the progress stream for a single build from the "event publishers" and thus maybe get rid of all the confusing context handling, the need for callbacks inside descriptor handlers, etc.
However, I say all this as another person who will not have time for it in the near future... @tonistiigi Do you think we should open an issue like "Refactor progress streams"? Happy to do so if you are okay with a somewhat vague issue like that.
yes, but try to put some description of what is actually wrong with the current one |
fixes #2248