-
Notifications
You must be signed in to change notification settings - Fork 5.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Logs API stops functioning when a pod fails #21287
base: master
Are you sure you want to change the base?
fix: Logs API stops functioning when a pod fails #21287
Conversation
🔴 Preview Environment stopped on BunnyshellSee: Environment Details | Pipeline Logs Available commands (reply to this comment):
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #21287 +/- ##
==========================================
- Coverage 55.18% 53.28% -1.90%
==========================================
Files 337 337
Lines 57058 57061 +3
==========================================
- Hits 31487 30407 -1080
- Misses 22875 24006 +1131
+ Partials 2696 2648 -48 ☔ View full report in Codecov by Sentry. |
e0d48bb
to
d0e06c7
Compare
0dbcf76
to
9035ea5
Compare
@@ -39,6 +39,11 @@ func parseLogsStream(podName string, stream io.ReadCloser, ch chan logEntry) { | |||
timeStampStr := parts[0] | |||
logTime, err := time.Parse(time.RFC3339Nano, timeStampStr) | |||
if err != nil { | |||
if strings.HasPrefix(line, "unable to retrieve container logs for ") || | |||
strings.HasPrefix(line, "Unable to retrieve container logs for ") { | |||
ch <- logEntry{line: line, podName: podName, timeStamp: time.Now()} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should err still be added here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, any error in the log channel would stop all other streams immediately.
argo-cd/server/application/application.go
Lines 1796 to 1804 in 728b31e
logStream := mergeLogStreams(streams, time.Millisecond*100) | |
sentCount := int64(0) | |
done := make(chan error) | |
go func() { | |
for entry := range logStream { | |
if entry.err != nil { | |
done <- entry.err | |
return | |
} else { |
for entry := range res { | ||
entries = append(entries, entry) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You probably need some wait before checking results to avoid flakiness. Or, use some channel for synchronization. This would also help to exit from a goroutine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have added done channel for sync,
Have increased the time window to 5 seconds, would 5 seconds be enough ?
849f7ae
to
c1f1255
Compare
Signed-off-by: Fluder-Paradyne <[email protected]>
Signed-off-by: Fluder-Paradyne <[email protected]>
…essages - Add sync and increase timeout for test Signed-off-by: Fluder-Paradyne <[email protected]>
c1f1255
to
086514c
Compare
Closes: #21286
Checks for error message that occurs when pods are not available and stop parsing
Checklist: