You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2020/04/10 12:27:07 UTC
[GitHub] [hadoop-ozone] smengcl opened a new pull request #806: HDDS-3374.
OMVolumeSetOwnerRequest doesn't check if user is already the owner
smengcl opened a new pull request #806: HDDS-3374. OMVolumeSetOwnerRequest doesn't check if user is already the owner
URL: https://github.com/apache/hadoop-ozone/pull/806
## What changes were proposed in this pull request?
1. OMVolumeSetOwnerRequest doesn't check if the user is already the owner
2. It also doesn't seem to remove the volume from the UserVolumeInfo from the previous owner.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-3374
## How was this patch tested?
The test case (`setOwner` twice on the same volume with the same user) should pass.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] xiaoyuyao commented on a change in pull request
#806: HDDS-3374. OMVolumeSetOwnerRequest doesn't check if user is already
the owner
Posted by GitBox <gi...@apache.org>.
xiaoyuyao commented on a change in pull request #806: HDDS-3374. OMVolumeSetOwnerRequest doesn't check if user is already the owner
URL: https://github.com/apache/hadoop-ozone/pull/806#discussion_r408229442
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/volume/OMVolumeSetOwnerResponse.java
##########
@@ -55,11 +56,27 @@ public OMVolumeSetOwnerResponse(@Nonnull OMResponse omResponse,
/**
* For when the request is not successful or it is a replay transaction.
Review comment:
should we remove this comment as checkStatusNotOK is now conditioned with success=true? Also, do we redirect existing callers for success=false case appropriately?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] smengcl commented on a change in pull request #806:
HDDS-3374. OMVolumeSetOwnerRequest doesn't check if user is already the owner
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #806: HDDS-3374. OMVolumeSetOwnerRequest doesn't check if user is already the owner
URL: https://github.com/apache/hadoop-ozone/pull/806#discussion_r408251745
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/volume/OMVolumeSetOwnerResponse.java
##########
@@ -55,11 +56,27 @@ public OMVolumeSetOwnerResponse(@Nonnull OMResponse omResponse,
/**
* For when the request is not successful or it is a replay transaction.
Review comment:
Though in all four cases the success flag is set to false, rendering the condition around `checkStatusNotOK()` (seemingly) pointless.
Still, this should act to prevent new code from calling this constructor when success is true and status is OK.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] smengcl commented on a change in pull request #806:
HDDS-3374. OMVolumeSetOwnerRequest doesn't check if user is already the owner
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #806: HDDS-3374. OMVolumeSetOwnerRequest doesn't check if user is already the owner
URL: https://github.com/apache/hadoop-ozone/pull/806#discussion_r408257039
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/volume/OMVolumeSetOwnerResponse.java
##########
@@ -55,11 +56,27 @@ public OMVolumeSetOwnerResponse(@Nonnull OMResponse omResponse,
/**
* For when the request is not successful or it is a replay transaction.
Review comment:
Just BTW in a discussion with @bharatviswa504 we concluded that the reason of setting success to `false` for "newOwner equals to oldOwner" case is that in [`addAcl`](https://github.com/apache/hadoop-ozone/blob/ebc616061da6f337626ee85e4282230aedfd748a/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/acl/AclOption.java#L58-L62) we have some logic to print different results on the client depending on the success flag. Though the success flag of `setOwner` isn't utilized at the moment, we might be able to use it later.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] smengcl commented on a change in pull request #806:
HDDS-3374. OMVolumeSetOwnerRequest doesn't check if user is already the owner
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #806: HDDS-3374. OMVolumeSetOwnerRequest doesn't check if user is already the owner
URL: https://github.com/apache/hadoop-ozone/pull/806#discussion_r408251745
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/volume/OMVolumeSetOwnerResponse.java
##########
@@ -55,11 +56,27 @@ public OMVolumeSetOwnerResponse(@Nonnull OMResponse omResponse,
/**
* For when the request is not successful or it is a replay transaction.
Review comment:
Though in all four cases the success flag is set to false, rendering the condition around `checkStatusNotOK()` pointless.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] xiaoyuyao commented on a change in pull request
#806: HDDS-3374. OMVolumeSetOwnerRequest doesn't check if user is already
the owner
Posted by GitBox <gi...@apache.org>.
xiaoyuyao commented on a change in pull request #806: HDDS-3374. OMVolumeSetOwnerRequest doesn't check if user is already the owner
URL: https://github.com/apache/hadoop-ozone/pull/806#discussion_r408233508
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/volume/OMVolumeSetOwnerResponse.java
##########
@@ -55,11 +56,27 @@ public OMVolumeSetOwnerResponse(@Nonnull OMResponse omResponse,
/**
* For when the request is not successful or it is a replay transaction.
Review comment:
I think the reply response is taken care by createReplayOMResponse now. So we can safely clean up the comments above.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] smengcl commented on a change in pull request #806:
HDDS-3374. OMVolumeSetOwnerRequest doesn't check if user is already the owner
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #806: HDDS-3374. OMVolumeSetOwnerRequest doesn't check if user is already the owner
URL: https://github.com/apache/hadoop-ozone/pull/806#discussion_r406887127
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOzoneManagerSetOwner.java
##########
@@ -0,0 +1,123 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.om;
+
+import java.io.IOException;
+import java.util.UUID;
+
+import org.apache.hadoop.hdds.conf.OzoneConfiguration;
+import org.apache.hadoop.ozone.MiniOzoneCluster;
+import org.apache.hadoop.ozone.client.ObjectStore;
+import org.apache.hadoop.ozone.client.OzoneClient;
+import org.apache.hadoop.ozone.client.protocol.ClientProtocol;
+import org.apache.hadoop.ozone.om.exceptions.OMException;
+import org.apache.hadoop.security.UserGroupInformation;
+
+import static org.apache.hadoop.hdds.scm.ScmConfigKeys.OZONE_SCM_RATIS_PIPELINE_LIMIT;
+import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_ACL_AUTHORIZER_CLASS;
+import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_ACL_AUTHORIZER_CLASS_NATIVE;
+import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_ACL_ENABLED;
+import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_ADMINISTRATORS;
+import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_OPEN_KEY_EXPIRE_THRESHOLD_SECONDS;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.Timeout;
+
+/**
+ * Test OzoneManager list volume operation under combinations of configs.
+ */
+public class TestOzoneManagerSetOwner {
+
+ @Rule
+ public Timeout timeout = new Timeout(120_000);
+
+ private UserGroupInformation loginUser;
+
+ @Before
+ public void init() throws Exception {
+ loginUser = UserGroupInformation.getLoginUser();
+ }
+
+ /**
+ * Create a MiniDFSCluster for testing.
+ */
+ private MiniOzoneCluster startCluster(boolean aclEnabled) throws Exception {
+
+ OzoneConfiguration conf = new OzoneConfiguration();
+ String clusterId = UUID.randomUUID().toString();
+ String scmId = UUID.randomUUID().toString();
+ String omId = UUID.randomUUID().toString();
+ conf.setInt(OZONE_OPEN_KEY_EXPIRE_THRESHOLD_SECONDS, 2);
+ conf.set(OZONE_ADMINISTRATORS, "user1");
+ conf.setInt(OZONE_SCM_RATIS_PIPELINE_LIMIT, 10);
+
+ // Use native impl here, default impl doesn't do actual checks
+ conf.set(OZONE_ACL_AUTHORIZER_CLASS, OZONE_ACL_AUTHORIZER_CLASS_NATIVE);
+ // Note: OM doesn't support live config reloading
+ conf.setBoolean(OZONE_ACL_ENABLED, aclEnabled);
+
Review comment:
Yes I acknowledge that. This integration test is a quick hack from https://github.com/apache/hadoop-ozone/pull/696 's test since I discovered this issue when debugging that PR. Just a POC for now. Will figure out a way to write this in `TestOMVolumeSetOwnerRequest`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] smengcl commented on a change in pull request #806:
HDDS-3374. OMVolumeSetOwnerRequest doesn't check if user is already the owner
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #806: HDDS-3374. OMVolumeSetOwnerRequest doesn't check if user is already the owner
URL: https://github.com/apache/hadoop-ozone/pull/806#discussion_r408257039
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/volume/OMVolumeSetOwnerResponse.java
##########
@@ -55,11 +56,27 @@ public OMVolumeSetOwnerResponse(@Nonnull OMResponse omResponse,
/**
* For when the request is not successful or it is a replay transaction.
Review comment:
Just BTW in a discussion with @bharatviswa504 we conclude the reason of setting success to `false` for "newOwner equals to oldOwner" case is that in `addAcl` we have [some logic](https://github.com/apache/hadoop-ozone/blob/ebc616061da6f337626ee85e4282230aedfd748a/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/acl/AclOption.java#L58-L62) to print different results on the client side depending on the success flag. Though the success flag of `setOwner` isn't utilized at the moment, we might be able to use it later.
And reason of returning status OK is that the client will throw exception if the status isn't OK.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] smengcl commented on issue #806: HDDS-3374.
OMVolumeSetOwnerRequest doesn't check if user is already the owner
Posted by GitBox <gi...@apache.org>.
smengcl commented on issue #806: HDDS-3374. OMVolumeSetOwnerRequest doesn't check if user is already the owner
URL: https://github.com/apache/hadoop-ozone/pull/806#issuecomment-613484487
Test looks good. Should be good to go. Please take a look at the latest changes.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] smengcl commented on a change in pull request #806:
HDDS-3374. OMVolumeSetOwnerRequest doesn't check if user is already the owner
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #806: HDDS-3374. OMVolumeSetOwnerRequest doesn't check if user is already the owner
URL: https://github.com/apache/hadoop-ozone/pull/806#discussion_r408262005
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/volume/OMVolumeSetOwnerResponse.java
##########
@@ -55,11 +56,27 @@ public OMVolumeSetOwnerResponse(@Nonnull OMResponse omResponse,
/**
* For when the request is not successful or it is a replay transaction.
Review comment:
> Though in all four cases the success flag is set to false, rendering the condition around `checkStatusNotOK()` pointless.
Maybe we should just remove the call to `checkStatusNotOK()` in this constructor now that we established this new use case. What do you think?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] smengcl commented on a change in pull request #806:
HDDS-3374. OMVolumeSetOwnerRequest doesn't check if user is already the owner
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #806: HDDS-3374. OMVolumeSetOwnerRequest doesn't check if user is already the owner
URL: https://github.com/apache/hadoop-ozone/pull/806#discussion_r406888013
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/OMVolumeRequest.java
##########
@@ -106,6 +106,13 @@ protected UserVolumeInfo addVolumeToOwnerList(UserVolumeInfo volumeList,
objectID = volumeList.getObjectID();
}
+ // Sanity check, a user should not own same volume twice
+ // TODO: May want to remove this due to perf if user owns a lot of volumes.
+ if (prevVolList.contains(volume)) {
Review comment:
I was also thinking of adding a new type of exception. But now I think @dineshchitlangia 's suggestion might be better -- handle it silently and maybe throw a warning.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] smengcl commented on a change in pull request #806:
HDDS-3374. OMVolumeSetOwnerRequest doesn't check if user is already the owner
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #806: HDDS-3374. OMVolumeSetOwnerRequest doesn't check if user is already the owner
URL: https://github.com/apache/hadoop-ozone/pull/806#discussion_r408251049
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/volume/OMVolumeSetOwnerResponse.java
##########
@@ -55,11 +56,27 @@ public OMVolumeSetOwnerResponse(@Nonnull OMResponse omResponse,
/**
* For when the request is not successful or it is a replay transaction.
Review comment:
I believe the comment is still accurate. There are four places that use this constructor at the moment:
- INVALID_REQUEST
```java
// In production this will never happen, this request will be called only
// when we have ownerName in setVolumePropertyRequest.
if (!setVolumePropertyRequest.hasOwnerName()) {
omResponse.setStatus(OzoneManagerProtocolProtos.Status.INVALID_REQUEST)
.setSuccess(false);
return new OMVolumeSetOwnerResponse(omResponse.build());
}
```
- Replay
```java
// Check if this transaction is a replay of ratis logs.
// If this is a replay, then the response has already been returned to
// the client. So take no further action and return a dummy
// OMClientResponse.
if (isReplay(ozoneManager, omVolumeArgs, transactionLogIndex)) {
LOG.debug("Replayed Transaction {} ignored. Request: {}",
transactionLogIndex, setVolumePropertyRequest);
return new OMVolumeSetOwnerResponse(createReplayOMResponse(omResponse));
}
```
- newOwner is the same as oldOwner (added in this PR)
```java
// 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());
}
```
- IOException
```java
} catch (IOException ex) {
exception = ex;
omClientResponse = new OMVolumeSetOwnerResponse(
createErrorOMResponse(omResponse, exception));
} finally {
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] bharatviswa504 commented on a change in pull request
#806: HDDS-3374. OMVolumeSetOwnerRequest doesn't check if user is already
the owner
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #806: HDDS-3374. OMVolumeSetOwnerRequest doesn't check if user is already the owner
URL: https://github.com/apache/hadoop-ozone/pull/806#discussion_r406973632
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/OMVolumeRequest.java
##########
@@ -106,6 +106,13 @@ protected UserVolumeInfo addVolumeToOwnerList(UserVolumeInfo volumeList,
objectID = volumeList.getObjectID();
}
+ // Sanity check, a user should not own same volume twice
+ // TODO: May want to remove this due to perf if user owns a lot of volumes.
+ if (prevVolList.contains(volume)) {
Review comment:
Can we document this behavior, so that the end-user knows about this behavior?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] smengcl commented on issue #806: HDDS-3374.
OMVolumeSetOwnerRequest doesn't check if user is already the owner
Posted by GitBox <gi...@apache.org>.
smengcl commented on issue #806: HDDS-3374. OMVolumeSetOwnerRequest doesn't check if user is already the owner
URL: https://github.com/apache/hadoop-ozone/pull/806#issuecomment-613588537
Thanks @dineshchitlangia @bharatviswa504 @xiaoyuyao for the review and comments!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] smengcl commented on a change in pull request #806:
HDDS-3374. OMVolumeSetOwnerRequest doesn't check if user is already the owner
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #806: HDDS-3374. OMVolumeSetOwnerRequest doesn't check if user is already the owner
URL: https://github.com/apache/hadoop-ozone/pull/806#discussion_r408279098
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/volume/OMVolumeSetOwnerResponse.java
##########
@@ -55,11 +56,27 @@ public OMVolumeSetOwnerResponse(@Nonnull OMResponse omResponse,
/**
* For when the request is not successful or it is a replay transaction.
Review comment:
> Just to add more info, we set the success flag and also there is a response which we return that is used by the end client which has a response flag which is set based on the result of the operation.
>
> ```
> OMClientResponse onSuccess(OMResponse.Builder omResponse,
> OmBucketInfo omBucketInfo, boolean operationResult) {
> omResponse.setSuccess(operationResult);
> omResponse.setAddAclResponse(AddAclResponse.newBuilder()
> .setResponse(operationResult));
> ```
>
> But in OmVolumeSetOwner case, there is no such flag. If we can introduce a flag in SetVolumePropertyResponse, we can use the success flag.
>
> ```
> omResponse.setSetVolumePropertyResponse(
> SetVolumePropertyResponse.newBuilder().build());
> ```
Good idea we can add `optional bool response = 1;` in [here](https://github.com/apache/hadoop-ozone/blob/15db251f16236228c6596253dab6946494fc8f5b/hadoop-ozone/common/src/main/proto/OzoneManagerProtocol.proto#L425-L427) in another jira.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] dineshchitlangia commented on a change in pull
request #806: HDDS-3374. OMVolumeSetOwnerRequest doesn't check if user is
already the owner
Posted by GitBox <gi...@apache.org>.
dineshchitlangia commented on a change in pull request #806: HDDS-3374. OMVolumeSetOwnerRequest doesn't check if user is already the owner
URL: https://github.com/apache/hadoop-ozone/pull/806#discussion_r406865345
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/OMVolumeRequest.java
##########
@@ -106,6 +106,13 @@ protected UserVolumeInfo addVolumeToOwnerList(UserVolumeInfo volumeList,
objectID = volumeList.getObjectID();
}
+ // Sanity check, a user should not own same volume twice
+ // TODO: May want to remove this due to perf if user owns a lot of volumes.
+ if (prevVolList.contains(volume)) {
+ throw new IOException("Invalid operation: User " + owner +
+ " is about to own a same volume " + volume + " twice!" +
+ " Check for DB consistency error.");
+ }
// Add the new volume to the list
prevVolList.add(volume);
Review comment:
Instead of throwing exception here, I was wondering if we can instead perform the subsequent "add new volume to list" if !prevVolList.contains(volume) and complement with WARN log.
```suggestion
// Avoid adding a user to the same volume twice
if (!prevVolList.contains(volume)) {
// Add the new volume to the list
prevVolList.add(volume);
UserVolumeInfo newVolList = UserVolumeInfo.newBuilder()
.setObjectID(objectID)
.setUpdateID(txID)
.addAllVolumeNames(prevVolList).build();
}
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] dineshchitlangia commented on a change in pull
request #806: HDDS-3374. OMVolumeSetOwnerRequest doesn't check if user is
already the owner
Posted by GitBox <gi...@apache.org>.
dineshchitlangia commented on a change in pull request #806: HDDS-3374. OMVolumeSetOwnerRequest doesn't check if user is already the owner
URL: https://github.com/apache/hadoop-ozone/pull/806#discussion_r406866343
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/OMVolumeSetOwnerRequest.java
##########
@@ -143,6 +143,11 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
oldOwner = omVolumeArgs.getOwnerName();
+ if (oldOwner.equals(newOwner)) {
+ throw new OMException("Owner of volume " + volume + " is already " +
+ newOwner, OMException.ResultCodes.ACCESS_DENIED);
+ }
+
Review comment:
Like previous comment, we can make similar change here too.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] bharatviswa504 commented on a change in pull request
#806: HDDS-3374. OMVolumeSetOwnerRequest doesn't check if user is already
the owner
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #806: HDDS-3374. OMVolumeSetOwnerRequest doesn't check if user is already the owner
URL: https://github.com/apache/hadoop-ozone/pull/806#discussion_r408262348
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/volume/OMVolumeSetOwnerResponse.java
##########
@@ -55,11 +56,27 @@ public OMVolumeSetOwnerResponse(@Nonnull OMResponse omResponse,
/**
* For when the request is not successful or it is a replay transaction.
Review comment:
Just to add more info, we set the success flag and also there is a response which we return that is used by the end client which has a response flag which is set based on the result of the operation.
```
OMClientResponse onSuccess(OMResponse.Builder omResponse,
OmBucketInfo omBucketInfo, boolean operationResult) {
omResponse.setSuccess(operationResult);
omResponse.setAddAclResponse(AddAclResponse.newBuilder()
.setResponse(operationResult));
```
But in OmVolumeSetOwner case, there is no such flag. If we can introduce a flag in SetVolumePropertyResponse, we can use the success flag.
omResponse.setSetVolumePropertyResponse(
SetVolumePropertyResponse.newBuilder().build());
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] smengcl commented on a change in pull request #806:
HDDS-3374. OMVolumeSetOwnerRequest doesn't check if user is already the owner
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #806: HDDS-3374. OMVolumeSetOwnerRequest doesn't check if user is already the owner
URL: https://github.com/apache/hadoop-ozone/pull/806#discussion_r406887127
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOzoneManagerSetOwner.java
##########
@@ -0,0 +1,123 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.om;
+
+import java.io.IOException;
+import java.util.UUID;
+
+import org.apache.hadoop.hdds.conf.OzoneConfiguration;
+import org.apache.hadoop.ozone.MiniOzoneCluster;
+import org.apache.hadoop.ozone.client.ObjectStore;
+import org.apache.hadoop.ozone.client.OzoneClient;
+import org.apache.hadoop.ozone.client.protocol.ClientProtocol;
+import org.apache.hadoop.ozone.om.exceptions.OMException;
+import org.apache.hadoop.security.UserGroupInformation;
+
+import static org.apache.hadoop.hdds.scm.ScmConfigKeys.OZONE_SCM_RATIS_PIPELINE_LIMIT;
+import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_ACL_AUTHORIZER_CLASS;
+import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_ACL_AUTHORIZER_CLASS_NATIVE;
+import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_ACL_ENABLED;
+import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_ADMINISTRATORS;
+import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_OPEN_KEY_EXPIRE_THRESHOLD_SECONDS;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.Timeout;
+
+/**
+ * Test OzoneManager list volume operation under combinations of configs.
+ */
+public class TestOzoneManagerSetOwner {
+
+ @Rule
+ public Timeout timeout = new Timeout(120_000);
+
+ private UserGroupInformation loginUser;
+
+ @Before
+ public void init() throws Exception {
+ loginUser = UserGroupInformation.getLoginUser();
+ }
+
+ /**
+ * Create a MiniDFSCluster for testing.
+ */
+ private MiniOzoneCluster startCluster(boolean aclEnabled) throws Exception {
+
+ OzoneConfiguration conf = new OzoneConfiguration();
+ String clusterId = UUID.randomUUID().toString();
+ String scmId = UUID.randomUUID().toString();
+ String omId = UUID.randomUUID().toString();
+ conf.setInt(OZONE_OPEN_KEY_EXPIRE_THRESHOLD_SECONDS, 2);
+ conf.set(OZONE_ADMINISTRATORS, "user1");
+ conf.setInt(OZONE_SCM_RATIS_PIPELINE_LIMIT, 10);
+
+ // Use native impl here, default impl doesn't do actual checks
+ conf.set(OZONE_ACL_AUTHORIZER_CLASS, OZONE_ACL_AUTHORIZER_CLASS_NATIVE);
+ // Note: OM doesn't support live config reloading
+ conf.setBoolean(OZONE_ACL_ENABLED, aclEnabled);
+
Review comment:
Yes I acknowledge that. This integration test is a quick hack from the https://github.com/apache/hadoop-ozone/pull/696 because I discovered this issue when debugging that PR. Just a POC for now. Will figure out a way to write this in `TestOMVolumeSetOwnerRequest`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] smengcl commented on a change in pull request #806:
HDDS-3374. OMVolumeSetOwnerRequest doesn't check if user is already the owner
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #806: HDDS-3374. OMVolumeSetOwnerRequest doesn't check if user is already the owner
URL: https://github.com/apache/hadoop-ozone/pull/806#discussion_r408326596
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/volume/OMVolumeSetOwnerResponse.java
##########
@@ -55,11 +56,27 @@ public OMVolumeSetOwnerResponse(@Nonnull OMResponse omResponse,
/**
* For when the request is not successful or it is a replay transaction.
Review comment:
Discussed with @xiaoyuyao and we agree the comment don't need to be changed at the moment.
Resolving this conversation. Will commit shortly.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] smengcl commented on a change in pull request #806:
HDDS-3374. OMVolumeSetOwnerRequest doesn't check if user is already the owner
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #806: HDDS-3374. OMVolumeSetOwnerRequest doesn't check if user is already the owner
URL: https://github.com/apache/hadoop-ozone/pull/806#discussion_r406888217
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/OMVolumeRequest.java
##########
@@ -106,6 +106,13 @@ protected UserVolumeInfo addVolumeToOwnerList(UserVolumeInfo volumeList,
objectID = volumeList.getObjectID();
}
+ // Sanity check, a user should not own same volume twice
+ // TODO: May want to remove this due to perf if user owns a lot of volumes.
+ if (prevVolList.contains(volume)) {
+ throw new IOException("Invalid operation: User " + owner +
+ " is about to own a same volume " + volume + " twice!" +
+ " Check for DB consistency error.");
+ }
// Add the new volume to the list
prevVolList.add(volume);
Review comment:
Will do. Thanks :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] smengcl commented on a change in pull request #806:
HDDS-3374. OMVolumeSetOwnerRequest doesn't check if user is already the owner
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #806: HDDS-3374. OMVolumeSetOwnerRequest doesn't check if user is already the owner
URL: https://github.com/apache/hadoop-ozone/pull/806#discussion_r408257039
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/volume/OMVolumeSetOwnerResponse.java
##########
@@ -55,11 +56,27 @@ public OMVolumeSetOwnerResponse(@Nonnull OMResponse omResponse,
/**
* For when the request is not successful or it is a replay transaction.
Review comment:
Just BTW in a discussion with @bharatviswa504 we concluded that the reason of setting success to `false` for "newOwner equals to oldOwner" case is that in [`addAcl`](https://github.com/apache/hadoop-ozone/blob/ebc616061da6f337626ee85e4282230aedfd748a/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/acl/AclOption.java#L58-L62) we have some logic to print different results on the client depending on the success flag. Though the success flag of `setOwner` isn't utilized at the moment, we might be able to use it later.
And reason of returning status OK is that the client will throw exception if the status isn't OK.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] smengcl merged pull request #806: HDDS-3374.
OMVolumeSetOwnerRequest doesn't check if user is already the owner
Posted by GitBox <gi...@apache.org>.
smengcl merged pull request #806: HDDS-3374. OMVolumeSetOwnerRequest doesn't check if user is already the owner
URL: https://github.com/apache/hadoop-ozone/pull/806
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] bharatviswa504 commented on a change in pull request
#806: HDDS-3374. OMVolumeSetOwnerRequest doesn't check if user is already
the owner
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #806: HDDS-3374. OMVolumeSetOwnerRequest doesn't check if user is already the owner
URL: https://github.com/apache/hadoop-ozone/pull/806#discussion_r406866823
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/OMVolumeRequest.java
##########
@@ -106,6 +106,13 @@ protected UserVolumeInfo addVolumeToOwnerList(UserVolumeInfo volumeList,
objectID = volumeList.getObjectID();
}
+ // Sanity check, a user should not own same volume twice
+ // TODO: May want to remove this due to perf if user owns a lot of volumes.
+ if (prevVolList.contains(volume)) {
Review comment:
I am thinking instead of returning an error like AccessDenied, can we return a code that this owner is already owner for this volume.
Because access Denied looks not proper here.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] smengcl commented on a change in pull request #806:
HDDS-3374. OMVolumeSetOwnerRequest doesn't check if user is already the owner
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #806: HDDS-3374. OMVolumeSetOwnerRequest doesn't check if user is already the owner
URL: https://github.com/apache/hadoop-ozone/pull/806#discussion_r408262005
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/volume/OMVolumeSetOwnerResponse.java
##########
@@ -55,11 +56,27 @@ public OMVolumeSetOwnerResponse(@Nonnull OMResponse omResponse,
/**
* For when the request is not successful or it is a replay transaction.
Review comment:
> Though in all four cases the success flag is set to false, rendering the condition around `checkStatusNotOK()` pointless.
Maybe we should just remove the call to `checkStatusNotOK()` in this constructor now that we established this new use case. What do you think?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] smengcl commented on a change in pull request #806:
HDDS-3374. OMVolumeSetOwnerRequest doesn't check if user is already the owner
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #806: HDDS-3374. OMVolumeSetOwnerRequest doesn't check if user is already the owner
URL: https://github.com/apache/hadoop-ozone/pull/806#discussion_r408251049
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/volume/OMVolumeSetOwnerResponse.java
##########
@@ -55,11 +56,27 @@ public OMVolumeSetOwnerResponse(@Nonnull OMResponse omResponse,
/**
* For when the request is not successful or it is a replay transaction.
Review comment:
I believe the comment is still accurate. There are four places that calls this constructor at the moment:
- INVALID_REQUEST
```java
// In production this will never happen, this request will be called only
// when we have ownerName in setVolumePropertyRequest.
if (!setVolumePropertyRequest.hasOwnerName()) {
omResponse.setStatus(OzoneManagerProtocolProtos.Status.INVALID_REQUEST)
.setSuccess(false);
return new OMVolumeSetOwnerResponse(omResponse.build());
}
```
- Replay
```java
// Check if this transaction is a replay of ratis logs.
// If this is a replay, then the response has already been returned to
// the client. So take no further action and return a dummy
// OMClientResponse.
if (isReplay(ozoneManager, omVolumeArgs, transactionLogIndex)) {
LOG.debug("Replayed Transaction {} ignored. Request: {}",
transactionLogIndex, setVolumePropertyRequest);
return new OMVolumeSetOwnerResponse(createReplayOMResponse(omResponse));
}
```
- newOwner is the same as oldOwner (added in this PR)
```java
// 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());
}
```
- IOException
```java
} catch (IOException ex) {
exception = ex;
omClientResponse = new OMVolumeSetOwnerResponse(
createErrorOMResponse(omResponse, exception));
} finally {
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] bharatviswa504 commented on a change in pull request
#806: HDDS-3374. OMVolumeSetOwnerRequest doesn't check if user is already
the owner
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #806: HDDS-3374. OMVolumeSetOwnerRequest doesn't check if user is already the owner
URL: https://github.com/apache/hadoop-ozone/pull/806#discussion_r406867227
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOzoneManagerSetOwner.java
##########
@@ -0,0 +1,123 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.om;
+
+import java.io.IOException;
+import java.util.UUID;
+
+import org.apache.hadoop.hdds.conf.OzoneConfiguration;
+import org.apache.hadoop.ozone.MiniOzoneCluster;
+import org.apache.hadoop.ozone.client.ObjectStore;
+import org.apache.hadoop.ozone.client.OzoneClient;
+import org.apache.hadoop.ozone.client.protocol.ClientProtocol;
+import org.apache.hadoop.ozone.om.exceptions.OMException;
+import org.apache.hadoop.security.UserGroupInformation;
+
+import static org.apache.hadoop.hdds.scm.ScmConfigKeys.OZONE_SCM_RATIS_PIPELINE_LIMIT;
+import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_ACL_AUTHORIZER_CLASS;
+import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_ACL_AUTHORIZER_CLASS_NATIVE;
+import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_ACL_ENABLED;
+import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_ADMINISTRATORS;
+import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_OPEN_KEY_EXPIRE_THRESHOLD_SECONDS;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.Timeout;
+
+/**
+ * Test OzoneManager list volume operation under combinations of configs.
+ */
+public class TestOzoneManagerSetOwner {
+
+ @Rule
+ public Timeout timeout = new Timeout(120_000);
+
+ private UserGroupInformation loginUser;
+
+ @Before
+ public void init() throws Exception {
+ loginUser = UserGroupInformation.getLoginUser();
+ }
+
+ /**
+ * Create a MiniDFSCluster for testing.
+ */
+ private MiniOzoneCluster startCluster(boolean aclEnabled) throws Exception {
+
+ OzoneConfiguration conf = new OzoneConfiguration();
+ String clusterId = UUID.randomUUID().toString();
+ String scmId = UUID.randomUUID().toString();
+ String omId = UUID.randomUUID().toString();
+ conf.setInt(OZONE_OPEN_KEY_EXPIRE_THRESHOLD_SECONDS, 2);
+ conf.set(OZONE_ADMINISTRATORS, "user1");
+ conf.setInt(OZONE_SCM_RATIS_PIPELINE_LIMIT, 10);
+
+ // Use native impl here, default impl doesn't do actual checks
+ conf.set(OZONE_ACL_AUTHORIZER_CLASS, OZONE_ACL_AUTHORIZER_CLASS_NATIVE);
+ // Note: OM doesn't support live config reloading
+ conf.setBoolean(OZONE_ACL_ENABLED, aclEnabled);
+
Review comment:
We don't need an IT test to test this behavior. We have a UT TestOMVolumeSetOwnerRequest which we can use this to cover this test.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] smengcl commented on a change in pull request #806:
HDDS-3374. OMVolumeSetOwnerRequest doesn't check if user is already the owner
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #806: HDDS-3374. OMVolumeSetOwnerRequest doesn't check if user is already the owner
URL: https://github.com/apache/hadoop-ozone/pull/806#discussion_r407297322
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/OMVolumeRequest.java
##########
@@ -106,6 +106,13 @@ protected UserVolumeInfo addVolumeToOwnerList(UserVolumeInfo volumeList,
objectID = volumeList.getObjectID();
}
+ // Sanity check, a user should not own same volume twice
+ // TODO: May want to remove this due to perf if user owns a lot of volumes.
+ if (prevVolList.contains(volume)) {
Review comment:
IMO the client shouldn't notice this. The warnings should only show up on the server (if any).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org