Skip to content

HDDS-16039. Introduce S3StorageClass mapping to OzoneStoragePolicy. - #11229

Open
devmadhuu wants to merge 6 commits into
apache:HDDS-11233from
devmadhuu:HDDS-16039
Open

devmadhuu wants to merge 6 commits into
apache:HDDS-11233from
devmadhuu:HDDS-16039

Conversation

@devmadhuu

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

This PR is to introduce S3StorageClass — a small enum that maps AWS S3 storage class strings (STANDARD, STANDARD_IA, GLACIER) to Ozone storage policies (HOT, WARM, COLD).

Two helper methods on the enum:

  • fromS3StorageClass(String) — parse the value of an incoming x-amz-storage-class header.
  • fromStoragePolicy(StoragePolicy) — go the other way, for building responses.

This PR is infrastructure only — no caller uses S3StorageClass yet. The S3 write path (PutObject) and read path (GetObject) will start calling it in follow-up PRs.

What is the link to the Apache JIRA

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

How was this patch tested?

New unit test TestS3StorageClass covers:

  • S3 string → enum for all three values, plus rejects null and unknown values.
  • Ozone StoragePolicy → enum for all three, plus rejects null.
  • Round-trip consistency across all enum values.

@devmadhuu
devmadhuu marked this pull request as ready for review September 11, 2026 11:09

@Gargi-jais11 Gargi-jais11 left a comment

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.

Thanks @devmadhuu for this PR.

Comment on lines +134 to +138
// S3 storage class values passed via the `x-amz-storage-class` request header.
public static final String S3_STORAGE_CLASS_STANDARD = "STANDARD";
public static final String S3_STORAGE_CLASS_STANDARD_IA = "STANDARD_IA";
public static final String S3_STORAGE_CLASS_GLACIER = "GLACIER";

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.

I don't know whether u are aware or not but S3StorageType.java in the S3 Gateway has similar enums means: “this S3 storage class string picks how many copies / EC layout the object gets” (STANDARD → 3-way Ratis, STANDARD_IA → EC, etc.). Almost all upload/download code uses that today.

This PR adds S3StorageClass, which means: “this string picks Hot / Warm / Cold (which disk tier).”

Those are two different ideas with similar names. That will confuse people and cause wrong imports later.

We should look into these and check how they both can be used independently.

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.

AWS's StorageClass https://aws.amazon.com/s3/storage-classes does not explicitly specify the replica types or storage media types for each StorageClass; instead, it defines concepts such as durability, SLA, zones, and latency , etc for different StorageClasses

Ozone’s S3 StorageClasses can have custom definitions; users can specify the storage medium (storage policy) and replica type through S3 StorageClass parameter, which does not conflict with AWS S3’s StorageClasses.

@devmadhuu devmadhuu Sep 23, 2026 •

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.

@xichen01 so what is the way to go forward ? should we combine both and adapt the concept of durability, SLA, zones, and latency , etc as below in existing S3StorageType

Add a second constructor arg. GLACIER gets null replication, meaning "no replication opinion, archive tier only" — AWS does not pin storage class to a replica count,
and inventing one for GLACIER would make it indistinguishable from STANDARD_IA on the reverse map.

   REDUCED_REDUNDANCY(RatisReplicationConfig.getInstance(ReplicationFactor.ONE),   HOT),                                                                                                                   
   STANDARD(RatisReplicationConfig.getInstance(ReplicationFactor.THREE),           HOT),                                                                                                                   
   STANDARD_IA(new ECReplicationConfig(3, 2),                                      WARM),                                                                                                                  
   GLACIER(null,                                                                   COLD);   

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.

@F64116045 F64116045 Sep 26, 2026 •

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.

@devmadhuu Thanks for proposing a way to combine the two mappings
If we combine them, would the reverse mapping then use both replication config and storage policy? I think it needs both to distinguish REDUCED_REDUNDANCY from STANDARD, since both are HOT. If so, is null still needed for GLACIER? COLD already distinguishes it from STANDARD_IA.

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.

Kindly have a look and we can discuss more if needed or good to go with this design ?
cc: @ChenSammi

@xichen01 xichen01 Oct 14, 2026 •

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.

@devmadhuu

@Gargi-jais11 Agree that fields should be optional. On your question "Where does the reverse direction get the policy?", this patch #11377 keys can override the bucket default.
The reverse direction reads KeyInfo.storagePolicy and never fills a missing value from the bucket. So SetBucketProperty to COLD won't relabel existing objects as GLACIER. KeyInfo.storagePolicy is only set when a policy was explicitly requested on the write, so a key that inherited its policy has null, and null falls back to the replication-based mapping.

Once the storage policy feature for OM is launched, every Key will have a storagePolicy. The priority order is as follows:

  1. Explicitly specified when the key is created;
  2. If not specified for the key, the bucket’s storage policy is used (if not null);
  3. If not specified when the key is created and the bucket’s storage policy is also null, OzoneStoragePolicy.getDefaultPolicy() is used;

can see:
https://github.com/apache/ozone/pull/11377/changes#diff-2841a017f8d700d802af1b1c49fea9ca281098728796384f5b2e79c23f09ab65R998

Thus, for keys created before the storage policy feature was launched, the storagePolicy is null; after the feature is launched, every Key will have a storagePolicy.

It can be understood that once a storage policy is specified, it will not change (unless you set it manually).

  • The storagePolicy attribute of a Key is independent and will not change based on changes to the bucket’s storagePolicy.
  • Even if a fallback occurs during key write (when allowFallbackStoragePolicy=true), this will not change the key’s storagePolicy. Fallback is an SCM-side behavior and does not affect the key’s storagePolicy

Therefore, the reverse map simply retrieves the storage policy directly based on the key (the same applies to replication type).

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.

@devmadhuu +1 to your proposal to add a gateway setting for the storage policy for STANDARD/REDUCED_REDUNDANCY storage classes.
IMO whether to strictly enforce storage policy for these classes and how can be scoped for a future phase.

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.

@devmadhuu +1 to the proposal.

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.

Thanks @xichen01, you're right, and I had it wrong. OM resolves the policy in OMKeyCreateRequest.preExecute (key, then bucket, then OzoneStoragePolicy.getDefaultPolicy()) and writes it back into KeyArgs, so every new key stores a policy. That includes keys in buckets with no policy, which get the default, WARM. Only keys written before the feature have null.

Thanks @Gargi-jais11 and @SaketaChalamchala for +1 to proposal. So the reverse direction just reads the key's own replication and storage policy. The rule stays lenient: replication decides the class, and only COLD changes it. null falls back to replication, as today.

This means mapping STANDARD to HOT through the gateway setting doesn't change what any object reports. Old keys (RATIS/THREE + null) and new ones (RATIS/THREE + HOT or WARM) all report STANDARD. It only changes where new STANDARD writes go. To keep that true, the setting will accept HOT, WARM or unset for STANDARD/REDUCED_REDUNDANCY, but not COLD.

try {
return S3StorageClass.valueOf(s3StorageClass.toUpperCase());
} catch (IllegalArgumentException e) {
throw new IllegalArgumentException(

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.

IllegalArgumentException is fine for a util with no callers yet but PutObject must map unknown classes to OS3Exception / InvalidStorageClass, not a 500.

@github-actions github-actions Bot added the s3 S3 Gateway label Sep 23, 2026
@@ -0,0 +1,101 @@
/*

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.

One thing worth settling before the enum shape: for S3 buckets that are links, which bucket supplies the policy?
A bucket created through the S3 API is a real bucket in /s3v, so inheriting from the bucket works fine there. But where an existing Ozone bucket is exposed via ozone sh bucket link, the gateway reads the link row's properties: getBucketInfo doesn't resolve links (OzoneManagerUtils:63), while the key write is rewritten to the resolved target (OMKeyRequest:141, bucket.update(keyArgs)).

Today that already means replication comes from the link rather than the target; if the tier defaults from the bucket policy, it hits the same split, with the link row unset and the real bucket carrying the intended policy.

@devmadhuu devmadhuu Sep 28, 2026 •

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.

Thanks @greenwich for raising the linked-bucket case. I traced the current HDDS-11233 flow. Although the low-level OzoneManagerUtils.getBucketInfo() returns the raw link row, the S3 client path goes through
OzoneManager.getBucketInfo(), which calls enrichLinkBucketInfo(). That method resolves the real target and withOperationalPropertiesFrom() copies both storagePolicy and defaultReplicationConfig from the target while preserving the link identity. The OM key-write path then resolves to the same target bucket. Therefore, the target bucket is the source of both policy and replication defaults. I’ll add an S3 linked-bucket regression test when the storage-class mapping is connected to the write path in the follow-up PR.

… spec and resolver.

Add S3StorageClassSpec to define what a storage class means in Ozone. All fields are optional, and it holds a replication type rather than a full replication config.

Add S3StorageClassResolver. Forward mapping rejects unknown classes; reverse mapping never fails. Replication decides the class, and only COLD changes it.

Reshape S3StorageClass as the list of names. Add REDUCED_REDUNDANCY and remove fromStoragePolicy.

Add gateway settings for the STANDARD and REDUCED_REDUNDANCY storage policy, accepting HOT, WARM or empty. COLD is rejected.

For EC, take the layout from the storage-config header, then the bucket's EC setting, then EC 3-2.

Deprecate S3StorageType and have it delegate to the resolver, with no change in behaviour.
@devmadhuu

Copy link
Copy Markdown
Contributor Author

@SaketaChalamchala @xichen01 Kindly have a relook. This is only a framework in this PR. read and write paths will follow in future PR

throw new IllegalArgumentException("Not supported s3StorageClass: null");
}
try {
return S3StorageClass.valueOf(s3StorageClass.toUpperCase(Locale.ROOT));

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.

Nit: The storage-class header is now parsed case-insensitively, which differs from today's behavior and from AWS.
We should match exactly (valueOf(s3StorageClass)), and change the test to assert that "standard" is rejected.

@SaketaChalamchala SaketaChalamchala Oct 21, 2026 •

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.

Please also trim s3StorageClass. Values like GLACIER may fail. Add a test to cover case-sensitivity and white spaces

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.

Done.

Comment on lines +169 to +172
String value = conf.getTrimmed(key, "");
if (value.isEmpty()) {
return null;
}

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 parsees and validates the setting on every toSpec call, so a bad value (COLD) only fails STANDARD/REDUCED_REDUNDANCY writes, with the same IllegalArgumentException as an unknown class. can we resolve both settings once at S3 Gateway startup so misconfiguration fails fast and the per-request error maps cleanly toInvalidStorageClass?

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.

Yes, will fix.

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.

Yes, I corrected it to validate at Gateway start.

Comment on lines +109 to +112
if (type == ReplicationType.RATIS) {
return RatisReplicationConfig.getInstance(spec.getReplicationFactor());
}
throw new IllegalArgumentException("Storage class spec has no supported replication type: "

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.

The javadoc in spec class says that unset fields are resolved elsewhere (e.g. from the bucket), but a null type is throws here. should this return bucketReplicationConfig instead, or should the javadoc be corrected?

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.

yeah, javadoc to be corrected as it is too broad. The spec can hold unset fields, but the current replication converter requires a supported replication type. Its bucket fallback applies to the EC layout after the spec has selected EC. I have corrected the wording.

@Gargi-jais11 Gargi-jais11 left a comment

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.

Thanks @devmadhuu for updating the PR. Overall LGTM!
Left few minor review comments.

Comment on lines +139 to +142
if (storagePolicy == OzoneStoragePolicy.COLD
&& replicationConfig.getReplicationType() == ReplicationType.RATIS) {
return S3StorageClass.GLACIER;
}

@SaketaChalamchala SaketaChalamchala Oct 21, 2026 •

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.

COLD + EC currently returns STANDARD_IA. Please add a test to cover this.

Suggested change
if (storagePolicy == OzoneStoragePolicy.COLD
&& replicationConfig.getReplicationType() == ReplicationType.RATIS) {
return S3StorageClass.GLACIER;
}
if (storagePolicy == OzoneStoragePolicy.COLD) {
if (replicationConfig.getReplicationType() == ReplicationType.RATIS) {
return S3StorageClass.GLACIER;
} else {
throw new IllegalArgumentException("Unsupported storage class spec");
}
}

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.

@SaketaChalamchala , There is already an explicit EC 6+3 + COLD → STANDARD_IA assertion in testReverseOnlyColdChangesTheClass(). I propose extending the EC-layout test to check the same result for COLD across the other supported layouts.

Based on your suggested code change above, you are proposing to make COLD + EC combination invalid. This method describes stored objects, whose replication and storage policy can be selected independently. We currently have no separate S3 class for EC+COLD, so it reports STANDARD_IA based on replication.

Throwing here would conflict with the reverse-mapping contract and could make response generation fail for an existing object once the read paths use this resolver. Reporting STANDARD_IA does not change the object’s stored COLD policy.

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.

@SaketaChalamchala , There is already an explicit EC 6+3 + COLD → STANDARD_IA assertion in testReverseOnlyColdChangesTheClass(). I propose extending the EC-layout test to check the same result for COLD across the other supported layouts.

As per your code suggestion, you mean we should reject the COLD + EC. This method describes stored objects, whose replication and storage policy can be selected independently. We currently have no separate S3 class for EC+COLD, so it reports STANDARD_IA based on replication.

Throwing here would conflict with the reverse-mapping contract and could make response generation fail for an existing object once the read paths use this resolver. Reporting STANDARD_IA does not change the object’s stored COLD policy.

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.

@devmadhuu Agree that failing the response generation is not ideal but, we also want to avoid metadata ambiguity and invisible storage transitions.
If the storage tier of EC data transitions to COLD (manual or auto-tiering) this transition is invisible to S3 clients and there is no way for S3 users to enforce this storage tier for EC data going forward. It will always be created in the WARM tier. The same goes for EC + HOT as well.

I would suggest not setting storage policy in STANDARD_IA definition just like we do for STANDARD. Maybe add a gateway config to decide storage policy on writes.

STANDARD_IA(S3_STORAGE_CLASS_STANDARD_IA, S3StorageClassSpec.newBuilder()
     .setReplicationType(ReplicationType.EC)
     .build())

Introduce GLACIER_DEEP_ARCHIVE for EC + COLD and document that

  1. existing storage classes STANDARD, STANDARD_IA and REDUCED_REDUNDANCY only map to replication layouts and that storage policy can be obtained from keyInfo/bucketInfo/server default configs (gateway configs are also not reliable as they can change between restarts).
  2. Only GLACIER family of classes which define replication layout and storage policy.
  3. Only HOT/WARM -> COLD storage transitions are currently captured through Storage classes (helpful is user use this info to enforce policies like backup, tiering etc).
  4. All replication layout transitions are captured through storage classes.

Please let me know if this makes sense or if you have any more thoughts.

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.

@SaketaChalamchala I am little confused now, we agreed to have gateway configs for admin to define in forward write operations for STANDARD and REDUCED_REDUNDANCY classes and that will be intentional right ? so confused with your below statement:

"_gateway configs are also not reliable as they can change between restarts"_

I agree that we should have similar config for STANDARD_IA also.

Consider this sequence once the S3 write integration is connected in follow up PRs:

Step | Gateway setting for STANDARD | Operation | Stored object policy

1 | HOT | Upload a.txt | HOT
2 | Change to WARM and restart | No object write | a.txt remains HOT
3 | WARM | Upload b.txt | WARM

Both objects can report the STANDARD storage class because both use RATIS/THREE
The administrator’s change will be intentional and valid. But the current gateway configuration cannot tell us the policy of a.txt
Current gateway configuration: WARM

a.txt’s stored policy:         HOT
b.txt’s stored policy:         WARM

Therefore:

  • For a new write: use the applicable gateway configuration to select the policy.
  • For an existing object: inspect that object’s stored metadata.

Changing a write setting does not itself change previously stored objects.

My understanding is that we should retain the gateway settings for STANDARD and REDUCED_REDUNDANCY writes, and consider the same approach for STANDARD_IA. These settings are loaded at startup, so an intentional configuration change followed by restart affects subsequent writes. Existing objects retain their stored policies, and reverse mapping uses their stored metadata rather than the current gateway settings. Is your concern about using gateway configuration to interpret existing objects, or are you also proposing changes to the write settings?

I have done the changes for gateway config for STANDARD_IA as well and removed hardcoded policy association with STANDARD_IA. Also I would suggest to use DEEP_ARCHIVE name instead of GLACIER_DEEP_ARCHIVE, but in terms they are both same in view of AWS specs.

switch (storageClass) {
case STANDARD:
return spec.withStoragePolicy(
getConfiguredStoragePolicy(conf, OZONE_S3G_STORAGE_CLASS_STANDARD_STORAGE_POLICY));

@SaketaChalamchala SaketaChalamchala Oct 21, 2026 •

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.

The storage policy fall back to bucket config or server default if OZONE_S3G_STORAGE_CLASS_STANDARD_STORAGE_POLICY is not set is missing here. Add test to cover this.

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.

@SaketaChalamchala The fallback is handled by OM rather than inside this resolver. When the Gateway setting is empty, the spec returns null to indicate that there is no policy override. The client omits that field from the request, and OMKeyRequest.getStoragePolicy() selects the bucket’s policy, then OM’s configured default if the bucket has none.

The current S3 tests verify the null result, but I agree that final-policy coverage would make this clearer. I propose testing bucket inheritance and the configured OM default in the OM tests, and covering the complete S3 path when the follow-up write PR connects the resolver.

I would preserve the null here so the Gateway does not override the policy of the actual bucket.

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.

@SaketaChalamchala The fallback is handled by OM rather than inside this resolver. When the Gateway setting is empty, the spec returns null to indicate that there is no policy override. The client omits that field from the request, and OMKeyRequest.getStoragePolicy() selects the bucket’s policy, then OM’s configured default if the bucket has none.

The current S3 tests verify the null result, but I agree that final-policy coverage would make this clearer. I propose testing bucket inheritance and the configured OM default in the OM tests, and covering the complete S3 path when the follow-up write PR connects the resolver.

I would preserve the null here so the Gateway does not override the policy of the actual bucket.

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.

Sounds good.

assertEquals(EC_6_3, S3StorageClassResolver.toReplicationConfig(ec, "", EC_6_3));
// A non-EC bucket replication is not used for an EC class.
assertEquals(EC_3_2, S3StorageClassResolver.toReplicationConfig(ec, null, RATIS_THREE));
assertEquals(EC_3_2, S3StorageClassResolver.toReplicationConfig(ec, null, null));

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.

nit: assert invalid storageConfigHeader throws.

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.

Agreed. The resolver already throws IllegalArgumentException for an invalid EC layout, but that case is missing from its tests. I’ll add assertions with and without a bucket configuration to verify that invalid input is rejected instead of falling back to the bucket or default layout. No production-code change is needed.

@greenwich

Copy link
Copy Markdown
Contributor

Question on the resolver design: should there be an operator switch that lets the bucket's settings take precedence over the client's storage class?

Without it, we get situations like #11679: since HDDS-10979 a STANDARD header overrides an EC bucket, and a copy tool silently wrote RATIS/THREE into our EC cluster. With storage policies, the same precedence (request > bucket > default) would let a client's GLACIER move data to COLD in a bucket set to HOT. S3StorageClassResolver seems like the natural place for such a switch, so it might be worth deciding now whether it should be possible, even if it's implemented later.

@adoroszlai

Copy link
Copy Markdown
Contributor

To save CI resources, please consider creating PRs as draft, and keeping them that way until CI in fork is passing and PR is approved by reviewers. That is, when further updates are not expected. Change this PR to draft only if c047df5 is not the last commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

s3 S3 Gateway

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants