Skip to content
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

logread: do not close socket too early #3968

Merged

Conversation

christoph-zededa
Copy link
Contributor

only close socket once reading is finished

- What I did
Fix bug where logread does not return anything

- How I did it
Closing socket only after reading from socket is finished.

- How to verify it
Run logread in eve and see that it now prints out log messages.

- Description for the changelog
Fix logread bug but not closing socket before reading.

- A picture of a cute animal (not mandatory but encouraged)

https://www.youtube.com/watch?v=AV9z0c1hjnA&pp=ygUPYmVzdCByYXQgdHJpY2tz

@deitch
Copy link
Collaborator

deitch commented Dec 18, 2023

Was the problem that conn.Close() was being called when StreamLogs() is finished, but the goroutine lasts much longer, and therefore it should not be closed until after the goroutine is complete?

@christoph-zededa
Copy link
Contributor Author

Was the problem that conn.Close() was being called when StreamLogs() is finished, but the goroutine lasts much longer, and therefore it should not be closed until after the goroutine is complete?

Yes.

only close socket once reading is finished

Signed-off-by: Christoph Ostarek <christoph@zededa.com>
@christoph-zededa christoph-zededa force-pushed the fix_logread_socket_closed_too_early branch from 8e9e151 to cc374a5 Compare December 18, 2023 13:12
@deitch
Copy link
Collaborator

deitch commented Dec 18, 2023

CI takes a while. I will keep an eye on it as well, but if you see it is green, feel free to comment.

@christoph-zededa
Copy link
Contributor Author

docker: Error response from daemon: Head "https://registry-1.docker.io/v2/tonistiigi/binfmt/manifests/latest": Get "https://auth.docker.io/token?account=githubactions&scope=repository%3Atonistiigi%2Fbinfmt%3Apull&service=registry.docker.io": read tcp 10.1.0.5:59566->54.196.99.49:443: read: connection reset by peer.

does not look like my change caused this ...

@deitch
Copy link
Collaborator

deitch commented Dec 18, 2023

does not look like my change caused this

It looks like a network issue, but much more fun if I get to blame someone. 😆

I will restart those tests.

@deitch deitch merged commit 59245e8 into linuxkit:master Dec 18, 2023
23 checks passed
christoph-zededa added a commit to christoph-zededa/eve that referenced this pull request Dec 18, 2023
this now includes linuxkit/linuxkit#3968

Signed-off-by: Christoph Ostarek <christoph@zededa.com>
eriknordmark pushed a commit to lf-edge/eve that referenced this pull request Dec 19, 2023
this now includes linuxkit/linuxkit#3968

Signed-off-by: Christoph Ostarek <christoph@zededa.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants