errose28 commented on a change in pull request #2857: URL: https://github.com/apache/ozone/pull/2857#discussion_r764454638
########## File path: hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/tenant/TenantDeleteHandler.java ########## @@ -18,18 +18,45 @@ package org.apache.hadoop.ozone.shell.tenant; import org.apache.hadoop.ozone.client.OzoneClient; +import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.DeleteTenantResponse; import org.apache.hadoop.ozone.shell.OzoneAddress; import picocli.CommandLine; +import java.io.IOException; + /** * ozone tenant delete. */ [email protected](name = "delete", - description = "Delete a tenant") [email protected](name = "delete", aliases = "remove", + description = "Delete an empty tenant. " + + "Will not remove the associated volume.") public class TenantDeleteHandler extends TenantHandler { + @CommandLine.Parameters(description = "Tenant name", arity = "1..1") + private String tenantId; + @Override protected void execute(OzoneClient client, OzoneAddress address) { - err().println("Not Implemented."); + try { + final DeleteTenantResponse resp = + client.getObjectStore().deleteTenant(tenantId); + out().println("Deleted tenant '" + tenantId + "'."); + long volumeRefCount = resp.getVolRefCount(); + assert(volumeRefCount >= 0L); + final String volumeName = resp.getVolumeName(); + final String extraPrompt = + "But the associated volume '" + volumeName + "' is not removed. "; + if (volumeRefCount == 0L) { + out().println(extraPrompt + "To delete it, run" + + "\n ozone sh volume delete " + volumeName + "\n"); + } else { + out().println(extraPrompt + "And it is still referenced by some other " + + "Ozone features (refCount is " + volumeRefCount + ")."); + } + } catch (IOException e) { + // Throw exception to make client exit code non-zero + throw new RuntimeException("Failed to delete tenant '" + tenantId + "': " Review comment: Same as above, I think the standard is to propagate the IOException here. ########## File path: hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/tenant/TenantSetSecretHandler.java ########## @@ -68,7 +68,7 @@ protected void execute(OzoneClient client, OzoneAddress address) if (omEx.getResult().equals(ACCESSID_NOT_FOUND)) { // Print to stderr here in order not to contaminate stdout just in // case -e is specified. - err().println("AccessId '" + accessId + "' doesn't exist"); + throw new RuntimeException("AccessId '" + accessId + "' doesn't exist"); Review comment: Same as above. Propagate the exception. ########## File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/tenant/OMTenantRequestHelper.java ########## @@ -94,26 +99,26 @@ static void checkTenantExistence(OMMetadataManager omMetadataManager, * Retrieve volume name of the tenant. */ static String getTenantVolumeName(OMMetadataManager omMetadataManager, - String tenantName) { + String tenantId) { final OmDBTenantInfo tenantInfo; try { - tenantInfo = omMetadataManager.getTenantStateTable().get(tenantName); + tenantInfo = omMetadataManager.getTenantStateTable().get(tenantId); } catch (IOException e) { throw new RuntimeException("Potential DB error. Unable to retrieve " - + "OmDBTenantInfo entry for tenant '" + tenantName + "'."); + + "OmDBTenantInfo entry for tenant '" + tenantId + "'."); Review comment: This IOException should be propagated to the caller (validateAndUpdateCache) since it indicates a problem reading from the DB. ########## File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/tenant/OMTenantDeleteRequest.java ########## @@ -18,33 +18,206 @@ */ package org.apache.hadoop.ozone.om.request.s3.tenant; +import com.google.common.base.Optional; +import com.google.common.base.Preconditions; +import org.apache.hadoop.hdds.utils.db.cache.CacheKey; +import org.apache.hadoop.hdds.utils.db.cache.CacheValue; +import org.apache.hadoop.ozone.OzoneConsts; +import org.apache.hadoop.ozone.audit.OMAction; +import org.apache.hadoop.ozone.om.OMMetadataManager; import org.apache.hadoop.ozone.om.OzoneManager; +import org.apache.hadoop.ozone.om.exceptions.OMException; +import org.apache.hadoop.ozone.om.helpers.OmDBTenantInfo; +import org.apache.hadoop.ozone.om.helpers.OmVolumeArgs; import org.apache.hadoop.ozone.om.ratis.utils.OzoneManagerDoubleBufferHelper; +import org.apache.hadoop.ozone.om.request.util.OmResponseUtil; import org.apache.hadoop.ozone.om.request.volume.OMVolumeRequest; import org.apache.hadoop.ozone.om.response.OMClientResponse; +import org.apache.hadoop.ozone.om.response.s3.tenant.OMTenantDeleteResponse; +import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.DeleteTenantRequest; +import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.DeleteTenantResponse; import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.OMRequest; +import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.OMResponse; +import org.apache.hadoop.ozone.security.acl.IAccessAuthorizer; +import org.apache.hadoop.ozone.security.acl.OzoneObj; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import java.io.IOException; +import java.util.HashMap; +import java.util.Map; + +import static org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes.TENANT_NOT_EMPTY; +import static org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes.TENANT_NOT_FOUND; +import static org.apache.hadoop.ozone.om.lock.OzoneManagerLock.Resource.VOLUME_LOCK; /** * Handles OMTenantDelete request. */ public class OMTenantDeleteRequest extends OMVolumeRequest { + public static final Logger LOG = + LoggerFactory.getLogger(OMTenantDeleteRequest.class); public OMTenantDeleteRequest(OMRequest omRequest) { super(omRequest); } @Override public OMRequest preExecute(OzoneManager ozoneManager) throws IOException { - return getOmRequest(); + + // Check Ozone cluster admin privilege + OMTenantRequestHelper.checkAdmin(ozoneManager); + + // TODO: TBD: Call ozoneManager.getMultiTenantManager().deleteTenant() ? + + return getOmRequest().toBuilder().setUserInfo(getUserInfo()).build(); } @Override + @SuppressWarnings("methodlength") public OMClientResponse validateAndUpdateCache( OzoneManager ozoneManager, long transactionLogIndex, OzoneManagerDoubleBufferHelper ozoneManagerDoubleBufferHelper) { - return null; + OMClientResponse omClientResponse = null; + final OMResponse.Builder omResponse = + OmResponseUtil.getOMResponseBuilder(getOmRequest()); + boolean acquiredVolumeLock = false; + final Map<String, String> auditMap = new HashMap<>(); + OMMetadataManager omMetadataManager = ozoneManager.getMetadataManager(); + final DeleteTenantRequest request = getOmRequest().getDeleteTenantRequest(); + final String tenantId = request.getTenantId(); + String volumeName = null; + boolean decVolumeRefCount = true; + + IOException exception = null; + OmVolumeArgs omVolumeArgs = null; + + try { + // Check tenant existence in tenantStateTable + if (!omMetadataManager.getTenantStateTable().isExist(tenantId)) { + LOG.debug("tenant: {} does not exist", tenantId); + throw new OMException("Tenant '" + tenantId + "' does not exist", + TENANT_NOT_FOUND); + } + + // Reading the TenantStateTable without lock as we don't have or need + // a TENANT_LOCK. The assumption is that OmDBTenantInfo is read-only + // once it is set during tenant creation. + final OmDBTenantInfo dbTenantInfo = + omMetadataManager.getTenantStateTable().get(tenantId); + volumeName = dbTenantInfo.getBucketNamespaceName(); + assert(volumeName != null); + + LOG.debug("Tenant '{}' has volume '{}'", tenantId, volumeName); + // decVolumeRefCount is true if volumeName is not empty string + decVolumeRefCount = volumeName.length() > 0; + + // Acquire the volume lock + Preconditions.checkNotNull(volumeName, Review comment: I don't understand this. We are reading from the DB here, and we have full control over what we write to the DB when creating the tenant. So why not just have create tenant always set the volumeName and remove all the extra checks here? ########## File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/s3/tenant/OMTenantDeleteResponse.java ########## @@ -0,0 +1,97 @@ +/** + * 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.response.s3.tenant; + +import com.google.common.base.Preconditions; +import org.apache.hadoop.hdds.utils.db.BatchOperation; +import org.apache.hadoop.ozone.om.OMMetadataManager; +import org.apache.hadoop.ozone.om.helpers.OmVolumeArgs; +import org.apache.hadoop.ozone.om.response.CleanupTableInfo; +import org.apache.hadoop.ozone.om.response.OMClientResponse; +import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.OMResponse; + +import javax.annotation.Nonnull; +import javax.annotation.Nullable; +import java.io.IOException; + +import static org.apache.hadoop.ozone.om.OmMetadataManagerImpl.TENANT_POLICY_TABLE; +import static org.apache.hadoop.ozone.om.OmMetadataManagerImpl.TENANT_STATE_TABLE; +import static org.apache.hadoop.ozone.om.OmMetadataManagerImpl.VOLUME_TABLE; + +/** + * Response for DeleteTenant request. + */ +@CleanupTableInfo(cleanupTables = { + TENANT_STATE_TABLE, + TENANT_POLICY_TABLE, + VOLUME_TABLE +}) +public class OMTenantDeleteResponse extends OMClientResponse { + + private String volumeName; + private OmVolumeArgs omVolumeArgs; + private String tenantId; + private String userPolicyGroupName; + private String bucketPolicyGroupName; + + public OMTenantDeleteResponse(@Nonnull OMResponse omResponse, + @Nonnull String volumeName, + @Nullable OmVolumeArgs omVolumeArgs, + @Nonnull String tenantId, + @Nonnull String userPolicyGroupName, + @Nonnull String bucketPolicyGroupName) { + super(omResponse); + this.volumeName = volumeName; + this.omVolumeArgs = omVolumeArgs; + this.tenantId = tenantId; + this.userPolicyGroupName = userPolicyGroupName; + this.bucketPolicyGroupName = bucketPolicyGroupName; + } + + /** + * For when the request is not successful. + * For a successful request, the other constructor should be used. + */ + public OMTenantDeleteResponse(@Nonnull OMResponse omResponse) { + super(omResponse); + checkStatusNotOK(); + } + + @Override + protected void addToDBBatch(OMMetadataManager omMetadataManager, + BatchOperation batchOperation) throws IOException { + + omMetadataManager.getTenantStateTable().deleteWithBatch( + batchOperation, tenantId); + + omMetadataManager.getTenantPolicyTable().deleteWithBatch( + batchOperation, userPolicyGroupName); + + omMetadataManager.getTenantPolicyTable().deleteWithBatch( + batchOperation, bucketPolicyGroupName); + + // The rest are the same as OMVolumeDeleteResponse Review comment: Old comment. Might want to change it to clarify that we are updating the volume ref count here. ########## File path: hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/tenant/TenantCreateHandler.java ########## @@ -17,42 +17,33 @@ */ package org.apache.hadoop.ozone.shell.tenant; -import org.apache.hadoop.hdds.cli.GenericCli; import org.apache.hadoop.ozone.client.OzoneClient; import org.apache.hadoop.ozone.shell.OzoneAddress; import picocli.CommandLine; import java.io.IOException; -import java.util.ArrayList; -import java.util.List; /** * ozone tenant create. */ @CommandLine.Command(name = "create", - description = "Create one or more tenants") + description = "Create a tenant." + + " This will also create a new Ozone volume for the tenant.") public class TenantCreateHandler extends TenantHandler { - @CommandLine.Spec - private CommandLine.Model.CommandSpec spec; - - @CommandLine.Parameters(description = "List of tenant names") - private List<String> tenants = new ArrayList<>(); + @CommandLine.Parameters(description = "Tenant name", arity = "1..1") + private String tenantId; @Override protected void execute(OzoneClient client, OzoneAddress address) { - if (tenants.size() > 0) { - for (String tenantName : tenants) { - try { - client.getObjectStore().createTenant(tenantName); - out().println("Created tenant '" + tenantName + "'."); - } catch (IOException e) { - err().println("Failed to create tenant '" + tenantName + "': " + - e.getMessage()); - } - } - } else { - GenericCli.missingSubcommand(spec); + try { + client.getObjectStore().createTenant(tenantId); + // TODO: Add return value and print volume name? + out().println("Created tenant '" + tenantId + "'."); + } catch (IOException e) { Review comment: Other OM CLIs propagate the IOException here, since the parent's execute definition throws it. This gives a full stack trace which is more useful for diagnosing problems. ########## File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/tenant/OMTenantRequestHelper.java ########## @@ -94,26 +99,26 @@ static void checkTenantExistence(OMMetadataManager omMetadataManager, * Retrieve volume name of the tenant. */ static String getTenantVolumeName(OMMetadataManager omMetadataManager, - String tenantName) { + String tenantId) { final OmDBTenantInfo tenantInfo; try { - tenantInfo = omMetadataManager.getTenantStateTable().get(tenantName); + tenantInfo = omMetadataManager.getTenantStateTable().get(tenantId); } catch (IOException e) { throw new RuntimeException("Potential DB error. Unable to retrieve " - + "OmDBTenantInfo entry for tenant '" + tenantName + "'."); + + "OmDBTenantInfo entry for tenant '" + tenantId + "'."); } if (tenantInfo == null) { throw new RuntimeException("Potential DB error or race condition. " - + "OmDBTenantInfo entry is missing for tenant '" + tenantName + "'."); + + "OmDBTenantInfo entry is missing for tenant '" + tenantId + "'."); Review comment: This should be thrown as an OMException with result code `TENANT_NOT_FOUND`. ########## File path: hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLock.java ########## @@ -59,6 +59,9 @@ * <tr> * <td> 5 </td> <td> Prefix Lock </td> * </tr> + * <tr> + * <td> 6 </td> <td> Tenant Lock </td> + * </tr> Review comment: Minor: update the doc here after removing the tenant lock. ########## File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/tenant/OMTenantRequestHelper.java ########## @@ -94,26 +99,26 @@ static void checkTenantExistence(OMMetadataManager omMetadataManager, * Retrieve volume name of the tenant. */ static String getTenantVolumeName(OMMetadataManager omMetadataManager, - String tenantName) { + String tenantId) { final OmDBTenantInfo tenantInfo; try { - tenantInfo = omMetadataManager.getTenantStateTable().get(tenantName); + tenantInfo = omMetadataManager.getTenantStateTable().get(tenantId); } catch (IOException e) { throw new RuntimeException("Potential DB error. Unable to retrieve " - + "OmDBTenantInfo entry for tenant '" + tenantName + "'."); + + "OmDBTenantInfo entry for tenant '" + tenantId + "'."); } if (tenantInfo == null) { throw new RuntimeException("Potential DB error or race condition. " - + "OmDBTenantInfo entry is missing for tenant '" + tenantName + "'."); + + "OmDBTenantInfo entry is missing for tenant '" + tenantId + "'."); } - final String volumeName = tenantInfo.getAccountNamespaceName(); + final String volumeName = tenantInfo.getBucketNamespaceName(); - if (StringUtils.isEmpty(tenantName)) { + if (StringUtils.isEmpty(tenantId)) { Review comment: This should check `volumeName`, not `tenantId`, and should throw an OMException with result code `VOLUME_NOT_FOUND`. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
