This is an automated email from the ASF dual-hosted git repository.
siyao pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/hadoop-ozone.git
The following commit(s) were added to refs/heads/master by this push:
new ac5fb0f HDDS-3374. OMVolumeSetOwnerRequest doesn't check if user is
already the owner (#806)
ac5fb0f is described below
commit ac5fb0f46f41e19e68d70673157e7fe439f1c9af
Author: Siyao Meng <[email protected]>
AuthorDate: Tue Apr 14 10:54:02 2020 -0700
HDDS-3374. OMVolumeSetOwnerRequest doesn't check if user is already the
owner (#806)
---
.../ozone/om/request/volume/OMVolumeRequest.java | 21 ++++++----
.../om/request/volume/OMVolumeSetOwnerRequest.java | 17 +++++---
.../response/volume/OMVolumeSetOwnerResponse.java | 21 +++++++++-
.../volume/TestOMVolumeSetOwnerRequest.java | 49 ++++++++++++++++++++++
4 files changed, 92 insertions(+), 16 deletions(-)
diff --git
a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/OMVolumeRequest.java
b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/OMVolumeRequest.java
index bbd0480..4c481a1 100644
---
a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/OMVolumeRequest.java
+++
b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/OMVolumeRequest.java
@@ -29,16 +29,23 @@ import
org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos
.UserVolumeInfo;
import org.apache.hadoop.hdds.utils.db.cache.CacheKey;
import org.apache.hadoop.hdds.utils.db.cache.CacheValue;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
import java.io.IOException;
import java.util.ArrayList;
+import java.util.HashSet;
import java.util.List;
+import java.util.Set;
/**
* Defines common methods required for volume requests.
*/
public abstract class OMVolumeRequest extends OMClientRequest {
+ private static final Logger LOG =
+ LoggerFactory.getLogger(OMVolumeRequest.class);
+
public OMVolumeRequest(OMRequest omRequest) {
super(omRequest);
}
@@ -99,22 +106,18 @@ public abstract class OMVolumeRequest extends
OMClientRequest {
OMException.ResultCodes.USER_TOO_MANY_VOLUMES);
}
- List<String> prevVolList = new ArrayList<>();
+ Set<String> volumeSet = new HashSet<>();
long objectID = txID;
if (volumeList != null) {
- prevVolList.addAll(volumeList.getVolumeNamesList());
+ volumeSet.addAll(volumeList.getVolumeNamesList());
objectID = volumeList.getObjectID();
}
-
- // Add the new volume to the list
- prevVolList.add(volume);
- UserVolumeInfo newVolList = UserVolumeInfo.newBuilder()
+ volumeSet.add(volume);
+ return UserVolumeInfo.newBuilder()
.setObjectID(objectID)
.setUpdateID(txID)
- .addAllVolumeNames(prevVolList).build();
-
- return newVolList;
+ .addAllVolumeNames(volumeSet).build();
}
/**
diff --git
a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/OMVolumeSetOwnerRequest.java
b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/OMVolumeSetOwnerRequest.java
index 6b603e5..fd0898d 100644
---
a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/OMVolumeSetOwnerRequest.java
+++
b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/OMVolumeSetOwnerRequest.java
@@ -71,7 +71,6 @@ public class OMVolumeSetOwnerRequest extends OMVolumeRequest {
SetVolumePropertyRequest setVolumePropertyRequest =
getOmRequest().getSetVolumePropertyRequest();
-
Preconditions.checkNotNull(setVolumePropertyRequest);
OMResponse.Builder omResponse = OMResponse.newBuilder().setCmdType(
@@ -112,7 +111,6 @@ public class OMVolumeSetOwnerRequest extends
OMVolumeRequest {
}
long maxUserVolumeCount = ozoneManager.getMaxUserVolumeCount();
-
String dbVolumeKey = omMetadataManager.getVolumeKey(volume);
OzoneManagerProtocolProtos.UserVolumeInfo oldOwnerVolumeList = null;
@@ -121,7 +119,6 @@ public class OMVolumeSetOwnerRequest extends
OMVolumeRequest {
acquiredVolumeLock = omMetadataManager.getLock().acquireWriteLock(
VOLUME_LOCK, volume);
-
omVolumeArgs = omMetadataManager.getVolumeTable().get(dbVolumeKey);
if (omVolumeArgs == null) {
@@ -143,6 +140,16 @@ public class OMVolumeSetOwnerRequest extends
OMVolumeRequest {
oldOwner = omVolumeArgs.getOwnerName();
+ // Return OK immediately if newOwner is the same as oldOwner.
+ if (oldOwner.equals(newOwner)) {
+ LOG.warn("Volume '{}' owner is already user '{}'.", volume, oldOwner);
+ omResponse.setStatus(OzoneManagerProtocolProtos.Status.OK)
+ .setMessage(
+ "Volume '" + volume + "' owner is already '" + newOwner + "'.")
+ .setSuccess(false);
+ return new OMVolumeSetOwnerResponse(omResponse.build());
+ }
+
acquiredUserLocks =
omMetadataManager.getLock().acquireMultiUserLock(newOwner, oldOwner);
@@ -165,8 +172,8 @@ public class OMVolumeSetOwnerRequest extends
OMVolumeRequest {
// Update cache.
omMetadataManager.getUserTable().addCacheEntry(
new CacheKey<>(omMetadataManager.getUserKey(newOwner)),
- new CacheValue<>(Optional.of(newOwnerVolumeList),
- transactionLogIndex));
+ new CacheValue<>(Optional.of(newOwnerVolumeList),
+ transactionLogIndex));
omMetadataManager.getUserTable().addCacheEntry(
new CacheKey<>(omMetadataManager.getUserKey(oldOwner)),
new CacheValue<>(Optional.of(oldOwnerVolumeList),
diff --git
a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/volume/OMVolumeSetOwnerResponse.java
b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/volume/OMVolumeSetOwnerResponse.java
index 8cd1f05..469f1ae 100644
---
a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/volume/OMVolumeSetOwnerResponse.java
+++
b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/volume/OMVolumeSetOwnerResponse.java
@@ -24,6 +24,7 @@ import org.apache.hadoop.ozone.om.OMMetadataManager;
import org.apache.hadoop.ozone.om.helpers.OmVolumeArgs;
import org.apache.hadoop.ozone.om.response.OMClientResponse;
+import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos;
import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos
.UserVolumeInfo;
import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos
@@ -55,11 +56,27 @@ public class OMVolumeSetOwnerResponse extends
OMClientResponse {
/**
* For when the request is not successful or it is a replay transaction.
- * For a successful request, the other constructor should be used.
+ * Or when newOwner is the same as oldOwner.
+ * For other successful requests, the other constructor should be used.
*/
public OMVolumeSetOwnerResponse(@Nonnull OMResponse omResponse) {
super(omResponse);
- checkStatusNotOK();
+ // When newOwner is the same as oldOwner, status is OK but success is
false.
+ // We want to bypass the check in this case.
+ if (omResponse.getSuccess()) {
+ checkStatusNotOK();
+ }
+ }
+
+ @Override
+ public void checkAndUpdateDB(OMMetadataManager omMetadataManager,
+ BatchOperation batchOperation) throws IOException {
+ // When newOwner is the same as oldOwner, status is OK but success is
false.
+ // We don't want to add it to DB batch in this case.
+ if (getOMResponse().getStatus() == OzoneManagerProtocolProtos.Status.OK &&
+ getOMResponse().getSuccess()) {
+ addToDBBatch(omMetadataManager, batchOperation);
+ }
}
@Override
diff --git
a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/volume/TestOMVolumeSetOwnerRequest.java
b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/volume/TestOMVolumeSetOwnerRequest.java
index 3c69a3b..c71c1a8 100644
---
a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/volume/TestOMVolumeSetOwnerRequest.java
+++
b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/volume/TestOMVolumeSetOwnerRequest.java
@@ -18,6 +18,9 @@
package org.apache.hadoop.ozone.om.request.volume;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
import java.util.UUID;
import org.junit.Assert;
@@ -189,4 +192,50 @@ public class TestOMVolumeSetOwnerRequest extends
TestOMVolumeRequest {
Assert.assertEquals(OzoneManagerProtocolProtos.Status.REPLAY,
omClientResponse.getOMResponse().getStatus());
}
+
+
+ @Test
+ public void testOwnSameVolumeTwice() throws Exception {
+ String volumeName = UUID.randomUUID().toString();
+ String owner = "user1";
+ TestOMRequestUtils.addVolumeToDB(volumeName, owner, omMetadataManager);
+ TestOMRequestUtils.addUserToDB(volumeName, owner, omMetadataManager);
+ String newOwner = "user2";
+
+ // Create request to set new owner
+ OMRequest omRequest =
+ TestOMRequestUtils.createSetVolumePropertyRequest(volumeName,
newOwner);
+
+ OMVolumeSetOwnerRequest setOwnerRequest =
+ new OMVolumeSetOwnerRequest(omRequest);
+ // Execute the request
+ setOwnerRequest.preExecute(ozoneManager);
+ OMClientResponse omClientResponse = setOwnerRequest.validateAndUpdateCache(
+ ozoneManager, 1, ozoneManagerDoubleBufferHelper);
+ // Response status should be OK and success flag should be true.
+ Assert.assertEquals(OzoneManagerProtocolProtos.Status.OK,
+ omClientResponse.getOMResponse().getStatus());
+ Assert.assertTrue(omClientResponse.getOMResponse().getSuccess());
+
+ // Execute the same request again but with higher index
+ setOwnerRequest.preExecute(ozoneManager);
+ omClientResponse = setOwnerRequest.validateAndUpdateCache(
+ ozoneManager, 2, ozoneManagerDoubleBufferHelper);
+ // Response status should be OK, but success flag should be false.
+ Assert.assertEquals(OzoneManagerProtocolProtos.Status.OK,
+ omClientResponse.getOMResponse().getStatus());
+ Assert.assertFalse(omClientResponse.getOMResponse().getSuccess());
+
+ // Check volume names list
+ OzoneManagerProtocolProtos.UserVolumeInfo userVolumeInfo =
+ omMetadataManager.getUserTable().get(newOwner);
+ Assert.assertNotNull(userVolumeInfo);
+ List<String> volumeNamesList = userVolumeInfo.getVolumeNamesList();
+ Assert.assertEquals(1, volumeNamesList.size());
+
+ Set<String> volumeNamesSet = new HashSet<>(volumeNamesList);
+ // If the set size isn't equal to list size, there are duplicates
+ // in the list (which was the bug before the fix).
+ Assert.assertEquals(volumeNamesList.size(), volumeNamesSet.size());
+ }
}
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
