HDDS-6063. [Multi-Tenant] Use VOLUME_LOCK in read and write requests, and some minor refactoring #3051

smengcl · 2022-02-07T21:00:56Z

What changes were proposed in this pull request?

We are already using VOLUME_LOCK for all write requests. It has been done in a previous jira. Just need to acquire the read lock in the appropriate read tenant db table requests. Basically just look for all access to these tables:

  // OMMetadataManager
  Table<String, OmDBAccessIdInfo> getTenantAccessIdTable();
  Table<String, OmDBKerberosPrincipalInfo> getPrincipalToAccessIdsTable();
  Table<String, OmDBTenantInfo> getTenantStateTable();
  Table<String, String> getTenantGroupTable();
  Table<String, String> getTenantRoleTable();
  Table<String, String> getTenantPolicyTable();

Rename all occurrences of:

tenantName -> tenantId
tenantUsername -> userPrincipal

for consistency.

Rename OMAssignUserToTenantRequest -> OMTenantAssignUserAccessIdRequest for consistency
Remove success field in tenant requests since they are redundant (the parent OMResponse already has a success flag built-in, no need to have another success flag in the response).

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-6063

How was this patch tested?

All existing tests should pass (after HDDS-6239).

Change-Id: I358503694ac0356c70a42155dd659ae454237a46

Change-Id: Icf8c71e1b6e3279a1e02f03160808af84221a5e4

Change-Id: I6c6feee24ccedfe9b80a360c97a4bf3884e678a9

Change-Id: If8c70a243b13d7da616197aa11759f6ac1ab7138

Change-Id: I3ca7c10ec2b5c0fe261f98beaa1bdb2b8f855ac7

Change-Id: Idcb99f885f28e50fdc012f52397703a9ecced6b4

Change-Id: I6584f6a8be06778299da9a7e55d2a1a21bf955ed

Change-Id: I16c0f29c614d585e5aba6adf31a13cced3582032

smengcl · 2022-02-13T05:41:49Z

CI: https://github.com/smengcl/hadoop-ozone/commits/HDDS-6063-CI

Conflicts: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java Change-Id: I4793869d2e6ea9a05e216eb52c7173c46bc65416

Change-Id: I6e77a25a4bccfe6878e438600a18420c98846aa7

smengcl · 2022-02-16T23:23:21Z

I have resolved conflicts by merging current feature branch into this PR.
This PR is ready for review. Pls take a look.
@avijayanhwx
@prashantpogde
@hanishakoneru

Change-Id: Ic6041395d5d109d2fd3309fe8670b9648b35ade0

Change-Id: I32567c795f9da57b28a3301f9c91994410d09abc

…d -- exception would have been thrown to the client anyway; (2) Rename all occurrences of: tenantName -> tenantId tenantUsername -> userPrincipal principal -> userPrincipal for consistency. (3) Rename `OMAssignUserToTenantRequest` -> `OMTenantAssignUserAccessIdRequest` for consistency. Change-Id: I47ceebef8352cd90d27fe2b48a876b69cae086fe

hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmDBTenantInfo.java

hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/multitenant/OzoneTenant.java

Change-Id: I564df483c89d8cb9b41939f914c19be3297ee810

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java

Change-Id: I02942c3fdba121f88c63b7f18371a76292380ef4

prashantpogde

The rest Look good to me.

Change-Id: Iafa9dea8713311962be923fefa5970c899ce8368

Change-Id: I26c8f00a732d806ea416c063c159cf49b0109945

errose28

Thanks for the fixes @smengcl. I think we also need to tweak OzoneManager#getS3VolumeContext after our decision to only use volume locks.
- We can't actually get a volume lock here since we don't know the volume until the end of the operations.
- We can remove the TODO and fix the code so that tenantInfo is used to get the s3 volume, instead of a redundant query to the DB.
- If tenantInfo is null, we should throw the OMException like it is, but not log an error since this could happen in a legitimate case.

errose28 · 2022-02-23T00:00:40Z

...p-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMMultiTenantManagerImpl.java

@@ -374,10 +374,15 @@ public void deactivateUser(String accessID)
  }

  @Override
-  public boolean isTenantAdmin(String user, String tenantName) {
+  public boolean isTenantAdmin(String user, String tenantId) {


There's a lot of problematic method stubs and unused methods in this class, and this is probably one of the worst examples. We must be sure to purge anything like this stub before the merge.

For admin checks specifically, it's a bit of a mess. At the top level, we have this method in OMMultiTenantManager, and OMTenantRequestHelper#checkTenantAdmin. There's one in the OzoneManager as well that OMTenantRequestHelper#checkTenantAdmin delegates to, but I don't think that belongs there because it does not use any OM instance fields and is only called from outside the OM.

As it is I'm not sure the difference between OMMultiTenantManager and OMTenantRequestHelper. OMTenantRequestHelper is not useful as a static utility class because it is called from requests which can already access OMMultiTenantManager through their OM instance. I think the two should be consolidated to reduce code duplication, bugs, and confusion. With these admin checks, for example, we have all three of those.

I think a follow up Jira is necessary to consolidate these two classes and clean them out so they only contain methods that are both needed and correctly implemented.

Yes. Ideally OMMultiTenantManager and OMTenantRequestHelper should be merged. While writing the Request code earlier it was easier to extract and add my own helper methods rather than calling OMMultiTenantManager methods. It is due for a refactoring. @prashantpogde and I can probably work together on this.

I filed HDDS-6387 for this. In the mean time, can we fix this particular case now since we don't want it to get lost in the merge? Probably easiest to have the one caller use the properly implemented version in OMTenantRequestHelper and make this stub return false for now to be safe.

(being) fixed in #3177

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java

...ain/java/org/apache/hadoop/ozone/om/request/s3/tenant/OMTenantAssignUserAccessIdRequest.java

Change-Id: I61fb43308482dcc7bbf7800a642d9ee83228b955

Change-Id: I23cd751800b3346b7bcf45440ac33e396ce48f06

errose28

Thanks for the updates @smengcl. Not sure if you saw my comment here but we should fix/update OzoneManager#getS3VolumeContext in this PR as well.

...ain/java/org/apache/hadoop/ozone/om/request/s3/tenant/OMTenantAssignUserAccessIdRequest.java

Change-Id: Idba3a5c45db129684d594e5908eb5427eddbe6f8

smengcl · 2022-03-02T16:23:38Z

Thanks for the updates @smengcl. Not sure if you saw my comment here but we should fix/update OzoneManager#getS3VolumeContext in this PR as well.

Yup. Sorry I missed that earlier somehow.

I have pushed a commit to address this.

It seems we can only protect OzoneAclUtils.accessIdToUserPrincipal with the volume lock, as volume name is acquired really late in getS3VolumeContext.

Change-Id: I36f5035f4478c8086bf071e4a4a307e997ceefb2

errose28

Just a few more comments on the new getS3VolumeContext changes, the rest looks good thanks @smengcl.

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java

Change-Id: I36e98a56301957c820843be614d1d32ac488ef41

errose28

Thanks for continuously updating this PR @smengcl. Current CI failure looks unrelated and the changes LGTM.

smengcl · 2022-03-07T19:15:37Z

Thanks a lot for the review! @errose28 @prashantpogde

CI passed on my fork's branch https://github.com/smengcl/hadoop-ozone/commits/HDDS-6063

Will merge shortly.

* HDDS-4944: (268 commits) HDDS-6366. [Multi-Tenant] Disallow specifying custom accessId in OzoneManager (apache#3166) HDDS-6275. [Multi-Tenant] Add feature documentation and CLI quick start guide (apache#3095) HDDS-6063. [Multi-Tenant] Use VOLUME_LOCK in read and write requests, and some minor refactoring (apache#3051) HDDS-6214. [Multi-Tenant] Fix KMS Encryption/Decryption (apache#3010) HDDS-6322. Fix Recon getting inccorrect sequenceNumber from OM (apache#3090) HDDS-5913. Avoid integer overflow when setting dfs.container.ratis.lo… (apache#2785) HDDS-6313. Remove replicas in ContainerStateMap when a container is deleted (apache#3086) HDDS-6186. Selective checks: skip integration check for unit test changes (apache#3061) HDDS-6310. Update json-smart to 2.4.7. (apache#3080) HDDS-6190. Cleanup unnecessary datanode id path checks. (apache#2993) HDDS-6304. Add enforcer to make sure ozone.version equals project.version (apache#3075) HDDS-6309. Update ozone-runner version to 20220212-1 (apache#3079) HDDS-6293. Allow using custom ozone-runner image (apache#3072) HDDS-4126. Freon key generator should support >2GB files. (apache#3054) HDDS-6088. Implement O3FS/OFS getFileChecksum() using file checksum helpers - addendum: fix checkstyle HDDS-6088. Implement O3FS/OFS getFileChecksum() using file checksum helpers. (apache#2935) HDDS-6084. [Multi-Tenant] Handle upgrades to version supporting S3 multi-tenancy (apache#3018) HDDS-6257. Wrong stack trace for S3 errors (apache#3066) HDDS-6278 Improve memory profile for listStatus API call. (apache#3053) HDDS-6285. ozonesecure-mr intermittently failing with timeout downloading packages (apache#3057) ...

HDDS-6063. [Multi-Tenant] Use VOLUME_LOCK in read and write requests

7eb07c6

smengcl requested review from prashantpogde and errose28 February 7, 2022 21:00

smengcl added 8 commits February 9, 2022 15:36

Add comment

e8358bb

Change-Id: I358503694ac0356c70a42155dd659ae454237a46

Add cache iterator; fix test

dbb2a05

Change-Id: Icf8c71e1b6e3279a1e02f03160808af84221a5e4

Add comment

e5afc0b

Change-Id: I6c6feee24ccedfe9b80a360c97a4bf3884e678a9

findbugs: compareTo

1c55613

Change-Id: If8c70a243b13d7da616197aa11759f6ac1ab7138

findbugs: compareTo 2nd try

d3c3dc2

Change-Id: I3ca7c10ec2b5c0fe261f98beaa1bdb2b8f855ac7

checkstyle

46100e7

Change-Id: Idcb99f885f28e50fdc012f52397703a9ecced6b4

findbugs: compareTo 3rd try

97358cb

Change-Id: I6584f6a8be06778299da9a7e55d2a1a21bf955ed

findbugs: override equals

e82c47c

Change-Id: I16c0f29c614d585e5aba6adf31a13cced3582032

swagle requested review from avijayanhwx and hanishakoneru February 15, 2022 19:25

smengcl added 2 commits February 16, 2022 15:20

Merge remote-tracking branch 'asf/HDDS-4944' into HDDS-6063

f68b139

Conflicts: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java Change-Id: I4793869d2e6ea9a05e216eb52c7173c46bc65416

checkstyle.

e407be2

Change-Id: I6e77a25a4bccfe6878e438600a18420c98846aa7

smengcl added 3 commits February 16, 2022 19:03

Retrigger CI. testKeyAcl passed locally.

ee273ec

Change-Id: Ic6041395d5d109d2fd3309fe8670b9648b35ade0

File new jiras for the remaining TODOs and insert the TODOs in place

dd7d731

Change-Id: I32567c795f9da57b28a3301f9c91994410d09abc

smengcl changed the title ~~HDDS-6063. [Multi-Tenant] Use VOLUME_LOCK in read and write requests~~ HDDS-6063. [Multi-Tenant] Use VOLUME_LOCK in read and write requests, and some minor refactoring Feb 17, 2022

prashantpogde reviewed Feb 17, 2022

View reviewed changes

hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmDBTenantInfo.java Show resolved Hide resolved

prashantpogde reviewed Feb 17, 2022

View reviewed changes

hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/multitenant/OzoneTenant.java Outdated Show resolved Hide resolved

More renaming; Remove cache iteration logic

710c6f0

Change-Id: I564df483c89d8cb9b41939f914c19be3297ee810

prashantpogde reviewed Feb 18, 2022

View reviewed changes

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java Show resolved Hide resolved

TestResultCodes

1df2678

Change-Id: I02942c3fdba121f88c63b7f18371a76292380ef4

prashantpogde reviewed Feb 18, 2022

View reviewed changes

prashantpogde approved these changes Feb 18, 2022

View reviewed changes

smengcl added 2 commits February 22, 2022 11:20

Retrigger

6e1418e

Change-Id: Iafa9dea8713311962be923fefa5970c899ce8368

Retrigger. Flakiness in filesystem-hdds integration

4e2f106

Change-Id: I26c8f00a732d806ea416c063c159cf49b0109945

errose28 reviewed Feb 23, 2022

View reviewed changes

smengcl added 2 commits February 24, 2022 10:36

Address comments

265e3d9

Change-Id: I61fb43308482dcc7bbf7800a642d9ee83228b955

Retrigger

f5ddac4

Change-Id: I23cd751800b3346b7bcf45440ac33e396ce48f06

errose28 reviewed Mar 1, 2022

View reviewed changes

...ain/java/org/apache/hadoop/ozone/om/request/s3/tenant/OMTenantAssignUserAccessIdRequest.java Show resolved Hide resolved

...ain/java/org/apache/hadoop/ozone/om/request/s3/tenant/OMTenantAssignUserAccessIdRequest.java Show resolved Hide resolved

Add volume lock in getS3VolumeContext

0d4c48a

Change-Id: Idba3a5c45db129684d594e5908eb5427eddbe6f8

Fix integration test

194231f

Change-Id: I36f5035f4478c8086bf071e4a4a307e997ceefb2

errose28 reviewed Mar 5, 2022

View reviewed changes

Address comments

22e4a97

Change-Id: I36e98a56301957c820843be614d1d32ac488ef41

errose28 approved these changes Mar 7, 2022

View reviewed changes

smengcl merged commit 5231830 into apache:HDDS-4944 Mar 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HDDS-6063. [Multi-Tenant] Use VOLUME_LOCK in read and write requests, and some minor refactoring #3051

HDDS-6063. [Multi-Tenant] Use VOLUME_LOCK in read and write requests, and some minor refactoring #3051

smengcl commented Feb 7, 2022 •

edited

smengcl commented Feb 13, 2022

smengcl commented Feb 16, 2022

prashantpogde left a comment

errose28 left a comment

errose28 Feb 23, 2022

smengcl Feb 24, 2022

errose28 Feb 28, 2022

smengcl Mar 8, 2022

errose28 left a comment

smengcl commented Mar 2, 2022

errose28 left a comment

errose28 left a comment

smengcl commented Mar 7, 2022

HDDS-6063. [Multi-Tenant] Use VOLUME_LOCK in read and write requests, and some minor refactoring #3051

HDDS-6063. [Multi-Tenant] Use VOLUME_LOCK in read and write requests, and some minor refactoring #3051

Conversation

smengcl commented Feb 7, 2022 • edited

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

smengcl commented Feb 13, 2022

smengcl commented Feb 16, 2022

prashantpogde left a comment

Choose a reason for hiding this comment

errose28 left a comment

Choose a reason for hiding this comment

errose28 Feb 23, 2022

Choose a reason for hiding this comment

smengcl Feb 24, 2022

Choose a reason for hiding this comment

errose28 Feb 28, 2022

Choose a reason for hiding this comment

smengcl Mar 8, 2022

Choose a reason for hiding this comment

errose28 left a comment

Choose a reason for hiding this comment

smengcl commented Mar 2, 2022

errose28 left a comment

Choose a reason for hiding this comment

errose28 left a comment

Choose a reason for hiding this comment

smengcl commented Mar 7, 2022

smengcl commented Feb 7, 2022 •

edited