fix: abandon the work applier reconciliation loop when the main context exits #343
Conversation
Signed-off-by: michaelawyu <chenyu1@microsoft.com>
| // The workqueue.ParallelizeUntil utility does not return errors even if its context has been | ||
| // cancelled (and some availability checks might not be completed yet); to catch such | ||
| // premature termination, the work applier checks for context cancellation directly. | ||
| if err := ctx.Err(); err != nil { |
There was a problem hiding this comment.
Why not do this check in framework.go and preprocess.go?
If this is applicable everywhere, maybe do this check and return error from
func (p *parallelizer) ParallelizeUntil(ctx context.Context, pieces int, doWork workqueue.DoWorkPieceFunc, operation string)
whenever ctx is cancelled?
There was a problem hiding this comment.
+1, ParallelizeUtil is an interface we own. We can probably return error from there.
There was a problem hiding this comment.
The reason that I raised this originally is "can we make it easier for the next person using ParallelizeUntil so that they won't forget to check for context cancellation if we can encapsulate the handling inside the parallelizer package." But it seems that there are some nuances so okay with this approach.
For example, in framework.go we are already handling the context cancellation error separately
There was a problem hiding this comment.
Hi Wei and Wantong! Yeah, actually that was my first attempt as well. The concern I had was that, there are cases where we run ParallelizeUntil in its own child context (primarily for the reason that the child context can be cancelled on its own to terminate the parallelization promptly); and if we do the checking with our wrapper, e.g.,
func (p *parallelizer) ParallelizeUntil(ctx context.Context, pieces int, doWork workqueue.DoWorkPieceFunc, operation string) {
...
workqueue.ParallelizeUntil(ctx, p.numOfWorkers, pieces, doWorkWithLogs)
if err := ctx.Done(); err != nil {...}
}two possibilities exist:
a) the child context is cancelled willingly by us, and the method returns an error;
b) the parent context gets cancelled by factors outside our control, which in turn cancels the child context, and we also receive an error
The complication is that, we couldn't really tell between the two possibilities by looking at the error, so we still need to inspect the parent context at the caller level to find out why things fail, which kind of defeats the purpose of putting the logic inside the wrapper. This is why in this PR the check is done at the caller level instead at the moment. If there's any concern or things that I've missed, please let me know 🙏
Signed-off-by: michaelawyu <chenyu1@microsoft.com>
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Signed-off-by: michaelawyu <chenyu1@microsoft.com>
3 participants
Description of your changes
This PR sets up the work applier to abandon the reconciliation loop when the main context exits in a proper way so that partially process manifests will not trigger error unexpectedly.
I have:
make reviewableto ensure this PR is ready for review.How has this code been tested
N/A
Special notes for your reviewer
N/A