From 836e6a187cb2c1cffa4bacea2b2a4bd5f1acf9f5 Mon Sep 17 00:00:00 2001 From: Josh Elser Date: Thu, 15 Feb 2018 18:00:09 -0500 Subject: [PATCH] HBASE-19953 Ensure post DDL hooks are only called after successful operations The 1.x functionality of Master DDL operations is that "post" observer hooks are only invoked when the DDL action was successful. With the async-ness of ProcV2, we find ourselves in a case where the post-hook may be invoked before the Procedure runs and fails. We need to introduce some blocking to wait and see if the Procedure is going to fail on a precondition before invoking the hook. --- .../hbase/master/ClusterSchemaServiceImpl.java | 2 +- .../org/apache/hadoop/hbase/master/HMaster.java | 12 +- .../AbstractStateMachineNamespaceProcedure.java | 13 ++ .../master/procedure/DeleteNamespaceProcedure.java | 33 +++-- .../master/procedure/ModifyNamespaceProcedure.java | 29 +++- .../master/procedure/ProcedurePrepareLatch.java | 7 + .../procedure/TestMasterObserverPostCalls.java | 151 +++++++++++++++++++++ 7 files changed, 231 insertions(+), 16 deletions(-) create mode 100644 hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestMasterObserverPostCalls.java diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/ClusterSchemaServiceImpl.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/ClusterSchemaServiceImpl.java index 4dd8de0152..0aefc676fd 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/ClusterSchemaServiceImpl.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/ClusterSchemaServiceImpl.java @@ -99,7 +99,7 @@ class ClusterSchemaServiceImpl extends AbstractService implements ClusterSchemaS public long modifyNamespace(NamespaceDescriptor namespaceDescriptor, final NonceKey nonceKey) throws IOException { return submitProcedure(new ModifyNamespaceProcedure( - this.masterServices.getMasterProcedureExecutor().getEnvironment(), namespaceDescriptor), + this.masterServices.getMasterProcedureExecutor().getEnvironment(), namespaceDescriptor, null), nonceKey); } 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 aa2213043b..d7fd0ba511 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 @@ -118,6 +118,7 @@ import org.apache.hadoop.hbase.master.normalizer.RegionNormalizer; import org.apache.hadoop.hbase.master.normalizer.RegionNormalizerChore; import org.apache.hadoop.hbase.master.normalizer.RegionNormalizerFactory; import org.apache.hadoop.hbase.master.procedure.CreateTableProcedure; +import org.apache.hadoop.hbase.master.procedure.DeleteNamespaceProcedure; import org.apache.hadoop.hbase.master.procedure.DeleteTableProcedure; import org.apache.hadoop.hbase.master.procedure.DisableTableProcedure; import org.apache.hadoop.hbase.master.procedure.EnableTableProcedure; @@ -125,6 +126,7 @@ import org.apache.hadoop.hbase.master.procedure.MasterProcedureConstants; import org.apache.hadoop.hbase.master.procedure.MasterProcedureEnv; import org.apache.hadoop.hbase.master.procedure.MasterProcedureScheduler; import org.apache.hadoop.hbase.master.procedure.MasterProcedureUtil; +import org.apache.hadoop.hbase.master.procedure.ModifyNamespaceProcedure; import org.apache.hadoop.hbase.master.procedure.ModifyTableProcedure; import org.apache.hadoop.hbase.master.procedure.ProcedurePrepareLatch; import org.apache.hadoop.hbase.master.procedure.RecoverMetaProcedure; @@ -2965,10 +2967,13 @@ public class HMaster extends HRegionServer implements MasterServices { @Override protected void run() throws IOException { getMaster().getMasterCoprocessorHost().preModifyNamespace(namespaceDescriptor); + ProcedurePrepareLatch latch = ProcedurePrepareLatch.createBlockingLatch(); LOG.info(getClientIdAuditPrefix() + " modify " + namespaceDescriptor); // Execute the operation synchronously - wait for the operation to complete before // continuing. - setProcId(getClusterSchema().modifyNamespace(namespaceDescriptor, getNonceKey())); + setProcId(submitProcedure(new ModifyNamespaceProcedure( + getMasterProcedureExecutor().getEnvironment(), namespaceDescriptor, latch))); + latch.await(); getMaster().getMasterCoprocessorHost().postModifyNamespace(namespaceDescriptor); } @@ -2998,7 +3003,10 @@ public class HMaster extends HRegionServer implements MasterServices { LOG.info(getClientIdAuditPrefix() + " delete " + name); // Execute the operation synchronously - wait for the operation to complete before // continuing. - setProcId(getClusterSchema().deleteNamespace(name, getNonceKey())); + ProcedurePrepareLatch latch = ProcedurePrepareLatch.createBlockingLatch(); + setProcId(submitProcedure(new DeleteNamespaceProcedure(procedureExecutor.getEnvironment(), name, latch))); + latch.await(); + // Will not be invoked in the face of Exception thrown by the Procedure's execution getMaster().getMasterCoprocessorHost().postDeleteNamespace(name); } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/AbstractStateMachineNamespaceProcedure.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/AbstractStateMachineNamespaceProcedure.java index 2c6ae34b6c..574706a9f4 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/AbstractStateMachineNamespaceProcedure.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/AbstractStateMachineNamespaceProcedure.java @@ -31,12 +31,21 @@ public abstract class AbstractStateMachineNamespaceProcedure extends StateMachineProcedure implements TableProcedureInterface { + private final ProcedurePrepareLatch syncLatch; + protected AbstractStateMachineNamespaceProcedure() { // Required by the Procedure framework to create the procedure on replay + syncLatch = null; } protected AbstractStateMachineNamespaceProcedure(final MasterProcedureEnv env) { + this(env, null); + } + + protected AbstractStateMachineNamespaceProcedure(final MasterProcedureEnv env, + final ProcedurePrepareLatch latch) { this.setOwner(env.getRequestUser()); + this.syncLatch = latch; } protected abstract String getNamespaceName(); @@ -69,4 +78,8 @@ public abstract class AbstractStateMachineNamespaceProcedure protected void releaseLock(final MasterProcedureEnv env) { env.getProcedureScheduler().wakeNamespaceExclusiveLock(this, getNamespaceName()); } + + protected void releaseSyncLatch() { + ProcedurePrepareLatch.releaseLatch(syncLatch, this); + } } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/DeleteNamespaceProcedure.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/DeleteNamespaceProcedure.java index 1c587eb55d..537c4b7af2 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/DeleteNamespaceProcedure.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/DeleteNamespaceProcedure.java @@ -57,7 +57,12 @@ public class DeleteNamespaceProcedure } public DeleteNamespaceProcedure(final MasterProcedureEnv env, final String namespaceName) { - super(env); + this(env, namespaceName, null); + } + + public DeleteNamespaceProcedure(final MasterProcedureEnv env, final String namespaceName, + final ProcedurePrepareLatch latch) { + super(env, latch); this.namespaceName = namespaceName; this.nsDescriptor = null; this.traceEnabled = null; @@ -74,7 +79,12 @@ public class DeleteNamespaceProcedure try { switch (state) { case DELETE_NAMESPACE_PREPARE: - prepareDelete(env); + boolean present = prepareDelete(env); + releaseSyncLatch(); + if (!present) { + assert isFailed() : "Delete namespace should have an exception here"; + return Flow.NO_MORE_STATE; + } setNextState(DeleteNamespaceState.DELETE_NAMESPACE_DELETE_FROM_NS_TABLE); break; case DELETE_NAMESPACE_DELETE_FROM_NS_TABLE: @@ -113,6 +123,7 @@ public class DeleteNamespaceProcedure // nothing to rollback, pre is just table-state checks. // We can fail if the table does not exist or is not disabled. // TODO: coprocessor rollback semantic is still undefined. + releaseSyncLatch(); return; } @@ -188,27 +199,33 @@ public class DeleteNamespaceProcedure * @param env MasterProcedureEnv * @throws IOException */ - private void prepareDelete(final MasterProcedureEnv env) throws IOException { + private boolean prepareDelete(final MasterProcedureEnv env) throws IOException { if (getTableNamespaceManager(env).doesNamespaceExist(namespaceName) == false) { - throw new NamespaceNotFoundException(namespaceName); + setFailure("master-delete-namespace", new NamespaceNotFoundException(namespaceName)); + return false; } if (NamespaceDescriptor.RESERVED_NAMESPACES.contains(namespaceName)) { - throw new ConstraintException("Reserved namespace "+ namespaceName +" cannot be removed."); + setFailure("master-delete-namespace", new ConstraintException( + "Reserved namespace "+ namespaceName +" cannot be removed.")); + return false; } int tableCount = 0; try { tableCount = env.getMasterServices().listTableDescriptorsByNamespace(namespaceName).size(); } catch (FileNotFoundException fnfe) { - throw new NamespaceNotFoundException(namespaceName); + setFailure("master-delete-namespace", new NamespaceNotFoundException(namespaceName)); + return false; } if (tableCount > 0) { - throw new ConstraintException("Only empty namespaces can be removed. " + - "Namespace "+ namespaceName + " has "+ tableCount +" tables"); + setFailure("master-delete-namespace", new ConstraintException("Only empty namespaces can be removed. " + + "Namespace "+ namespaceName + " has "+ tableCount +" tables")); + return false; } // This is used for rollback nsDescriptor = getTableNamespaceManager(env).get(namespaceName); + return true; } /** diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/ModifyNamespaceProcedure.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/ModifyNamespaceProcedure.java index 2a7dc5b28b..f21b6d07b7 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/ModifyNamespaceProcedure.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/ModifyNamespaceProcedure.java @@ -22,6 +22,7 @@ import java.io.IOException; import org.apache.hadoop.hbase.NamespaceDescriptor; import org.apache.hadoop.hbase.NamespaceNotFoundException; +import org.apache.hadoop.hbase.constraint.ConstraintException; import org.apache.yetus.audience.InterfaceAudience; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -50,7 +51,12 @@ public class ModifyNamespaceProcedure public ModifyNamespaceProcedure(final MasterProcedureEnv env, final NamespaceDescriptor newNsDescriptor) { - super(env); + this(env, newNsDescriptor, null); + } + + public ModifyNamespaceProcedure(final MasterProcedureEnv env, + final NamespaceDescriptor newNsDescriptor, final ProcedurePrepareLatch latch) { + super(env, latch); this.oldNsDescriptor = null; this.newNsDescriptor = newNsDescriptor; this.traceEnabled = null; @@ -66,7 +72,12 @@ public class ModifyNamespaceProcedure try { switch (state) { case MODIFY_NAMESPACE_PREPARE: - prepareModify(env); + boolean success = prepareModify(env); + releaseSyncLatch(); + if (!success) { + assert isFailed() : "Modify namespace should have an exception here"; + return Flow.NO_MORE_STATE; + } setNextState(ModifyNamespaceState.MODIFY_NAMESPACE_UPDATE_NS_TABLE); break; case MODIFY_NAMESPACE_UPDATE_NS_TABLE: @@ -96,6 +107,7 @@ public class ModifyNamespaceProcedure if (state == ModifyNamespaceState.MODIFY_NAMESPACE_PREPARE) { // nothing to rollback, pre-modify is just checks. // TODO: coprocessor rollback semantic is still undefined. + releaseSyncLatch(); return; } @@ -173,14 +185,21 @@ public class ModifyNamespaceProcedure * @param env MasterProcedureEnv * @throws IOException */ - private void prepareModify(final MasterProcedureEnv env) throws IOException { + private boolean prepareModify(final MasterProcedureEnv env) throws IOException { if (getTableNamespaceManager(env).doesNamespaceExist(newNsDescriptor.getName()) == false) { - throw new NamespaceNotFoundException(newNsDescriptor.getName()); + setFailure("master-modify-namespace", new NamespaceNotFoundException(newNsDescriptor.getName())); + return false; + } + try { + getTableNamespaceManager(env).validateTableAndRegionCount(newNsDescriptor); + } catch (ConstraintException e) { + setFailure("master-modify-namespace", e); + return false; } - getTableNamespaceManager(env).validateTableAndRegionCount(newNsDescriptor); // This is used for rollback oldNsDescriptor = getTableNamespaceManager(env).get(newNsDescriptor.getName()); + return true; } /** diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/ProcedurePrepareLatch.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/ProcedurePrepareLatch.java index 535f2888dd..b267ffda8e 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/ProcedurePrepareLatch.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/ProcedurePrepareLatch.java @@ -57,6 +57,13 @@ public abstract class ProcedurePrepareLatch { return hasProcedureSupport(major, minor) ? noopLatch : new CompatibilityLatch(); } + /** + * Creates a latch which blocks. + */ + public static ProcedurePrepareLatch createBlockingLatch() { + return new CompatibilityLatch(); + } + private static boolean hasProcedureSupport(int major, int minor) { return VersionInfoUtil.currentClientHasMinimumVersion(major, minor); } diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestMasterObserverPostCalls.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestMasterObserverPostCalls.java new file mode 100644 index 0000000000..a2ae6d865e --- /dev/null +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestMasterObserverPostCalls.java @@ -0,0 +1,151 @@ +package org.apache.hadoop.hbase.master.procedure; + +import java.io.IOException; +import java.util.Optional; +import java.util.concurrent.atomic.AtomicInteger; + +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.hbase.CoprocessorEnvironment; +import org.apache.hadoop.hbase.HBaseClassTestRule; +import org.apache.hadoop.hbase.HBaseTestingUtility; +import org.apache.hadoop.hbase.NamespaceDescriptor; +import org.apache.hadoop.hbase.TableName; +import org.apache.hadoop.hbase.client.Admin; +import org.apache.hadoop.hbase.client.ColumnFamilyDescriptorBuilder; +import org.apache.hadoop.hbase.client.TableDescriptorBuilder; +import org.apache.hadoop.hbase.coprocessor.MasterCoprocessor; +import org.apache.hadoop.hbase.coprocessor.MasterCoprocessorEnvironment; +import org.apache.hadoop.hbase.coprocessor.MasterObserver; +import org.apache.hadoop.hbase.coprocessor.ObserverContext; +import org.apache.hadoop.hbase.master.HMaster; +import org.apache.hadoop.hbase.master.MasterCoprocessorHost; +import org.apache.hadoop.hbase.testclassification.MasterTests; +import org.apache.hadoop.hbase.testclassification.MediumTests; +import org.apache.hadoop.hbase.util.Bytes; +import org.junit.AfterClass; +import org.junit.BeforeClass; +import org.junit.ClassRule; +import org.junit.Test; +import org.junit.experimental.categories.Category; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.fail; + +@Category({MasterTests.class, MediumTests.class}) +public class TestMasterObserverPostCalls { + + @ClassRule + public static final HBaseClassTestRule CLASS_RULE = + HBaseClassTestRule.forClass(TestMasterObserverPostCalls.class); + + private static final Logger LOG = LoggerFactory.getLogger(TestMasterObserverPostCalls.class); + protected static final HBaseTestingUtility UTIL = new HBaseTestingUtility(); + + @BeforeClass + public static void setupCluster() throws Exception { + setupConf(UTIL.getConfiguration()); + UTIL.startMiniCluster(1); + } + + private static void setupConf(Configuration conf) { + conf.setInt(MasterProcedureConstants.MASTER_PROCEDURE_THREADS, 1); + conf.set(MasterCoprocessorHost.MASTER_COPROCESSOR_CONF_KEY, MasterObserverForTest.class.getName()); + } + + @AfterClass + public static void cleanupTest() throws Exception { + try { + UTIL.shutdownMiniCluster(); + } catch (Exception e) { + LOG.warn("failure shutting down cluster", e); + } + } + + public static class MasterObserverForTest implements MasterCoprocessor, MasterObserver { + private AtomicInteger postHookCalls = null; + + @Override + public Optional getMasterObserver() { + return Optional.of(this); + } + + @Override + public void start(@SuppressWarnings("rawtypes") CoprocessorEnvironment ctx) throws IOException { + this.postHookCalls = new AtomicInteger(0); + } + + @Override + public void postDeleteNamespace(ObserverContext ctx, + String namespace) { + postHookCalls.incrementAndGet(); + } + + @Override + public void postModifyNamespace(ObserverContext ctx, NamespaceDescriptor desc) { + postHookCalls.incrementAndGet(); + } + } + + @Test + public void testPostDeleteNamespace() throws IOException { + final Admin admin = UTIL.getAdmin(); + final String ns = "postdeletens"; + final TableName tn1 = TableName.valueOf(ns, "table1"); + + admin.createNamespace(NamespaceDescriptor.create(ns).build()); + admin.createTable(TableDescriptorBuilder.newBuilder(tn1) + .addColumnFamily(ColumnFamilyDescriptorBuilder.newBuilder(Bytes.toBytes("f1")).build()) + .build()); + + HMaster master = UTIL.getMiniHBaseCluster().getMaster(); + MasterObserverForTest observer = master.getMasterCoprocessorHost().findCoprocessor(MasterObserverForTest.class); + int preCount = observer.postHookCalls.get(); + try { + admin.deleteNamespace(ns); + fail("Deleting a non-empty namespace should be disallowed"); + } catch (IOException e) { + // Pass + } + int postCount = observer.postHookCalls.get(); + assertEquals("Expected no invocations of postDeleteNamespace when the operation fails", preCount, postCount); + + // Disable and delete the table so that we can delete the NS. + admin.disableTable(tn1); + admin.deleteTable(tn1); + + // Validate that the postDeletNS hook is invoked + preCount = observer.postHookCalls.get(); + admin.deleteNamespace(ns); + postCount = observer.postHookCalls.get(); + assertEquals("Expected 1 invocation of postDeleteNamespace", preCount + 1, postCount); + } + + @Test + public void testPostModifyNamespace() throws IOException { + final Admin admin = UTIL.getAdmin(); + final String ns = "postmodifyns"; + + NamespaceDescriptor nsDesc = NamespaceDescriptor.create(ns).build(); + admin.createNamespace(nsDesc); + + HMaster master = UTIL.getMiniHBaseCluster().getMaster(); + MasterObserverForTest observer = master.getMasterCoprocessorHost().findCoprocessor(MasterObserverForTest.class); + int preCount = observer.postHookCalls.get(); + try { + admin.modifyNamespace(NamespaceDescriptor.create("nonexistent").build()); + fail("Modifying a missing namespace should fail"); + } catch (IOException e) { + // Pass + } + int postCount = observer.postHookCalls.get(); + assertEquals("Expected no invocations of postModifyNamespace when the operation fails", preCount, postCount); + + // Validate that the postDeletNS hook is invoked + preCount = observer.postHookCalls.get(); + admin.modifyNamespace(NamespaceDescriptor.create(nsDesc).addConfiguration("foo", "bar").build()); + postCount = observer.postHookCalls.get(); + assertEquals("Expected 1 invocation of postModifyNamespace", preCount + 1, postCount); + } +} -- 2.16.1