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 5baf30846e08..a9ae6f72a29b 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 @@ -2232,8 +2232,13 @@ CompletableFuture regionOpening(RegionStateNode regionNode) { } // should be called under the RegionStateNode lock - // The parameter 'giveUp' means whether we will try to open the region again, if it is true, then - // we will persist the FAILED_OPEN state into hbase:meta. + // Called when a region open attempt has failed. The 'giveUp' parameter says whether we will try + // to open the region again. If true, this is a terminal failure with no more retries: we set + // RegionState.State to FAILED_OPEN and persist it to hbase:meta, and only remove the region from + // its old target server's bookkeeping once that persist succeeds. If false, we are about to + // retry the open with a new plan, so RegionState.State is intentionally left untouched (e.g. + // still OPENING), meta is not touched, and we remove the region from its old target server's + // bookkeeping immediately. In both cases the removal only happens if there was an old location. CompletableFuture regionFailedOpen(RegionStateNode regionNode, boolean giveUp) { RegionState.State state = regionNode.getState(); ServerName regionLocation = regionNode.getRegionLocation(); diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/CloseRegionProcedure.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/CloseRegionProcedure.java index 344d7e9ad7e6..23068b3fa088 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/CloseRegionProcedure.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/CloseRegionProcedure.java @@ -113,8 +113,9 @@ protected void updateTransitionWithoutPersistingToMeta(MasterProcedureEnv env, } @Override - protected void restoreSucceedState(AssignmentManager am, RegionStateNode regionNode, long seqId) - throws IOException { + protected void restoreSucceedState(AssignmentManager am, RegionStateNode regionNode, + TransitionCode transitionCode, long seqId) throws IOException { + // CLOSE has no failure variant, so transitionCode is always CLOSED here if (regionNode.getState() == State.CLOSED) { // should have already been persisted, ignore return; diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/OpenRegionProcedure.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/OpenRegionProcedure.java index 21218a1177a6..8e5151f2c808 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/OpenRegionProcedure.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/OpenRegionProcedure.java @@ -127,7 +127,12 @@ protected void updateTransitionWithoutPersistingToMeta(MasterProcedureEnv env, @Override protected void restoreSucceedState(AssignmentManager am, RegionStateNode regionNode, - long openSeqNum) throws IOException { + TransitionCode transitionCode, long openSeqNum) throws IOException { + if (transitionCode == TransitionCode.FAILED_OPEN) { + // will not persist to meta if giveUp is false, matches the live reportTransition path + am.regionFailedOpen(regionNode, false); + return; + } if (regionNode.getState() == State.OPEN) { // should have already been persisted, ignore return; diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/RegionRemoteProcedureBase.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/RegionRemoteProcedureBase.java index fe8c9bfdd7c1..dc0fb5defe2e 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/RegionRemoteProcedureBase.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/RegionRemoteProcedureBase.java @@ -268,12 +268,12 @@ void serverCrashed(MasterProcedureEnv env, RegionStateNode regionNode, ServerNam } protected abstract void restoreSucceedState(AssignmentManager am, RegionStateNode regionNode, - long seqId) throws IOException; + TransitionCode transitionCode, long seqId) throws IOException; void stateLoaded(AssignmentManager am, RegionStateNode regionNode) { if (state == RegionRemoteProcedureBaseState.REGION_REMOTE_PROCEDURE_REPORT_SUCCEED) { try { - restoreSucceedState(am, regionNode, seqId); + restoreSucceedState(am, regionNode, transitionCode, seqId); } catch (IOException e) { // should not happen as we are just restoring the state throw new AssertionError(e); diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/TestOpenRegionProcedureRestoreFailedOpen.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/TestOpenRegionProcedureRestoreFailedOpen.java new file mode 100644 index 000000000000..8f4df4f43162 --- /dev/null +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/TestOpenRegionProcedureRestoreFailedOpen.java @@ -0,0 +1,190 @@ +/* + * 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.assignment; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.io.IOException; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicReference; +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.hbase.HBaseTestingUtil; +import org.apache.hadoop.hbase.HConstants; +import org.apache.hadoop.hbase.PleaseHoldException; +import org.apache.hadoop.hbase.TableName; +import org.apache.hadoop.hbase.client.RegionInfo; +import org.apache.hadoop.hbase.master.HMaster; +import org.apache.hadoop.hbase.master.MasterServices; +import org.apache.hadoop.hbase.master.RegionPlan; +import org.apache.hadoop.hbase.master.RegionState; +import org.apache.hadoop.hbase.master.procedure.MasterProcedureEnv; +import org.apache.hadoop.hbase.master.procedure.MasterProcedureTestingUtility; +import org.apache.hadoop.hbase.master.region.MasterRegion; +import org.apache.hadoop.hbase.procedure2.ProcedureExecutor; +import org.apache.hadoop.hbase.testclassification.MasterTests; +import org.apache.hadoop.hbase.testclassification.MediumTests; +import org.apache.hadoop.hbase.util.Bytes; +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Tag; +import org.junit.jupiter.api.Test; + +import org.apache.hadoop.hbase.shaded.protobuf.ProtobufUtil; +import org.apache.hadoop.hbase.shaded.protobuf.generated.RegionServerStatusProtos.RegionStateTransition; +import org.apache.hadoop.hbase.shaded.protobuf.generated.RegionServerStatusProtos.RegionStateTransition.TransitionCode; +import org.apache.hadoop.hbase.shaded.protobuf.generated.RegionServerStatusProtos.ReportRegionStateTransitionRequest; +import org.apache.hadoop.hbase.shaded.protobuf.generated.RegionServerStatusProtos.ReportRegionStateTransitionResponse; + +/** + * HBASE-30357: OpenRegionProcedure#restoreSucceedState() must not force the region state to OPEN on + * master-failover restore when the persisted transition code is actually FAILED_OPEN. + *

+ * To reproduce the exact crash window without racing a genuinely reporting RS, we intercept the + * RS's real OPENED report on the master side and rewrite it to FAILED_OPEN before it is persisted. + * While still on the RPC handler thread (i.e. before the woken child OpenRegionProcedure gets a + * chance to run its own execute() and persist anything to meta), we lock the RegionStateNode and + * perform a genuine restart of the master's ProcedureExecutor/AssignmentManager, forcing a real + * reload from the WALProcedureStore and hbase:meta - exactly the mechanism restoreSucceedState() is + * meant to handle. + */ +@Tag(MasterTests.TAG) +@Tag(MediumTests.TAG) +public class TestOpenRegionProcedureRestoreFailedOpen { + + private static final long AWAIT_TIMEOUT_SECONDS = 30; + + private static final AtomicReference ARRIVE = new AtomicReference<>(); + + private static final AtomicReference PROCEED = new AtomicReference<>(); + + private static final class AssignmentManagerForTest extends AssignmentManager { + + public AssignmentManagerForTest(MasterServices master, MasterRegion masterRegion) { + super(master, masterRegion); + } + + @Override + public ReportRegionStateTransitionResponse reportRegionStateTransition( + ReportRegionStateTransitionRequest req) throws PleaseHoldException { + RegionStateTransition transition = req.getTransition(0); + RegionInfo hri = ProtobufUtil.toRegionInfo(transition.getRegionInfo(0)); + if (transition.getTransitionCode() != TransitionCode.OPENED || !hri.getTable().equals(NAME)) { + return super.reportRegionStateTransition(req); + } + CountDownLatch arrive = ARRIVE.getAndSet(null); + if (arrive == null) { + return super.reportRegionStateTransition(req); + } + ReportRegionStateTransitionRequest failedOpenReq = req.toBuilder() + .setTransition(0, transition.toBuilder().setTransitionCode(TransitionCode.FAILED_OPEN) + .setOpenSeqNum(HConstants.NO_SEQNUM).build()) + .build(); + RegionStateNode regionNode = getRegionStates().getRegionStateNode(hri); + // AssignmentManager#updateRegionTransition() (called from super.reportRegionStateTransition + // below) also locks this same RegionStateNode; that only works here because the lock is + // reentrant for the same thread (see RegionStateNodeLock#lock0). + regionNode.lock(); + try { + // persists REPORT_SUCCEED/FAILED_OPEN to the real WALProcedureStore and wakes the child + // OpenRegionProcedure, but since we still hold the RegionStateNode lock here (reentrant, + // same thread), the woken child can not resume and complete its own meta update - this is + // exactly the window a real master crash would leave us in. + ReportRegionStateTransitionResponse resp = super.reportRegionStateTransition(failedOpenReq); + arrive.countDown(); + CountDownLatch proceed = PROCEED.get(); + if (!proceed.await(AWAIT_TIMEOUT_SECONDS, TimeUnit.SECONDS)) { + throw new RuntimeException("Timed out waiting for PROCEED"); + } + return resp; + } catch (InterruptedException e) { + throw new RuntimeException(e); + } finally { + regionNode.unlock(); + } + } + } + + public static final class HMasterForTest extends HMaster { + + public HMasterForTest(Configuration conf) throws IOException { + super(conf); + } + + @Override + protected AssignmentManager createAssignmentManager(MasterServices master, + MasterRegion masterRegion) { + return new AssignmentManagerForTest(master, masterRegion); + } + } + + private static final HBaseTestingUtil UTIL = new HBaseTestingUtil(); + + private static final TableName NAME = + TableName.valueOf("TestOpenRegionProcedureRestoreFailedOpen"); + + private static final byte[] CF = Bytes.toBytes("cf"); + + @BeforeAll + public static void setUpBeforeClass() throws Exception { + UTIL.getConfiguration().setClass(HConstants.MASTER_IMPL, HMasterForTest.class, HMaster.class); + UTIL.startMiniCluster(1); + UTIL.createTable(NAME, CF); + UTIL.waitTableAvailable(NAME); + } + + @AfterAll + public static void tearDownAfterClass() throws Exception { + UTIL.shutdownMiniCluster(); + } + + @Test + public void testRestoreDoesNotForceOpenAfterFailedOpen() throws Exception { + HMaster master = UTIL.getMiniHBaseCluster().getMaster(); + ProcedureExecutor procExec = master.getMasterProcedureExecutor(); + AssignmentManager am = master.getAssignmentManager(); + RegionInfo region = UTIL.getAdmin().getRegions(NAME).get(0); + RegionStateNode regionNode = am.getRegionStates().getRegionStateNode(region); + + CountDownLatch arrive = new CountDownLatch(1); + CountDownLatch proceed = new CountDownLatch(1); + ARRIVE.set(arrive); + PROCEED.set(proceed); + Future future = am.moveAsync( + new RegionPlan(region, regionNode.getRegionLocation(), regionNode.getRegionLocation())); + // arrive counts down only after the RS's OPENED report is intercepted, rewritten to + // FAILED_OPEN, and persisted, so waiting on it alone is sufficient synchronization + assertTrue(arrive.await(AWAIT_TIMEOUT_SECONDS, TimeUnit.SECONDS)); + + MasterProcedureTestingUtility.restartMasterProcedureExecutor(procExec); + RegionStateNode reloaded = am.getRegionStates().getRegionStateNode(region); + // still OPENING: restarting the ProcedureExecutor re-triggers AssignmentManager#joinCluster's + // meta scan, which reloads the state from the persisted hbase:meta column before + // restoreSucceedState() runs; regionFailedOpen(regionNode, false) then only detaches the + // region from its (now-defunct) server, it does not change the state. + // TransitRegionStateProcedure is the one that decides to give up + // (and thus set FAILED_OPEN) or retry the open, and with the + // default (effectively unbounded) hbase.assignment.maximum.attempts, it never gives up here. + assertEquals(RegionState.State.OPENING, reloaded.getState()); + + proceed.countDown(); + future.get(AWAIT_TIMEOUT_SECONDS, TimeUnit.SECONDS); + } +}