[CI] SingleNodeDiscoveryIT testCannotJoinNodeWithSingleNodeDiscovery failing #106425
Comments
davidkyle
added
:Distributed/Network
|
Pinging @elastic/es-distributed (Team:Distributed) |
elasticsearchmachine
added
blocker
Team:Distributed
|
This seems like a test issue which may or may not be related to #102516. But I think this is for delivery team maybe. Feel free to relabel if that's not the case. |
pxsalehi
added
:Delivery/Build
elasticsearchmachine
added
Team:Delivery
|
Pinging @elastic/es-delivery (Team:Delivery) |
|
I don't have much insight into the internal test cluster fixtures. This is an area with fuzzy ownership to be honest but I think core/infra is the most appropriate. |
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
|
I investigated this a bit, but could not come to any conclusions. The assertion is cryptic, but it results from a MockLogAppender not being started when a message is appended to it. Yet that check happens within log4j (AppenderControl), and goes to the status logger. So we don't have any control or ability to fail harder when this happens to find where it is actually occurring. My only thought for a potential fix is to always start MockLogAppender instances within the constructor. I don't see a downside to this, but perhaps I'm missing something. |
|
I think it might just not be threadsafe to remove appenders from loggers and stop the appender while other threads might be logging stuff. The following test pretty reliably fails for me: public void testConcurrentLogAndLifecycle() throws Exception {
final var keepGoing = new AtomicBoolean(true);
final var logThread = new Thread(() -> {
while (keepGoing.get()) {
logger.info("test");
}
});
logThread.start();
final var appender = new MockLogAppender();
for (int i = 0; i < 1000; i++) {
try (var ignored = appender.capturing(MockLogAppenderTests.class)) {
Thread.yield();
}
}
keepGoing.set(false);
logThread.join();
} |
rjernst
added
low-risk
Many uses of MockLogAppender predate the addition of the helper method to create a Releasable for restoring logging, which allows the use of try-with-resource blocks. This commit converts current existing uses of MockLogAppender to use capturing. relates elastic#106425
|
Thanks @DaveCTurner, you are right, it appears modifying appenders is simply not threadsafe. I have an idea which seems to work: adding a single appender during test static init (much like some other appenders we add in ESTestCase). That single appender internally keeps track of mapping between loggers and expections in a threadsafe way. Witht hat change I was able to get your example test above to pass consistently over many runs. However, there are a lot of places adding the MockLogAppender to appenders directly which needs to be addressed first. I've opened #108114 to do that. |
Many uses of MockLogAppender predate the addition of the helper method to create a Releasable for restoring logging, which allows the use of try-with-resource blocks. This commit converts current existing uses of MockLogAppender to use capturing. relates #106425
Adding and removing appenders in Log4j is not threadsafe. Yet some tests rely on capturing logging by adding an in memory appender, MockLogAppender. This commit makes the mock logging threadsafe by creating a new, singular appender for mock logging that delegates, in a threadsafe way, to the existing appenders created. Confusingly MockLogAppender is no longer really an appender, but I'm leaving clarifying that for a followup so as to limit the scope of this PR. closes elastic#106425
Adding and removing appenders in Log4j is not threadsafe. Yet some tests rely on capturing logging by adding an in memory appender, MockLogAppender. This commit makes the mock logging threadsafe by creating a new, singular appender for mock logging that delegates, in a threadsafe way, to the existing appenders created. Confusingly MockLogAppender is no longer really an appender, but I'm leaving clarifying that for a followup so as to limit the scope of this PR. closes #106425
Adding and removing appenders in Log4j is not threadsafe. Yet some tests rely on capturing logging by adding an in memory appender, MockLogAppender. This commit makes the mock logging threadsafe by creating a new, singular appender for mock logging that delegates, in a threadsafe way, to the existing appenders created. Confusingly MockLogAppender is no longer really an appender, but I'm leaving clarifying that for a followup so as to limit the scope of this PR. closes elastic#106425
Build scan:
https://gradle-enterprise.elastic.co/s/a5dvvhunctzui/tests/:server:internalClusterTest/org.elasticsearch.discovery.single.SingleNodeDiscoveryIT/testCannotJoinNodeWithSingleNodeDiscovery
Reproduction line:
Applicable branches:
main
Reproduces locally?:
Didn't try
Failure history:
Failure dashboard for
org.elasticsearch.discovery.single.SingleNodeDiscoveryIT#testCannotJoinNodeWithSingleNodeDiscoveryFailure excerpt:
The text was updated successfully, but these errors were encountered: