HDDS-8118. Fail container delete on non empty chunks dir #4367

kerneltime · 2023-03-09T04:52:28Z

What changes were proposed in this pull request?

Add a defensive check if the container directory is empty before deleting the container to make sure data is not lost. This may leave a small amount of orphaned data before HDDS-8117 is complete.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-8118

How was this patch tested?

Integration test included

duongkame · 2023-03-09T06:14:43Z

hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/ScmConfigKeys.java

Do we need a description for this in ozone-default.xml?

I don't think we need to add this config to ozone-default.xml we do not expect outside of debug scenarios to flip this config option.

...ner-service/src/main/java/org/apache/hadoop/ozone/container/common/interfaces/Container.java

...tainer-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java

Add a defensive check if the container directory is empty before deleting the container to make sure data is not lost. This may leave a small amount of orphaned data before HDDS-8117 is complete.

duongkame

LGTM. Will need another review from someone familiar with container layout to ensure the no-blocks check.

adoroszlai · 2023-03-09T09:26:34Z

Some test failures look related:

https://github.com/apache/ozone/actions/runs/4371786680/jobs/7648011148#step:6:1235
https://github.com/apache/ozone/actions/runs/4371786680/jobs/7648010490#step:5:6001

hadoop-hdds/common/src/main/resources/ozone-default.xml

...tainer-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java

kerneltime · 2023-03-10T01:38:23Z

Some test failures look related:

https://github.com/apache/ozone/actions/runs/4371786680/jobs/7648011148#step:6:1235 https://github.com/apache/ozone/actions/runs/4371786680/jobs/7648010490#step:5:6001

Fixed.

Change-Id: I6e76194de6150e1da286cbbacefccd1659bdb2a5

arp7 · 2023-03-10T17:42:26Z

hadoop-hdds/common/src/main/resources/ozone-default.xml

      without using the optimised DAG based pruning approach
    </description>
  </property>
+  <property>


Any benefit to making this configurable?

This is just in case we want to change the behavior once we are confident of blockCount over presence on disk.

arp7 · 2023-03-10T17:44:05Z

.../src/main/java/org/apache/hadoop/ozone/container/keyvalue/helpers/KeyValueContainerUtil.java

+   * @return true if the directory containing blocks is empty
+   * @throws IOException
+   */
+  public static boolean noBlocksInContainer(KeyValueContainerData


I think we want to check RocksDB and not chunks on disk. The latter may always be present due to orphaned writes. We only care about committed blocks which have entries in RocksDB.

The likelihood of having blocks written to disk but not in DN RocksDB will go down with https://issues.apache.org/jira/browse/HDDS-8117
We will still have the issue of having blocks on DN that are not on OM which needs to be dealt with separately.

I think we should check RocksDB in this PR and switch to using the directory check as part of HDDS-8117, or maybe wait until after HDDS-8138. Otherwise we may get some alarming false positives. In order to tell whether the exception is a false positive or not we would need to check RocksDB anyways, so I don't think the error message is useful without a RocksDB check given how the rest of the code works right now.

The read data is the block. We should work towards making sure blocks are written only if valid, and if they are on disk, we should err towards not cleaning them up. Even with HDDS-8138, I would like to keep this check in place. We can get HDDS-8117 in but while that is being worked on we should check in this fix to avoid any data loss which is the main motivation for this PR.

After offline discussion, we decided to keep this PR as is, since it is better than no check at all. A follow up PR will be coming shortly that will add a RocksDB check and log message here as well to help with debugging.

Change-Id: If410fa45b19cb22b9e3e56a4203be07a4dd36d4a

kerneltime · 2023-03-10T21:53:40Z

The tests under TestOzoneFileSystem passes locally for me, and tests under TestOMRatisSnapshots are not related to the PR as they are OM specific around the installation of snapshots.

errose28

Thanks for the fix @kerneltime

errose28 · 2023-03-10T23:11:26Z

.../src/main/java/org/apache/hadoop/ozone/container/keyvalue/helpers/KeyValueContainerUtil.java

+   * @return true if the directory containing blocks is empty
+   * @throws IOException
+   */
+  public static boolean noBlocksInContainer(KeyValueContainerData


I think we should check RocksDB in this PR and switch to using the directory check as part of HDDS-8117, or maybe wait until after HDDS-8138. Otherwise we may get some alarming false positives. In order to tell whether the exception is a false positive or not we would need to check RocksDB anyways, so I don't think the error message is useful without a RocksDB check given how the rest of the code works right now.

errose28 · 2023-03-11T01:26:03Z

Thanks for adding this check @kerneltime. The RocksDB check will be added in addition to the directory check in a follow up jira.

* master: (262 commits) HDDS-8153. Integrate ContainerBalancer with MoveManager (apache#4391) HDDS-8090. When getBlock from a datanode fails, retry other datanodes. (apache#4357) HDDS-8163 Use try-with-resources to ensure close rockdb connection in SstFilteringService (apache#4402) HDDS-8065. Provide GNU long options (apache#4394) HDDS-7930. [addendum] input stream does not refresh expired block token. HDDS-7930. input stream does not refresh expired block token. (apache#4378) HDDS-7740. [Snapshot] Implement SnapshotDeletingService (apache#4244) HDDS-8076. Use container cache in Key listing API. (apache#4346) HDDS-8091. [addendum] Generate list of config tags from ConfigTag enum - Hadoop 3.1 compatibility fix (apache#4374) HDDS-8144. TestDefaultCertificateClient#testTimeBeforeExpiryGracePeriod fails as we approach DST. (apache#4382) HDDS-8151. Support fine grained lifetime for root CA certificate (apache#4386) HDDS-8150. RpcClientTest and ConfigurationSourceTest not run due to naming convention (apache#4388) HDDS-8131. Add Configuration for OM Ratis Log Purge Tuning Parameters. (apache#4371) HDDS-8133. Create ozone sh key checksum command (apache#4375) HDDS-8142. Check if no entries in Block DB for a container on container delete (apache#4379) HDDS-8118. Fail container delete on non empty chunks dir (apache#4367) HDDS-8028. JNI for RocksDB SST Dump tool (apache#4315) HDDS-8129. ContainerStateMachine allows two different tasks with the same container id running in parallel. (apache#4370) HDDS-8119. Remove loosely related AutoCloseable from SendContainerOutputStream (apache#4368) close db connection (apache#4366) ...

(cherry picked from commit 11f3d72)

kerneltime requested review from adoroszlai, duongkame and errose28 March 9, 2023 04:53

duongkame reviewed Mar 9, 2023

View reviewed changes

kerneltime force-pushed the master branch from 5089df5 to 3a99d4b Compare March 9, 2023 06:31

HDDS-8118. Fail container delete on non empty chunks dir

7f4d566

Add a defensive check if the container directory is empty before deleting the container to make sure data is not lost. This may leave a small amount of orphaned data before HDDS-8117 is complete.

kerneltime force-pushed the master branch from 3a99d4b to 7f4d566 Compare March 9, 2023 06:35

Address review comments

a6e85ec

duongkame approved these changes Mar 9, 2023

View reviewed changes

adoroszlai reviewed Mar 9, 2023

View reviewed changes

hadoop-hdds/common/src/main/resources/ozone-default.xml Outdated Show resolved Hide resolved

...tainer-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java Show resolved Hide resolved

Fix integration tests and unit tests

39a8185

kerneltime force-pushed the master branch from 9a25fb2 to 39a8185 Compare March 10, 2023 01:37

Fix checkstyle

0428ce6

kerneltime force-pushed the master branch from 18d7a2b to 0428ce6 Compare March 10, 2023 07:47

Remove extra new line

6e041a2

Change-Id: I6e76194de6150e1da286cbbacefccd1659bdb2a5

arp7 reviewed Mar 10, 2023

View reviewed changes

The config does not need to be part of ozone-default.xml

e76e593

Change-Id: If410fa45b19cb22b9e3e56a4203be07a4dd36d4a

errose28 reviewed Mar 10, 2023

View reviewed changes

errose28 approved these changes Mar 11, 2023

View reviewed changes

errose28 merged commit 11f3d72 into apache:master Mar 11, 2023

k5342 pushed a commit to pfnet/ozone that referenced this pull request May 10, 2023

HDDS-8118. Fail container delete on non empty chunks dir (apache#4367)

23e6e3b

(cherry picked from commit 11f3d72)

kuenishi pushed a commit to pfnet/ozone that referenced this pull request Aug 14, 2023

HDDS-8118. Fail container delete on non empty chunks dir (apache#4367)

f83b437

(cherry picked from commit 11f3d72)

HDDS-8118. Fail container delete on non empty chunks dir #4367

HDDS-8118. Fail container delete on non empty chunks dir #4367

Uh oh!

Conversation

kerneltime commented Mar 9, 2023

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

duongkame left a comment

Choose a reason for hiding this comment

Uh oh!

adoroszlai commented Mar 9, 2023

Uh oh!

Uh oh!

Uh oh!

kerneltime commented Mar 10, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kerneltime commented Mar 10, 2023

Uh oh!

errose28 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

errose28 commented Mar 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants