Use the pg_isready command to asses whether PostgreSQL is ready or not
#1093
Conversation
✅ Deploy Preview for testcontainers-dotnet ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
3ec3b04 to
6c5c6a2
Compare
This demonstrates that the wait strategy that counts the occurrences of "database system is ready to accept connections" in the logs does not work reliably. This issue becomes more likely to happen with the recently introduced reuse feature.
6c5c6a2 to
f312550
Compare
| .Concat(stdout.Split(LineEndings, StringSplitOptions.RemoveEmptyEntries)) | ||
| .Concat(stderr.Split(LineEndings, StringSplitOptions.RemoveEmptyEntries)) | ||
| .Count(line => line.Contains("database system is ready to accept connections"))); | ||
| return ((PostgreSqlContainer)container).IsReadyAsync(); | ||
| } | ||
| } |
| public async Task StopAndStartMultipleTimes() | ||
| { | ||
| // Given | ||
| var timeoutSource = new CancellationTokenSource(TimeSpan.FromSeconds(60)); | ||
|
|
||
| // When | ||
| var exception = await RestartAsync(timeoutSource.Token); | ||
|
|
||
| // Then | ||
| Assert.Null(exception); | ||
| } |
There was a problem hiding this comment.
While this will work for PostgreSQL, it won't work for other modules, specifically for those who use log messages to indicate readiness. I was thinking about a more general approach to utilize the existing module tests. What do you think about the following proposal? We can then update the module's tests over time.
namespace DotNet.Testcontainers.Commons;
public class SharedContainerInstance<TContainer> : IAsyncLifetime
where TContainer : IContainer
{
public SharedContainerInstance(TContainer container)
{
Container = container;
}
public TContainer Container { get; }
public Task InitializeAsync()
{
return Task.CompletedTask;
}
public Task DisposeAsync()
{
return Container.DisposeAsync().AsTask();
}
}diff --git a/tests/Testcontainers.PostgreSql.Tests/PostgreSqlContainerTest.cs b/tests/Testcontainers.PostgreSql.Tests/PostgreSqlContainerTest.cs
index 5433e8c..a966ca9 100644
--- a/tests/Testcontainers.PostgreSql.Tests/PostgreSqlContainerTest.cs
+++ b/tests/Testcontainers.PostgreSql.Tests/PostgreSqlContainerTest.cs
@@ -1,17 +1,22 @@
namespace Testcontainers.PostgreSql;
-public sealed class PostgreSqlContainerTest : IAsyncLifetime
+public sealed class PostgreSqlContainerTest : IClassFixture<PostgreSqlContainerTest.SharedContainerInstance>, IAsyncLifetime
{
- private readonly PostgreSqlContainer _postgreSqlContainer = new PostgreSqlBuilder().Build();
+ private readonly PostgreSqlContainerTest.SharedContainerInstance _sharedContainerInstance;
+
+ public PostgreSqlContainerTest(PostgreSqlContainerTest.SharedContainerInstance sharedContainerInstance)
+ {
+ _sharedContainerInstance = sharedContainerInstance;
+ }
public Task InitializeAsync()
{
- return _postgreSqlContainer.StartAsync();
+ return _sharedContainerInstance.Container.StartAsync();
}
public Task DisposeAsync()
{
- return _postgreSqlContainer.DisposeAsync().AsTask();
+ return _sharedContainerInstance.Container.StopAsync();
}
[Fact]
@@ -19,7 +24,7 @@ public sealed class PostgreSqlContainerTest : IAsyncLifetime
public void ConnectionStateReturnsOpen()
{
// Given
- using DbConnection connection = new NpgsqlConnection(_postgreSqlContainer.GetConnectionString());
+ using DbConnection connection = new NpgsqlConnection(_sharedContainerInstance.Container.GetConnectionString());
// When
connection.Open();
@@ -36,10 +41,19 @@ public sealed class PostgreSqlContainerTest : IAsyncLifetime
const string scriptContent = "SELECT 1;";
// When
- var execResult = await _postgreSqlContainer.ExecScriptAsync(scriptContent)
+ var execResult = await _sharedContainerInstance.Container.ExecScriptAsync(scriptContent)
.ConfigureAwait(true);
// When
Assert.True(0L.Equals(execResult.ExitCode), execResult.Stderr);
}
+
+ [UsedImplicitly]
+ public sealed class SharedContainerInstance : SharedContainerInstance<PostgreSqlContainer>
+ {
+ public SharedContainerInstance()
+ : base(new PostgreSqlBuilder().Build())
+ {
+ }
+ }
}
\ No newline at end of fileThere was a problem hiding this comment.
While this will work for PostgreSQL, it won't work for other modules, specifically for those who use log messages to indicate readiness.
I think we are not on the same page on this topic. The purpose of the StopAndStartMultipleTimes that was added for the PostgreSQL module was in my mind a way to prove that the log-based wait strategy was incorrect, in test driven development style.
- Run the
StopAndStartMultipleTimestest - ❌ Test does not pass (times out after one minute)
- Fix the wait strategy by using
pg_isreadyinstead of reading logs - Run the
StopAndStartMultipleTimestest again - ✅ The test now passes indicating that the
pg_isreadystrategy implementation is correct
If I understand your intention correctly, you'd like to introduce a way to start/stop containers before/after each tests. I think that could be a good addition but should probably be handled in a separate pull request.
There was a problem hiding this comment.
Yes, I understand your intention. I think we are talking about the same thing; my approach ensures that the service running inside the container is ready too.
If the container indicates readiness after a stop, that does not necessarily mean that the service running inside the container is ready. Consider log message wait strategies, for example. If we simply check for "log message contains x", this may indicate readiness too early, because the message already exists from a previous start.
My intention is to ensure that not only the wait strategy passes but also that the service running inside the container is ready from the user's perspective, utilizing our existing tests.
If we stop and start the container in between, and the second test fails or hangs, we now know the wait strategy is broken. I agree; your suggestion is more explicit but lacks real readiness.
There was a problem hiding this comment.
OK, we are indeed on the same page! And going a bit further to ensure that the service is actually running is a good idea. 👍
I see you already started working on it in the feature/set-created-started-stopped-container-timestamp-pass-wait-strategy branch. I was hoping to be able to use ContainerFixture from my Testcontainers.Xunit branch which serves the exact same purpose as SharedContainerInstance. But that branches depends on #1100.
I'm still convinced that a dedicated Testcontainers.Xunit NuGet package that could be used both from inside Testcontainers test projects and for consumer integration tests is a great idea. 😉
There was a problem hiding this comment.
I was hoping to be able to use
ContainerFixturefrom my Testcontainers.Xunit branch which serves the exact same purpose asSharedContainerInstance. But that branches depends on #1100.
The PR does not contain the Xunit project, right? I think it is a good idea! A lot of developers will benefit from it. I have not taken a close look into the PR that involves the changes to the logging implementation yet. I took a quick look, and it is quite big. IIRC, I proposed to split it into two steps: one that provides an internal WithLogger(ILogger) builder method, and another that makes the necessary changes regarding logging the container runtime information and making the method public. After that, we can add the dedicated Xunit, package. WDYT? Splitting it makes reviewing much easier.
I'm still convinced that a dedicated
Testcontainers.XunitNuGet package that could be used both from inside Testcontainers test projects and for consumer integration tests is a great idea. 😉
✅
BTW, did you notice the changes I made in the mentioned branch above to the PostgreSQL's wait strategy? Through the issue and the referred GitHub discussion, I noticed that PostgreSQL logs the interfaces to which it is listening. Maybe we do not need to invoke pg_isready.
There was a problem hiding this comment.
I'll reply in the comments of #1100 for all the ILogger related stuff.
BTW, did you notice the changes I made in the mentioned branch above to the PostgreSQL's wait strategy? Through the issue and the referred GitHub discussion, I noticed that PostgreSQL logs the interfaces to which it is listening. Maybe we do not need to invoke
pg_isready.
I've seen you made some changes about the strategy. I still think that leveraging pg_isready is the right thing to do. I never liked making decisions based on reading logs of another program. Those are just logs and — although unlikely — could change in the future. The whole purpose of the pg_isready command line tool is to test whether a PostgreSQL database is ready and that is pretty much guaranteed not to break in future versions. From an engineering standpoint it feels much better to me. Also, it's very close to the MySQL / MariaDb strategies.
There was a problem hiding this comment.
I agree. Logs are always a bit tricky, especially across different versions. As long as pg_isready is reliable, I am fine. Did you check other PostgreSQL images (versions) as well (I guess they contain pg_isready too)? Let's merge develop and complete this pull request (after #1110 if you do not mind).
There was a problem hiding this comment.
Good point about older versions. The pg_isready command was introduced in PostgreSQL 9.3 so I will revisit this PR once #1110 is merged. I will test with PostgreSQL 8 (which is the oldest image available on Docker Hub) and see what happens.
There was a problem hiding this comment.
I guess if versions 14, 15, and 16 support it, it is good enough. Developers can still override the wait strategy or use the generic builder if an older version is really necessary.
bf08a0e to
cf0fdf1
Compare
|
Superseded by #1111. |
What does this PR do?
This pull request uses the builtin
pg_isreadycommand instead of searching the logs fordatabase system is ready to accept connectionsin order to determine whether the PostgreSQL database is ready or not.Why is it important?
Under some circumstances (described in #1092), starting a
PostgreSqlContainermight hang forever.Related issues
Closes #1092
How to test this PR
A new test (
Testcontainers.PostgreSql.PostgreSqlContainerTest.StopAndStartMultipleTimes) was added to demonstrates how the hang might happen and how usingpg_isreadyinstead addresses this issue.