fix: Wait for context cancel in k8s pod watcher #6643
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Marking as Draft so I can see the behavior on kokoro. |
Codecov Report
@@ Coverage Diff @@
## main #6643 +/- ##
==========================================
- Coverage 70.48% 69.98% -0.50%
==========================================
Files 515 522 +7
Lines 23150 23732 +582
==========================================
+ Hits 16317 16609 +292
- Misses 5776 6038 +262
- Partials 1057 1085 +28
Continue to review full report at Codecov.
|
halvards
force-pushed
the
pod-watcher-deadlock
branch
from
182e2a3 to
01212be
Compare
|
Integration test fails on kokoro at a different point: skaffold/integration/dev_test.go Line 123 in 151b8d3 There was a similar failure in the integration tests running on GitHub Actions for a previous commit: https://github.com/GoogleContainerTools/skaffold/pull/6643/checks?check_run_id=3729355176 Is there more than one issue? |
halvards
force-pushed
the
pod-watcher-deadlock
branch
from
01212be to
a89db39
Compare
Make the Kubernetes pod watcher context aware, and select on `context.Done()`. This means we can stop waiting, and acting on, pod events when the context has been cancelled. Remove waiting on `context.Done()` in the Kubernetes log aggregator, container manager, and pod port forwarder. This is to eliminate the chance that the pod watcher sends a `PodEvent` on a channel without a waiting receiver. This should help avoid deadlocks that can occur when pod watcher event receivers stop reading from the channel that they've registered with the pod watcher. We still close the channels on the receiver side, which could increase the chances of regression and re-occurrence of this issue. Also uses a RWMutex in the pod watcher, though we could move this change to a separate commit. Fixes: GoogleContainerTools#6424
halvards
force-pushed
the
pod-watcher-deadlock
branch
from
a89db39 to
f27a579
Compare
MarlonGamez
approved these changes
Sep 29, 2021
|
mad props! thanks for getting to the bottom of this. |
halvards
added a commit
to halvards/skaffold
that referenced
this pull request
Oct 6, 2021
Adding log statements to better understand what's happening in kokoro. Unfortunately I haven't been able to reproduce the problem locally (on Linux) so far. Related: GoogleContainerTools#6424, GoogleContainerTools#6643, GoogleContainerTools#6662
halvards
added a commit
to halvards/skaffold
that referenced
this pull request
Oct 7, 2021
Debugging the flaky `TestDevGracefulCancel/multi-config-microservices` integration test showed that the kubectl port forwarder was stuck with goroutines waiting on a channels(one per resource). Search for `goroutine 235` and `goroutine 234` in this Kokoro log: https://source.cloud.google.com/results/invocations/a9749ab5-8762-4319-a2be-f67c7440f7a2/targets/skaffold%2Fpresubmit/log This change means that the forwarder also listens for context canceled. **Related**: GoogleContainerTools#6424, GoogleContainerTools#6643, GoogleContainerTools#6662, GoogleContainerTools#6685
briandealwis
pushed a commit
that referenced
this pull request
Oct 7, 2021
Debugging the flaky `TestDevGracefulCancel/multi-config-microservices` integration test showed that the kubectl port forwarder was stuck with goroutines waiting on a channels(one per resource). Search for `goroutine 235` and `goroutine 234` in this Kokoro log: https://source.cloud.google.com/results/invocations/a9749ab5-8762-4319-a2be-f67c7440f7a2/targets/skaffold%2Fpresubmit/log This change means that the forwarder also listens for context canceled. **Related**: #6424, #6643, #6662, #6685
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Make the Kubernetes pod watcher context aware, and select on
context.Done(). This means we can stop waiting, and acting on, pod events when the context has been cancelled.Remove waiting on
context.Done()in the Kubernetes log aggregator, container manager, and pod port forwarder. This is to eliminate the chance that the pod watcher sends aPodEventon a channel without a waiting receiver.This should help avoid deadlocks that can occur when pod watcher event receivers stop reading from the channel that they've registered with the pod watcher.
We still close the channels on the receiver side, which could increase the chances of regression and re-occurrence of this issue.
Also uses a RWMutex in the pod watcher, though we could move this change to a separate PR.
Fixes: #6424