HDDS-6235. Empty KeyValueContainers are replicated without chunks directory. #3052

errose28 · 2022-02-07T23:29:43Z

What changes were proposed in this pull request?

Include empty chunks directory in the tarball when replicating a container.
Recreate directory entries when unpacking the tarball.
On datanode startup, create an empty chunks directory if no chunks directory is present.
- Repairs existing containers now that the bug is fixed.

What is the link to the Apache JIRA

How was this patch tested?

Unit test added, which passed only after the fix was applied.
Manually tested chunks directory creation on startup in docker:
1. Create empty container with ozone admin container create
2. Delete its chunks directory.
3. Restart the datanode.
4. Check that empty chunks directory is created.

@REPLicated

* master: (27 commits) HDDS-6206. Application errors must not flood system log (apache#3001) HDDS-6181. Change SCMHAInvocationHandler#invokeRatis() logging to TRACE (apache#2992) HDDS-5529. For any IOexception from @REPLicated method we should throw it (apache#2788) HDDS-6201. Fix NPE for DataScanner with scanned container deleted by others. (apache#3005) HDDS-6239. ozonesecure-mr failing with No URLs in mirrorlist (apache#3029) HDDS-6227. Test helpers should observe naming conditions (apache#3020) HDDS-6205. Add CLI command to display the latest Replication Manager report (apache#3013) HDDS-6192. feature/Observability.md translated to Chinese (apache#2994) HDDS-6219. Switch to RATIS ReplicationType from STAND_ALONE in our tests. (apache#3014) HDDS-6211. [Docs] Image styling on deployed site does not replicate local builds. (apache#3007) HDDS-6177. Extend container info command to include replica details (apache#2995) HDDS-6128. CLI tool that downloads all the block replicas and creates a manifest file (apache#2987) HDDS-6191. Intermittent failure in TestDeleteWithSlowFollower (apache#3015) HDDS-6216. Move OMOpenKeysDeleteRequest to package om.request.key (apache#3011) HDDS-6203. CleanUp incomplete gz files during Container move (apache#3000) HDDS-3408. Rename ChunkLayOutVersion to ContainerLayoutVersion. (apache#2983) HDDS-6109. Preserve the underlying exception raised in client lib. (apache#2989) HDDS-6135. SCM Container DB bootstrap on Recon startup for SCM HA. (apache#2972) HDDS-6202. Avoid using jmh-generator-annprocess since it is GPL2.0. (apache#2998) HDDS-6163. Fix PATH in docker image (apache#2967) ...

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

hanishakoneru

Thanks @errose28 for working on fixing this.
Wouldn't marking a container Unhealthy, if it's metadata scan fails, be too aggressive?
This container would never be replicated and if for some reason this replica is also lost, it could lead to data loss, right? If the container is still recoverable, I think we shouldn't mark it as unhealthy. It would be safer to replicate the container and try fixing it later.
Let me know your thoughts.

nandakumar131

LGTM. Thanks @errose28 for working on this.

* master: (43 commits) HDDS-6212. SCM Container DB bootstrap on Recon startup for secure cluster (apache#3027) HDDS-6234. Repair containers affected by incorrect used bytes and block count. (apache#3042) HDDS-6262. ozone insight log stops working after OM DBUpdates message (apache#3044) HDDS-6290. operational-state and node-state options in datanode list CLI not working correctly (apache#3105) HDDS-6314. ConcurrentModificationException getting SCMContainerMetrics (apache#3101) HDDS-6284. Add BlockDeletingService worker size config (apache#3056) HDDS-6324. Do not trigger CI by reopening PR (apache#3092) HDDS-6283. Change ContainerStateMachine ContainerOpExecutor name (apache#3055) HDDS-6331. Remove toString in debug log parameters within SCMCommonPlacementPolicy (apache#3098) HDDS-6330. Remove unnecessary duplicate semicolons (apache#3097) HDDS-6305: Add metrics - number of FSO bucket creates (apache#3077) HDDS-6311. Fix number of keys displayed in Recon Overview. (apache#3081) HDDS-6325. Fix interface ClientProtocol methods typo setThreadLocalS3Auth and clearThreadLocalS3Auth (apache#3093) HDDS-6322. Fix Recon getting inccorrect sequenceNumber from OM (apache#3090) HDDS-5913. Avoid integer overflow when setting dfs.container.ratis.lo… (apache#2785) HDDS-6313. Remove replicas in ContainerStateMap when a container is deleted (apache#3086) HDDS-6186. Selective checks: skip integration check for unit test changes (apache#3061) HDDS-6310. Update json-smart to 2.4.7. (apache#3080) HDDS-6190. Cleanup unnecessary datanode id path checks. (apache#2993) HDDS-6304. Add enforcer to make sure ozone.version equals project.version (apache#3075) ...

errose28 · 2022-02-17T19:57:27Z

Thanks for reviews @nandakumar131 @hanishakoneru @guihecheng. After some offline discussion we decided to remove the metadata scan on import/export and the unhealthy mark if there's an issue with the container on export. I filed HDDS-6344 for gentler handling of incorrect but recoverable containers, which could be used to flag container content errors on import/export.

errose28 · 2022-02-17T21:42:27Z

After this patch, missing chunks directories will be recreated in KeyValueContainerData#parseKVContainerData. This means the test in the recently merged metadata inspector patch to fix missing chunks directories fails and is not needed. I removed the metadata inspector test, and replaced it with a new unit test that makes sure missing chunks directories are created when KeyValueContainerData#parseKVContainerData is called.

hanishakoneru · 2022-02-18T00:45:41Z

LGTM. +1 pending CI.
Unit test failure is unrelated. Retriggering CI.

adoroszlai

Thanks @errose28 for the patch, LGTM. I only have two minor suggestions.

Also, would you mind adding a unit test in TestTarContainerPacker?

...-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueContainer.java

...ner-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/TarContainerPacker.java

errose28 · 2022-02-22T20:10:09Z

Thanks for the review @adoroszlai. I was initially confused by your test comments, but then realized the merge had deleted my unit test, so that was a good catch. I restored the test so hopefully that part makes more sense now.

adoroszlai

Thanks @errose28 for updating the patch.

...ner-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/TarContainerPacker.java

symious · 2022-02-24T05:07:31Z

@errose28 Thanks for the patch, our cluster does have the same issue while doing replication containers.

One thing I noticed while running the unit test is even without the changes in TarContainerPacker, the test also passed. Does that mean the key point to this issue is the creation of the CHUNK_PATH in KeyValueContainerUtil?

symious · 2022-02-24T05:24:48Z

...ner-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/TarContainerPacker.java

+    archiveOutput.putArchiveEntry(entry);
+
+    // Add files in the directory.
    try (Stream<Path> dirEntries = Files.list(dir)) {


I think the error is thrown here, so it still needs the CHUNK file to exist?

Reproduced by adding new File(data.getChunksPath()).delete(); after checking the state of original container.

@symious the error is thrown because the chunk directory does not exist. Not having chunk files in the directory should not be a problem here.

hanishakoneru · 2022-02-24T16:45:40Z

One thing I noticed while running the unit test is even without the changes in TarContainerPacker, the test also passed. Does that mean the key point to this issue is the creation of the CHUNK_PATH in KeyValueContainerUtil?

KeyValueContainerUtil change fixes existing containers with missing chunks directory. This could have occurred when a container with empty chunks directory was replicated before. The change in TarContainerPacker is needed so that when containers with empty chunks directories are replicated, the replica container get the chunks directory even though it does not have any contents.

hanishakoneru · 2022-02-24T16:47:22Z

Thanks @errose28 for fixing this and @nandakumar131, @adoroszlai , @avijayanhwx and @symious for reviews.
Merging the patch with 4 +1s.

errose28 · 2022-02-24T17:02:33Z

Hi @symious following up on your questions:

Does that mean the key point to this issue is the creation of the CHUNK_PATH in KeyValueContainerUtil?

There's two parts to this PR: preventing the issue and fixing existing containers. I think this comment might answer the question.

One thing I noticed while running the unit test is even without the changes in TarContainerPacker, the test also passed

I cannot repro this. Running master from 68c5ac5 (before this patch was merged), I copy/pasted in the changes to TestKeyValueContainer and both new tests failed. Can you please check again?

symious · 2022-02-24T23:28:04Z

@hanishakoneru Thanks for the explaination.

@errose28 I created a branch with the changes in TarContainerPacker removed here https://github.com/symious/ozone/tree/HDDS-6235, the unit test passed, could you help to check?

symious · 2022-02-24T23:53:43Z

I think the process sequence are as follows:

container exported to FileOutputStream
peer use importContainerData to import the contaienr.
TarContainerPacker unpack the container from the FileInputStream. After the unpack, the directory of "chunks" shouldn't be there.
But in KeyValueContainerUtil.parseKVContainerData(containerData, config); since the new logic was added that if the chunk directory doesn't exists, we create it.

So even without the changes in TarContainerPacker, the unit test still passed.

errose28 added 6 commits February 7, 2022 09:18

Add unit test to repro the issue

a37536c

Fail container import/export on metadata scan failure

62a4e18

Unit test passes after fix

2381174

Fix checkstyle and add chunks dir creation on startup

44cc975

Undo unnecessary edit when detecting tar entry type

bc9827a

errose28 requested review from avijayanhwx and hanishakoneru February 8, 2022 00:06

Findbugs

4bfec10

guihecheng reviewed Feb 8, 2022

View reviewed changes

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

hanishakoneru reviewed Feb 15, 2022

View reviewed changes

nandakumar131 approved these changes Feb 16, 2022

View reviewed changes

errose28 added 2 commits February 17, 2022 10:41

Remove container scans on import/export

e8e2cc9

errose28 added 2 commits February 17, 2022 13:27

Adjust missing chunks dir tests after metadata inspector merge

d28f10c

Remove unused imports

e332cce

adoroszlai reviewed Feb 18, 2022

View reviewed changes

...-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueContainer.java Show resolved Hide resolved

...ner-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/TarContainerPacker.java Outdated Show resolved Hide resolved

errose28 added 2 commits February 22, 2022 11:46

Restore unit test that was removed by the merge

42c85bc

Create directory as part of original extractEntry method

0193016

One space for checkstyle

cd963fa

adoroszlai approved these changes Feb 23, 2022

View reviewed changes

adoroszlai reviewed Feb 23, 2022

View reviewed changes

...ner-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/TarContainerPacker.java Outdated Show resolved Hide resolved

No more duplicate create directory calls

5ccd826

symious reviewed Feb 24, 2022

View reviewed changes

avijayanhwx approved these changes Feb 24, 2022

View reviewed changes

hanishakoneru merged commit a53a3a3 into apache:master Feb 24, 2022

This was referenced Mar 11, 2022

HDDS-5150 when datanode restart,reload the container data will check the chunksfile #2192

Closed

HDDS-5149 when source datanode download container tar from target datanode,but the target datanode container file missing,import error #2376

Closed

HDDS-6235. Empty KeyValueContainers are replicated without chunks directory. #3052

HDDS-6235. Empty KeyValueContainers are replicated without chunks directory. #3052

Uh oh!

Conversation

errose28 commented Feb 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

Uh oh!

hanishakoneru left a comment

Choose a reason for hiding this comment

Uh oh!

nandakumar131 left a comment

Choose a reason for hiding this comment

Uh oh!

errose28 commented Feb 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

errose28 commented Feb 17, 2022

Uh oh!

hanishakoneru commented Feb 18, 2022

Uh oh!

adoroszlai left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

errose28 commented Feb 22, 2022

Uh oh!

adoroszlai left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

symious commented Feb 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

symious Feb 24, 2022

Choose a reason for hiding this comment

Uh oh!

hanishakoneru Feb 24, 2022

Choose a reason for hiding this comment

Uh oh!

hanishakoneru commented Feb 24, 2022

Uh oh!

hanishakoneru commented Feb 24, 2022

Uh oh!

errose28 commented Feb 24, 2022

Uh oh!

symious commented Feb 24, 2022

Uh oh!

symious commented Feb 24, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

errose28 commented Feb 7, 2022 •

edited

Loading

errose28 commented Feb 17, 2022 •

edited

Loading

symious commented Feb 24, 2022 •

edited

Loading