From 4d01ec87123caddf3d1fbdceb3a18dfed9fba549 Mon Sep 17 00:00:00 2001 From: Kevin Rathbun Date: Wed, 17 Jan 2024 10:23:12 -0500 Subject: [PATCH] Stricter checks before transitioning table state Closes #4132 Prior to these changes, users were able to manually transition a table from the NEW state to another state. More specifically, calls to TableOperations.online() and TableOperations.offline() when the table was in the NEW state were acceptable. Users should not be able to transition a table from the NEW state as this should only be done by the system upon successfully creating/cloning/importing the table. Changes: - Added new param expectedCurrStates to TableManager.transitionTableState() - Users can now only manually change a table from [ONLINE/OFFLINE]->[ONLINE/OFFLINE] (FateServiceHandler) - Added field expectedCurrStates to ChangeTableState - Table clones, creations, and imports now explicitly expect the current state before completion to be NEW (FinishCloneTable, FinishCreateTable, FinishImportTable) - Table deletions now explicitly expect the current state before deletion to be only ONLINE or OFFLINE (DeleteTable) --- .../accumulo/server/tables/TableManager.java | 21 ++++++++++--------- .../accumulo/manager/FateServiceHandler.java | 13 ++++++++---- .../manager/tableOps/ChangeTableState.java | 9 ++++++-- .../tableOps/clone/FinishCloneTable.java | 9 ++++++-- .../tableOps/create/FinishCreateTable.java | 6 ++++-- .../manager/tableOps/delete/DeleteTable.java | 5 ++++- .../tableImport/FinishImportTable.java | 5 ++++- 7 files changed, 46 insertions(+), 22 deletions(-) diff --git a/server/base/src/main/java/org/apache/accumulo/server/tables/TableManager.java b/server/base/src/main/java/org/apache/accumulo/server/tables/TableManager.java index 215129cc795..973427a8a2d 100644 --- a/server/base/src/main/java/org/apache/accumulo/server/tables/TableManager.java +++ b/server/base/src/main/java/org/apache/accumulo/server/tables/TableManager.java @@ -134,19 +134,20 @@ public TableState getTableState(TableId tableId) { return tableStateCache.get(tableId); } - public synchronized void transitionTableState(final TableId tableId, final TableState newState) { + public synchronized void transitionTableState(final TableId tableId, final TableState newState, + final Set expectedCurrStates) { Preconditions.checkArgument(newState != TableState.UNKNOWN); String statePath = zkRoot + Constants.ZTABLES + "/" + tableId + Constants.ZTABLE_STATE; try { - zoo.mutateOrCreate(statePath, newState.name().getBytes(UTF_8), oldData -> { - TableState oldState = TableState.UNKNOWN; - if (oldData != null) { - oldState = TableState.valueOf(new String(oldData, UTF_8)); + zoo.mutateOrCreate(statePath, newState.name().getBytes(UTF_8), currData -> { + TableState currState = TableState.UNKNOWN; + if (currData != null) { + currState = TableState.valueOf(new String(currData, UTF_8)); } // this check makes the transition operation idempotent - if (oldState == newState) { + if (currState == newState) { return null; // already at desired state, so nothing to do } @@ -154,7 +155,7 @@ public synchronized void transitionTableState(final TableId tableId, final Table // +--------+ // v | // NEW -> (ONLINE|OFFLINE)+--- DELETING - switch (oldState) { + switch (currState) { case NEW: transition = (newState == TableState.OFFLINE || newState == TableState.ONLINE); break; @@ -168,10 +169,10 @@ public synchronized void transitionTableState(final TableId tableId, final Table transition = false; break; } - if (!transition) { - throw new IllegalTableTransitionException(oldState, newState); + if (!transition || !expectedCurrStates.contains(currState)) { + throw new IllegalTableTransitionException(currState, newState); } - log.debug("Transitioning state for table {} from {} to {}", tableId, oldState, newState); + log.debug("Transitioning state for table {} from {} to {}", tableId, currState, newState); return newState.name().getBytes(UTF_8); }); } catch (Exception e) { diff --git a/server/manager/src/main/java/org/apache/accumulo/manager/FateServiceHandler.java b/server/manager/src/main/java/org/apache/accumulo/manager/FateServiceHandler.java index 6c6d0f5a32c..a09087de6bc 100644 --- a/server/manager/src/main/java/org/apache/accumulo/manager/FateServiceHandler.java +++ b/server/manager/src/main/java/org/apache/accumulo/manager/FateServiceHandler.java @@ -61,6 +61,7 @@ import org.apache.accumulo.core.data.NamespaceId; import org.apache.accumulo.core.data.TableId; import org.apache.accumulo.core.fate.ReadOnlyTStore.TStatus; +import org.apache.accumulo.core.manager.state.tables.TableState; import org.apache.accumulo.core.manager.thrift.FateOperation; import org.apache.accumulo.core.manager.thrift.FateService; import org.apache.accumulo.core.manager.thrift.ThriftPropertyException; @@ -388,9 +389,11 @@ public void executeFateOperation(TInfo tinfo, TCredentials c, long opid, FateOpe } goalMessage += "Online table " + tableId; + final Set expectedCurrStates = Set.of(TableState.ONLINE, TableState.OFFLINE); manager.fate().seedTransaction(op.toString(), opid, - new TraceRepo<>(new ChangeTableState(namespaceId, tableId, tableOp)), autoCleanup, - goalMessage); + new TraceRepo<>( + new ChangeTableState(namespaceId, tableId, tableOp, expectedCurrStates)), + autoCleanup, goalMessage); break; } case TABLE_OFFLINE: { @@ -413,9 +416,11 @@ public void executeFateOperation(TInfo tinfo, TCredentials c, long opid, FateOpe } goalMessage += "Offline table " + tableId; + final Set expectedCurrStates = Set.of(TableState.ONLINE, TableState.OFFLINE); manager.fate().seedTransaction(op.toString(), opid, - new TraceRepo<>(new ChangeTableState(namespaceId, tableId, tableOp)), autoCleanup, - goalMessage); + new TraceRepo<>( + new ChangeTableState(namespaceId, tableId, tableOp, expectedCurrStates)), + autoCleanup, goalMessage); break; } case TABLE_MERGE: { diff --git a/server/manager/src/main/java/org/apache/accumulo/manager/tableOps/ChangeTableState.java b/server/manager/src/main/java/org/apache/accumulo/manager/tableOps/ChangeTableState.java index 38025c916ee..63a51a5df82 100644 --- a/server/manager/src/main/java/org/apache/accumulo/manager/tableOps/ChangeTableState.java +++ b/server/manager/src/main/java/org/apache/accumulo/manager/tableOps/ChangeTableState.java @@ -18,6 +18,8 @@ */ package org.apache.accumulo.manager.tableOps; +import java.util.Set; + import org.apache.accumulo.core.clientImpl.thrift.TableOperation; import org.apache.accumulo.core.data.NamespaceId; import org.apache.accumulo.core.data.TableId; @@ -32,11 +34,14 @@ public class ChangeTableState extends ManagerRepo { private TableId tableId; private NamespaceId namespaceId; private TableOperation top; + private final Set expectedCurrStates; - public ChangeTableState(NamespaceId namespaceId, TableId tableId, TableOperation top) { + public ChangeTableState(NamespaceId namespaceId, TableId tableId, TableOperation top, + Set expectedCurrStates) { this.tableId = tableId; this.namespaceId = namespaceId; this.top = top; + this.expectedCurrStates = expectedCurrStates; if (top != TableOperation.ONLINE && top != TableOperation.OFFLINE) { throw new IllegalArgumentException(top.toString()); @@ -58,7 +63,7 @@ public Repo call(long tid, Manager env) { ts = TableState.OFFLINE; } - env.getTableManager().transitionTableState(tableId, ts); + env.getTableManager().transitionTableState(tableId, ts, expectedCurrStates); Utils.unreserveNamespace(env, namespaceId, tid, false); Utils.unreserveTable(env, tableId, tid, true); LoggerFactory.getLogger(ChangeTableState.class).debug("Changed table state {} {}", tableId, ts); diff --git a/server/manager/src/main/java/org/apache/accumulo/manager/tableOps/clone/FinishCloneTable.java b/server/manager/src/main/java/org/apache/accumulo/manager/tableOps/clone/FinishCloneTable.java index 47107444fa2..a9f333e38c7 100644 --- a/server/manager/src/main/java/org/apache/accumulo/manager/tableOps/clone/FinishCloneTable.java +++ b/server/manager/src/main/java/org/apache/accumulo/manager/tableOps/clone/FinishCloneTable.java @@ -18,6 +18,8 @@ */ package org.apache.accumulo.manager.tableOps.clone; +import java.util.Set; + import org.apache.accumulo.core.fate.Repo; import org.apache.accumulo.core.manager.state.tables.TableState; import org.apache.accumulo.manager.Manager; @@ -47,10 +49,13 @@ public Repo call(long tid, Manager environment) { // may never create files.. therefore there is no need to consume namenode space w/ directories // that are not used... tablet will create directories as needed + final Set expectedCurrStates = Set.of(TableState.NEW); if (cloneInfo.keepOffline) { - environment.getTableManager().transitionTableState(cloneInfo.tableId, TableState.OFFLINE); + environment.getTableManager().transitionTableState(cloneInfo.tableId, TableState.OFFLINE, + expectedCurrStates); } else { - environment.getTableManager().transitionTableState(cloneInfo.tableId, TableState.ONLINE); + environment.getTableManager().transitionTableState(cloneInfo.tableId, TableState.ONLINE, + expectedCurrStates); } Utils.unreserveNamespace(environment, cloneInfo.srcNamespaceId, tid, false); diff --git a/server/manager/src/main/java/org/apache/accumulo/manager/tableOps/create/FinishCreateTable.java b/server/manager/src/main/java/org/apache/accumulo/manager/tableOps/create/FinishCreateTable.java index cdd6ad7291e..60b1a3e60af 100644 --- a/server/manager/src/main/java/org/apache/accumulo/manager/tableOps/create/FinishCreateTable.java +++ b/server/manager/src/main/java/org/apache/accumulo/manager/tableOps/create/FinishCreateTable.java @@ -19,6 +19,7 @@ package org.apache.accumulo.manager.tableOps.create; import java.io.IOException; +import java.util.Set; import org.apache.accumulo.core.client.admin.InitialTableState; import org.apache.accumulo.core.fate.Repo; @@ -50,13 +51,14 @@ public long isReady(long tid, Manager environment) { @Override public Repo call(long tid, Manager env) throws Exception { + final Set expectedCurrStates = Set.of(TableState.NEW); if (tableInfo.getInitialTableState() == InitialTableState.OFFLINE) { env.getContext().getTableManager().transitionTableState(tableInfo.getTableId(), - TableState.OFFLINE); + TableState.OFFLINE, expectedCurrStates); } else { env.getContext().getTableManager().transitionTableState(tableInfo.getTableId(), - TableState.ONLINE); + TableState.ONLINE, expectedCurrStates); } Utils.unreserveNamespace(env, tableInfo.getNamespaceId(), tid, false); diff --git a/server/manager/src/main/java/org/apache/accumulo/manager/tableOps/delete/DeleteTable.java b/server/manager/src/main/java/org/apache/accumulo/manager/tableOps/delete/DeleteTable.java index c578f64b89d..2a8560eb477 100644 --- a/server/manager/src/main/java/org/apache/accumulo/manager/tableOps/delete/DeleteTable.java +++ b/server/manager/src/main/java/org/apache/accumulo/manager/tableOps/delete/DeleteTable.java @@ -18,6 +18,8 @@ */ package org.apache.accumulo.manager.tableOps.delete; +import java.util.Set; + import org.apache.accumulo.core.clientImpl.thrift.TableOperation; import org.apache.accumulo.core.data.NamespaceId; import org.apache.accumulo.core.data.TableId; @@ -47,7 +49,8 @@ public long isReady(long tid, Manager env) throws Exception { @Override public Repo call(long tid, Manager env) { - env.getTableManager().transitionTableState(tableId, TableState.DELETING); + final Set expectedCurrStates = Set.of(TableState.ONLINE, TableState.OFFLINE); + env.getTableManager().transitionTableState(tableId, TableState.DELETING, expectedCurrStates); env.getEventCoordinator().event("deleting table %s ", tableId); return new CleanUp(tableId, namespaceId); } diff --git a/server/manager/src/main/java/org/apache/accumulo/manager/tableOps/tableImport/FinishImportTable.java b/server/manager/src/main/java/org/apache/accumulo/manager/tableOps/tableImport/FinishImportTable.java index e5df7220430..e48e9364dff 100644 --- a/server/manager/src/main/java/org/apache/accumulo/manager/tableOps/tableImport/FinishImportTable.java +++ b/server/manager/src/main/java/org/apache/accumulo/manager/tableOps/tableImport/FinishImportTable.java @@ -20,6 +20,8 @@ import static org.apache.accumulo.core.Constants.IMPORT_MAPPINGS_FILE; +import java.util.Set; + import org.apache.accumulo.core.fate.Repo; import org.apache.accumulo.core.manager.state.tables.TableState; import org.apache.accumulo.manager.Manager; @@ -52,8 +54,9 @@ public Repo call(long tid, Manager env) throws Exception { } } + final Set expectedCurrStates = Set.of(TableState.NEW); final TableState newState = tableInfo.keepOffline ? TableState.OFFLINE : TableState.ONLINE; - env.getTableManager().transitionTableState(tableInfo.tableId, newState); + env.getTableManager().transitionTableState(tableInfo.tableId, newState, expectedCurrStates); Utils.unreserveNamespace(env, tableInfo.namespaceId, tid, false); Utils.unreserveTable(env, tableInfo.tableId, tid, true);