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 2021/11/08 12:45:50 UTC
[GitHub] [ozone] smengcl opened a new pull request #2814: HDDS-5891. OFS mkdir -p does not work as expected for bucket creation when volume exists due to volume create ACL check
smengcl opened a new pull request #2814:
URL: https://github.com/apache/ozone/pull/2814
https://issues.apache.org/jira/browse/HDDS-5891
1. `BucketManagerImpl#getBucketInfo` can now throw `VOLUME_NOT_FOUND` if the parent volume **doesn't exist**, and will only throw `BUCKET_NOT_FOUND` if the parent volume **exists** but the bucket doesn't. Before this change it throws `BUCKET_NOT_FOUND` regardless of whether parent volume exists. Related prior comment: https://github.com/apache/ozone/pull/2412#discussion_r672513719
2. With the patch, `BasicRootedOzoneClientAdapterImpl#getBucket` will no longer attempt to create the volume if the volume exists. Previously it wouldn't distinguish the different results. This fixes the issue mentioned in the jira title, where `getBucket` can fail when a non-privileged user tries to `createIfNotExist` but fail because he/she wouldn't pass the volume create permission check -- which doesn't make sense to be checked in the first place in this case.
## Testing
- Some integration/acceptance tests might be broken as a result of (1). Pending CI.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] smengcl commented on pull request #2814: HDDS-5891. OFS mkdir -p does not work as expected for bucket creation when volume exists due to volume create ACL check
Posted by GitBox <gi...@apache.org>.
smengcl commented on pull request #2814:
URL: https://github.com/apache/ozone/pull/2814#issuecomment-963509230
> Hi @smengcl, thank you for working on this forward.
>
> Please help me understand what is the aim here, as I am not sure why are we doing the getBucket call this way. In my head, the algorithm is simply this:
>
> * get the volume from the objectStore
> * if volume exists get the bucket from the volume
> * if volume does not exist, and if we should create it create it
> * if the bucket does not exist create it
>
> This is better reflected by #2815, the one @dombizita created, based on our earlier discussions with her.
>
> I would like to understand why we are using the ObjectStore's client proxy to directly do OzoneManager protocol calls, when we can get the volume, and from the volume we can get the bucket later on, is there a good reason to turn to the underlying API I don't see, or we can even get rid of using the OzoneManager protocol proxy directly?
>
> On the other hand, I pretty much agree that the BucketManager#getBucketInfo(volName, bucketName) method should throw a VOLUME_NOT_FOUND in case the volume does not exist, that is a much clearer cause in this case.
Thanks @fapifta for the comment. Thanks @dombizita for #2815 .
> This is better reflected by #2815, the one @dombizita created, based on our earlier discussions with her.
Yes #2815 is fine and it is better for compatibility (concern for compatibility is why I posted my PR #2814 in draft). #2814 and #2815 are just two different approaches to the same problem.
> I would like to understand why we are using the ObjectStore's client proxy to directly do OzoneManager protocol calls
The reason I used a single `proxy.getBucketDetails` call when writing OFS was to reduce unnecessary round trip between the client and OM (`getVolume` then `getBucket` adds an extra round trip). We had some discussion about the round trip concerns in the original OFS PR and offline.
I am totally fine with #2815 if we are okay on taking the potential performance impact hit for now (not sure if adding an extra round trip would actually add meaningful perf impact in the first place, happy to talk about it briefly), to block other work and improve on this 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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] fapifta commented on pull request #2814: HDDS-5891. OFS mkdir -p does not work as expected for bucket creation when volume exists due to volume create ACL check
Posted by GitBox <gi...@apache.org>.
fapifta commented on pull request #2814:
URL: https://github.com/apache/ozone/pull/2814#issuecomment-964667502
Hmm, looking into it further, that perf impact might be huge, as if we do a getVolumeInfo first, then a getBucketInfo based on the volume, and if we use this in almost all operations as we do now, then that is a burden we do not want to take on for sure.
So even though the other solution for me seems easier to understand, it hurts with the performance tradeoff...
If we are concerned about compatibility, we need to use getBucketDetails(vol, buck) in the initial try catch, as otherwise, we are causing performance degradation of a lot of unrelated calls, but if we do not want to change the behaviour of BucketManager, we need to change how we handle errors, and there get volume first, and create only if it does not exist I believe...
On the other hand, I would leave the change of BucketManager still on the table, as if volume does not exists, it would be the correct behaviour to throw a volume not found, instead of the bucket not found or any other resultcode in the OMException. If you check, createBucket already does it this way, and if getBucketInfo(vol, buck) switches to this behaviour, your solution is the one I would vote for...
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] smengcl commented on pull request #2814: HDDS-5891. OFS mkdir -p does not work as expected for bucket creation when volume exists due to volume create ACL check
Posted by GitBox <gi...@apache.org>.
smengcl commented on pull request #2814:
URL: https://github.com/apache/ozone/pull/2814#issuecomment-970071542
Thanks @fapifta for the +1.
Finally got a green CI run after 6+ retriggers. `master` branch tests are quite a bit unstable lately.
Merging in a minute.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] fapifta commented on pull request #2814: HDDS-5891. OFS mkdir -p does not work as expected for bucket creation when volume exists due to volume create ACL check
Posted by GitBox <gi...@apache.org>.
fapifta commented on pull request #2814:
URL: https://github.com/apache/ozone/pull/2814#issuecomment-963367938
Hi @smengcl,
thank you for working on this forward.
Please help me understand what is the aim here, as I am not sure why are we doing the getBucket call this way. In my head, the algorithm is simply this:
- get the volume from the objectStore
- if volume exists get the bucket from the volume
- if volume does not exist, and if we should create it create it
- if the bucket does not exist create it
This is better reflected by #2815, the one @dombizita created, based on our earlier discussions with her.
I would like to understand why we are using the ObjectStore's client proxy to directly do OzoneManager protocol calls, when we can get the volume, and from the volume we can get the bucket later on, is there a good reason to turn to the underlying API I don't see, or we can even get rid of using the OzoneManager protocol proxy directly?
On the other hand, I pretty much agree that the BucketManager#getBucketInfo(volName, bucketName) method should throw a VOLUME_NOT_FOUND in case the volume does not exist, that is a much clearer cause in this case.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] smengcl commented on pull request #2814: HDDS-5891. OFS mkdir -p does not work as expected for bucket creation when volume exists due to volume create ACL check
Posted by GitBox <gi...@apache.org>.
smengcl commented on pull request #2814:
URL: https://github.com/apache/ozone/pull/2814#issuecomment-968182925
Thanks @fapifta for filing HDDS-5986. I have already filed HDDS-5969 earlier that has a patch file attached to it to address the same issue so I have marked HDDS-5986 as duplicated.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] smengcl edited a comment on pull request #2814: HDDS-5891. OFS mkdir -p does not work as expected for bucket creation when volume exists due to volume create ACL check
Posted by GitBox <gi...@apache.org>.
smengcl edited a comment on pull request #2814:
URL: https://github.com/apache/ozone/pull/2814#issuecomment-963509230
> Hi @smengcl, thank you for working on this forward.
>
> Please help me understand what is the aim here, as I am not sure why are we doing the getBucket call this way. In my head, the algorithm is simply this:
>
> * get the volume from the objectStore
> * if volume exists get the bucket from the volume
> * if volume does not exist, and if we should create it create it
> * if the bucket does not exist create it
>
> This is better reflected by #2815, the one @dombizita created, based on our earlier discussions with her.
>
> I would like to understand why we are using the ObjectStore's client proxy to directly do OzoneManager protocol calls, when we can get the volume, and from the volume we can get the bucket later on, is there a good reason to turn to the underlying API I don't see, or we can even get rid of using the OzoneManager protocol proxy directly?
>
> On the other hand, I pretty much agree that the BucketManager#getBucketInfo(volName, bucketName) method should throw a VOLUME_NOT_FOUND in case the volume does not exist, that is a much clearer cause in this case.
Thanks @fapifta for the comment. Thanks @dombizita for #2815 .
> This is better reflected by #2815, the one @dombizita created, based on our earlier discussions with her.
Yes #2815 is fine and it is better for compatibility (concern for compatibility is why I posted my PR #2814 in draft). #2814 and #2815 can be seen as two different approaches to the same problem.
> I would like to understand why we are using the ObjectStore's client proxy to directly do OzoneManager protocol calls
The reason I used a single `proxy.getBucketDetails` call when writing OFS was to reduce unnecessary round trip between the client and OM (`getVolume` then `getBucket` adds an extra round trip). There were some discussion about the extra round trip offline and IIRC in the original OFS PR as well.
I am totally fine with #2815 if we are okay on taking the potential performance impact hit for now (not sure if adding an extra round trip would actually add meaningful perf impact in the first place, happy to talk about it briefly), to block other work and improve on this 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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] smengcl commented on pull request #2814: HDDS-5891. OFS mkdir -p does not work as expected for bucket creation when volume exists due to volume create ACL check
Posted by GitBox <gi...@apache.org>.
smengcl commented on pull request #2814:
URL: https://github.com/apache/ozone/pull/2814#issuecomment-967415350
Hi @fapifta , thanks for the comment. I have updated the PR. It is ready for review. Would you like to take a look?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] fapifta commented on a change in pull request #2814: HDDS-5891. OFS mkdir -p does not work as expected for bucket creation when volume exists due to volume create ACL check
Posted by GitBox <gi...@apache.org>.
fapifta commented on a change in pull request #2814:
URL: https://github.com/apache/ozone/pull/2814#discussion_r748781771
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestRootedOzoneFileSystem.java
##########
@@ -1513,4 +1529,65 @@ public void testRenameDestinationParentDoesntExist() throws Exception {
}
}
+ @Test
+ public void testNonPrivilegedUserMkdirCreateBucket() throws IOException {
+ // This test is only meaningful when ACL is enabled
+ if (!enableAcl) {
Review comment:
I think we should use org.junit.Assume.assumeTrue(enableAcl) here to avoid the test run in cases where acl-s are disabled. I think it is a more standard solution to avoid test runs in case of a specific parameter value.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] smengcl edited a comment on pull request #2814: HDDS-5891. OFS mkdir -p does not work as expected for bucket creation when volume exists due to volume create ACL check
Posted by GitBox <gi...@apache.org>.
smengcl edited a comment on pull request #2814:
URL: https://github.com/apache/ozone/pull/2814#issuecomment-963509230
> Hi @smengcl, thank you for working on this forward.
>
> Please help me understand what is the aim here, as I am not sure why are we doing the getBucket call this way. In my head, the algorithm is simply this:
>
> * get the volume from the objectStore
> * if volume exists get the bucket from the volume
> * if volume does not exist, and if we should create it create it
> * if the bucket does not exist create it
>
> This is better reflected by #2815, the one @dombizita created, based on our earlier discussions with her.
>
> I would like to understand why we are using the ObjectStore's client proxy to directly do OzoneManager protocol calls, when we can get the volume, and from the volume we can get the bucket later on, is there a good reason to turn to the underlying API I don't see, or we can even get rid of using the OzoneManager protocol proxy directly?
>
> On the other hand, I pretty much agree that the BucketManager#getBucketInfo(volName, bucketName) method should throw a VOLUME_NOT_FOUND in case the volume does not exist, that is a much clearer cause in this case.
Thanks @fapifta for the comment. Thanks @dombizita for #2815 .
> This is better reflected by #2815, the one @dombizita created, based on our earlier discussions with her.
Yes #2815 is fine and it is better for compatibility (concern for compatibility is why I posted my PR #2814 in draft). #2814 and #2815 can be seen as two different approaches to the same problem.
> I would like to understand why we are using the ObjectStore's client proxy to directly do OzoneManager protocol calls
The reason I used a single `proxy.getBucketDetails` call when writing OFS was to reduce unnecessary round trip between the client and OM (`getVolume` then `getBucket` adds an extra round trip). There were some discussion about the extra round trip offline and IIRC in the original OFS PR as well.
I am totally fine with #2815 in order to unblock other work, and improve on this later. Not sure if adding an extra round trip would really add meaningful perf impact in the first place, happy to discuss about that.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] smengcl commented on a change in pull request #2814: HDDS-5891. OFS mkdir -p does not work as expected for bucket creation when volume exists due to volume create ACL check
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #2814:
URL: https://github.com/apache/ozone/pull/2814#discussion_r748783682
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestRootedOzoneFileSystem.java
##########
@@ -1513,4 +1529,65 @@ public void testRenameDestinationParentDoesntExist() throws Exception {
}
}
+ @Test
+ public void testNonPrivilegedUserMkdirCreateBucket() throws IOException {
+ // This test is only meaningful when ACL is enabled
+ if (!enableAcl) {
Review comment:
Done
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] smengcl merged pull request #2814: HDDS-5891. OFS mkdir -p does not work as expected for bucket creation when volume exists due to volume create ACL check
Posted by GitBox <gi...@apache.org>.
smengcl merged pull request #2814:
URL: https://github.com/apache/ozone/pull/2814
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org