Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,7 @@ private OzoneConsts() {
public static final String SRC_KEY = "srcKey";
public static final String DST_KEY = "dstKey";
public static final String USED_BYTES = "usedBytes";
public static final String USED_NAMESPACE = "usedNamespace";
public static final String QUOTA_IN_BYTES = "quotaInBytes";
public static final String QUOTA_IN_COUNTS = "quotaInCounts";
Copy link
Contributor

@linyiqun linyiqun Dec 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I see we defined usedBytes corresponding to quotaInBytes, so quotaInCounts expected to be quotaInNamespace. This is just my first feeling for this variable name. We could revisit these later in a separate JIRA to make this more understandable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. quotaInCounts seems not very readable to me as well :) I didn't change it in this PR as it is not relevant.

Created https://issues.apache.org/jira/browse/HDDS-4582 to revisit it later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I started to use quotaInNamespace in my new code. HDDS-4582 will be replacing existing quotaInCounts to quotaInNamesapce

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me.

public static final String OBJECT_ID = "objectID";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,10 @@ public class OzoneVolume extends WithMetadata {
* Quota of bucket count allocated for the Volume.
*/
private long quotaInCounts;
/**
* Bucket namespace quota usage.
*/
private long usedNamespace;
/**
* Creation time of the volume.
*/
Expand Down Expand Up @@ -126,11 +130,13 @@ public OzoneVolume(ConfigurationSource conf, ClientProtocol proxy,
@SuppressWarnings("parameternumber")
public OzoneVolume(ConfigurationSource conf, ClientProtocol proxy,
String name, String admin, String owner, long quotaInBytes,
long quotaInCounts, long creationTime, long modificationTime,
List<OzoneAcl> acls, Map<String, String> metadata) {
long quotaInCounts, long usedNamespace, long creationTime,
long modificationTime, List<OzoneAcl> acls,
Map<String, String> metadata) {
this(conf, proxy, name, admin, owner, quotaInBytes, quotaInCounts,
creationTime, acls, metadata);
this.modificationTime = Instant.ofEpochMilli(modificationTime);
this.usedNamespace = usedNamespace;
}

@SuppressWarnings("parameternumber")
Expand All @@ -149,11 +155,12 @@ public OzoneVolume(ConfigurationSource conf, ClientProtocol proxy,
@SuppressWarnings("parameternumber")
public OzoneVolume(ConfigurationSource conf, ClientProtocol proxy,
String name, String admin, String owner, long quotaInBytes,
long quotaInCounts, long creationTime, long modificationTime,
List<OzoneAcl> acls) {
long quotaInCounts, long usedNamespace, long creationTime,
long modificationTime, List<OzoneAcl> acls) {
this(conf, proxy, name, admin, owner, quotaInBytes, quotaInCounts,
creationTime, acls);
this.modificationTime = Instant.ofEpochMilli(modificationTime);
this.usedNamespace = usedNamespace;
}

@VisibleForTesting
Expand Down Expand Up @@ -256,6 +263,14 @@ public List<OzoneAcl> getAcls() {
return acls;
}

/**
* Returns used bucket namespace.
* @return usedNamespace
*/
public long getUsedNamespace() {
return usedNamespace;
}

/**
* Sets/Changes the owner of this Volume.
* @param userName new owner
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,7 @@ public void createVolume(String volumeName, VolumeArgs volArgs)
builder.setOwnerName(owner);
builder.setQuotaInBytes(quotaInBytes);
builder.setQuotaInCounts(quotaInCounts);
builder.setUsedNamespace(0L);
builder.addAllMetadata(volArgs.getMetadata());

//Remove duplicates and add ACLs
Expand Down Expand Up @@ -308,6 +309,7 @@ public OzoneVolume getVolumeDetails(String volumeName)
volume.getOwnerName(),
volume.getQuotaInBytes(),
volume.getQuotaInCounts(),
volume.getUsedNamespace(),
volume.getCreationTime(),
volume.getModificationTime(),
volume.getAclMap().ozoneAclGetProtobuf().stream().
Expand Down Expand Up @@ -342,6 +344,7 @@ public List<OzoneVolume> listVolumes(String volumePrefix, String prevVolume,
volume.getOwnerName(),
volume.getQuotaInBytes(),
volume.getQuotaInCounts(),
volume.getUsedNamespace(),
volume.getCreationTime(),
volume.getModificationTime(),
volume.getAclMap().ozoneAclGetProtobuf().stream().
Expand All @@ -364,6 +367,7 @@ public List<OzoneVolume> listVolumes(String user, String volumePrefix,
volume.getOwnerName(),
volume.getQuotaInBytes(),
volume.getQuotaInCounts(),
volume.getUsedNamespace(),
volume.getCreationTime(),
volume.getModificationTime(),
volume.getAclMap().ozoneAclGetProtobuf().stream().
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ public final class OmVolumeArgs extends WithObjectID implements Auditable {
private long modificationTime;
private long quotaInBytes;
private long quotaInCounts;
private long usedNamespace;
private final OmOzoneAclMap aclMap;

/**
Expand All @@ -54,6 +55,7 @@ public final class OmVolumeArgs extends WithObjectID implements Auditable {
* @param volume - volume name
* @param quotaInBytes - Volume Quota in bytes.
* @param quotaInCounts - Volume Quota in counts.
* @param usedNamespace - Volume Namespace Quota Usage in counts.
* @param metadata - metadata map for custom key/value data.
* @param aclMap - User to access rights map.
* @param creationTime - Volume creation time.
Expand All @@ -64,14 +66,15 @@ public final class OmVolumeArgs extends WithObjectID implements Auditable {
@SuppressWarnings({"checkstyle:ParameterNumber", "This is invoked from a " +
"builder."})
private OmVolumeArgs(String adminName, String ownerName, String volume,
long quotaInBytes, long quotaInCounts, Map<String, String> metadata,
OmOzoneAclMap aclMap, long creationTime, long modificationTime,
long objectID, long updateID) {
long quotaInBytes, long quotaInCounts, long usedNamespace,
Map<String, String> metadata, OmOzoneAclMap aclMap, long creationTime,
long modificationTime, long objectID, long updateID) {
this.adminName = adminName;
this.ownerName = ownerName;
this.volume = volume;
this.quotaInBytes = quotaInBytes;
this.quotaInCounts = quotaInCounts;
this.usedNamespace = usedNamespace;
this.metadata = metadata;
this.aclMap = aclMap;
this.creationTime = creationTime;
Expand Down Expand Up @@ -173,6 +176,21 @@ public OmOzoneAclMap getAclMap() {
return aclMap;
}

/**
* increase used bucket namespace by n.
*/
public void incrUsedNamespace(long n) {
usedNamespace += n;
}

/**
* Returns used bucket namespace.
* @return usedNamespace
*/
public long getUsedNamespace() {
return usedNamespace;
}

/**
* Returns new builder class that builds a OmVolumeArgs.
*
Expand All @@ -194,6 +212,8 @@ public Map<String, String> toAuditMap() {
auditMap.put(OzoneConsts.QUOTA_IN_BYTES, String.valueOf(this.quotaInBytes));
auditMap.put(OzoneConsts.QUOTA_IN_COUNTS,
String.valueOf(this.quotaInCounts));
auditMap.put(OzoneConsts.USED_NAMESPACE,
String.valueOf(this.usedNamespace));
auditMap.put(OzoneConsts.OBJECT_ID, String.valueOf(this.getObjectID()));
auditMap.put(OzoneConsts.UPDATE_ID, String.valueOf(this.getUpdateID()));
return auditMap;
Expand Down Expand Up @@ -227,6 +247,7 @@ public static class Builder {
private long modificationTime;
private long quotaInBytes;
private long quotaInCounts;
private long usedNamespace;
private Map<String, String> metadata;
private OmOzoneAclMap aclMap;
private long objectID;
Expand Down Expand Up @@ -259,6 +280,8 @@ public Builder setUpdateID(long id) {
public Builder() {
metadata = new HashMap<>();
aclMap = new OmOzoneAclMap();
quotaInBytes = OzoneConsts.QUOTA_RESET;
quotaInCounts = OzoneConsts.QUOTA_RESET;
}

public Builder setAdminName(String admin) {
Expand Down Expand Up @@ -296,6 +319,11 @@ public Builder setQuotaInCounts(long quotaCounts) {
return this;
}

public Builder setUsedNamespace(long namespaceUsage) {
this.usedNamespace = namespaceUsage;
return this;
}

public Builder addMetadata(String key, String value) {
metadata.put(key, value); // overwrite if present.
return this;
Expand All @@ -322,8 +350,8 @@ public OmVolumeArgs build() {
Preconditions.checkNotNull(ownerName);
Preconditions.checkNotNull(volume);
return new OmVolumeArgs(adminName, ownerName, volume, quotaInBytes,
quotaInCounts, metadata, aclMap, creationTime, modificationTime,
objectID, updateID);
quotaInCounts, usedNamespace, metadata, aclMap, creationTime,
modificationTime, objectID, updateID);
}

}
Expand All @@ -336,6 +364,7 @@ public VolumeInfo getProtobuf() {
.setVolume(volume)
.setQuotaInBytes(quotaInBytes)
.setQuotaInCounts(quotaInCounts)
.setUsedNamespace(usedNamespace)
.addAllMetadata(KeyValueUtil.toProtobuf(metadata))
.addAllVolumeAcls(aclList)
.setCreationTime(
Expand All @@ -356,6 +385,7 @@ public static OmVolumeArgs getFromProtobuf(VolumeInfo volInfo)
volInfo.getVolume(),
volInfo.getQuotaInBytes(),
volInfo.getQuotaInCounts(),
volInfo.getUsedNamespace(),
KeyValueUtil.getFromProtobuf(volInfo.getMetadataList()),
aclMap,
volInfo.getCreationTime(),
Expand All @@ -372,6 +402,7 @@ public String getObjectInfo() {
", owner='" + ownerName + '\'' +
", creationTime='" + creationTime + '\'' +
", quotaInBytes='" + quotaInBytes + '\'' +
", usedNamespace='" + usedNamespace + '\'' +
'}';
}

Expand All @@ -387,7 +418,7 @@ public OmVolumeArgs copyObject() {
OmOzoneAclMap cloneAclMap = aclMap.copyObject();

return new OmVolumeArgs(adminName, ownerName, volume, quotaInBytes,
quotaInCounts, cloneMetadata, cloneAclMap, creationTime,
modificationTime, objectID, updateID);
quotaInCounts, usedNamespace, cloneMetadata, cloneAclMap,
creationTime, modificationTime, objectID, updateID);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -885,6 +885,51 @@ public void testCheckUsedBytesQuota() throws IOException {
Assert.assertEquals(3, countException);
}

@Test
public void testVolumeUsedNamespace() throws IOException {
String volumeName = UUID.randomUUID().toString();
String bucketName = UUID.randomUUID().toString();
String bucketName2 = UUID.randomUUID().toString();
OzoneVolume volume = null;

// set Volume namespace quota as 1
store.createVolume(volumeName,
VolumeArgs.newBuilder().setQuotaInCounts(1L).build());
volume = store.getVolume(volumeName);
// The initial value should be 0
Assert.assertEquals(0L, volume.getUsedNamespace());
volume.createBucket(bucketName);
// Used namespace should be 1
volume = store.getVolume(volumeName);
Assert.assertEquals(1L, volume.getUsedNamespace());

try {
volume.createBucket(bucketName2);
} catch (IOException ex) {
GenericTestUtils.assertExceptionContains("QUOTA_EXCEEDED", ex);
}

// test linked bucket
String targetVolName = UUID.randomUUID().toString();
store.createVolume(targetVolName);
OzoneVolume volumeWithLinkedBucket = store.getVolume(targetVolName);
String targetBucketName = UUID.randomUUID().toString();
BucketArgs.Builder argsBuilder = new BucketArgs.Builder()
.setStorageType(StorageType.DEFAULT)
.setVersioning(false)
.setSourceVolume(volumeName)
.setSourceBucket(bucketName);
volumeWithLinkedBucket.createBucket(targetBucketName, argsBuilder.build());
// Used namespace should be 0 because linked bucket does not consume
// namespace quota
Assert.assertEquals(0L, volumeWithLinkedBucket.getUsedNamespace());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not fully get this point, why this bucket create way will not increased the namespace quota? Just curious for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amaliujia , could you add a new UT for bucket link case? Linked bucket should not be counted in the namespace quota.

This comes from one of the comment above. I consider this is more like a decision that linked bucket should not consume quota. At least linked bucket should not consume space quota. I think to make consistent, do not apply namespace quota also makes sense.


volume.deleteBucket(bucketName);
// Used namespace should be 0
volume = store.getVolume(volumeName);
Assert.assertEquals(0L, volume.getUsedNamespace());
}

private void writeKey(OzoneBucket bucket, String keyName,
ReplicationFactor replication, String value, int valueLength)
throws IOException{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -364,7 +364,7 @@ message VolumeInfo {
optional uint64 updateID = 9;
optional uint64 modificationTime = 10;
optional uint64 quotaInCounts = 11;

optional uint64 usedNamespace = 12;
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -205,14 +205,22 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
// Add default acls from volume.
addDefaultAcls(omBucketInfo, omVolumeArgs);

// check namespace quota
checkQuotaInNamespace(omVolumeArgs, 1L);

// update used namespace for volume
omVolumeArgs.incrUsedNamespace(1L);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before this update, we need to do the namespace quota check in checkQuotaBytesValid.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also updated the test to test quota exceed case.

// Update table cache.
metadataManager.getVolumeTable().addCacheEntry(new CacheKey<>(volumeKey),
new CacheValue<>(Optional.of(omVolumeArgs), transactionLogIndex));
metadataManager.getBucketTable().addCacheEntry(new CacheKey<>(bucketKey),
new CacheValue<>(Optional.of(omBucketInfo), transactionLogIndex));

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also update the volume table cache like above, as omVolumeArgs be updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

O I see. it make senses.

omResponse.setCreateBucketResponse(
CreateBucketResponse.newBuilder().build());
omClientResponse = new OMBucketCreateResponse(omResponse.build(),
omBucketInfo);
omBucketInfo, omVolumeArgs.copyObject());
} catch (IOException ex) {
exception = ex;
omClientResponse = new OMBucketCreateResponse(
Expand Down Expand Up @@ -301,6 +309,25 @@ private BucketEncryptionInfoProto getBeinfo(
return bekb.build();
}

/**
* Check namespace quota.
*/
private void checkQuotaInNamespace(OmVolumeArgs omVolumeArgs,
long allocatedNamespace) throws IOException {
if (omVolumeArgs.getQuotaInCounts() != OzoneConsts.QUOTA_RESET) {
long usedNamespace = omVolumeArgs.getUsedNamespace();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use omVolumeArgs.getQuotaInCounts() != OzoneConsts.QUOTA_RESET to replace above condition check? That will be the better check in case maybe OzoneConsts.QUOTA_RESET value can be changed to the positive number.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make senses! Done!

long quotaInNamespace = omVolumeArgs.getQuotaInCounts();
long toUseNamespaceInTotal = usedNamespace + allocatedNamespace;
if (quotaInNamespace < toUseNamespaceInTotal) {
throw new OMException("The namespace quota of Volume:"
+ omVolumeArgs.getVolume() + " exceeded: quotaInNamespace: "
+ quotaInNamespace + " but namespace consumed: "
+ toUseNamespaceInTotal + ".",
OMException.ResultCodes.QUOTA_EXCEEDED);
}
}
}

public boolean checkQuotaBytesValid(OMMetadataManager metadataManager,
OmVolumeArgs omVolumeArgs, OmBucketInfo omBucketInfo, String volumeKey)
throws IOException {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import java.util.Map;

import com.google.common.base.Optional;
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.slf4j.Logger;
Expand Down Expand Up @@ -134,9 +135,23 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
omResponse.setDeleteBucketResponse(
DeleteBucketResponse.newBuilder().build());

// update used namespace for volume
String volumeKey = omMetadataManager.getVolumeKey(volumeName);
OmVolumeArgs omVolumeArgs =
omMetadataManager.getVolumeTable().getReadCopy(volumeKey);
if (omVolumeArgs == null) {
throw new OMException("Volume " + volumeName + " is not found",
OMException.ResultCodes.VOLUME_NOT_FOUND);
}
omVolumeArgs.incrUsedNamespace(-1L);
// Update table cache.
omMetadataManager.getVolumeTable().addCacheEntry(
new CacheKey<>(volumeKey),
new CacheValue<>(Optional.of(omVolumeArgs), transactionLogIndex));

Copy link
Contributor

@linyiqun linyiqun Dec 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Volume table cache also need to be updated here as we update table cache in OMBucketCreateRequest.java

// Add to double buffer.
omClientResponse = new OMBucketDeleteResponse(omResponse.build(),
volumeName, bucketName);
volumeName, bucketName, omVolumeArgs.copyObject());
} catch (IOException ex) {
success = false;
exception = ex;
Expand Down
Loading