fix: Support Postgres ready_conditions for already mounted volumes#416
fix: Support Postgres ready_conditions for already mounted volumes#416cbrevik wants to merge 6 commits intotestcontainers:mainfrom
Conversation
DDtKey
left a comment
There was a problem hiding this comment.
Thank you for the contribution!
However testcontainers-rs allows to override ready-conditions and it depends on the use-case
See my comment in the related issue: #415 (comment)
| WaitFor::message_on_stderr("database system is ready to accept connections"), | ||
| WaitFor::message_on_stdout("database system is ready to accept connections"), |
There was a problem hiding this comment.
It's intentional to wait the message against both - as in case of not initialized DB it may lead to flakiness
See
#158
and my comment:
#415 (comment)
we may introduce separate method to specify that DB already initialized or allow configuring named volume on Image side (so it will automatically change the set of conditions)
But I don't think we should change these expectations
There was a problem hiding this comment.
Would possibly doing it like testcontainers-dotnet (using pg_isready, introduced in psql 9.3, in 2013) be a way to support both cases without custom set of conditions for different setups?:
There was a problem hiding this comment.
Updated PR with suggestion on how to do it with pg_isready. Followed the same pattern as the Kafka-module and did it in exec_after_start:
There was a problem hiding this comment.
Hmm, but running tests it seems like it does not actually wait 🙈 So I guess this doesn't work like I thought it would.
There was a problem hiding this comment.
Figured out the issue, container was not ready to accept connections. So adopting a hybrid strategy of checking just once for database system is ready to accept connections also then enables checking for final readiness with pg_isready afterwards
There was a problem hiding this comment.
I'm afraid that first occurrence of database system is ready to accept connections still may lead to flakiness, it might be a log of initialization and pg_isready can be called between init and actual start
There was a problem hiding this comment.
Okay, I thought the point of pg_isready that as soon as the image is loaded that tool makes it easier to avoid flakiness. Would it be possible to have a "ready condition" that uses a ExecCommand (sort of like testcontainers-dotnet) that can be retried until exit code is a specific one? I'm thinking since that works for testcontainers-dotnet.
I see it is also used in testcontainers-elixir: https://github.com/testcontainers/testcontainers-elixir/blob/62cef58c518f1ab63859d9327825db5b9a8f21d8/lib/container/postgres_container.ex#L228
In testcontainers-node: https://github.com/testcontainers/testcontainers-node/blob/075ade49273d553d00bed5fb418de2db3ffb1c6c/packages/modules/postgresql/src/postgresql-container.ts#L42
And in testcontainers-php: https://github.com/testcontainers/testcontainers-php/blob/8602d6545713bd91d6d875de55be9db660f5b68c/src/Modules/PostgresContainer.php#L25
There was a problem hiding this comment.
AFAIK pg_isready is still flaky thing during init scenario
- from pg docs:
pg_isready returns 0 to the shell if the server is accepting connections normally, 1 if the server is rejecting connections (for example during startup), 2 if there was no response to the connection attempt, and 3 if no attempt was made (for example due to invalid parameters).
- How to determine when new container is truly "ready" docker-library/postgres#146 (comment)
- Confirm a container is ready to run after init docker-library/postgres#326
testcontainers-javastill rely on 2 log entries instead ofpg_isready
Would it be possible to have a "ready condition" that uses a ExecCommand
We can, but it requires separate PR and interface proposal. We already have functionality that allows to reach similar behavior via healthcheck override - it's different, but it's docker-way for similar needs.
The only issue is that we don't expose it as part of Image itself, only as an extension to override for now (we can extend in fact)
We also had another PR that attempted to support this - perhaps I need to re-consider this (for example testcontainers-node also uses healthcheck functionality for that)
However, considering the same flakiness problem with pg_isready, it doesn’t appear to be a crucial functionality for this PR.
Fixes #415
Or rather a suggested fix. Might be other things log lines that are more universal across Postgres-containers?