Skip to content

HDDS-1737. Add Volume check in KeyManager and File Operations. #2

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

Closed
cxorm wants to merge 7 commits into from
Closed

HDDS-1737. Add Volume check in KeyManager and File Operations. #2

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6b64242
HDDS-1737. Add Volume check in KeyManager and File Operations.
cxorm Oct 7, 2019
992fc8b
Revert "HDDS-1737. Add Volume check in KeyManager and File Operations."
cxorm Oct 8, 2019
f3faf74
Add tests aiming to modify requests with full cache
cxorm Oct 10, 2019
322b3c6
HDDS-1737. Add Volume check in KeyManager and File Operations.
cxorm Oct 10, 2019
9b12ce3
Merge remote-tracking branch 'apache/master' into HDDS-1737
cxorm Oct 13, 2019
ce96784
Merge remote-tracking branch 'apache/master' into HDDS-1737
cxorm Oct 16, 2019
6d7157a
Modify assert method in TestOMDirectoryCreateRequest.java
cxorm Oct 16, 2019
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
16 changes: 4 additions & 12 deletions ...nager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMDirectoryCreateRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,9 +62,8 @@
import org.apache.hadoop.hdds.utils.db.cache.CacheValue;


import static org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes.BUCKET_NOT_FOUND;
import static org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes.FILE_ALREADY_EXISTS;
import static org.apache.hadoop.ozone.om.lock.OzoneManagerLock.Resource.BUCKET_LOCK;
import static org.apache.hadoop.ozone.om.lock.OzoneManagerLock.Resource.BUCKET_LOCK;
import static org.apache.hadoop.ozone.om.request.file.OMFileRequest.OMDirectoryResult.DIRECTORY_EXISTS_IN_GIVENPATH;
import static org.apache.hadoop.ozone.om.request.file.OMFileRequest.OMDirectoryResult.FILE_EXISTS_IN_GIVENPATH;
import static org.apache.hadoop.ozone.om.request.file.OMFileRequest.OMDirectoryResult.NONE;
Expand Down Expand Up @@ -139,23 +138,16 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
acquiredLock = omMetadataManager.getLock().acquireWriteLock(BUCKET_LOCK,
volumeName, bucketName);

// TODO: Not checking volume exist here, once we have full cache we can
// add volume exist check also.

OmBucketInfo omBucketInfo = omMetadataManager.getBucketTable().get(
omMetadataManager.getBucketKey(volumeName, bucketName));

if (omBucketInfo == null) {
throw new OMException("Bucket not found " + bucketName,
BUCKET_NOT_FOUND);
}
validateBucketAndVolume(omMetadataManager, volumeName, bucketName);

// Need to check if any files exist in the given path, if they exist we
// cannot create a directory with the given key.
OMFileRequest.OMDirectoryResult omDirectoryResult =
OMFileRequest.verifyFilesInPath(omMetadataManager,
volumeName, bucketName, keyName, Paths.get(keyName));

OmBucketInfo omBucketInfo = omMetadataManager.getBucketTable().get(
omMetadataManager.getBucketKey(volumeName, bucketName));
OmKeyInfo dirKeyInfo = null;
if (omDirectoryResult == FILE_EXISTS ||
omDirectoryResult == FILE_EXISTS_IN_GIVENPATH) {
Expand Down
11 changes: 3 additions & 8 deletions ...ne-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMFileCreateRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,14 +183,7 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
acquiredLock = omMetadataManager.getLock().acquireWriteLock(BUCKET_LOCK,
volumeName, bucketName);

OmBucketInfo bucketInfo =
omMetadataManager.getBucketTable().get(
omMetadataManager.getBucketKey(volumeName, bucketName));

if (bucketInfo == null) {
throw new OMException("Bucket " + bucketName + " not found",
OMException.ResultCodes.BUCKET_NOT_FOUND);
}
validateBucketAndVolume(omMetadataManager, volumeName, bucketName);

if (keyName.length() == 0) {
// Check if this is the root of the filesystem.
Expand Down Expand Up @@ -255,6 +248,8 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
}

// do open key
OmBucketInfo bucketInfo = omMetadataManager.getBucketTable().get(
omMetadataManager.getBucketKey(volumeName, bucketName));
encryptionInfo = getFileEncryptionInfo(ozoneManager, bucketInfo);
omKeyInfo = prepareKeyInfo(omMetadataManager, keyArgs,
omMetadataManager.getOzoneKey(volumeName, bucketName,
Expand Down
11 changes: 4 additions & 7 deletions ...zone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyDeleteRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,7 @@
import org.apache.hadoop.hdds.utils.db.cache.CacheKey;
import org.apache.hadoop.hdds.utils.db.cache.CacheValue;

import static org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes
.KEY_NOT_FOUND;
import static org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes.KEY_NOT_FOUND;
import static org.apache.hadoop.ozone.om.lock.OzoneManagerLock.Resource.BUCKET_LOCK;

/**
Expand Down Expand Up @@ -117,12 +116,10 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
acquiredLock = omMetadataManager.getLock().acquireWriteLock(BUCKET_LOCK,
volumeName, bucketName);

// Not doing bucket/volume checks here. In this way we can avoid db
// checks for them.
// TODO: Once we have volume/bucket full cache, we can add
// them back, as these checks will be inexpensive at that time.
OmKeyInfo omKeyInfo = omMetadataManager.getKeyTable().get(objectKey);
// Validate bucket and volume exists or not.
validateBucketAndVolume(omMetadataManager, volumeName, bucketName);

OmKeyInfo omKeyInfo = omMetadataManager.getKeyTable().get(objectKey);
if (omKeyInfo == null) {
throw new OMException("Key not found", KEY_NOT_FOUND);
}
Expand Down
6 changes: 2 additions & 4 deletions ...zone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRenameRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,10 +123,8 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
acquiredLock = omMetadataManager.getLock().acquireWriteLock(BUCKET_LOCK,
volumeName, bucketName);

// Not doing bucket/volume checks here. In this way we can avoid db
// checks for them.
// TODO: Once we have volume/bucket full cache, we can add
// them back, as these checks will be inexpensive at that time.
// Validate bucket and volume exists or not.
validateBucketAndVolume(omMetadataManager, volumeName, bucketName);

// fromKeyName should exist
String fromKey = omMetadataManager.getOzoneKey(
Expand Down
34 changes: 33 additions & 1 deletion ...r/src/test/java/org/apache/hadoop/ozone/om/request/file/TestOMDirectoryCreateRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,38 @@ public void testValidateAndUpdateCache() throws Exception {

}

@Test
public void testValidateAndUpdateCacheWithVolumeNotFound() throws Exception {
String volumeName = "vol1";
String bucketName = "bucket1";
String keyName = RandomStringUtils.randomAlphabetic(5);
for (int i =0; i< 3; i++) {
keyName += "/" + RandomStringUtils.randomAlphabetic(5);
}

OMRequest omRequest = createDirectoryRequest(volumeName, bucketName,
keyName);
OMDirectoryCreateRequest omDirectoryCreateRequest =
new OMDirectoryCreateRequest(omRequest);

OMRequest modifiedOmRequest =
omDirectoryCreateRequest.preExecute(ozoneManager);

omDirectoryCreateRequest = new OMDirectoryCreateRequest(modifiedOmRequest);

OMClientResponse omClientResponse =
omDirectoryCreateRequest.validateAndUpdateCache(ozoneManager, 100L,
ozoneManagerDoubleBufferHelper);

Assert.assertTrue(omClientResponse.getOMResponse().getStatus()
Copy link
Contributor

@jojochuang jojochuang Oct 15, 2019

Choose a reason for hiding this comment

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

use Assert.assertEquals() instead

== OzoneManagerProtocolProtos.Status.VOLUME_NOT_FOUND);

// Key should not exist in DB
Assert.assertTrue(omMetadataManager.getKeyTable().get(
Copy link
Contributor

@jojochuang jojochuang Oct 15, 2019

Choose a reason for hiding this comment

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

use assertNull()

Copy link
Member Author

@cxorm cxorm Oct 16, 2019

Choose a reason for hiding this comment

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

Thanks @jojochuang for the review, I am going to fix it.

omMetadataManager.getOzoneDirKey(
volumeName, bucketName, keyName)) == null);

}

@Test
public void testValidateAndUpdateCacheWithBucketNotFound() throws Exception {
Expand All @@ -171,6 +203,7 @@ public void testValidateAndUpdateCacheWithBucketNotFound() throws Exception {
omDirectoryCreateRequest.preExecute(ozoneManager);

omDirectoryCreateRequest = new OMDirectoryCreateRequest(modifiedOmRequest);
TestOMRequestUtils.addVolumeToDB(volumeName, omMetadataManager);

OMClientResponse omClientResponse =
omDirectoryCreateRequest.validateAndUpdateCache(ozoneManager, 100L,
Expand All @@ -183,7 +216,6 @@ public void testValidateAndUpdateCacheWithBucketNotFound() throws Exception {
Assert.assertTrue(omMetadataManager.getKeyTable().get(
omMetadataManager.getOzoneDirKey(
volumeName, bucketName, keyName)) == null);

}

@Test
Expand Down
21 changes: 21 additions & 0 deletions ...anager/src/test/java/org/apache/hadoop/ozone/om/request/file/TestOMFileCreateRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos
.OMRequest;

import static org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.Status.VOLUME_NOT_FOUND;
import static org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.Status.BUCKET_NOT_FOUND;
import static org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.Status.FILE_ALREADY_EXISTS;
import static org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.Status.NOT_A_FILE;
Expand Down Expand Up @@ -177,6 +178,26 @@ public void testValidateAndUpdateCache() throws Exception {

}

@Test
public void testValidateAndUpdateCacheWithVolumeNotFound() throws Exception {
OMRequest omRequest = createFileRequest(volumeName, bucketName, keyName,
HddsProtos.ReplicationFactor.ONE, HddsProtos.ReplicationType.RATIS,
false, true);

OMFileCreateRequest omFileCreateRequest = new OMFileCreateRequest(
omRequest);

OMRequest modifiedOmRequest = omFileCreateRequest.preExecute(ozoneManager);

omFileCreateRequest = new OMFileCreateRequest(modifiedOmRequest);

OMClientResponse omFileCreateResponse =
omFileCreateRequest.validateAndUpdateCache(ozoneManager, 100L,
ozoneManagerDoubleBufferHelper);
Assert.assertEquals(VOLUME_NOT_FOUND,
omFileCreateResponse.getOMResponse().getStatus());

}

@Test
public void testValidateAndUpdateCacheWithBucketNotFound() throws Exception {
Expand Down
26 changes: 20 additions & 6 deletions ...-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyDeleteRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,26 +107,40 @@ public void testValidateAndUpdateCacheWithKeyNotFound() throws Exception {
omClientResponse.getOMResponse().getStatus());
}


@Test
public void testValidateAndUpdateCacheWithOutVolumeAndBucket()
throws Exception {
public void testValidateAndUpdateCacheWithVolumeNotFound() throws Exception {
OMRequest modifiedOmRequest =
doPreExecute(createDeleteKeyRequest());

OMKeyDeleteRequest omKeyDeleteRequest =
new OMKeyDeleteRequest(modifiedOmRequest);

// In actual implementation we don't check for bucket/volume exists
// during delete key. So it should still return error KEY_NOT_FOUND

OMClientResponse omClientResponse =
omKeyDeleteRequest.validateAndUpdateCache(ozoneManager,
100L, ozoneManagerDoubleBufferHelper);

Assert.assertEquals(OzoneManagerProtocolProtos.Status.KEY_NOT_FOUND,
Assert.assertEquals(OzoneManagerProtocolProtos.Status.VOLUME_NOT_FOUND,
omClientResponse.getOMResponse().getStatus());
}

@Test
public void testValidateAndUpdateCacheWithBucketNotFound() throws Exception {
OMRequest modifiedOmRequest =
doPreExecute(createDeleteKeyRequest());

OMKeyDeleteRequest omKeyDeleteRequest =
new OMKeyDeleteRequest(modifiedOmRequest);

TestOMRequestUtils.addVolumeToDB(volumeName, omMetadataManager);

OMClientResponse omClientResponse =
omKeyDeleteRequest.validateAndUpdateCache(ozoneManager,
100L, ozoneManagerDoubleBufferHelper);

Assert.assertEquals(OzoneManagerProtocolProtos.Status.BUCKET_NOT_FOUND,
omClientResponse.getOMResponse().getStatus());
}

/**
* This method calls preExecute and verify the modified request.
Expand Down
28 changes: 23 additions & 5 deletions ...-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyRenameRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,8 @@ public void testValidateAndUpdateCache() throws Exception {
OMRequest modifiedOmRequest =
doPreExecute(createRenameKeyRequest(toKeyName));

TestOMRequestUtils.addVolumeAndBucketToDB(volumeName, bucketName,
omMetadataManager);
TestOMRequestUtils.addKeyToTable(false, volumeName, bucketName, keyName,
clientID, replicationType, replicationFactor, omMetadataManager);

Expand Down Expand Up @@ -112,16 +114,32 @@ public void testValidateAndUpdateCacheWithKeyNotFound() throws Exception {

}

@Test
public void testValidateAndUpdateCacheWithVolumeNotFound() throws Exception {
String toKeyName = UUID.randomUUID().toString();
OMRequest modifiedOmRequest =
doPreExecute(createRenameKeyRequest(toKeyName));

OMKeyRenameRequest omKeyRenameRequest =
new OMKeyRenameRequest(modifiedOmRequest);

OMClientResponse omKeyRenameResponse =
omKeyRenameRequest.validateAndUpdateCache(ozoneManager, 100L,
ozoneManagerDoubleBufferHelper);

Assert.assertEquals(OzoneManagerProtocolProtos.Status.VOLUME_NOT_FOUND,
omKeyRenameResponse.getOMResponse().getStatus());

}

@Test
public void testValidateAndUpdateCacheWithOutVolumeAndBucket()
throws Exception {
public void testValidateAndUpdateCacheWithBucketNotFound() throws Exception {
String toKeyName = UUID.randomUUID().toString();
OMRequest modifiedOmRequest =
doPreExecute(createRenameKeyRequest(toKeyName));

// In actual implementation we don't check for bucket/volume exists
// during delete key. So it should still return error KEY_NOT_FOUND
// Add only volume entry to DB.
TestOMRequestUtils.addVolumeToDB(volumeName, omMetadataManager);

OMKeyRenameRequest omKeyRenameRequest =
new OMKeyRenameRequest(modifiedOmRequest);
Expand All @@ -130,7 +148,7 @@ public void testValidateAndUpdateCacheWithOutVolumeAndBucket()
omKeyRenameRequest.validateAndUpdateCache(ozoneManager, 100L,
ozoneManagerDoubleBufferHelper);

Assert.assertEquals(OzoneManagerProtocolProtos.Status.KEY_NOT_FOUND,
Assert.assertEquals(OzoneManagerProtocolProtos.Status.BUCKET_NOT_FOUND,
omKeyRenameResponse.getOMResponse().getStatus());

}
Expand Down