Skip to content

HDDS-7871. Fix false positive in KeyManagerImpl#createFakeDirIfShould() #4236

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
adoroszlai merged 19 commits into from
Feb 8, 2023
Merged

HDDS-7871. Fix false positive in KeyManagerImpl#createFakeDirIfShould() #4236

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
b4d0bce
Fix false positive in KeyManagerImpl#createFakeDirIfShould()
kaijchen Feb 2, 2023
1f74524
refactor
kaijchen Feb 2, 2023
ff893a0
check the ResultCode
kaijchen Feb 3, 2023
5481eb1
update comments
kaijchen Feb 3, 2023
f36f33d
change test name
kaijchen Feb 3, 2023
24c6987
update comments
kaijchen Feb 3, 2023
bc34bb9
refactor test
kaijchen Feb 4, 2023
ec2093a
refactor test: regulate key name
kaijchen Feb 4, 2023
df7ad01
add helper function for assert
kaijchen Feb 4, 2023
ac754c0
enhance test false positive
kaijchen Feb 4, 2023
276ce86
add test false negative
kaijchen Feb 4, 2023
17b5378
fix style
kaijchen Feb 4, 2023
a18bef4
reposition helper functions
kaijchen Feb 4, 2023
7ec03e2
check keyName bucketName volumeName
kaijchen Feb 7, 2023
3162b58
switch to JUnit5 parameterized test
kaijchen Feb 7, 2023
b8790fe
simplify createFile
kaijchen Feb 7, 2023
790b427
switch to JUnit5 assertions
kaijchen Feb 7, 2023
cb66839
merge creation and verify
kaijchen Feb 7, 2023
7a21877
Merge branch 'master' into HDDS-7871
kaijchen Feb 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
113 changes: 113 additions & 0 deletions ...p-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestKeyManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,9 @@
import java.util.Set;
import java.util.TreeSet;
import java.util.UUID;
import java.util.stream.Stream;

import org.apache.commons.lang3.tuple.Pair;
import org.apache.hadoop.conf.StorageUnit;
import org.apache.hadoop.hdds.HddsConfigKeys;
import org.apache.hadoop.hdds.client.BlockID;
Expand Down Expand Up @@ -120,6 +122,9 @@
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.Timeout;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.MethodSource;

import org.mockito.Mockito;

Expand Down Expand Up @@ -158,6 +163,7 @@ public class TestKeyManagerImpl {
private static long scmBlockSize;
private static final String KEY_NAME = "key1";
private static final String BUCKET_NAME = "bucket1";
private static final String BUCKET2_NAME = "bucket2";
private static final String VERSIONED_BUCKET_NAME = "versionedBucket1";
private static final String VOLUME_NAME = "vol1";
private static OzoneManagerProtocol writeClient;
Expand Down Expand Up @@ -218,6 +224,7 @@ public static void setUp() throws Exception {
ResultCodes.SAFE_MODE_EXCEPTION));
createVolume(VOLUME_NAME);
createBucket(VOLUME_NAME, BUCKET_NAME, false);
createBucket(VOLUME_NAME, BUCKET2_NAME, false);
createBucket(VOLUME_NAME, VERSIONED_BUCKET_NAME, true);
}

Expand Down Expand Up @@ -1340,6 +1347,67 @@ public void testGetFileStatusWithFakeDir() throws IOException {
assertTrue(ozoneFileStatus.isFile());
}

private static Stream<Arguments> fakeDirScenarios() {
final String bucket1 = BUCKET_NAME;
final String bucket2 = BUCKET2_NAME;

return Stream.of(
Arguments.of(
"false positive",
Stream.of(
Pair.of(bucket1, "dir1/file1"),
Pair.of(bucket2, "dir2/file2")
),
// positives
Stream.of(
Pair.of(bucket1, "dir1"),
Pair.of(bucket2, "dir2")
),
// negatives
Stream.of(
Pair.of(bucket1, "dir0"),
// RocksIterator#seek("volume1/bucket1/dir2/") will position
// at the 2nd dbKey "volume1/bucket2/dir2/file2", which is
// not belong to bucket1.
// This might be a false positive, see HDDS-7871.
Pair.of(bucket1, "dir2"),
Pair.of(bucket2, "dir0"),
Pair.of(bucket2, "dir1"),
Pair.of(bucket2, "dir3")
)
),
Arguments.of(
"false negative",
Stream.of(
Pair.of(bucket1, "dir1/file1"),
Pair.of(bucket1, "dir1/file2"),
Pair.of(bucket1, "dir1/file3"),
Pair.of(bucket2, "dir1/file1"),
Pair.of(bucket2, "dir1/file2")
),
// positives
Stream.of(
Pair.of(bucket1, "dir1"),
Pair.of(bucket2, "dir1")
),
// negatives
Stream.empty()
)
);
}

@ParameterizedTest(name = "{0}")
@MethodSource("fakeDirScenarios")
public void testGetFileStatusWithFakeDirs(
String description,
Stream<Pair<String, String>> keys,
Stream<Pair<String, String>> positives,
Stream<Pair<String, String>> negatives) {
keys.forEach(f -> createFile(f.getLeft(), f.getRight()));
positives.forEach(f -> assertIsDirectory(f.getLeft(), f.getRight()));
negatives.forEach(f -> assertFileNotFound(f.getLeft(), f.getRight()));
}

@Test
public void testRefreshPipeline() throws Exception {

Expand Down Expand Up @@ -1574,6 +1642,51 @@ private List<String> createFiles(String parent,
return keyNames;
}

private void createFile(String bucketName, String keyName) {
try {
OmKeyArgs keyArgs = createBuilder(bucketName).setKeyName(keyName).build();
OpenKeySession keySession = writeClient.openKey(keyArgs);
keyArgs.setLocationInfoList(keySession.getKeyInfo()
.getLatestVersionLocations().getLocationList());
writeClient.commitKey(keyArgs, keySession.getId());

// verify key exist in table
OmKeyInfo keyInfo = metadataManager.getKeyTable(getDefaultBucketLayout())
.get(metadataManager.getOzoneKey(VOLUME_NAME, bucketName, keyName));
assertNotNull(keyInfo);
assertEquals(VOLUME_NAME, keyInfo.getVolumeName());
assertEquals(bucketName, keyInfo.getBucketName());
assertEquals(keyName, keyInfo.getKeyName());
} catch (IOException e) {
throw new RuntimeException(e);
}
}

private void assertFileNotFound(String bucketName, String keyName) {
try {
OmKeyArgs keyArgs = createBuilder(bucketName).setKeyName(keyName).build();
OMException ex = assertThrows(OMException.class,
() -> keyManager.getFileStatus(keyArgs));
assertEquals(OMException.ResultCodes.FILE_NOT_FOUND, ex.getResult());
} catch (IOException e) {
throw new RuntimeException(e);
}
}

private void assertIsDirectory(String bucketName, String keyName) {
try {
OmKeyArgs keyArgs = createBuilder(bucketName).setKeyName(keyName).build();
OzoneFileStatus ozoneFileStatus = keyManager.getFileStatus(keyArgs);
OmKeyInfo keyInfo = ozoneFileStatus.getKeyInfo();
assertEquals(VOLUME_NAME, keyInfo.getVolumeName());
assertEquals(bucketName, keyInfo.getBucketName());
assertEquals(keyName, keyInfo.getFileName());
assertTrue(ozoneFileStatus.isDirectory());
} catch (IOException e) {
throw new RuntimeException(e);
}
}

private OmKeyArgs.Builder createBuilder() throws IOException {
return createBuilder(BUCKET_NAME);
}
Expand Down
23 changes: 10 additions & 13 deletions hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1178,21 +1178,18 @@ private OmKeyInfo createFakeDirIfShould(String volume, String bucket,
String keyName, BucketLayout layout) throws IOException {
OmKeyInfo fakeDirKeyInfo = null;
String dirKey = OzoneFSUtils.addTrailingSlashIfNeeded(keyName);
String fileKeyBytes = metadataManager.getOzoneKey(volume, bucket, keyName);
String targetKey = OzoneFSUtils.addTrailingSlashIfNeeded(
metadataManager.getOzoneKey(volume, bucket, keyName));
try (TableIterator<String, ? extends Table.KeyValue<String, OmKeyInfo>>
keyTblItr = metadataManager.getKeyTable(layout).iterator()) {
Table.KeyValue<String, OmKeyInfo> keyValue =
keyTblItr
.seek(OzoneFSUtils.addTrailingSlashIfNeeded(fileKeyBytes));

if (keyValue != null) {
Path fullPath = Paths.get(keyValue.getValue().getKeyName());
Path subPath = Paths.get(dirKey);
OmKeyInfo omKeyInfo = keyValue.getValue();
if (fullPath.startsWith(subPath)) {
// create fake directory
fakeDirKeyInfo = createDirectoryKey(omKeyInfo, dirKey);
}
Table.KeyValue<String, OmKeyInfo> keyValue = keyTblItr.seek(targetKey);

// HDDS-7871: RocksIterator#seek() may position at the key
Copy link
Contributor

@hemantk-12 hemantk-12 Feb 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it because seek may position past target or bloom filter or when item is not in the table?
As per my understanding it seems like false positive because of bloom filter. Please correct me if I'm wrong.

When I was testing it, my test was failure for following scenario.

Items in the table was:

KeyInfoTable
/vol1/bucket1/key1
/vol1/bucket1/key2
/vol1/bucket1/key3
/vol1/bucket1/key4
/vol1/bucket1/key5

Was looking for /vol2/bucket2/key1 and getting false positive.

May point is if it is because of bloom filter and seek is pointing to wrong item, we might miss the "fakeDir".
Eg. You have following table

KeyInfoTable
/vol1/bucket1/dir1/key1
/vol1/bucket1/dir1/key2
/vol1/bucket1/dir1/key3
/vol1/bucket2/dir1/key1
/vol1/bucket2/dir1/key2

and now you are looking for /vol1/bucket2/dir1 but seek is pointing to /vol1/bucket1/dir1/key1. So it will be a miss all the time now.

Copy link
Member Author

@kaijchen kaijchen Feb 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the info. I will look into this scenario also.

Copy link
Member Author

@kaijchen kaijchen Feb 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I'm still reading the RocksDB bloom filter docs, I have added more test cases to cover your scenarios.

Was looking for /vol2/bucket2/key1 and getting false positive.

Looking for /vol1/bucket2/dir3/ is added in my false positive test. Is it equivalent?

and now you are looking for /vol1/bucket2/dir1 but seek is pointing to /vol1/bucket1/dir1/key1. So it will be a miss all the time now.

TestFalseNegative is added, but I'm not sure why it might happen.

// past the target, we should check the full dbKeyName.
// For example, seeking "/vol1/bucket1/dir2/" may return a key
// in different volume/bucket, such as "/vol1/bucket2/dir2/key2".
if (keyValue != null && keyValue.getKey().startsWith(targetKey)) {
fakeDirKeyInfo = createDirectoryKey(keyValue.getValue(), dirKey);
}
}

Expand Down