From df01469141e3933ca35785c25e1e29f59129cc85 Mon Sep 17 00:00:00 2001 From: Mukul Kumar Singh Date: Wed, 10 Apr 2019 18:00:10 +0530 Subject: [PATCH] HDDS-1401. Static ContainerCache in Datanodes can result in overwrite of container db. Contributed by Mukul Kumar Singh. (#708) --- .../common/utils/ContainerCache.java | 36 ++++++++++--------- .../keyvalue/helpers/BlockUtils.java | 2 +- .../keyvalue/impl/BlockManagerImpl.java | 2 +- .../keyvalue/TestKeyValueContainer.java | 2 +- .../hdds/scm/block/BlockManagerImpl.java | 4 +-- .../common/impl/TestContainerPersistence.java | 2 +- .../TestCloseContainerByPipeline.java | 17 +++++++++ 7 files changed, 42 insertions(+), 23 deletions(-) diff --git a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/utils/ContainerCache.java b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/utils/ContainerCache.java index a533684f02c..25d1bdf2918 100644 --- a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/utils/ContainerCache.java +++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/utils/ContainerCache.java @@ -69,15 +69,15 @@ public final class ContainerCache extends LRUMap { /** * Closes a db instance. * - * @param containerID - ID of the container to be closed. + * @param containerPath - path of the container db to be closed. * @param db - db instance to close. */ - private void closeDB(long containerID, MetadataStore db) { + private void closeDB(String containerPath, MetadataStore db) { if (db != null) { try { db.close(); - } catch (IOException e) { - LOG.error("Error closing DB. Container: " + containerID, e); + } catch (Exception e) { + LOG.error("Error closing DB. Container: " + containerPath, e); } } } @@ -93,7 +93,7 @@ public final class ContainerCache extends LRUMap { while (iterator.hasNext()) { iterator.next(); MetadataStore db = (MetadataStore) iterator.getValue(); - closeDB(((Number)iterator.getKey()).longValue(), db); + closeDB((String)iterator.getKey(), db); } // reset the cache cache.clear(); @@ -107,14 +107,18 @@ public final class ContainerCache extends LRUMap { */ @Override protected boolean removeLRU(LinkEntry entry) { + MetadataStore db = (MetadataStore) entry.getValue(); + String dbFile = (String)entry.getKey(); lock.lock(); try { - MetadataStore db = (MetadataStore) entry.getValue(); - closeDB(((Number)entry.getKey()).longValue(), db); + closeDB(dbFile, db); + return true; + } catch (Exception e) { + LOG.error("Eviction for db:{} failed", dbFile, e); + return false; } finally { lock.unlock(); } - return true; } /** @@ -133,7 +137,7 @@ public final class ContainerCache extends LRUMap { "Container ID cannot be negative."); lock.lock(); try { - MetadataStore db = (MetadataStore) this.get(containerID); + MetadataStore db = (MetadataStore) this.get(containerDBPath); if (db == null) { db = MetadataStoreBuilder.newBuilder() @@ -142,7 +146,7 @@ public final class ContainerCache extends LRUMap { .setConf(conf) .setDBType(containerDBType) .build(); - this.put(containerID, db); + this.put(containerDBPath, db); } return db; } catch (Exception e) { @@ -157,16 +161,14 @@ public final class ContainerCache extends LRUMap { /** * Remove a DB handler from cache. * - * @param containerID - ID of the container. + * @param containerPath - path of the container db file. */ - public void removeDB(long containerID) { - Preconditions.checkState(containerID >= 0, - "Container ID cannot be negative."); + public void removeDB(String containerPath) { lock.lock(); try { - MetadataStore db = (MetadataStore)this.get(containerID); - closeDB(containerID, db); - this.remove(containerID); + MetadataStore db = (MetadataStore)this.get(containerPath); + closeDB(containerPath, db); + this.remove(containerPath); } finally { lock.unlock(); } diff --git a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/helpers/BlockUtils.java b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/helpers/BlockUtils.java index 200e8ea9427..996b5922fe5 100644 --- a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/helpers/BlockUtils.java +++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/helpers/BlockUtils.java @@ -95,7 +95,7 @@ public final class BlockUtils { Preconditions.checkNotNull(container); ContainerCache cache = ContainerCache.getInstance(conf); Preconditions.checkNotNull(cache); - cache.removeDB(container.getContainerID()); + cache.removeDB(container.getDbFile().getAbsolutePath()); } /** diff --git a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/impl/BlockManagerImpl.java b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/impl/BlockManagerImpl.java index 86865acb4a7..3033dd9017d 100644 --- a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/impl/BlockManagerImpl.java +++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/impl/BlockManagerImpl.java @@ -160,7 +160,7 @@ public class BlockManagerImpl implements BlockManager { } byte[] kData = db.get(Longs.toByteArray(blockID.getLocalID())); if (kData == null) { - throw new StorageContainerException("Unable to find the block.", + throw new StorageContainerException("Unable to find the block." + blockID, NO_SUCH_BLOCK); } ContainerProtos.BlockData blockData = diff --git a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueContainer.java b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueContainer.java index 1aa736158a9..8e2986cca6f 100644 --- a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueContainer.java +++ b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueContainer.java @@ -195,7 +195,7 @@ public class TestKeyValueContainer { for (int i = 0; i < numberOfKeysToWrite; i++) { metadataStore.put(("test" + i).getBytes(UTF_8), "test".getBytes(UTF_8)); } - metadataStore.close(); + BlockUtils.removeDB(keyValueContainerData, conf); Map metadata = new HashMap<>(); metadata.put("key1", "value1"); diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/BlockManagerImpl.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/BlockManagerImpl.java index d15f07b496d..0ca66709f9e 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/BlockManagerImpl.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/BlockManagerImpl.java @@ -189,8 +189,8 @@ public class BlockManagerImpl implements BlockManager, BlockmanagerMXBean { // factors are handled by pipeline creator pipeline = pipelineManager.createPipeline(type, factor); } catch (IOException e) { - LOG.error("pipeline creation failed type:{} factor:{}", type, - factor, e); + LOG.error("Pipeline creation failed for type:{} factor:{}", + type, factor, e); break; } } else { diff --git a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/container/common/impl/TestContainerPersistence.java b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/container/common/impl/TestContainerPersistence.java index f2e44cb2dd6..838dd9ebb78 100644 --- a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/container/common/impl/TestContainerPersistence.java +++ b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/container/common/impl/TestContainerPersistence.java @@ -255,7 +255,7 @@ public class TestContainerPersistence { "Container cannot be deleted because it is not empty."); container2.delete(); Assert.assertTrue(containerSet.getContainerMapCopy() - .containsKey(testContainerID1)); + .containsKey(testContainerID2)); } @Test diff --git a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/container/common/statemachine/commandhandler/TestCloseContainerByPipeline.java b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/container/common/statemachine/commandhandler/TestCloseContainerByPipeline.java index fc6a74d64e1..4a86f440170 100644 --- a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/container/common/statemachine/commandhandler/TestCloseContainerByPipeline.java +++ b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/container/common/statemachine/commandhandler/TestCloseContainerByPipeline.java @@ -34,16 +34,21 @@ import org.apache.hadoop.ozone.client.OzoneClient; import org.apache.hadoop.ozone.client.OzoneClientFactory; import org.apache.hadoop.ozone.client.io.OzoneOutputStream; import org.apache.hadoop.ozone.container.common.impl.ContainerData; +import org.apache.hadoop.ozone.container.common.interfaces.Container; +import org.apache.hadoop.ozone.container.keyvalue.KeyValueContainerData; +import org.apache.hadoop.ozone.container.keyvalue.helpers.BlockUtils; import org.apache.hadoop.ozone.om.helpers.OmKeyArgs; import org.apache.hadoop.ozone.om.helpers.OmKeyLocationInfo; import org.apache.hadoop.ozone.protocol.commands.CloseContainerCommand; import org.apache.hadoop.test.GenericTestUtils; +import org.apache.hadoop.utils.MetadataStore; import org.junit.AfterClass; import org.junit.Assert; import org.junit.BeforeClass; import org.junit.Test; import java.io.IOException; +import java.util.ArrayList; import java.util.HashMap; import java.util.List; import java.util.concurrent.TimeoutException; @@ -221,13 +226,25 @@ public class TestCloseContainerByPipeline { List datanodes = pipeline.getNodes(); Assert.assertEquals(3, datanodes.size()); + List metadataStores = new ArrayList<>(datanodes.size()); for (DatanodeDetails details : datanodes) { Assert.assertFalse(isContainerClosed(cluster, containerID, details)); //send the order to close the container cluster.getStorageContainerManager().getScmNodeManager() .addDatanodeCommand(details.getUuid(), new CloseContainerCommand(containerID, pipeline.getId())); + int index = cluster.getHddsDatanodeIndex(details); + Container dnContainer = cluster.getHddsDatanodes().get(index) + .getDatanodeStateMachine().getContainer().getContainerSet() + .getContainer(containerID); + metadataStores.add(BlockUtils.getDB((KeyValueContainerData) dnContainer + .getContainerData(), conf)); } + + // There should be as many rocks db as the number of datanodes in pipeline. + Assert.assertEquals(datanodes.size(), + metadataStores.stream().distinct().count()); + // Make sure that it is CLOSED for (DatanodeDetails datanodeDetails : datanodes) { GenericTestUtils.waitFor(