You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "sodonnel (via GitHub)" <gi...@apache.org> on 2023/09/19 15:53:16 UTC
[GitHub] [ozone] sodonnel opened a new pull request, #5324: HDDS-9322. Remove duplicate containers when loading volumes on a datanode
sodonnel opened a new pull request, #5324:
URL: https://github.com/apache/ozone/pull/5324
## What changes were proposed in this pull request?
Prior to [HDDS-5032](https://issues.apache.org/jira/browse/HDDS-5032), if the same container is found on multiple volumes, then the second volume to load it fails the entire volume.
After [HDDS-5032](https://issues.apache.org/jira/browse/HDDS-5032), the exception is caught so the volume doesn't fail, but depending on which container is loaded faster, either one of the replicas could win. Over several restarts, the container on either volume could be the one loaded, potentially resulting in inconsistencies.
This change catches the error, and then removes one of the duplicates based on the BCSID. The container with the largest BCSID is the one kept, while the other is removed.
This will free the disk space taken by the duplicate container, and also avoid the chance of a different copy being loaded on each restart.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-9322
## How was this patch tested?
Modified a test to have duplicated containers and ensure they are handled based on BCSID.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9322. Remove duplicate containers when loading volumes on a datanode [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai merged PR #5324:
URL: https://github.com/apache/ozone/pull/5324
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sodonnel commented on a diff in pull request #5324: HDDS-9322. Remove duplicate containers when loading volumes on a datanode
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
sodonnel commented on code in PR #5324:
URL: https://github.com/apache/ozone/pull/5324#discussion_r1338876307
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/ozoneimpl/TestContainerReader.java:
##########
@@ -378,11 +406,83 @@ public void testMultipleContainerReader() throws Exception {
" costs " + (System.currentTimeMillis() - startTime) / 1000 + "s");
Assert.assertEquals(containerCount,
containerSet.getContainerMap().entrySet().size());
+ Assert.assertEquals(volumeSet.getFailedVolumesList().size(), 0);
+
+ // One of the conflict01 or conflict02 should have had its container path
+ // removed.
+ List<Path> paths = new ArrayList<>();
+ paths.add(Paths.get(conflict01.getContainerData().getContainerPath()));
+ paths.add(Paths.get(conflict02.getContainerData().getContainerPath()));
+ int exist = 0;
+ for (Path p : paths) {
+ if (Files.exists(p)) {
+ exist++;
+ }
+ }
+ Assert.assertEquals(1, exist);
+ Assert.assertTrue(paths.contains(Paths.get(
+ containerSet.getContainer(0).getContainerData().getContainerPath())));
+
Review Comment:
I wasn't sure what you were suggesting here, but I added in a few extra assertions on the container stored in the set.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sodonnel commented on a diff in pull request #5324: HDDS-9322. Remove duplicate containers when loading volumes on a datanode
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
sodonnel commented on code in PR #5324:
URL: https://github.com/apache/ozone/pull/5324#discussion_r1338261404
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/ContainerReader.java:
##########
@@ -240,6 +249,80 @@ public void verifyAndFixupContainerData(ContainerData containerData)
}
}
+ private void resolveDuplicate(KeyValueContainer existing,
+ KeyValueContainer toAdd) throws IOException {
+ if (existing.getContainerData().getReplicaIndex() != 0 ||
+ toAdd.getContainerData().getReplicaIndex() != 0) {
+ // This is an EC container. As EC Containers don't have a BSCID, we can't
+ // know which one has the most recent data. Additionally, it is possible
+ // for both copies to have a different replica index for the same
+ // container. Therefore we just let whatever one is loaded first win AND
+ // leave the other one on disk.
+ LOG.warn("Container {} is present at {} and at {}. Both are EC " +
+ "containers. Leaving both containers on disk.",
+ existing.getContainerData().getContainerID(),
+ existing.getContainerData().getContainerPath(),
+ toAdd.getContainerData().getContainerPath());
+ return;
+ }
+
+ long existingBCSID = existing.getBlockCommitSequenceId();
+ ContainerProtos.ContainerDataProto.State existingState
+ = existing.getContainerState();
+ long toAddBCSID = toAdd.getBlockCommitSequenceId();
+ ContainerProtos.ContainerDataProto.State toAddState
+ = toAdd.getContainerState();
+
+ if (existingState != toAddState) {
+ if (existingState == CLOSED) {
+ // If we have mis-matched states, always pick a closed one
+ LOG.warn("Container {} is present at {} with state CLOSED and at " +
+ "{} with state {}. Removing the latter container.",
+ existing.getContainerData().getContainerID(),
+ existing.getContainerData().getContainerPath(),
+ toAdd.getContainerData().getContainerPath(), toAddState);
+ KeyValueContainerUtil.removeContainer(toAdd.getContainerData(),
+ hddsVolume.getConf());
Review Comment:
RemoveContainer calls:
```
KeyValueContainerUtil.removeContainerDB(containerData, conf);
KeyValueContainerUtil.moveToDeletedContainerDir(containerData,
containerData.getVolume());
```
So it should result in it getting removed from disk.
I believe we should remove them, as otherwise what else is going to clean them up, and there could be a significant number of containers and hence disk space used.
For there to be 2 copies of a container on a single DN, one of the DNs volumes originally containing the containers must have been marked failed for some time. During that time, a new copy of the container was replicated from another source to bring the redundancy from that failure back up to the expected level. Then, we have had a DN restart bringing the old containers back online.
So the redundancy should be fine - we have 2 copies available on this DN too, so one should go. What is the value in keeping both? It will just request in queries and confusion about why the DN is consuming more space than it should, and if there are a lot of these containers, nobody is going to be able to go through each manually to make a decision on each of them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sodonnel commented on pull request #5324: HDDS-9322. Remove duplicate containers when loading volumes on a datanode
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
sodonnel commented on PR #5324:
URL: https://github.com/apache/ozone/pull/5324#issuecomment-1734109118
I made the changes suggested and also made sure it will not delete EC containers. This makes EC be handled in the same way as currently. I think we will need another Jira to deal with EC rather than holding up progress on this one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] errose28 commented on a diff in pull request #5324: HDDS-9322. Remove duplicate containers when loading volumes on a datanode
Posted by "errose28 (via GitHub)" <gi...@apache.org>.
errose28 commented on code in PR #5324:
URL: https://github.com/apache/ozone/pull/5324#discussion_r1337861966
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/ozoneimpl/TestContainerReader.java:
##########
@@ -378,11 +406,83 @@ public void testMultipleContainerReader() throws Exception {
" costs " + (System.currentTimeMillis() - startTime) / 1000 + "s");
Assert.assertEquals(containerCount,
containerSet.getContainerMap().entrySet().size());
+ Assert.assertEquals(volumeSet.getFailedVolumesList().size(), 0);
+
+ // One of the conflict01 or conflict02 should have had its container path
+ // removed.
+ List<Path> paths = new ArrayList<>();
+ paths.add(Paths.get(conflict01.getContainerData().getContainerPath()));
+ paths.add(Paths.get(conflict02.getContainerData().getContainerPath()));
+ int exist = 0;
+ for (Path p : paths) {
+ if (Files.exists(p)) {
+ exist++;
+ }
+ }
+ Assert.assertEquals(1, exist);
+ Assert.assertTrue(paths.contains(Paths.get(
+ containerSet.getContainer(0).getContainerData().getContainerPath())));
+
Review Comment:
We should probably check the `containerSet` field in this class as well, since that is the datanode's actual view of the container state.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/ContainerReader.java:
##########
@@ -240,6 +249,80 @@ public void verifyAndFixupContainerData(ContainerData containerData)
}
}
+ private void resolveDuplicate(KeyValueContainer existing,
+ KeyValueContainer toAdd) throws IOException {
+ if (existing.getContainerData().getReplicaIndex() != 0 ||
+ toAdd.getContainerData().getReplicaIndex() != 0) {
+ // This is an EC container. As EC Containers don't have a BSCID, we can't
+ // know which one has the most recent data. Additionally, it is possible
+ // for both copies to have a different replica index for the same
+ // container. Therefore we just let whatever one is loaded first win AND
+ // leave the other one on disk.
+ LOG.warn("Container {} is present at {} and at {}. Both are EC " +
+ "containers. Leaving both containers on disk.",
+ existing.getContainerData().getContainerID(),
+ existing.getContainerData().getContainerPath(),
+ toAdd.getContainerData().getContainerPath());
+ return;
+ }
+
+ long existingBCSID = existing.getBlockCommitSequenceId();
+ ContainerProtos.ContainerDataProto.State existingState
+ = existing.getContainerState();
+ long toAddBCSID = toAdd.getBlockCommitSequenceId();
+ ContainerProtos.ContainerDataProto.State toAddState
+ = toAdd.getContainerState();
+
+ if (existingState != toAddState) {
+ if (existingState == CLOSED) {
+ // If we have mis-matched states, always pick a closed one
+ LOG.warn("Container {} is present at {} with state CLOSED and at " +
+ "{} with state {}. Removing the latter container.",
+ existing.getContainerData().getContainerID(),
+ existing.getContainerData().getContainerPath(),
+ toAdd.getContainerData().getContainerPath(), toAddState);
+ KeyValueContainerUtil.removeContainer(toAdd.getContainerData(),
+ hddsVolume.getConf());
Review Comment:
Do we actually want to remove the container from the disk? It might be safer just to skip loading it. In the current code a correct delete would have to go through `KeyValueHandler#deleteInternal` anyways. Just calling this method could result in the DB entries being removed but the container remaining on disk, which would show up as corruption on the next startup.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9322. Remove duplicate containers when loading volumes on a datanode [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #5324:
URL: https://github.com/apache/ozone/pull/5324#issuecomment-1858841821
@errose28 would you like to take another look?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9322. Remove duplicate containers when loading volumes on a datanode [ozone]
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
sodonnel commented on PR #5324:
URL: https://github.com/apache/ozone/pull/5324#issuecomment-1820920011
@sumitagrawl I had not planned to handle this as part of this PR. The duplicate containers should be rare, and then adding the delete transaction would be an even more rare event. I think lost deletes are a wider potential problem, and we don't really have a solution for them in Ozone so far. Possibly something in Recon needs to be able to find orphan blocks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9322. Remove duplicate containers when loading volumes on a datanode [ozone]
Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.
sumitagrawl commented on PR #5324:
URL: https://github.com/apache/ozone/pull/5324#issuecomment-1840070362
> @sumitagrawl I had not planned to handle this as part of this PR. The duplicate containers should be rare, and then adding the delete transaction would be an even more rare event. I think lost deletes are a wider potential problem, and we don't really have a solution for them in Ozone so far. Possibly something in Recon needs to be able to find orphan blocks.
Others LGTM
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sodonnel commented on pull request #5324: HDDS-9322. Remove duplicate containers when loading volumes on a datanode
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
sodonnel commented on PR #5324:
URL: https://github.com/apache/ozone/pull/5324#issuecomment-1731877535
> * EC containers will all have BCSID = 0. We may end up loading an old open EC container without all the data.
Yea, EC is a problem. I was considering putting in something to avoid deleting the EC ones, and it also makes me wonder if EC needs to have an incrementing BCSID too. As things stand, without this change, a random container could get loaded on restart. I am not sure if there is anything we can use on EC containers to decide.
Another issue with EC is that you may have replicaIndex=1 and replicaIndex, say 3 on the DN. We can only load and keep one of them - what to do then!?
> I think that a CLOSED container should always win
CLOSED should always have the highest BCSID, but perhaps there are some edge cases where it would not. But it does make sense to put unhealthy last if there is a tie.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] errose28 commented on pull request #5324: HDDS-9322. Remove duplicate containers when loading volumes on a datanode
Posted by "errose28 (via GitHub)" <gi...@apache.org>.
errose28 commented on PR #5324:
URL: https://github.com/apache/ozone/pull/5324#issuecomment-1731806587
Should we use container state as the tiebreaker before BCSID? I think that a CLOSED container should always win. Just checking the BCSID does not always imply this:
- EC containers will all have BCSID = 0. We may end up loading an old open EC container without all the data.
- Unhealthy and closed containers may have the same BCSID. We should use the closed one in this case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9322. Remove duplicate containers when loading volumes on a datanode [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #5324:
URL: https://github.com/apache/ozone/pull/5324#issuecomment-1865064002
Thanks @sodonnel for the patch, @errose28, @sumitagrawl for the review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org