fix context leaks in GCS datasource and registry transport#4162
fix context leaks in GCS datasource and registry transport#4162immanuwell wants to merge 1 commit into
Conversation
kubevirt-bot
added
release-note-none
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @immanuwell. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/test all |
|
coverage: 49.495% (-0.03%) from 49.521% — immanuwell:fix-context-leaks-importer into kubevirt:main |
|
/test pull-containerized-data-importer-e2e-destructive |
|
@immanuwell: The following tests failed, say
DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
immanuwell
force-pushed
the
fix-context-leaks-importer
branch
from
b5255b3 to
340a7d6
Compare
|
@akalenyu done, I addressed all your suggestions, PTAL |
| Expect(ProcessingPhaseError).To(Equal(result)) | ||
| }) | ||
|
|
||
| It("Close should cancel the context before closing readers", func() { |
Signed-off-by: Immanuel Tikhonov <pchpr.00@list.ru>
immanuwell
force-pushed
the
fix-context-leaks-importer
branch
from
340a7d6 to
fb58e7c
Compare
|
@akalenyu done, PTAL |
akalenyu
left a comment
akalenyu
left a comment
There was a problem hiding this comment.
looks good, just unsure about the extra defensiveness with ctx == nil
/test all
| if ctx == nil { | ||
| ctx = context.Background() | ||
| } |
There was a problem hiding this comment.
don't think we need this?
| if ctx == nil { | ||
| ctx = context.Background() | ||
| } |
There was a problem hiding this comment.
same, we own all the calls and can guarantee context is passed
|
@immanuwell: The following tests failed, say
DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
4 participants
What this PR does / why we need it:
Three context bugs in pkg/importer that have been quietly leaking resources:
NewGCSDataSource discards the cancel func from context.WithTimeout - there's even a nolint:govet todo comment acknowledging it. The context is never cancelled so goroutines/timers leak on every GCS import. Fix: store cancel in the struct, call it in Close() (and on error paths).
GetImageDigest creates a timeout ctx via commandTimeoutContext() but then passes context.Background() to src.GetManifest() - so the timeout has no effect on the actual manifest fetch, it can hang forever.
validateImagePlatformMatch hardcodes context.Background() even though the caller has a proper timeout ctx. Fixed by threading ctx through as a param.
How to reproduce 1 (GCS leak):
How to reproduce 2 (GetManifest ignores timeout):
Release note: