HDDS-10300. InstallSnapshot may fail if OM metadata dir and OM DB dir are in different local storage partitions. #6226

aswinshakil · 2024-02-15T23:53:52Z

What changes were proposed in this pull request?

When we hard link files between different disk/partition we face this Invalid cross-device link error. Because we can't create hard links between different disks. In this patch, Instead of creating a hard link we move the file to the target directory.

What is the link to the Apache JIRA

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

How was this patch tested?

Existing Tests

… are in different local storage partitions.

Galsza

Thank you for working on this. I've left 2 minor comments, please fix them

Galsza · 2024-02-16T10:15:36Z

...one/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestOmSnapshotUtils.java

Please write a test case where we assert for the new functionality. (New file is in a different partition than the old file)

...p-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/OmSnapshotUtils.java

hemantk-12

Thanks @aswinshakil for the quick fix.

Left some comments.

...p-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/OmSnapshotUtils.java

hemantk-12

LGTM.

Regarding @Galsza's comment to add test, it would be great if can be added as part of this change otherwise create a follow up task.

swamirishi

@aswinshakil Thanks for the patch. The patch overall looks good to me, apart from a new nitpicky changes and a test case for the same function would be great.

swamirishi · 2024-02-22T04:32:20Z

...p-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/OmSnapshotUtils.java

+    if (isSamePartition(fullFromPath.getParent(), fullToPath.getParent())) {
+      Files.createLink(fullToPath, fullFromPath);
+    } else {
+      Files.copy(fullFromPath, fullToPath, StandardCopyOption.REPLACE_EXISTING);


Is replace existing correct? Can this kind of a situation occur where we are re-copying stuff? We shouldn't inadvertently delete data.

Can we do some kind of a checksum verification if the fullToPath exists.

copy to tmp file

Verify checksum if the files exist

if checksums don't match raise exception.

The file shouldn't exist at all, I'm doing a replace just as a precaution. I'm following the normal OM bootstrap process, We don't do a checksum check for that either.

swamirishi · 2024-02-22T04:36:18Z

...one/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestOmSnapshotUtils.java

    assertFalse(f1Link.exists());

-    OmSnapshotUtils.linkFiles(tree1, tree2);
+    OmSnapshotUtils.linkFilesOrCopy(tree1, tree2);


Please add similar test case for copy case, when both files are in different partitions. You can achieve this by mocking the layer.

Mocking this isSamePartition function should help. From what I know you cannot mock Files interface through mockito but PowerMockito can do it.

errose28 · 2024-02-22T23:44:52Z

Copying seems like a bad solution since we will lose efficiency of snapshots space and diff. Additionally there is no indication to the user that this will happen if they set up their configurations in this way, and the state machines, although functionally the same, will be different across OM replicas when only one node installs a snapshot.

Why not use either of these simpler options:

Rebuild hardlinks after moving the DB from the snapshot install directory to the OM DB directory.
Deprecate configs for setting snapshot install location to a different directory and put it directly in the OM DB directory. I don't see an advantage to doing the initial download in a different place.

aswinshakil · 2024-02-23T22:54:08Z

@errose28 Thanks for taking a look at it. For the first approach, With moving and creating a hardlink we would be doing double the work. Instead, why not move to the correct directory and avoid the hard linking part altogether?

I agree with 2nd, I'm not sure why the snapshot download directory is in a different disk. Ideally, both should be in the same disk.

hemantk-12 · 2024-02-24T01:33:36Z

@errose28 regarding point#2

2. Deprecate configs for setting snapshot install location to a different directory and put it directly in the OM DB directory. I don't see an advantage to doing the initial download in a different place.

@aswinshakil, @swamirishi and I had a discussion about it and we don't see any problem if candidate dir just resides inside ozone.om.db.dirs.

I looked into code and config for the snapshot dir is ozone.om.ratis.snapshot.dir and if it is not set, it falls back to ozone.metadata.dirs. There is no call out or recommendation to have ozone.om.ratis.snapshot.dir or ozone.metadata.dirs in the same partition as ozone.om.db.dirs. Original PR#703 discussion.

If we can get to an agreement that ozone.om.ratis.snapshot.dir should be in the same partition as ozone.om.db.dirs. I think we should add it to release notes and make it a general recommendation. Or we can simply deprecate ozone.om.ratis.snapshot.dir config as you said by moving snapshot dir inside ozone.om.db.dirs.

errose28 · 2024-02-28T00:41:43Z

I was talking about this with @aswinshakil and there is another problem we should consider: the size of the Ratis snapshot might be far greater than the size of the final OM DB. This is because the Ratis snapshot has "inflated" all the hardlinks, so if there are many filesystem snapshots on dense buckets, the OM DB device needs an undetermined amount of space greater than the current DB in order to install the ratis snapshot. If this space is not available and we remove the configs, there is no good way out of the situation. We already don't have great handling to make sure there is always room to install a ratis snapshot, but the hardlink inflation makes the problem worse.

One idea to mitigate the problem:

Allow ratis snapshot to be installed wherever is configured, as it is currently.
Copy files one-by-one to the OM DB dir. If the file is in the hardlink list, create a link instead of a new file.
- This keeps space required on the OM DB device to no more than double that of the current DB, regardless of the amount of snapshots and hardlinks. It still does not make sure there is always space, but it is not worse than the current system behavior.

aswinshakil · 2024-02-29T23:00:45Z

Discussed with @hemantk-12 and @swamirishi offline, Right now installSnapshotFromLeader is synchronized so this is where the follower downloads the checkpoint(candidate) from the leader and moves it to the actual OM DB. There will be only one download happening at any point in time.

This can happen in the same partition because we would be doing the same thing if we downloaded it in a different partition and moved that into OM DB's partition. Instead, download the checkpoint(candidate) in the OM DB partition.

We need to do a few things, deprecate configs ozone.om.ratis.snapshot.dir which allows the user to specify different partitions and also don't fall back to ozone.metadata.dirs. Instead, use ozone.om.db.dirs to download the checkpoint and avoid this problem altogether.

errose28 · 2024-03-05T18:55:40Z

Regarding this previous comment, it turns out that the snapshot install only copies one copy of each SST file needed, and then builds the hardlinks from there. That means the Ratis snapshot will not be bigger than the OM DB and we should be able to put it right on the main DB disk without any more space concerns than what we already have.

I'm +1 for deprecating the configs.

weimingdiit · 2024-09-22T03:23:39Z

@errose28 @aswinshakil Hi, the issue we encountered is similar, and we would like to know if this issue will continue to progress according to the discussion above?

errose28 · 2024-09-27T19:22:07Z

Hi @weimingdiit based on the discussion here I think we would like to simplify the snapshot install configuration by deprecating ozone.om.ratis.snapshot.dir and putting the snapshot directly in ozone.om.db.dir, or ozone.om.metadata.dir if the DB dir is not configured. This makes sure that hardlinks and directory renames always work. I'll close this PR since I don't think it lines up with the current proposal and I'll update the Jira. This should be a minor change if you want to take it on.

aswinshakil added 2 commits February 15, 2024 15:48

HDDS-10300. InstallSnapshot may fail if OM metadata dir and OM DB dir…

ddc682a

… are in different local storage partitions.

HDDS-10300. Fix findbugs.

919016a

Galsza suggested changes Feb 16, 2024

View reviewed changes

hemantk-12 reviewed Feb 21, 2024

View reviewed changes

Move common code to helper and address reviews.

114b1d4

hemantk-12 approved these changes Feb 21, 2024

View reviewed changes

aswinshakil added 3 commits February 21, 2024 16:28

Fix findbugs.

386856e

Fix findbugs.

fcdd82d

Fix checkstyle.

e7b5925

swamirishi requested changes Feb 22, 2024

View reviewed changes

adoroszlai marked this pull request as draft March 3, 2024 09:06

errose28 closed this Sep 27, 2024

HDDS-10300. InstallSnapshot may fail if OM metadata dir and OM DB dir are in different local storage partitions. #6226

HDDS-10300. InstallSnapshot may fail if OM metadata dir and OM DB dir are in different local storage partitions. #6226

Uh oh!

Conversation

aswinshakil commented Feb 15, 2024

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

Galsza left a comment

Choose a reason for hiding this comment

Uh oh!

Galsza Feb 16, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hemantk-12 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hemantk-12 left a comment

Choose a reason for hiding this comment

Uh oh!

swamirishi left a comment

Choose a reason for hiding this comment

Uh oh!

swamirishi Feb 22, 2024

Choose a reason for hiding this comment

Uh oh!

swamirishi Feb 22, 2024

Choose a reason for hiding this comment

Uh oh!

aswinshakil Feb 23, 2024

Choose a reason for hiding this comment

Uh oh!

swamirishi Feb 22, 2024

Choose a reason for hiding this comment

Uh oh!

swamirishi Feb 22, 2024

Choose a reason for hiding this comment

Uh oh!

errose28 commented Feb 22, 2024

Uh oh!

aswinshakil commented Feb 23, 2024

Uh oh!

hemantk-12 commented Feb 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

errose28 commented Feb 28, 2024

Uh oh!

aswinshakil commented Feb 29, 2024

Uh oh!

errose28 commented Mar 5, 2024

Uh oh!

weimingdiit commented Sep 22, 2024

Uh oh!

errose28 commented Sep 27, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

hemantk-12 commented Feb 24, 2024 •

edited

Loading