[breaking] Add err chan to log producer and don't panic on error #1971

Tofel · 2023-12-05T12:47:39Z

What does this PR do?

As per conversation I changed the way starting of log producer works. Now it won't panic if there's an error and instead send it over an error channel. When that happens producer state is cleaned, so that it can be started again. Callers can listen to to the error channel and if there's an error decide how to handle.

Also, it's now required to pass context timeout value, when starting log producer. I've added some boundaries there, it has to be within <5,60> seconds. If it's too low the http connection to get the logs is too short and some logs might get missed randomly (happened, when I was setting it to 1s), if it's too long the connection might timeout.

Now... I was thinking about not breaking the method signature and instead adding a new field to the Container struct that would store the timeout by default set to 5 seconds and then having a setter method to modify it. If you think that's better I could look into it.

Last, but not least. Tests are very simple ones. In our downstream project I have more complex ones which actually focus on testing a detached goroutine that's listening to errors and restarting log producer if it's needed. If current approach is accepted I could play around with Mockery and add some tests here as well.

Why is it important?

We don't want unrecoverable panics if there's an error, when reading logs. It should be up to client to decide what should be done if there's an issue.

Related issues

#1278
#1911

How to test this PR

No straight-forward way. In our downstream project we are using only a limited set of methods from Container interface, so I narrowed it even further, put them in a separate interface and in tests created a mocked container with very stripped-down log-related logic that allowed me inject errors at various points. Without it, though... it's not trivial since you'd need to either disrupt/pause the network connection with Docker container so that reading logs fails due to timeout. Or use a bogus container that makes the connection timeout.

…e sent there instead of causing a panic

netlify · 2023-12-05T12:47:45Z

✅ Deploy Preview for testcontainers-go ready!

Name	Link
🔨 Latest commit	`681fdaf`
🔍 Latest deploy log	https://app.netlify.com/sites/testcontainers-go/deploys/659d516ea9fd1e0007804940
😎 Deploy Preview	https://deploy-preview-1971--testcontainers-go.netlify.app/features/follow_logs
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

mdelapenya · 2023-12-05T13:35:50Z

Before starting the review of this PR, I'd like to let you know that I'd like to plan a total refactor of the log producers feature: #525, so would like to know if we could work on this as a solid feature, instead of patching something that is not as usable as desired

Tofel · 2023-12-05T17:19:31Z

@mdelapenya gotcha. Regarding what you wanna do there, we get the logs from log producer and then our log component has different log targets: in-memory, file, loki. It buffers all logs and then flushes then, when you want to.

Anyway, I will leave the PR open, because what you wanna do doesn't sound like something easy and fast to do :-)

mdelapenya

@Tofel I left a few comments before merging this. Please let me know what you think

docker.go

… instead of erroring on incorrect values

Tofel · 2023-12-20T16:36:22Z

@mdelapenya is there anything left for me to do before we can merge it? I checked the failure in the pipeline, but it doesn't seem related to my changes at all:

=== Failed
=== FAIL: . TestPrependHubRegistrySubstitutor/should_prepend_the_hub_registry_to_images_from_Docker_Hub/plain_image (0.00s)
    image_substitutors_test.go:27: expected my-registry/foo, got foo:latest

=== FAIL: . TestPrependHubRegistrySubstitutor/should_prepend_the_hub_registry_to_images_from_Docker_Hub (0.00s)

=== FAIL: . TestPrependHubRegistrySubstitutor (0.00s)

mdelapenya

LGTM, thanks!

I added a NIT comment regarding a test method name, but I will merge it as soon it's changed, without waiting for the CI

logconsumer_test.go

fix typo Co-authored-by: Manuel de la Peña <social.mdelapenya@gmail.com>

mdelapenya

LGTM! Thanks!

mdelapenya · 2023-12-22T11:56:34Z

Hey @Tofel I think the PR is fine, although I see a consistent failure in the tests when Ryuk is disabled. Please see https://github.com/testcontainers/testcontainers-go/actions/runs/7299330173/job/19892388225?pr=1971 There are two containers leaked. I thought it was caused for something else, but I see this consistent error build after build.

Could you take a look please?

OTOH, I was planning to include this PR in the next release (today), but finding this makes me postpone it to the following one. Hope you understand.

Tofel · 2023-12-22T13:06:46Z

@mdelapenya sure, totally understand, I'd do the same. Will have a look and fix, but probably won't be able to do it this year :/

mdelapenya · 2023-12-22T14:06:38Z

but probably won't be able to do it this year

Thankfully the rest of this year is really short 😂 . Thanks for understanding, see you next year 🎄

Tofel · 2024-01-04T21:40:10Z

@mdelapenya should be fixed now, I didn't call terminateContainerOnEnd() in my tests, which made containers run indefinitely

Happy new year 🥂

docker.go

mdelapenya

@Tofel This LGTM, thanks for your time here 👏👏👏

I added a comment regarding the docs for the changes, in two places: the comments for the methods and our docs website. But I'd be fine with including the docs website in a separate PR if you prefer it.

Thanks!

Tofel · 2024-01-08T18:36:34Z

@mdelapenya done! by website did you mean the markdown file you mentioned or something yet different?

mdelapenya

Thanks for adding the docs, I think it's now clear how to use the new features. I added two comments regarding formatting in the docs. Once fixed, I think this is ready to be merged.

docs/features/follow_logs.md

Tofel · 2024-01-09T13:05:00Z

done!

docs/features/follow_logs.md

mdelapenya

LGTM, thanks for your work here!

Would you mind checking if the linked issues/PRs can be potentially closed?

Cheers!

* main: chore: move internal/testcontainersdocker package's files to internal/core (testcontainers#2083) GenericContainer: in case of error: return a reference to the failed container (testcontainers#2082) [breaking] Add err chan to log producer and don't panic on error (testcontainers#1971) chore: enrich HTTP headers to the Docker daemon with the project path (testcontainers#2080) fix: align codeql versions in GH workflow (testcontainers#2081) chore(deps): bump go.mongodb.org/mongo-driver in /modules/mongodb (testcontainers#2065) chore(deps): bump github.com/shirou/gopsutil/v3 from 3.23.11 to 3.23.12 (testcontainers#2068) fix(modules.gcloud): pass as ptr to allow request customization (testcontainers#1972) chore(deps): bump github.com/twmb/franz-go in /modules/redpanda (testcontainers#2072) chore(deps): bump k8s.io/api, k8s.io/apimachinery, k8s.io/client-go from 0.28.4 to 0.29.0 in /modules/k3s (testcontainers#2078) chore(deps): bump github.com/ClickHouse/clickhouse-go/v2 (testcontainers#2066) chore(deps): bump github.com/google/uuid from 1.4.0 to 1.5.0 (testcontainers#2077) bump google.golang.org/api from 0.153.0 to 0.154.0, cloud.google.com/go/spanner from 1.53.1 to 1.54.0, bump google.golang.org/grpc from 1.59.0 to 1.60.1 in /modules/gcloud (testcontainers#2076) chore(deps): bump github.com/aws/aws-sdk-go-v2 from 1.23.5 to 1.24.0 (credentials from 1.16.9 to 1.16.13, service/s3 from 1.47.1 to 1.47.7) in /modules/localstack (testcontainers#2075) chore(deps): bump github/codeql-action from 2 to 3 (testcontainers#2056) chore(deps): bump test-summary/action from 2.1 to 2.2 (testcontainers#2058) chore(deps): bump actions/setup-go from 4 to 5 (testcontainers#2057)

Tofel · 2024-01-10T15:40:26Z

fix: don't panic when logs waits for more than 5 seconds (v2) #1278 -- I think we can close this one as it was retrying conecting again in a loop instead of panicking, but currently the same behaviour can be achieved using approach similar to what what I put in the example (listening in a detached goroutine to the error channels, sleeping for 1s if there's an error and starting log producer again)
fix: fix panic when getting container logs #1911 -- as well as instead of panicking it would go back to beginning immediately and start over

@mdelapenya: do you have another release planned?

Tofel added 4 commits November 28, 2023 13:06

added error channel to container, so that log producers errors will b…

9b1cc6b

…e sent there instead of causing a panic

use mutex to avoid race conditions around stopping log producer

9878fec

use bounded start timeout

fb18ac4

remove test that no longer makes sense

e6fbf94

Tofel requested a review from a team as a code owner December 5, 2023 12:47

Tofel changed the title ~~Add err chan to log producer~~ [breaking] Add err chan to log producer and don't panic on error Dec 5, 2023

mdelapenya reviewed Dec 15, 2023

View reviewed changes

docker.go Outdated Show resolved Hide resolved

docker.go Show resolved Hide resolved

docker.go Outdated Show resolved Hide resolved

CR changes: functional option for timeout, default timeout to min/max…

3d67a04

… instead of erroring on incorrect values

mdelapenya previously approved these changes Dec 20, 2023

View reviewed changes

logconsumer_test.go Outdated Show resolved Hide resolved

mdelapenya self-assigned this Dec 20, 2023

mdelapenya added the breaking change Causing compatibility issues. label Dec 20, 2023

Update logconsumer_test.go

6c705f1

fix typo Co-authored-by: Manuel de la Peña <social.mdelapenya@gmail.com>

Tofel dismissed mdelapenya’s stale review via 6c705f1 December 21, 2023 00:13

Tofel and others added 3 commits December 21, 2023 01:13

Merge branch 'main' into add_err_chan_to_log_producer

14abdd5

Merge branch 'main' into add_err_chan_to_log_producer

f99cc67

Merge branch 'main' into add_err_chan_to_log_producer

cdbbae0

mdelapenya previously approved these changes Dec 22, 2023

View reviewed changes

mdelapenya mentioned this pull request Dec 26, 2023

fix: fix log producer stopping block indefinitely #1783

Open

Tofel and others added 2 commits January 4, 2024 22:26

Merge branch 'main' into add_err_chan_to_log_producer

67e17a0

add missing container termination on test end

a674957

Tofel dismissed mdelapenya’s stale review via a674957 January 4, 2024 21:37

mdelapenya reviewed Jan 8, 2024

View reviewed changes

docker.go Show resolved Hide resolved

mdelapenya reviewed Jan 8, 2024

View reviewed changes

update log following logs

0895e77

mdelapenya reviewed Jan 9, 2024

View reviewed changes

docs/features/follow_logs.md Outdated Show resolved Hide resolved

docs/features/follow_logs.md Show resolved Hide resolved

docs/features/follow_logs.md Outdated Show resolved Hide resolved

fix docs formatting

9bc895c

mdelapenya reviewed Jan 9, 2024

View reviewed changes

docs/features/follow_logs.md Outdated Show resolved Hide resolved

chore: use H2 instead

681fdaf

mdelapenya approved these changes Jan 9, 2024

View reviewed changes

mdelapenya merged commit 47a5bbb into testcontainers:main Jan 9, 2024
116 checks passed

This was referenced Jan 11, 2024

fix: fix panic when getting container logs #1911

Closed

fix: don't panic when logs waits for more than 5 seconds (v2) #1278

Closed

fix: don't panic when logs waits for more than 5 seconds #1777

Closed

mdelapenya mentioned this pull request Feb 14, 2024

[Bug]: StartLogProducer does not drain channel when EOF is received #1407

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[breaking] Add err chan to log producer and don't panic on error #1971

[breaking] Add err chan to log producer and don't panic on error #1971

Tofel commented Dec 5, 2023

netlify bot commented Dec 5, 2023 •

edited

mdelapenya commented Dec 5, 2023

Tofel commented Dec 5, 2023

mdelapenya left a comment

Tofel commented Dec 20, 2023

mdelapenya left a comment

mdelapenya left a comment

mdelapenya commented Dec 22, 2023

Tofel commented Dec 22, 2023

mdelapenya commented Dec 22, 2023

Tofel commented Jan 4, 2024

mdelapenya left a comment •

edited

Tofel commented Jan 8, 2024

mdelapenya left a comment

Tofel commented Jan 9, 2024

mdelapenya left a comment •

edited

Tofel commented Jan 10, 2024

[breaking] Add err chan to log producer and don't panic on error #1971

[breaking] Add err chan to log producer and don't panic on error #1971

Conversation

Tofel commented Dec 5, 2023

What does this PR do?

Why is it important?

Related issues

How to test this PR

netlify bot commented Dec 5, 2023 • edited

✅ Deploy Preview for testcontainers-go ready!

mdelapenya commented Dec 5, 2023

Tofel commented Dec 5, 2023

mdelapenya left a comment

Choose a reason for hiding this comment

Tofel commented Dec 20, 2023

mdelapenya left a comment

Choose a reason for hiding this comment

mdelapenya left a comment

Choose a reason for hiding this comment

mdelapenya commented Dec 22, 2023

Tofel commented Dec 22, 2023

mdelapenya commented Dec 22, 2023

Tofel commented Jan 4, 2024

mdelapenya left a comment • edited

Choose a reason for hiding this comment

Tofel commented Jan 8, 2024

mdelapenya left a comment

Choose a reason for hiding this comment

Tofel commented Jan 9, 2024

mdelapenya left a comment • edited

Choose a reason for hiding this comment

Tofel commented Jan 10, 2024

netlify bot commented Dec 5, 2023 •

edited

mdelapenya left a comment •

edited

mdelapenya left a comment •

edited