From c551df770c50f04d7d95378fbfb8997cd43ec335 Mon Sep 17 00:00:00 2001 From: Clebert Suconic Date: Fri, 7 Aug 2020 11:03:04 -0400 Subject: [PATCH] ARTEMIS-2868 Protect Topology Updates from Split Brain on broker shutdown as well --- .../activemq/artemis/cli/commands/Stop.java | 3 ++- .../artemis/core/client/impl/Topology.java | 6 +++++ .../core/client/impl/TopologyManager.java | 1 + .../cluster/impl/ClusterConnectionImpl.java | 22 +++++++++++---- .../tests/smoke/common/SmokeTestBase.java | 10 +++++++ .../tests/smoke/dnsswitch/DNSSwitchTest.java | 27 ++++++++++++++++--- 6 files changed, 59 insertions(+), 10 deletions(-) diff --git a/artemis-cli/src/main/java/org/apache/activemq/artemis/cli/commands/Stop.java b/artemis-cli/src/main/java/org/apache/activemq/artemis/cli/commands/Stop.java index 5f89253707..c071dacd9a 100644 --- a/artemis-cli/src/main/java/org/apache/activemq/artemis/cli/commands/Stop.java +++ b/artemis-cli/src/main/java/org/apache/activemq/artemis/cli/commands/Stop.java @@ -24,6 +24,7 @@ import org.apache.activemq.artemis.dto.BrokerDTO; @Command(name = "stop", description = "stops the broker instance") public class Stop extends Configurable { + public static final String STOP_FILE_NAME = "STOP_ME"; @Override public Object execute(ActionContext context) throws Exception { super.execute(context); @@ -31,7 +32,7 @@ public class Stop extends Configurable { File file = broker.server.getConfigurationFile().getParentFile(); - File stopFile = new File(file, "STOP_ME"); + File stopFile = new File(file, STOP_FILE_NAME); stopFile.createNewFile(); diff --git a/artemis-core-client/src/main/java/org/apache/activemq/artemis/core/client/impl/Topology.java b/artemis-core-client/src/main/java/org/apache/activemq/artemis/core/client/impl/Topology.java index 432d49b333..749f167b98 100644 --- a/artemis-core-client/src/main/java/org/apache/activemq/artemis/core/client/impl/Topology.java +++ b/artemis-core-client/src/main/java/org/apache/activemq/artemis/core/client/impl/Topology.java @@ -310,6 +310,12 @@ public final class Topology { boolean removeMember(final long uniqueEventID, final String nodeId) { TopologyMemberImpl member; + + if (manager != null && !manager.removeMember(uniqueEventID, nodeId)) { + logger.debugf("TopologyManager rejected the update towards %s", nodeId); + return false; + } + synchronized (this) { member = topology.get(nodeId); if (member != null) { diff --git a/artemis-core-client/src/main/java/org/apache/activemq/artemis/core/client/impl/TopologyManager.java b/artemis-core-client/src/main/java/org/apache/activemq/artemis/core/client/impl/TopologyManager.java index 611bf732ef..76c9e24a7b 100644 --- a/artemis-core-client/src/main/java/org/apache/activemq/artemis/core/client/impl/TopologyManager.java +++ b/artemis-core-client/src/main/java/org/apache/activemq/artemis/core/client/impl/TopologyManager.java @@ -19,4 +19,5 @@ package org.apache.activemq.artemis.core.client.impl; public interface TopologyManager { boolean updateMember(long uniqueEventID, String nodeId, TopologyMemberImpl memberInput); + boolean removeMember(long uniqueEventID, String nodeId); } diff --git a/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/cluster/impl/ClusterConnectionImpl.java b/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/cluster/impl/ClusterConnectionImpl.java index 5abba5d603..03c5c0c027 100644 --- a/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/cluster/impl/ClusterConnectionImpl.java +++ b/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/cluster/impl/ClusterConnectionImpl.java @@ -521,17 +521,29 @@ public final class ClusterConnectionImpl implements ClusterConnection, AfterConn @Override public boolean updateMember(long uniqueEventID, String nodeId, TopologyMemberImpl memberInput) { if (splitBrainDetection && nodeId.equals(nodeManager.getNodeId().toString())) { - TopologyMemberImpl member = topology.getMember(nodeId); - if (member != null) { - if (member.getLive() != null && memberInput.getLive() != null && !member.getLive().isSameParams(connector)) { - ActiveMQServerLogger.LOGGER.possibleSplitBrain(nodeId, memberInput.toString()); - } + if (memberInput.getLive() != null && !memberInput.getLive().isSameParams(connector)) { + ActiveMQServerLogger.LOGGER.possibleSplitBrain(nodeId, memberInput.toString()); } memberInput.setLive(connector); } return true; } + /** + * From topologyManager + * @param uniqueEventID + * @param nodeId + * @return + */ + @Override + public boolean removeMember(final long uniqueEventID, final String nodeId) { + if (splitBrainDetection && nodeId.equals(nodeManager.getNodeId().toString())) { + ActiveMQServerLogger.LOGGER.possibleSplitBrain(nodeId, nodeId); + return false; + } + return true; + } + @Override public void setSplitBrainDetection(boolean splitBrainDetection) { this.splitBrainDetection = splitBrainDetection; diff --git a/tests/smoke-tests/src/test/java/org/apache/activemq/artemis/tests/smoke/common/SmokeTestBase.java b/tests/smoke-tests/src/test/java/org/apache/activemq/artemis/tests/smoke/common/SmokeTestBase.java index 543de503cc..964b749b7f 100644 --- a/tests/smoke-tests/src/test/java/org/apache/activemq/artemis/tests/smoke/common/SmokeTestBase.java +++ b/tests/smoke-tests/src/test/java/org/apache/activemq/artemis/tests/smoke/common/SmokeTestBase.java @@ -18,12 +18,15 @@ package org.apache.activemq.artemis.tests.smoke.common; import java.io.File; +import java.io.IOException; import java.util.HashSet; import java.util.Set; +import org.apache.activemq.artemis.cli.commands.Stop; import org.apache.activemq.artemis.tests.util.ActiveMQTestBase; import org.apache.activemq.artemis.util.ServerUtil; import org.junit.After; +import org.junit.Assert; public class SmokeTestBase extends ActiveMQTestBase { Set processes = new HashSet<>(); @@ -51,6 +54,13 @@ public class SmokeTestBase extends ActiveMQTestBase { } } + protected static void stopServerWithFile(String serverLocation) throws IOException { + File serverPlace = new File(serverLocation); + File etcPlace = new File(serverPlace, "etc"); + File stopMe = new File(etcPlace, Stop.STOP_FILE_NAME); + Assert.assertTrue(stopMe.createNewFile()); + } + public static String getServerLocation(String serverName) { return basedir + "/target/" + serverName; } diff --git a/tests/smoke-tests/src/test/java/org/apache/activemq/artemis/tests/smoke/dnsswitch/DNSSwitchTest.java b/tests/smoke-tests/src/test/java/org/apache/activemq/artemis/tests/smoke/dnsswitch/DNSSwitchTest.java index 2be1541467..e426dbaa36 100644 --- a/tests/smoke-tests/src/test/java/org/apache/activemq/artemis/tests/smoke/dnsswitch/DNSSwitchTest.java +++ b/tests/smoke-tests/src/test/java/org/apache/activemq/artemis/tests/smoke/dnsswitch/DNSSwitchTest.java @@ -663,13 +663,25 @@ public class DNSSwitchTest extends SmokeTestBase { } - @Test - public void testWithoutPing() throws Throwable { - spawnRun(serverLocation, "testWithoutPing", getServerLocation(SERVER_LIVE), getServerLocation(SERVER_BACKUP)); + public void testWithoutPingKill() throws Throwable { + spawnRun(serverLocation, "testWithoutPing", getServerLocation(SERVER_LIVE), getServerLocation(SERVER_BACKUP), "1"); } + @Test + public void testWithoutPingRestart() throws Throwable { + spawnRun(serverLocation, "testWithoutPing", getServerLocation(SERVER_LIVE), getServerLocation(SERVER_BACKUP), "0"); + } + /** + * arg[0] = constant "testWithoutPing" to be used on reflection through main(String arg[]) + * arg[1] = serverlive + * arg[2] = server backup + * arg[3] = 1 | 0 (kill the backup = 1, stop the backup = 0); + * @param args + * @throws Throwable + */ public static void testWithoutPing(String[] args) throws Throwable { + boolean killTheBackup = Integer.parseInt(args[3]) == 1; NetUtil.netUp(FIRST_IP, "lo:first"); NetUtil.netUp(SECOND_IP, "lo:second"); @@ -719,7 +731,13 @@ public class DNSSwitchTest extends SmokeTestBase { System.out.println("Forcing backup down and restarting it"); System.out.println("*******************************************************************************************************************************"); - serverBackup.destroyForcibly(); + if (killTheBackup) { + serverBackup.destroyForcibly(); + } else { + String serverLocation = args[2]; + stopServerWithFile(serverLocation); + Assert.assertTrue(serverBackup.waitFor(10, TimeUnit.SECONDS)); + } cleanupData(SERVER_BACKUP); @@ -740,6 +758,7 @@ public class DNSSwitchTest extends SmokeTestBase { } + private static void connectAndWaitBackup() throws Exception { ActiveMQConnectionFactory connectionFactory = new ActiveMQConnectionFactory("tcp://FIRST:61616?ha=true"); Assert.assertTrue(connectionFactory.getServerLocator().isHA());