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 2022/02/01 23:55:27 UTC
[GitHub] [ozone] errose28 commented on a change in pull request #3018: HDDS-6084. [Multi-Tenant] Handle upgrades to version supporting S3 multi-tenancy
errose28 commented on a change in pull request #3018:
URL: https://github.com/apache/ozone/pull/3018#discussion_r797106028
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/upgrade/OMLayoutFeatureAspect.java
##########
@@ -111,4 +117,8 @@ public void beforeRequestApplyTxn(final JoinPoint joinPoint)
om.getVersionManager(), lf.name());
}
+ public static OMLayoutFeatureAspect aspectOf() {
Review comment:
Is this method called anywhere?
##########
File path: hadoop-ozone/ozone-manager/src/main/resources/META-INF/aop.xml
##########
@@ -0,0 +1,23 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ Licensed 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
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ 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. See accompanying LICENSE file.
+-->
+<aspectj>
+ <aspects>
+ <aspect name="org.apache.hadoop.ozone.om.upgrade.OMLayoutFeatureAspect"/>
+ <weaver options="-verbose -showWeaveInfo">
+ <include within="org.apache.hadoop.ozone.protocolPB.OzoneManagerRequestHandler"/>
+ <include within="org.apache.hadoop.ozone.protocolPB.OzoneManagerProtocolServerSideTranslatorPB"/>
Review comment:
Why is this class needed here?
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/multitenant/TestMultiTenantVolume.java
##########
@@ -77,14 +82,52 @@ public void testDefaultS3Volume() throws Exception {
assertS3BucketNotFound(store, bucketName);
}
+ private void expectFailurePreFinalization(VoidCallable eval)
+ throws Exception {
+ LambdaTestUtils.intercept(OMException.class,
+ "cannot be invoked before finalization", eval);
+ }
+
@Test
public void testS3TenantVolume() throws Exception {
+
final String tenant = "tenant";
final String principal = "username";
final String bucketName = "bucket";
final String accessID = UUID.randomUUID().toString();
ObjectStore store = getStoreForAccessID(accessID);
+
+ // None of the tenant APIs is usable before the upgrade finalization step
Review comment:
nit. Can we move the block from here to finalization to a helper method?
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/upgrade/OMLayoutFeature.java
##########
@@ -29,8 +29,9 @@
*/
public enum OMLayoutFeature implements LayoutFeature {
////////////////////////////// //////////////////////////////
- INITIAL_VERSION(0, "Initial Layout Version");
-
+ INITIAL_VERSION(0, "Initial Layout Version"),
+ // TODO: Have a better name / description? Create table action on upgrade?
+ MULTITENANCY_SCHEMA(1, "Multi-tenancy Schema");
Review comment:
I don't think we need an upgrade action, since the s3 secrets are staying in the same table. I think we can just call this feature `MULTITENANCY`.
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/upgrade/OMLayoutFeatureAspect.java
##########
@@ -54,11 +54,17 @@ public void checkLayoutFeature(JoinPoint joinPoint) throws IOException {
String featureName = ((MethodSignature) joinPoint.getSignature())
.getMethod().getAnnotation(DisallowedUntilLayoutVersion.class)
.value().name();
- LayoutVersionManager lvm = null;
+ LayoutVersionManager lvm;
+ final Object[] args = joinPoint.getArgs();
if (joinPoint.getTarget() instanceof OzoneManagerRequestHandler) {
OzoneManager ozoneManager = ((OzoneManagerRequestHandler)
joinPoint.getTarget()).getOzoneManager();
lvm = ozoneManager.getVersionManager();
+ } else if (args != null && args.length > 0 &&
+ args[0] instanceof OzoneManager) {
+ // Get OzoneManager instance from preExecute first arg
Review comment:
Maybe check `joinPoint.getTarget() instanceof OMClientRequest && joinPoint.toShortString().equals(preExecute)`, either instead of or in addition to the argument checks? I think that is the only other place we would expect to put this annotation besides the request handler so that might help enforce and document it better.
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/multitenant/TestMultiTenantVolume.java
##########
@@ -77,14 +82,52 @@ public void testDefaultS3Volume() throws Exception {
assertS3BucketNotFound(store, bucketName);
}
+ private void expectFailurePreFinalization(VoidCallable eval)
+ throws Exception {
+ LambdaTestUtils.intercept(OMException.class,
+ "cannot be invoked before finalization", eval);
+ }
+
@Test
public void testS3TenantVolume() throws Exception {
+
final String tenant = "tenant";
final String principal = "username";
final String bucketName = "bucket";
final String accessID = UUID.randomUUID().toString();
ObjectStore store = getStoreForAccessID(accessID);
+
+ // None of the tenant APIs is usable before the upgrade finalization step
+ expectFailurePreFinalization(store::listTenant);
+ expectFailurePreFinalization(() -> store.listUsersInTenant(tenant, ""));
+ expectFailurePreFinalization(() -> store.tenantGetUserInfo(principal));
+ expectFailurePreFinalization(() -> store.createTenant(tenant));
+ expectFailurePreFinalization(() ->
+ store.tenantAssignUserAccessId(principal, tenant, accessID));
+ expectFailurePreFinalization(() ->
+ store.tenantAssignAdmin(principal, tenant, true));
+ expectFailurePreFinalization(() ->
+ store.tenantRevokeAdmin(accessID, tenant));
+ expectFailurePreFinalization(() ->
+ store.tenantRevokeUserAccessId(accessID));
+ expectFailurePreFinalization(() -> store.deleteTenant(tenant));
+
+ // S3 get/set/revoke secret APIs still work before finalization
+ final String accessId = "testUser1accessId1";
+ S3SecretValue s3SecretValue = store.getS3Secret(accessId);
+ Assert.assertEquals(accessId, s3SecretValue.getAwsAccessKey());
+ final String setSecret = "testsecret";
+ s3SecretValue = store.setS3Secret(accessId, setSecret);
+ Assert.assertEquals(accessId, s3SecretValue.getAwsAccessKey());
+ Assert.assertEquals(setSecret, s3SecretValue.getAwsSecret());
+ store.revokeS3Secret(accessId);
+
+
+ // Trigger OM finalization
+ cluster.getOzoneManager().finalizeUpgrade("clientId1");
Review comment:
This should be submitted through the client so it goes through Ratis and updates the on disk state as well. Finalization is async so the test should wait on queryUpgradeFinalizationProgress to return `Status#ALREADY_FINALIZED`.
--
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