refactor(kubernetes): manage log streaming at pod-level #303
Conversation
This file contains 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
Replaces manual Pods Watch API call with a "container is terminated" signal in a containerTracker. That signal is controlled by the PodTracker based on Add/Update/Delete pod events from the PodInformer (which does its own watch/list API calls internally).
the containerTracker has locks in it, so we don't want to copy it. We want to get references to it.
cognifloyd
added
bug
Codecov Report
@@ Coverage Diff @@
## master #303 +/- ##
==========================================
- Coverage 79.77% 79.35% -0.42%
==========================================
Files 67 68 +1
Lines 4958 5154 +196
==========================================
+ Hits 3955 4090 +135
- Misses 859 911 +52
- Partials 144 153 +9
|
This was referenced Apr 9, 2022
copart-jafloyd
force-pushed
the
k8s-watch-stream
branch
2 times, most recently
from
2e950ab
to
fc3ab81
Compare
copart-jafloyd
force-pushed
the
k8s-watch-stream
branch
from
d703e14
to
eb7572f
Compare
Instead of figuring out how to mock a podInformer resync (which the Informer was not designed to allow), we just call the HandlePodUpdate method and trust that the informer works with the API correctly (so no need to test that).
The k8s runtime needs to cache the logs before the executor requests them. So, expose the maxLogSize so that the cached logs can be truncated near that size.
this is needed for log streaming
We need to make sure to capture all of the logs.
This is how stern does it, and it lets us detect the end of the stream.
bug in default ObjectReactor: github.com/kubernetes/client-go/issues/873
Also clean up test to call to call out what was an implicit use of podTracker and make its use make sense for the test.
copart-jafloyd
force-pushed
the
k8s-watch-stream
branch
from
eb7572f
to
4103334
Compare
|
This approach did not pan out. Closing. |
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.
Part of go-vela/community#519
Builds on #302 which should be merged first.
This is a bug fix and an enhancement. The goal is to improve log streaming with the kubernetes runtime
to prevent missing logs, logs that can be observed when tailing with something like
stern.Only these commits are part of this PR (everything after #302)
TODO:
Alternative fixes that didn't work out:
Closes #277
Closes #286