[NSFS | GLACIER] Add support for GLACIER_DA and forced GLACIER #9219
Conversation
WalkthroughAdds a config flag to force GLACIER storage class, introduces a new GLACIER_DA storage class and parsing, updates NamespaceFS to treat multiple Glacier classes uniformly, and injects a request-time override in the S3 REST flow to rewrite STANDARD storage class to GLACIER when configured. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant R as S3 REST Handler
participant U as s3_utils
participant N as NamespaceFS / Downstream
C->>R: PUT/POST with headers (x-amz-storage-class)
Note over R: After header validation
R->>U: override_storage_class(req)
alt NSFS_GLACIER_FORCE_STORAGE_CLASS = true
U-->>R: rewrite `x-amz-storage-class` -> `GLACIER` when resolved class == STANDARD
else NSFS_GLACIER_FORCE_STORAGE_CLASS = false
U-->>R: no change
end
R->>N: continue processing (populate_request_additional_info_or_redirect)
N-->>C: normal response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used🧬 Code graph analysis (1)src/endpoint/s3/s3_utils.js (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
tangledbytes
force-pushed
the
utkarsh/feat/glacier-da-storage-class
branch
from
1f45c29 to
d8ab6ce
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/sdk/namespace_fs.js (4)
1076-1084: Reads from GLACIER_DA must require restore as wellCurrently only GLACIER is blocked until restored; GLACIER_DA objects would be readable immediately. If GLACIER_DA represents Deep Archive, this violates expected semantics.
Apply:
- if (obj_storage_class === s3_utils.STORAGE_CLASS_GLACIER) { + if (obj_storage_class === s3_utils.STORAGE_CLASS_GLACIER || + obj_storage_class === s3_utils.STORAGE_CLASS_GLACIER_DA) { if (obj_restore_status?.ongoing || !obj_restore_status?.expiry_time) { dbg.warn('read_object_stream: object is not restored yet', obj_restore_status); throw new S3Error(S3Error.InvalidObjectState); } }
1374-1392: Copy semantics for GLACIER_DAServer-side copy fallback and “must be restored before copy” checks apply only to GLACIER; extend to GLACIER_DA to avoid linking/copying archived content improperly.
Apply:
- if (src_storage_class === s3_utils.STORAGE_CLASS_GLACIER) { + if (src_storage_class === s3_utils.STORAGE_CLASS_GLACIER || + src_storage_class === s3_utils.STORAGE_CLASS_GLACIER_DA) { if (src_restore_status?.ongoing || !src_restore_status?.expiry_time) { dbg.warn('_validate_upload: object is not restored yet', src_restore_status); throw new S3Error(S3Error.InvalidObjectState); } - - return true; + return true; } @@ - return params.copy_source && params.storage_class === s3_utils.STORAGE_CLASS_GLACIER; + return params.copy_source && + [s3_utils.STORAGE_CLASS_GLACIER, s3_utils.STORAGE_CLASS_GLACIER_DA] + .includes(params.storage_class);
1433-1440: Migration WAL for GLACIER_DAIf GLACIER_DA requires migration, include it here; otherwise explain why it’s excluded.
Apply:
- if (params.storage_class === s3_utils.STORAGE_CLASS_GLACIER) { + if (params.storage_class === s3_utils.STORAGE_CLASS_GLACIER || + params.storage_class === s3_utils.STORAGE_CLASS_GLACIER_DA) { await this.append_to_migrate_wal(file_path); }
1-1: Treat GLACIER_DA like GLACIER in all glacier-specific checksSeveral places compare storage_class strictly to s3_utils.STORAGE_CLASS_GLACIER and will miss STORAGE_CLASS_GLACIER_DA — this causes incorrect restore/read/listing/expiry behavior. Fix by checking both constants or adding/using a central is_glacier(storage_class) helper.
Locations to change:
- src/sdk/glacier.js:314–317 — get_restore_status returns undefined unless STORAGE_CLASS_GLACIER.
- src/sdk/namespace_fs.js:1076–1084 — read_object_stream gate uses obj_storage_class === s3_utils.STORAGE_CLASS_GLACIER.
- src/sdk/namespace_fs.js:1376–1386 — copy-source validation uses src_storage_class === s3_utils.STORAGE_CLASS_GLACIER.
- src/sdk/namespace_fs.js:2296–2304 — restore_object assumes undefined means "not glacier" (relies on get_restore_status).
- src/sdk/namespace_fs.js:3690–3694 — _glacier_force_expire_on_get checks storage_class !== s3_utils.STORAGE_CLASS_GLACIER.
- src/endpoint/s3/ops/s3_get_object.js:42–50 — S3 GET blocks reads only when object_md.storage_class === s3_utils.STORAGE_CLASS_GLACIER.
🧹 Nitpick comments (5)
config.js (1)
199-206: Clarify force semantics and guard against misconfigAs written, NSFS_GLACIER_FORCE_STORAGE_CLASS can rewrite uploads to GLACIER even when NSFS_GLACIER_ENABLED is false, which would later be rejected by NamespaceFS._is_storage_class_supported. Please document that forcing requires NSFS_GLACIER_ENABLED=true, or gate the rewrite accordingly in the header-processing layer.
Would you like me to add a small guard in the header rewrite to check config.NSFS_GLACIER_ENABLED first and log a warning if forcing is set without enabling Glacier?
src/endpoint/s3/s3_utils.js (1)
24-25: Define and export a single source of truth for Glacier-class setAvoid scattering the set of Glacier-like classes across files. Export a frozen list here and reuse it elsewhere (e.g., namespace_fs).
Apply:
/** @type {nb.StorageClass} */ const STORAGE_CLASS_GLACIER_IR = 'GLACIER_IR'; // "S3 Glacier Instant Retrieval" /** @type {nb.StorageClass} */ const STORAGE_CLASS_GLACIER_DA = 'GLACIER_DA'; // "DBS3 specific Storage Class" + +// Classes that represent cold/archive semantics in NSFS +const GLACIER_CLASSES = Object.freeze([ + STORAGE_CLASS_GLACIER, + STORAGE_CLASS_GLACIER_DA, + STORAGE_CLASS_GLACIER_IR, +]);And simplify parse:
function parse_storage_class(storage_class) { if (!storage_class) return STORAGE_CLASS_STANDARD; if (storage_class === STORAGE_CLASS_STANDARD) return STORAGE_CLASS_STANDARD; - if (storage_class === STORAGE_CLASS_GLACIER) return STORAGE_CLASS_GLACIER; - if (storage_class === STORAGE_CLASS_GLACIER_DA) return STORAGE_CLASS_GLACIER_DA; - if (storage_class === STORAGE_CLASS_GLACIER_IR) return STORAGE_CLASS_GLACIER_IR; + if (GLACIER_CLASSES.includes(storage_class)) return storage_class; throw new Error(`No such s3 storage class ${storage_class}`); }Export it:
exports.STORAGE_CLASS_GLACIER_IR = STORAGE_CLASS_GLACIER_IR; exports.STORAGE_CLASS_GLACIER_DA = STORAGE_CLASS_GLACIER_DA; +exports.GLACIER_CLASSES = GLACIER_CLASSES;src/sdk/namespace_fs.js (3)
3613-3623: Support matrix: force vs enable mismatchGood to include GLACIER_DA and GLACIER_IR in the supported set. One caveat: when NSFS_GLACIER_FORCE_STORAGE_CLASS=true (header rewrite to GLACIER), but NSFS_GLACIER_ENABLED=false, uploads will be rejected here. Either document that forcing implies enabling or allow forcing to bypass this check for GLACIER (and GLACIER_DA) uploads.
I can wire a guard in the header rewrite path or adjust this check; confirm the intended behavior for combinations (3) and (4) in the PR description.
3692-3699: Force-expire-on-get: consider GLACIER_DAIf deep-archive should also be expired-on-get, extend the check; if not, please document the difference.
Apply:
- const storage_class = s3_utils.parse_storage_class(stat.xattr[Glacier.STORAGE_CLASS_XATTR]); - if (storage_class !== s3_utils.STORAGE_CLASS_GLACIER) return; + const storage_class = s3_utils.parse_storage_class(stat.xattr[Glacier.STORAGE_CLASS_XATTR]); + if (![s3_utils.STORAGE_CLASS_GLACIER, s3_utils.STORAGE_CLASS_GLACIER_DA].includes(storage_class)) return;
3613-3623: Deduplicate Glacier-class logic by reusing s3_utilsPrefer importing a shared set (e.g., s3_utils.GLACIER_CLASSES) instead of re-declaring glacier_storage_classes locally.
Apply:
- const glacier_storage_classes = [ - s3_utils.STORAGE_CLASS_GLACIER, - s3_utils.STORAGE_CLASS_GLACIER_DA, - s3_utils.STORAGE_CLASS_GLACIER_IR, - ]; + const glacier_storage_classes = s3_utils.GLACIER_CLASSES;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
config.js(1 hunks)src/endpoint/s3/s3_utils.js(3 hunks)src/sdk/namespace_fs.js(1 hunks)src/sdk/nb.d.ts(1 hunks)src/util/http_utils.js(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/endpoint/s3/s3_utils.js (2)
src/endpoint/s3/ops/s3_put_object.js (1)
storage_class(23-23)src/endpoint/s3/ops/s3_post_object_uploads.js (1)
storage_class(16-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build Noobaa Image
- GitHub Check: run-jest-unit-tests
🔇 Additional comments (1)
src/sdk/nb.d.ts (1)
20-21: Type extension looks goodAdding 'GLACIER_DA' to StorageClass aligns the types with the new class.
tangledbytes
force-pushed
the
utkarsh/feat/glacier-da-storage-class
branch
from
d8ab6ce to
fcd6987
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/endpoint/s3/s3_utils.js (2)
383-390: Map invalid storage class to S3Error instead of generic ErrorThrowing a generic Error likely surfaces as 500. Return a 400-class S3Error.
- throw new Error(`No such s3 storage class ${storage_class}`); + throw new S3Error({ ...S3Error.InvalidArgument, message: `Unsupported storage class ${storage_class}` });
825-833: Scope the override to upload ops to avoid surprising mutationsLimit override to put_object and create MPU; keep behavior centralized and future‑proof.
-function override_storage_class(req) { - if ( - config.NSFS_GLACIER_FORCE_STORAGE_CLASS && - parse_storage_class_header(req) === STORAGE_CLASS_STANDARD - ) { - req.headers['x-amz-storage-class'] = STORAGE_CLASS_GLACIER; - } -} +function override_storage_class(req) { + if (!config.NSFS_GLACIER_FORCE_STORAGE_CLASS) return; + // Only for ops where storage class is meaningful + if (!(req.op_name === 'put_object' || req.op_name === 'post_object_uploads')) return; + if (parse_storage_class_header(req) === STORAGE_CLASS_STANDARD) { + req.headers['x-amz-storage-class'] = STORAGE_CLASS_GLACIER; + } +}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
config.js(1 hunks)src/endpoint/s3/s3_rest.js(1 hunks)src/endpoint/s3/s3_utils.js(4 hunks)src/sdk/namespace_fs.js(1 hunks)src/sdk/nb.d.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- config.js
- src/sdk/nb.d.ts
- src/sdk/namespace_fs.js
🧰 Additional context used
🧬 Code graph analysis (1)
src/endpoint/s3/s3_utils.js (2)
src/endpoint/s3/ops/s3_put_object.js (1)
storage_class(23-23)src/endpoint/s3/ops/s3_post_object_uploads.js (1)
storage_class(16-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Noobaa Image
- GitHub Check: run-package-lock-validation
- GitHub Check: run-jest-unit-tests
🔇 Additional comments (3)
src/endpoint/s3/s3_utils.js (3)
23-25: Add GLACIER_DA constant — OKConstant addition and typing look good.
837-838: Export GLACIER_DA — OKExport aligns with added constant and parsing.
879-879: Export override_storage_class — OKMakes the helper available to s3_rest flow.
| // Will override the storage class if configured | ||
| s3_utils.override_storage_class(req); | ||
|
|
There was a problem hiding this comment.
Do not mutate headers before SigV4 auth; move override after authenticate_request()
Changing x-amz-storage-class pre-auth can invalidate AWS SigV4 when that header is signed. Apply override after authenticate_request(req) and before authorize_request(req); also limit to relevant ops.
Apply this diff:
- // Will override the storage class if configured
- s3_utils.override_storage_class(req);
+ // (moved below) override storage class only after signature authPlace this block immediately after authenticate_request(req);:
+ // Safe to mutate after signature auth and before policy checks.
+ // Limit to ops that accept x-amz-storage-class (PUT object, MPU create).
+ if (req.op_name === 'put_object' || req.op_name === 'post_object_uploads') {
+ s3_utils.override_storage_class(req);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Will override the storage class if configured | |
| s3_utils.override_storage_class(req); | |
| // (moved below) override storage class only after signature auth |
| // Will override the storage class if configured | |
| s3_utils.override_storage_class(req); | |
| // Safe to mutate after signature auth and before policy checks. | |
| // Limit to ops that accept x-amz-storage-class (PUT object, MPU create). | |
| if (req.op_name === 'put_object' || req.op_name === 'post_object_uploads') { | |
| s3_utils.override_storage_class(req); | |
| } |
🤖 Prompt for AI Agents
In src/endpoint/s3/s3_rest.js around lines 110-112, the code mutates x-amz-*
headers before SigV4 authentication by calling
s3_utils.override_storage_class(req); move this call so it runs immediately
after authenticate_request(req) and before authorize_request(req) to avoid
invalidating signed headers, and restrict the override to only the S3 operations
that accept a storage-class header (e.g., PUT object/ multipart upload
completes) rather than running for all requests.
fcd6987 to
1ddb315
Compare
Signed-off-by: Utkarsh Srivastava <[email protected]>
tangledbytes
force-pushed
the
utkarsh/feat/glacier-da-storage-class
branch
from
1ddb315 to
65f77f1
Compare
Explain the Changes
This PR adds support for
GLACIER_DAstorage class and adds support forcedGLACIERstorage class. With the changes in this PR, ifNSFS_GLACIER_FORCE_STORAGE_CLASS = truethen when no storage class is provided OR whenSTANDARDstorage class is provided, it will be treated asGLACIERstorage class object. The following combinations are expected and supported:NSFS_GLACIER_FORCE_STORAGE_CLASS = false; DENY_UPLOAD_TO_STORAGE_CLASS_STANDARD = false- No storage class or storage classSTANDARDwould work.NSFS_GLACIER_FORCE_STORAGE_CLASS = false; DENY_UPLOAD_TO_STORAGE_CLASS_STANDARD = true- No storage class or storage classSTANDARDwould not work.NSFS_GLACIER_FORCE_STORAGE_CLASS = true; DENY_UPLOAD_TO_STORAGE_CLASS_STANDARD = true- No storage class or storage classSTANDARDwould work however the storage class would be implicitly changed toGLACIER.NSFS_GLACIER_FORCE_STORAGE_CLASS = true; DENY_UPLOAD_TO_STORAGE_CLASS_STANDARD = false- Same as above.The above tries to achive the ask which was:
Issues: Fixed #xxx / Gap #xxx
#9184
Testing Instructions:
Summary by CodeRabbit