diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java index 67152e29f9..c9e147d25b 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java @@ -883,11 +883,16 @@ public class HMaster extends HRegionServer implements MasterServices { status.setStatus("Initializing meta table if this is a new deploy"); InitMetaProcedure initMetaProc = null; - if (assignmentManager.getRegionStates().getRegionState(RegionInfoBuilder.FIRST_META_REGIONINFO) - .isOffline()) { + RegionState metaRegionState = + assignmentManager.getRegionStates().getRegionState(RegionInfoBuilder.FIRST_META_REGIONINFO); + if (!metaRegionState.isOpened()) { Optional> optProc = procedureExecutor.getProcedures().stream() .filter(p -> p instanceof InitMetaProcedure).findAny(); - if (optProc.isPresent()) { + // check if we are not loading a successful procedure by the last master as Meta is still not + // in OPEN state + // this also helps in unnecessary waiting on the latch(and get stuck) as the countdown was + // reset and will never be down to zero as the procedure is not running + if (optProc.isPresent() && !((InitMetaProcedure) optProc.get()).isSuccess()) { initMetaProc = (InitMetaProcedure) optProc.get(); } else { // schedule an init meta procedure if meta has not been deployed yet diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignProcedure.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignProcedure.java index a0297b3ac3..99352508c3 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignProcedure.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignProcedure.java @@ -340,7 +340,11 @@ public class AssignProcedure extends RegionTransitionProcedure { setTransitionState(RegionTransitionState.REGION_TRANSITION_FINISH); break; case FAILED_OPEN: - handleFailure(env, regionNode); + try { + handleFailure(env, regionNode); + } catch (IOException e) { + LOG.warn("Failed state=" + State.FAILED_OPEN, e); + } break; default: throw new UnexpectedStateException("Received report unexpected " + code + @@ -350,22 +354,27 @@ public class AssignProcedure extends RegionTransitionProcedure { } /** - * Called when dispatch or subsequent OPEN request fail. Can be run by the - * inline dispatch call or later by the ServerCrashProcedure. Our state is - * generally OPENING. Cleanup and reset to OFFLINE and put our Procedure - * State back to REGION_TRANSITION_QUEUE so the Assign starts over. + * Called when dispatch or subsequent OPEN request fail. Can be run by the inline dispatch call or + * later by the ServerCrashProcedure. Our state is generally OPENING. Cleanup and reset to OFFLINE + * and put our Procedure State back to REGION_TRANSITION_QUEUE so the Assign starts over. + * @throws IOException */ - private void handleFailure(final MasterProcedureEnv env, final RegionStateNode regionNode) { + private void handleFailure(final MasterProcedureEnv env, final RegionStateNode regionNode) + throws IOException { if (incrementAndCheckMaxAttempts(env, regionNode)) { aborted.set(true); } this.forceNewPlan = true; this.targetServer = null; - regionNode.offline(); - // We were moved to OPENING state before dispatch. Undo. It is safe to call - // this method because it checks for OPENING first. - env.getAssignmentManager().undoRegionAsOpening(regionNode); - setTransitionState(RegionTransitionState.REGION_TRANSITION_QUEUE); + + try { + // We were moved to OPENING state before dispatch. Undo. It is safe to call + // this method because it checks for OPENING first. + env.getAssignmentManager().undoRegionAsOpening(regionNode); + } finally { + regionNode.offline(); + setTransitionState(RegionTransitionState.REGION_TRANSITION_QUEUE); + } } private boolean incrementAndCheckMaxAttempts(final MasterProcedureEnv env, @@ -388,7 +397,11 @@ public class AssignProcedure extends RegionTransitionProcedure { @Override protected boolean remoteCallFailed(final MasterProcedureEnv env, final RegionStateNode regionNode, final IOException exception) { - handleFailure(env, regionNode); + try { + handleFailure(env, regionNode); + } catch (IOException e) { + return false; + } return true; } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManager.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManager.java index c4441af31c..102ce21602 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManager.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManager.java @@ -1420,7 +1420,7 @@ public class AssignmentManager implements ServerListener { metrics.incrementOperationCounter(); } - public void undoRegionAsOpening(final RegionStateNode regionNode) { + public void undoRegionAsOpening(final RegionStateNode regionNode) throws IOException{ boolean opening = false; synchronized (regionNode) { if (regionNode.isInState(State.OPENING)) { @@ -1429,8 +1429,17 @@ public class AssignmentManager implements ServerListener { } // Should we update hbase:meta? } - if (opening) { - // TODO: Metrics. Do opposite of metrics.incrementOperationCounter(); + // TODO: Metrics. Do opposite of metrics.incrementOperationCounter(); + if (opening && isMetaRegion(regionNode.getRegionInfo())) { + try { + MetaTableLocator.setMetaLocation(master.getZooKeeper(), regionNode.getRegionLocation(), + State.OFFLINE); + for (RegionInfo hri : getMetaRegionSet()) { + setMetaAssigned(hri, false); + } + } catch (KeeperException e) { + throw new IOException(e); + } } } diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java index 574db2f428..e0dfc60a5b 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java @@ -2658,7 +2658,7 @@ public class HBaseTestingUtility extends HBaseZKTestingUtility { decrementMinRegionServerCount(); } - private void decrementMinRegionServerCount() { + public void decrementMinRegionServerCount() { // decrement the count for this.conf, for newly spwaned master // this.hbaseCluster shares this configuration too decrementMinRegionServerCount(getConfiguration()); diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestMetaShutdownHandler.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestMetaShutdownHandler.java index 7faed1c2b1..22bc31ee5a 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestMetaShutdownHandler.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestMetaShutdownHandler.java @@ -33,6 +33,7 @@ import org.apache.hadoop.hbase.Waiter; import org.apache.hadoop.hbase.master.assignment.RegionStates; import org.apache.hadoop.hbase.testclassification.MediumTests; import org.apache.hadoop.hbase.util.Bytes; +import org.apache.hadoop.hbase.util.JVMClusterUtil.MasterThread; import org.apache.hadoop.hbase.zookeeper.MetaTableLocator; import org.apache.hadoop.hbase.zookeeper.ZKUtil; import org.apache.hadoop.hbase.zookeeper.ZNodePaths; @@ -60,7 +61,7 @@ public class TestMetaShutdownHandler { @BeforeClass public static void setUpBeforeClass() throws Exception { - TEST_UTIL.startMiniCluster(1, 3, null, null, MyRegionServer.class); + TEST_UTIL.startMiniCluster(2, 4, null, null, MyRegionServer.class); } @AfterClass @@ -98,7 +99,6 @@ public class TestMetaShutdownHandler { RegionState metaState = MetaTableLocator.getMetaRegionState(master.getZooKeeper()); assertEquals("Wrong state for meta!", RegionState.State.OPEN, metaState.getState()); assertNotEquals("Meta is on master!", metaServerName, master.getServerName()); - // Delete the ephemeral node of the meta-carrying region server. // This is trigger the expire of this region server on the master. String rsEphemeralNodePath = @@ -115,11 +115,12 @@ public class TestMetaShutdownHandler { && !serverManager.areDeadServersInProgress(); } }); + master = cluster.getMaster(); LOG.info("Past wait on RIT"); TEST_UTIL.waitUntilNoRegionsInTransition(60000); // Now, make sure meta is assigned assertTrue("Meta should be assigned", - regionStates.isRegionOnline(HRegionInfo.FIRST_META_REGIONINFO)); + master.getAssignmentManager().getRegionStates().isRegionOnline(HRegionInfo.FIRST_META_REGIONINFO)); // Now, make sure meta is registered in zk metaState = MetaTableLocator.getMetaRegionState(master.getZooKeeper()); assertEquals("Meta should not be in transition", RegionState.State.OPEN, @@ -130,10 +131,63 @@ public class TestMetaShutdownHandler { metaState.getServerName(), metaServerName); } + /** + * Master should be able to recover from any unexpected state of meta-region-server znode. + */ + @Test + public void testMetaAssignmentFailure() throws Exception { + final MiniHBaseCluster cluster = TEST_UTIL.getHBaseCluster(); + HMaster master = cluster.getMaster(); + RegionStates regionStates = master.getAssignmentManager().getRegionStates(); + ServerName metaServerName = + regionStates.getRegionServerOfRegion(HRegionInfo.FIRST_META_REGIONINFO); + if (master.getServerName().equals(metaServerName) || metaServerName == null + || !metaServerName.equals(cluster.getServerHoldingMeta())) { + // Move meta off master so that SCP for meta RIT should not be triggered when we take master + // down + metaServerName = + cluster.getLiveRegionServerThreads().get(0).getRegionServer().getServerName(); + master.move(HRegionInfo.FIRST_META_REGIONINFO.getEncodedNameAsBytes(), + Bytes.toBytes(metaServerName.getServerName())); + TEST_UTIL.waitUntilNoRegionsInTransition(60000); + metaServerName = regionStates.getRegionServerOfRegion(HRegionInfo.FIRST_META_REGIONINFO); + } + RegionState metaState = MetaTableLocator.getMetaRegionState(master.getZooKeeper()); + assertEquals("Wrong state for meta!", RegionState.State.OPEN, metaState.getState()); + assertNotEquals("Meta is on master!", metaServerName, master.getServerName()); + + // Setting meta state to incorrect state OPENING, to see if master restarts or standby node can + // recover it + MetaTableLocator.setMetaLocation(master.getZooKeeper(), metaServerName, + RegionState.State.OPENING); + TEST_UTIL.decrementMinRegionServerCount(); + master.abort("Abort to test whether standby assign the meta OPENING region"); + final HMaster oldMaster = master; + TEST_UTIL.waitFor(120000, 200, new Waiter.Predicate() { + @Override + public boolean evaluate() throws Exception { + // test that standby master should be able to recover meta + return cluster.getMaster() != null && cluster.getMaster().isInitialized() + && oldMaster != cluster.getMaster(); + } + }); + master = cluster.getMaster(); + LOG.info("Past wait on RIT"); + TEST_UTIL.waitUntilNoRegionsInTransition(60000); + // Now, make sure meta is assigned + assertTrue("Meta should be assigned", master.getAssignmentManager().getRegionStates() + .isRegionOnline(HRegionInfo.FIRST_META_REGIONINFO)); + // Now, make sure meta is registered in zk as well + metaState = MetaTableLocator.getMetaRegionState(master.getZooKeeper()); + assertEquals("Meta should not be in transition", RegionState.State.OPEN, metaState.getState()); + assertEquals("Meta should be assigned", metaState.getServerName(), master.getAssignmentManager() + .getRegionStates().getRegionServerOfRegion(HRegionInfo.FIRST_META_REGIONINFO)); + } + public static class MyRegionServer extends MiniHBaseClusterRegionServer { - public MyRegionServer(Configuration conf) throws IOException, KeeperException, - InterruptedException { + public MyRegionServer(Configuration conf) + throws IOException, KeeperException, InterruptedException { super(conf); } diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/MockMasterServices.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/MockMasterServices.java index ec6b82ef77..9e7b0e6274 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/MockMasterServices.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/MockMasterServices.java @@ -324,10 +324,6 @@ public class MockMasterServices extends MockNoopMasterServices { public MockRegionStateStore(final MasterServices master) { super(master); } - - @Override - public void updateRegionLocation(RegionStates.RegionStateNode regionNode) throws IOException { - } } @Override diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestMetaAssignmentFailureDuringServerCrash.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestMetaAssignmentFailureDuringServerCrash.java new file mode 100644 index 0000000000..6d8072bd11 --- /dev/null +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestMetaAssignmentFailureDuringServerCrash.java @@ -0,0 +1,245 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.hadoop.hbase.master.procedure; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +import java.io.IOException; +import java.util.Arrays; +import java.util.HashSet; +import java.util.NavigableMap; +import java.util.Set; +import java.util.SortedSet; +import java.util.concurrent.ConcurrentSkipListMap; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.atomic.AtomicBoolean; +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.hbase.HBaseClassTestRule; +import org.apache.hadoop.hbase.HBaseTestingUtility; +import org.apache.hadoop.hbase.ServerName; +import org.apache.hadoop.hbase.client.RegionInfo; +import org.apache.hadoop.hbase.client.RegionInfoBuilder; +import org.apache.hadoop.hbase.master.MasterServices; +import org.apache.hadoop.hbase.master.RegionState; +import org.apache.hadoop.hbase.master.RegionState.State; +import org.apache.hadoop.hbase.master.assignment.AssignProcedure; +import org.apache.hadoop.hbase.master.assignment.AssignmentManager; +import org.apache.hadoop.hbase.master.assignment.MockMasterServices; +import org.apache.hadoop.hbase.master.assignment.RegionStates; +import org.apache.hadoop.hbase.master.assignment.RegionStates.RegionStateNode; +import org.apache.hadoop.hbase.procedure2.NoNodeDispatchException; +import org.apache.hadoop.hbase.procedure2.NoServerDispatchException; +import org.apache.hadoop.hbase.procedure2.NullTargetServerDispatchException; +import org.apache.hadoop.hbase.procedure2.ProcedureSuspendedException; +import org.apache.hadoop.hbase.testclassification.MasterTests; +import org.apache.hadoop.hbase.testclassification.SmallTests; +import org.apache.hadoop.hbase.util.FSUtils; +import org.apache.hadoop.hbase.zookeeper.MetaTableLocator; +import org.apache.hadoop.hbase.zookeeper.ZKUtil; +import org.apache.hadoop.hbase.zookeeper.ZKWatcher; +import org.apache.hadoop.hbase.zookeeper.ZNodePaths; +import org.junit.Before; +import org.junit.ClassRule; +import org.junit.Test; +import org.junit.experimental.categories.Category; +import org.mockito.Mockito; + +import org.apache.hadoop.hbase.shaded.protobuf.generated.AdminProtos.ExecuteProceduresRequest; +import org.apache.hadoop.hbase.shaded.protobuf.generated.AdminProtos.ExecuteProceduresResponse; + +@Category({ MasterTests.class, SmallTests.class }) +public class TestMetaAssignmentFailureDuringServerCrash { + private HBaseTestingUtility UTIL; + private MockRSProcedureDispatcher rsDispatcher; + private MockMasterServices master; + private AssignmentManager am; + private NavigableMap> regionsToRegionServers = + new ConcurrentSkipListMap>(); + private ZKWatcher zookeeper; + private static final CountDownLatch latch = new CountDownLatch(2); + + private void setupConfiguration(Configuration conf) throws Exception { + FSUtils.setRootDir(conf, UTIL.getDataTestDir()); + conf.setInt(MasterProcedureConstants.MASTER_PROCEDURE_THREADS, 5); + conf.setInt(RSProcedureDispatcher.RS_RPC_STARTUP_WAIT_TIME_CONF_KEY, 1000); + conf.setInt(AssignmentManager.ASSIGN_MAX_ATTEMPTS, 2); + } + + @Before + public void setUp() throws Exception { + UTIL = new HBaseTestingUtility(); + UTIL.startMiniZKCluster(); + setupConfiguration(UTIL.getConfiguration()); + this.zookeeper = UTIL.getZooKeeperWatcher(); + master = new MockMasterServices(UTIL.getConfiguration(), this.regionsToRegionServers) { + public org.apache.hadoop.hbase.zookeeper.ZKWatcher getZooKeeper() { + return zookeeper; + } + }; + rsDispatcher = new MockRSProcedureDispatcher(master); + master.start(2, rsDispatcher); + am = master.getAssignmentManager(); + } + + @ClassRule + public static final HBaseClassTestRule CLASS_RULE = + HBaseClassTestRule.forClass(TestMetaAssignmentFailureDuringServerCrash.class); + + private interface MockRSExecutor { + ExecuteProceduresResponse sendRequest(ServerName server, ExecuteProceduresRequest req) + throws IOException; + } + + private class BadRsExecutor implements MockRSExecutor { + @Override + public ExecuteProceduresResponse sendRequest(ServerName server, ExecuteProceduresRequest req) + throws IOException { + latch.countDown(); + // we will reject every request + throw new IOException("can not open"); + + } + } + + private class MockRSProcedureDispatcher extends RSProcedureDispatcher { + private MockRSExecutor mockRsExec; + + public MockRSProcedureDispatcher(final MasterServices master) { + super(master); + } + + public void setMockRsExecutor(final MockRSExecutor mockRsExec) { + this.mockRsExec = mockRsExec; + } + + @Override + public void addOperationToNode(ServerName key, + org.apache.hadoop.hbase.procedure2.RemoteProcedureDispatcher.RemoteProcedure rp) + throws NullTargetServerDispatchException, NoServerDispatchException, + NoNodeDispatchException { + // Directly go to remoteDispatch to avoid managing nodeMap etc + remoteDispatch(key, new HashSet(Arrays.asList(rp))); + } + + @Override + protected void remoteDispatch(ServerName serverName, Set remoteProcedures) { + submitTask(new MockRemoteCall(serverName, remoteProcedures)); + } + + private class MockRemoteCall extends ExecuteProceduresRemoteCall { + public MockRemoteCall(final ServerName serverName, final Set operations) { + super(serverName, operations); + } + + @Override + protected ExecuteProceduresResponse sendRequest(final ServerName serverName, + final ExecuteProceduresRequest request) throws IOException { + return mockRsExec.sendRequest(serverName, request); + } + } + } + + public static class AssignProcedureRemoteServerUnavailable extends AssignProcedure { + public final AtomicBoolean remoteCallFailedWasCalled = new AtomicBoolean(false); + private final RegionStates.RegionStateNode rsn; + + public AssignProcedureRemoteServerUnavailable(RegionInfo regionInfo, + RegionStates.RegionStateNode rsn) { + super(regionInfo); + this.rsn = rsn; + } + + /** + * Override so can change access from protected to public. + */ + @Override + public boolean updateTransition(MasterProcedureEnv env, RegionStates.RegionStateNode regionNode) + throws IOException, ProcedureSuspendedException { + return super.updateTransition(env, regionNode); + } + + /** + * Override so can change access from protected to public. + */ + @Override + public boolean startTransition(MasterProcedureEnv env, RegionStateNode regionNode) + throws IOException { + return super.startTransition(env, regionNode); + } + + @Override + public RegionStates.RegionStateNode getRegionState(MasterProcedureEnv env) { + // Do this so we don't have to mock a bunch of stuff. + return this.rsn; + } + + @Override + public void remoteCallFailed(final MasterProcedureEnv env, final ServerName serverName, + final IOException exception) { + try { + super.remoteCallFailed(env, serverName, exception); + } finally { + this.remoteCallFailedWasCalled.set(true); + latch.countDown(); + } + } + }; + + /** + * Test if Assign procedure fails to assign Meta to a bad server, meta should be left in a OFFLINE + * state after failure + * @throws Exception + */ + @Test + public void testMetaAssignmentWithBadServer() throws Exception { + RegionInfo ri = RegionInfoBuilder.FIRST_META_REGIONINFO; + RegionStates.RegionStateNode rsn = new RegionStates.RegionStateNode(ri); + rsn.setRegionLocation(ServerName.valueOf("localhost", 100, 1)); + // RS which will reject any request, here we are testing open region request + BadRsExecutor badRsExecutor = new BadRsExecutor(); + rsDispatcher.setMockRsExecutor(badRsExecutor); + MasterProcedureEnv env = Mockito.mock(MasterProcedureEnv.class); + Mockito.when(env.getAssignmentManager()).thenReturn(am); + Mockito.when(env.getMasterServices()).thenReturn(master); + Mockito.when(env.getRemoteDispatcher()).thenReturn(rsDispatcher); + + AssignProcedureRemoteServerUnavailable assignProcedure = + new AssignProcedureRemoteServerUnavailable(ri, rsn); + ZNodePaths znodePaths = new ZNodePaths(UTIL.getConfiguration()); + ZKUtil.createWithParents(zookeeper, znodePaths.baseZNode); + am.markRegionAsOpening(rsn); + // after marking region for OPENING , meta-regionserver znode will be in OPENING state. + RegionState metaRegionState = MetaTableLocator.getMetaRegionState(zookeeper); + assertEquals("Meta should be in OPENING state", RegionState.State.OPENING, + metaRegionState.getState()); + // Update transition request will fail because bad regionserver will reject the open region + // request + assignProcedure.updateTransition(env, rsn); + // wait for undo to complete + latch.await(); + metaRegionState = MetaTableLocator.getMetaRegionState(zookeeper); + assertTrue(assignProcedure.remoteCallFailedWasCalled.get()); + + assertTrue(rsn.isInState(State.OFFLINE)); + // meta znode should be in OFFLINE state after UNDO opening + assertEquals("Meta should be in OFFLINE state after failure", RegionState.State.OFFLINE, + metaRegionState.getState()); + } + +}