Skip to content
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
5fe2ff7
Add existing update / object IDs to OmKeyArgs and OmKeyInfo
Mar 12, 2024
aa3bd32
modified createKeyRequest and commitKeyRequest with tests
Mar 13, 2024
9799ba5
Push objectid / updateid through various classes
Mar 14, 2024
5040d2b
Adapt client to be able to overwrite a key
Mar 15, 2024
6f45e67
Remove debugging log messages
Mar 15, 2024
0635134
Fix find bugs
Mar 15, 2024
c151122
Ensure test only runs for non FSO for now
Mar 15, 2024
908f9cd
Fix similar commit test on FSO
Mar 15, 2024
3c9c0e2
Rename overWrite to overwrite
Mar 20, 2024
17b5ff1
Overload constructors to avoid passing nulls for the new variables
Mar 20, 2024
0fe12c5
Avoid setting acl in overwrite request. Enhance test to ensure object…
Mar 20, 2024
ae4044c
Add overwrite object / update ID to the audit if they are present
Mar 21, 2024
7bb59e9
Minor review comments
Mar 22, 2024
8890ed8
Remove overwriteObjectID and base all logic on updateID only
Mar 22, 2024
a360d45
Block non ObjectStore buckets from using optimistic overwrite for now
Mar 22, 2024
3f3a49d
Validate existing ACLs are copied over from existing to overwrite key
Mar 22, 2024
736740a
Merge branch 'master' into HDDS-10527
Apr 2, 2024
525685c
Trigger rebuild
Apr 2, 2024
112e2e1
Rename overwriteUpdateID to overwriteGeneration
Apr 30, 2024
bcd1a28
Remove updateID references from client and replace with generation
Apr 30, 2024
4e6a97c
Change client API to rewriteKey
Apr 30, 2024
247620a
Fixed logic to not copy meta data from existing key, but take it from…
Apr 30, 2024
c6bc7e7
Merge branch 'master' into HDDS-10527
Apr 30, 2024
b91a051
Review comments
May 7, 2024
9dc6d31
Change BucketLayout.DEFAULT to getBucketLayout()
May 9, 2024
80e1802
Move getGeneration to OMKeyInfo
May 9, 2024
bb7b74f
Rename any instances of overwrite to rewrite
May 9, 2024
a87e092
Remove generation from OzoneKey
May 9, 2024
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 @@ -343,6 +343,7 @@ private OzoneConsts() {
public static final String BUCKET_LAYOUT = "bucketLayout";
public static final String TENANT = "tenant";
public static final String USER_PREFIX = "userPrefix";
public static final String OVERWRITE_UPDATE_ID = "overwriteUpdateID";

// For multi-tenancy
public static final String TENANT_ID_USERNAME_DELIMITER = "$";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -469,6 +469,30 @@ public OzoneOutputStream createKey(String key, long size,
.createKey(volumeName, name, key, size, replicationConfig, keyMetadata);
}

/**
* Overwrite an existing key using optimistic locking. The existingKey must exist in Ozone to allow
* the new key to be created with the same name. Additionally, the existing Key must not have been
* modified since the time it's details were read. This is controlled by the updateID
* field in the existing Key. If the key is replaced or updated the updateID will change. If the
* updateID has changed since the existing Key was read, either the initial key create will fail,
* or the key will fail to commit after the data has been written as the checks are carried out
* both a key open and commit time.
*
* For now this feature only works on Object Store Buckets. FSO support will be added a later.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Overwrite an existing key using optimistic locking. The existingKey must exist in Ozone to allow
* the new key to be created with the same name. Additionally, the existing Key must not have been
* modified since the time it's details were read. This is controlled by the updateID
* field in the existing Key. If the key is replaced or updated the updateID will change. If the
* updateID has changed since the existing Key was read, either the initial key create will fail,
* or the key will fail to commit after the data has been written as the checks are carried out
* both a key open and commit time.
*
* For now this feature only works on Object Store Buckets. FSO support will be added a later.
* This API allows to atomically update an existing key. The key read before invoking this API
* should remain unchanged for this key to be written. This is controlled by the updateID
* field in the existing Key param. If the key is replaced or updated the updateID will change. If the
* updateID has changed since the existing Key was read, either the initial key create will fail,
* or the key will fail to commit after the data has been written as the checks are carried out
* both a key open and commit time.
*
* For now this feature only works on Object Store Buckets. FSO support will be added a later.

*
* @param existingKey Name of the key to be created.
* @param replicationConfig Replication configuration.
* @return OzoneOutputStream to which the data has to be written.
* @throws IOException
*/
public OzoneOutputStream overwriteKey(OzoneKeyDetails existingKey, ReplicationConfig replicationConfig)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
public OzoneOutputStream overwriteKey(OzoneKeyDetails existingKey, ReplicationConfig replicationConfig)
public OzoneOutputStream atomicUpdateKey(OzoneKeyDetails existingKey, ReplicationConfig replicationConfig)

throws IOException {
if (this.bucketLayout != BucketLayout.OBJECT_STORE) {
throw new IllegalArgumentException("Optimistic locking is only supported on Object Store Buckets");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This validation should move into OM to allow older clients to be able to update keys when OM supports it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We are going to do this on a branch now and plan to immediately work on the FSO implementation so I will remove this check entirely.

}
return proxy.overwriteKey(existingKey, replicationConfig);
}

/**
* Creates a new key in the bucket, with default replication type RATIS and
* with replication factor THREE.
Expand Down Expand Up @@ -1779,7 +1803,7 @@ private void addKeyPrefixInfoToResultList(String keyPrefix,
keyInfo.getDataSize(), keyInfo.getCreationTime(),
keyInfo.getModificationTime(),
keyInfo.getReplicationConfig(),
keyInfo.isFile());
keyInfo.isFile(), keyInfo.getUpdateID());
keysResultList.add(ozoneKey);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,11 +70,19 @@ public class OzoneKey {
* Constructs OzoneKey from OmKeyInfo.
*
*/
Comment thread
adoroszlai marked this conversation as resolved.
Outdated

/**
* The update ID of an existing key. This will be null if this OzoneKey
* object has not been created from an existing key read from OM, as OM allocates
* the update ID on commit of the key.
*/
private final Long updateID;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We are leaking internal names into public APIs. For public facing API, I would model it how google APIs have documented theirs : https://cloud.google.com/storage/docs/metadata#generation-number
(We do not have the split between metadata only updates vs. whole object.)

For now, I would change this to generation and document the guarantees that an application has around the use of generation. This should apply to the entire PR, where we use updateID which is a more nebulous name.


@SuppressWarnings("parameternumber")
public OzoneKey(String volumeName, String bucketName,
String keyName, long size, long creationTime,
long modificationTime, ReplicationConfig replicationConfig,
boolean isFile) {
boolean isFile, Long updateID) {
this.volumeName = volumeName;
this.bucketName = bucketName;
this.name = keyName;
Expand All @@ -83,6 +91,16 @@ public OzoneKey(String volumeName, String bucketName,
this.modificationTime = Instant.ofEpochMilli(modificationTime);
this.replicationConfig = replicationConfig;
this.isFile = isFile;
this.updateID = updateID;
}

@SuppressWarnings("parameternumber")
public OzoneKey(String volumeName, String bucketName,
String keyName, long size, long creationTime,
long modificationTime, ReplicationConfig replicationConfig,
boolean isFile) {
this(volumeName, bucketName, keyName, size, creationTime, modificationTime, replicationConfig,
isFile, null);
}

@SuppressWarnings("parameternumber")
Expand All @@ -91,10 +109,19 @@ public OzoneKey(String volumeName, String bucketName,
long modificationTime, ReplicationConfig replicationConfig,
Map<String, String> metadata, boolean isFile) {
this(volumeName, bucketName, keyName, size, creationTime,
modificationTime, replicationConfig, isFile);
modificationTime, replicationConfig, isFile, null);
this.metadata.putAll(metadata);
}

@SuppressWarnings("parameternumber")
public OzoneKey(String volumeName, String bucketName,
String keyName, long size, long creationTime,
long modificationTime, ReplicationConfig replicationConfig,
Map<String, String> metadata, boolean isFile, Long updateID) {
this(volumeName, bucketName, keyName, size, creationTime,
modificationTime, replicationConfig, isFile, updateID);
this.metadata.putAll(metadata);
}
/**
* Returns Volume Name associated with the Key.
*
Expand Down Expand Up @@ -179,6 +206,11 @@ public ReplicationConfig getReplicationConfig() {
return replicationConfig;
}

@JsonIgnore
public Long getUpdateID() {
return updateID;
}

/**
* Returns indicator if key is a file.
* @return file
Expand All @@ -191,7 +223,7 @@ public static OzoneKey fromKeyInfo(OmKeyInfo keyInfo) {
return new OzoneKey(keyInfo.getVolumeName(), keyInfo.getBucketName(),
keyInfo.getKeyName(), keyInfo.getDataSize(), keyInfo.getCreationTime(),
keyInfo.getModificationTime(), keyInfo.getReplicationConfig(),
keyInfo.getMetadata(), keyInfo.isFile());
keyInfo.getMetadata(), keyInfo.isFile(), keyInfo.getUpdateID());
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -53,14 +53,31 @@ public OzoneKeyDetails(String volumeName, String bucketName, String keyName,
Map<String, String> metadata,
FileEncryptionInfo feInfo,
CheckedSupplier<OzoneInputStream, IOException> contentSupplier,
boolean isFile) {
boolean isFile, Long updateID) {
super(volumeName, bucketName, keyName, size, creationTime,
modificationTime, replicationConfig, metadata, isFile);
modificationTime, replicationConfig, metadata, isFile, updateID);
this.ozoneKeyLocations = ozoneKeyLocations;
this.feInfo = feInfo;
this.contentSupplier = contentSupplier;
}

/**
* Constructs OzoneKeyDetails from OmKeyInfo.
*/
@SuppressWarnings("parameternumber")
public OzoneKeyDetails(String volumeName, String bucketName, String keyName,
long size, long creationTime, long modificationTime,
List<OzoneKeyLocation> ozoneKeyLocations,
ReplicationConfig replicationConfig,
Map<String, String> metadata,
FileEncryptionInfo feInfo,
CheckedSupplier<OzoneInputStream, IOException> contentSupplier,
boolean isFile) {
this(volumeName, bucketName, keyName, size, creationTime,
modificationTime, ozoneKeyLocations, replicationConfig, metadata, feInfo, contentSupplier,
isFile, null);
}

/**
* Returns the location detail information of the specific Key.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -353,6 +353,21 @@ OzoneOutputStream createKey(String volumeName, String bucketName,
Map<String, String> metadata)
throws IOException;

/**
* Overwrite an existing key using optimistic locking. The OzoneKeyDetails passed must contain
* the updateID of the key to be overwritten. The existing key must also exist on
* OM and have the same UpdateID as the one passed in or the key create / commit will fail.
*
* Currently does not work for FSO buckets. Support will be added later.
*
* @param keyToOverwrite Existing key to overwrite
* @param replicationConfig The replication configuration for the new key
* @return {@link OzoneOutputStream}
* @throws IOException
*/
OzoneOutputStream overwriteKey(OzoneKeyDetails keyToOverwrite, ReplicationConfig replicationConfig)
throws IOException;

/**
* Writes a key in an existing bucket.
* @param volumeName Name of the Volume
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1374,26 +1374,7 @@ public OzoneOutputStream createKey(
ReplicationConfig replicationConfig,
Map<String, String> metadata)
throws IOException {
verifyVolumeName(volumeName);
verifyBucketName(bucketName);
if (checkKeyNameEnabled) {
HddsClientUtils.verifyKeyName(keyName);
}
HddsClientUtils.checkNotNull(keyName);
if (omVersion
.compareTo(OzoneManagerVersion.ERASURE_CODED_STORAGE_SUPPORT) < 0) {
if (replicationConfig != null &&
replicationConfig.getReplicationType()
== HddsProtos.ReplicationType.EC) {
throw new IOException("Can not set the replication of the key to"
+ " Erasure Coded replication, as OzoneManager does not support"
+ " Erasure Coded replication.");
}
}

if (replicationConfig != null) {
replicationConfigValidator.validate(replicationConfig);
}
createKeyPreChecks(volumeName, bucketName, keyName, replicationConfig);

OmKeyArgs.Builder builder = new OmKeyArgs.Builder()
.setVolumeName(volumeName)
Expand All @@ -1416,6 +1397,63 @@ public OzoneOutputStream createKey(
return createOutputStream(openKey);
}

@Override
public OzoneOutputStream overwriteKey(OzoneKeyDetails keyToOverwrite, ReplicationConfig replicationConfig)
throws IOException {
if (keyToOverwrite == null) {
throw new IllegalArgumentException("KeyToOverwrite cannot be null");
}
if (keyToOverwrite.getUpdateID() == null) {
throw new IllegalArgumentException("KeyToOverwrite UpdateID cannot be null");
}
createKeyPreChecks(keyToOverwrite.getVolumeName(), keyToOverwrite.getBucketName(),
keyToOverwrite.getName(), replicationConfig);

OmKeyArgs.Builder builder = new OmKeyArgs.Builder()
.setVolumeName(keyToOverwrite.getVolumeName())
.setBucketName(keyToOverwrite.getBucketName())
.setKeyName(keyToOverwrite.getName())
.setDataSize(keyToOverwrite.getDataSize())
.setReplicationConfig(replicationConfig)
.addAllMetadataGdpr(keyToOverwrite.getMetadata())
.setLatestVersionLocation(getLatestVersionLocation)
.setOverwriteUpdateID(keyToOverwrite.getUpdateID());

OpenKeySession openKey = ozoneManagerClient.openKey(builder.build());
// For bucket with layout OBJECT_STORE, when create an empty file (size=0),
// OM will set DataSize to OzoneConfigKeys#OZONE_SCM_BLOCK_SIZE,
// which will cause S3G's atomic write length check to fail,
// so reset size to 0 here.
if (isS3GRequest.get() && keyToOverwrite.getDataSize() == 0) {
openKey.getKeyInfo().setDataSize(0);
}
return createOutputStream(openKey);
}

private void createKeyPreChecks(String volumeName, String bucketName, String keyName,
ReplicationConfig replicationConfig) throws IOException {
verifyVolumeName(volumeName);
verifyBucketName(bucketName);
if (checkKeyNameEnabled) {
HddsClientUtils.verifyKeyName(keyName);
}
HddsClientUtils.checkNotNull(keyName);
if (omVersion
.compareTo(OzoneManagerVersion.ERASURE_CODED_STORAGE_SUPPORT) < 0) {
if (replicationConfig != null &&
replicationConfig.getReplicationType()
== HddsProtos.ReplicationType.EC) {
throw new IOException("Can not set the replication of the key to"
+ " Erasure Coded replication, as OzoneManager does not support"
+ " Erasure Coded replication.");
}
}

if (replicationConfig != null) {
replicationConfigValidator.validate(replicationConfig);
}
}

@Override
public OzoneDataStreamOutput createStreamKey(
String volumeName, String bucketName, String keyName, long size,
Expand Down Expand Up @@ -1652,7 +1690,8 @@ public List<OzoneKey> listKeys(String volumeName, String bucketName,
key.getCreationTime(),
key.getModificationTime(),
key.getReplicationConfig(),
key.isFile()))
key.isFile(),
key.getUpdateID()))
.collect(Collectors.toList());
}
}
Expand Down Expand Up @@ -1703,7 +1742,7 @@ private OzoneKeyDetails getOzoneKeyDetails(OmKeyInfo keyInfo) {
keyInfo.getModificationTime(), ozoneKeyLocations,
keyInfo.getReplicationConfig(), keyInfo.getMetadata(),
keyInfo.getFileEncryptionInfo(),
() -> getInputStreamWithRetryFunction(keyInfo), keyInfo.isFile());
() -> getInputStreamWithRetryFunction(keyInfo), keyInfo.isFile(), keyInfo.getUpdateID());
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,12 @@ public final class OmKeyArgs implements Auditable {
private final boolean recursive;
private final boolean headOp;
private final boolean forceUpdateContainerCacheFromSCM;
// Overwrite updateID when used in key creation indicates that a
// key with the same keyName should exist with the given update ID.
// Upon commit, the updateID of that existing key should be unchanged.
// This is a form of optimistic locking, to ensure the key is not modified between
// the time the key is read and the time the key is written.
private Long overwriteUpdateID = null;

private OmKeyArgs(Builder b) {
this.volumeName = b.volumeName;
Expand All @@ -70,6 +76,7 @@ private OmKeyArgs(Builder b) {
this.recursive = b.recursive;
this.headOp = b.headOp;
this.forceUpdateContainerCacheFromSCM = b.forceUpdateContainerCacheFromSCM;
this.overwriteUpdateID = b.overwriteUpdateID;
}

public boolean getIsMultipartKey() {
Expand Down Expand Up @@ -144,6 +151,10 @@ public boolean isForceUpdateContainerCacheFromSCM() {
return forceUpdateContainerCacheFromSCM;
}

public Long getOverwriteUpdateID() {
return overwriteUpdateID;
}

@Override
public Map<String, String> toAuditMap() {
Map<String, String> auditMap = new LinkedHashMap<>();
Expand All @@ -166,7 +177,7 @@ public void addLocationInfo(OmKeyLocationInfo locationInfo) {
}

public OmKeyArgs.Builder toBuilder() {
return new OmKeyArgs.Builder()
OmKeyArgs.Builder builder = new OmKeyArgs.Builder()
.setVolumeName(volumeName)
.setBucketName(bucketName)
.setKeyName(keyName)
Expand All @@ -182,11 +193,16 @@ public OmKeyArgs.Builder toBuilder() {
.setLatestVersionLocation(latestVersionLocation)
.setAcls(acls)
.setForceUpdateContainerCacheFromSCM(forceUpdateContainerCacheFromSCM);

if (overwriteUpdateID != null) {
builder.setOverwriteUpdateID(overwriteUpdateID);
}
return builder;
}

@Nonnull
public KeyArgs toProtobuf() {
return KeyArgs.newBuilder()
KeyArgs.Builder builder = KeyArgs.newBuilder()
.setVolumeName(getVolumeName())
.setBucketName(getBucketName())
.setKeyName(getKeyName())
Expand All @@ -195,8 +211,11 @@ public KeyArgs toProtobuf() {
.setLatestVersionLocation(getLatestVersionLocation())
.setHeadOp(isHeadOp())
.setForceUpdateContainerCacheFromSCM(
isForceUpdateContainerCacheFromSCM())
.build();
isForceUpdateContainerCacheFromSCM());
if (overwriteUpdateID != null) {
builder.setOverwriteUpdateID(overwriteUpdateID);
}
return builder.build();
}

/**
Expand All @@ -219,6 +238,7 @@ public static class Builder {
private boolean recursive;
private boolean headOp;
private boolean forceUpdateContainerCacheFromSCM;
private Long overwriteUpdateID = null;

public Builder setVolumeName(String volume) {
this.volumeName = volume;
Expand Down Expand Up @@ -313,6 +333,11 @@ public Builder setForceUpdateContainerCacheFromSCM(boolean value) {
return this;
}

public Builder setOverwriteUpdateID(long overwriteUpdateID) {
this.overwriteUpdateID = overwriteUpdateID;
return this;
}

public OmKeyArgs build() {
return new OmKeyArgs(this);
}
Expand Down
Loading