From 49166ac2f31468d791b42a89aca78d1366bfb438 Mon Sep 17 00:00:00 2001 From: Keith Turner Date: Tue, 1 Apr 2025 17:21:43 +0000 Subject: [PATCH] Fixes race condition in deleting namespaces Namespace deletion checked that no tables existed in a namespace in the Accumulo client. No checks were done on the servers side. This has race conditions where the set of tables in a namespace changes after the client side check. Added a server side check that is done after the namespace lock is acquired. The client side check should be removed, however this would not be safe to do in a bug fix release. If the client side check is removed in 2.1.4 and a 2.1.4 client runs with a 2.1.3 server then nothing would do the check. To test this change the client side check was commented out and NamespaceIT.deleteNonEmptyNamespace() was run. This shows the change works now, but regressions in this new code will not be detected. In main/4.0 the client side check can be removed. --- .../clientImpl/NamespaceOperationsImpl.java | 11 +++++++ .../namespace/delete/DeleteNamespace.java | 30 +++++++++++++++++-- 2 files changed, 39 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/org/apache/accumulo/core/clientImpl/NamespaceOperationsImpl.java b/core/src/main/java/org/apache/accumulo/core/clientImpl/NamespaceOperationsImpl.java index 451496ed9c5..2d831f244f0 100644 --- a/core/src/main/java/org/apache/accumulo/core/clientImpl/NamespaceOperationsImpl.java +++ b/core/src/main/java/org/apache/accumulo/core/clientImpl/NamespaceOperationsImpl.java @@ -136,6 +136,11 @@ public void create(String namespace) } } + // This constant is used to pass information between server and client. When changing this + // consider the implication of the client and server processes running slightly different versions + // of software. This could cause the server processes to have different values for this constant. + public static final String TABLES_EXISTS_IN_NAMESPACE_INDICATOR = "TABLES_PRESENT_IN_NAMESPACE"; + @Override public void delete(String namespace) throws AccumuloException, AccumuloSecurityException, NamespaceNotFoundException, NamespaceNotEmptyException { @@ -158,6 +163,12 @@ public void delete(String namespace) throws AccumuloException, AccumuloSecurityE try { doNamespaceFateOperation(FateOperation.NAMESPACE_DELETE, args, opts, namespace); + } catch (AccumuloException ae) { + if (ae.getMessage().contains(TABLES_EXISTS_IN_NAMESPACE_INDICATOR)) { + throw new NamespaceNotEmptyException(namespaceId.canonical(), namespace, null, ae); + } else { + throw ae; + } } catch (NamespaceExistsException e) { // should not happen throw new AssertionError(e); diff --git a/server/manager/src/main/java/org/apache/accumulo/manager/tableOps/namespace/delete/DeleteNamespace.java b/server/manager/src/main/java/org/apache/accumulo/manager/tableOps/namespace/delete/DeleteNamespace.java index df935593f56..9723099dc17 100644 --- a/server/manager/src/main/java/org/apache/accumulo/manager/tableOps/namespace/delete/DeleteNamespace.java +++ b/server/manager/src/main/java/org/apache/accumulo/manager/tableOps/namespace/delete/DeleteNamespace.java @@ -18,8 +18,16 @@ */ package org.apache.accumulo.manager.tableOps.namespace.delete; +import java.util.List; + +import org.apache.accumulo.core.client.NamespaceNotFoundException; +import org.apache.accumulo.core.clientImpl.AcceptableThriftTableOperationException; +import org.apache.accumulo.core.clientImpl.NamespaceOperationsImpl; +import org.apache.accumulo.core.clientImpl.Namespaces; import org.apache.accumulo.core.clientImpl.thrift.TableOperation; +import org.apache.accumulo.core.clientImpl.thrift.TableOperationExceptionType; import org.apache.accumulo.core.data.NamespaceId; +import org.apache.accumulo.core.data.TableId; import org.apache.accumulo.core.fate.Repo; import org.apache.accumulo.manager.Manager; import org.apache.accumulo.manager.tableOps.ManagerRepo; @@ -41,7 +49,26 @@ public long isReady(long id, Manager environment) throws Exception { } @Override - public Repo call(long tid, Manager environment) { + public Repo call(long tid, Manager environment) + throws AcceptableThriftTableOperationException { + try { + // Namespaces.getTableIds(..) uses the following cache, clear the cache to force + // Namespaces.getTableIds(..) to read from zookeeper. + environment.getContext().clearTableListCache(); + // Since we have a write lock on the namespace id and all fate table operations get a read + // lock on the namespace id there is no need to worry about a fate operation concurrently + // changing table ids in this namespace. + List tableIdsInNamespace = + Namespaces.getTableIds(environment.getContext(), namespaceId); + if (!tableIdsInNamespace.isEmpty()) { + throw new AcceptableThriftTableOperationException(null, null, TableOperation.DELETE, + TableOperationExceptionType.OTHER, + NamespaceOperationsImpl.TABLES_EXISTS_IN_NAMESPACE_INDICATOR); + } + } catch (NamespaceNotFoundException e) { + // not expected to happen since we have a write lock on the namespace + throw new IllegalStateException(e); + } environment.getEventCoordinator().event("deleting namespace %s ", namespaceId); return new NamespaceCleanUp(namespaceId); } @@ -50,5 +77,4 @@ public Repo call(long tid, Manager environment) { public void undo(long id, Manager environment) { Utils.unreserveNamespace(environment, namespaceId, id, true); } - }