[NC | NSFS] add support for DMAPI xattr based GLACIER storage class #8028

tangledbytes · 2024-05-08T12:52:27Z

Explain the changes

This PR adds support expands the GLACIER storage class by automatically deriving the state of the object if config.NSFS_GLACIER_USE_DMAPI = true (not the default).

Testing Instructions:

./node_modules/.bin/mocha src/test/unit_tests/test_nsfs_glacier_backend.js

Doc added/updated
Tests added

guymguym · 2024-05-08T21:55:52Z

src/sdk/nsfs_glacier_backend/backend.js

@@ -153,7 +169,22 @@ class GlacierBackend {
     * @returns {nb.RestoreStatus | undefined}
     */
    static get_restore_status(xattr, now, file_path) {
-        if (xattr[GlacierBackend.STORAGE_CLASS_XATTR] !== s3_utils.STORAGE_CLASS_GLACIER) return;
+		if (GlacierBackend.storage_class_from_xattr(xattr, config.NSFS_GLACIER_USE_DMAPI) !== s3_utils.STORAGE_CLASS_GLACIER) {


split long line -

Suggested change

if (GlacierBackend.storage_class_from_xattr(xattr, config.NSFS_GLACIER_USE_DMAPI) !== s3_utils.STORAGE_CLASS_GLACIER) {

const storage_class = GlacierBackend.storage_class_from_xattr(xattr, config.NSFS_GLACIER_USE_DMAPI);

if (storage_class !== s3_utils.STORAGE_CLASS_GLACIER) {

guymguym · 2024-05-08T21:58:00Z

src/sdk/nsfs_glacier_backend/backend.js

+                expiry.setDate(expiry.getDate() + config.S3_RESTORE_REQUEST_MAX_DAYS);
+
+                return {
+                    state: 'RESTORED',


use constant -

Suggested change

state: 'RESTORED',

state: GlacierBackend.RESTORE_STATUS_RESTORED,

guymguym · 2024-05-08T22:16:29Z

src/sdk/nsfs_glacier_backend/backend.js

+        if (config.NSFS_GLACIER_USE_DMAPI) {
+            if (xattr[GlacierBackend.GPFS_DMAPI_XATTR_TAPE_PREMIG]) {
+                const expiry = new Date();
+                expiry.setDate(expiry.getDate() + config.S3_RESTORE_REQUEST_MAX_DAYS);


I think we should have a specific config for this amount of days config.NSFS_GLACIER_DMAPI_PMIG_DAYS which will be initialized by default to config.S3_RESTORE_REQUEST_MAX_DAYS (which is 30), but could be overridden in config.json. The reason is that I'm not sure that the default we pick now would make sense to all cases. Perhaps in some cases replying with now+1day make better sense for example.

Also we can add comment in the code to explain:

// we do not know for how long the file is going to remain available, // the expiry is set to now + fixed config, which means it's always appears // to the user with the same amount of time left before it expires. expiry.setDate(expiry.getDate() + config.NSFS_GLACIER_DMAPI_PMIG_DAYS);

guymguym · 2024-05-08T22:18:30Z

src/sdk/nsfs_glacier_backend/backend.js

+	 */
+	static storage_class_from_xattr(xattr, use_dmapi) {
+		if (
+            use_dmapi &&


I think you can simply use config.NSFS_GLACIER_USE_DMAPI inside here and avoid passing it in which will also reduce the clutter in calling this function.

Suggested change

use_dmapi &&

config.NSFS_GLACIER_USE_DMAPI &&

Using global configs makes testing quite tricky that's why I deliberately added it as a parameter wherever I could.

ok, so maybe just set this config as the default for the argument so you can still override for testing but don't need to pass in normal calls

static storage_class_from_xattr(xattr, use_dmapi = config.NSFS_GLACIER_USE_DMAPI) {

guymguym · 2024-05-08T22:23:47Z

src/sdk/nsfs_glacier_backend/backend.js

+
+        if (config.NSFS_GLACIER_USE_DMAPI) {
+            if (xattr[GlacierBackend.GPFS_DMAPI_XATTR_TAPE_PREMIG]) {
+                const expiry = new Date();


this expiry might deserve a distinct name, like

Suggested change

const expiry = new Date();

const premig_expiry = new Date();

guymguym · 2024-05-08T22:24:40Z

src/sdk/nsfs_glacier_backend/backend.js

@@ -39,6 +40,20 @@ class GlacierBackend {

    static STORAGE_CLASS_XATTR = 'user.storage_class';

+	/**


indentation

guymguym · 2024-05-08T22:25:51Z

src/sdk/namespace_fs.js

+            if (
+                config.NSFS_GLACIER_USE_DMAPI &&
+                stat.xattr[GlacierBackend.GPFS_DMAPI_XATTR_TAPE_INDICATOR]
+            ) {
+                // If the item is not already premigrated then throw error
+                if (!stat.xattr[GlacierBackend.GPFS_DMAPI_XATTR_TAPE_PREMIG]) {
+                    throw new Error('cannot restore externally managed object');
+                }
+
+                // If the item is premigrated then its a no-op
+                // Should result in HTTP: 200 OK
+                return false;
+            }
+


How about extracting this code to a function in GlacierBackend

Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com> improve restore check Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com> improve restore tests Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com> improve restore tests Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com> remove unused deps Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com> fix race in the test Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com> make linter happy Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com> address review comments Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com>

guymguym · 2024-05-29T20:42:28Z

src/native/fs/fs_napi.cpp

+const static std::vector<std::string> GPFS_XATTRS{
+	GPFS_ENCRYPTION_XATTR_NAME,
+	GPFS_DMAPI_XATTR_TAPE_INDICATOR,
+	GPFS_DMAPI_XATTR_TAPE_PREMIG,
+};


this list is going to be used in every stat call... I think we want to optimize it so that it will not include dmapi xattrs when config.NSFS_GLACIER_USE_DMAPI is false.

For this I would add this to namespace_fs in prepare_fs_context to set fs_context.use_dmapi = config.NSFS_GLACIER_USE_DMAPI. This is also needed in manage_nsfs_glacier in force_gpfs_fs_context ().

Then back here in fs_napi you can store it in FSWorker and pass it to get_fd_gpfs_xattr... a long way... but it would save a these two calls when the config is false which I think is worth the coding effort.

guymguym · 2024-05-29T20:44:05Z

src/sdk/nsfs_glacier_backend/backend.js

@@ -5,6 +5,7 @@ const nb_native = require('../../util/nb_native');
 const s3_utils = require('../../endpoint/s3/s3_utils');
 const { round_up_to_next_time_of_day } = require('../../util/time_utils');
 const dbg = require('../../util/debug_module')(__filename);
+const config = require('../../../config');

 class GlacierBackend {


What do you think about renaming this class to be called simply Glacier?
This is now used in so many places, that I wish it was more concise...
I also think this file can move to src/sdk/glacier.js and src/sdk/glacier_tapecloud.js

pull-request-size bot added the size/L label May 8, 2024

tangledbytes force-pushed the utkarsh/feat/dmapi-based-glacier branch from 1af7dee to c92287a Compare May 8, 2024 13:19

guymguym added this to the 5.15.4 milestone May 8, 2024

guymguym requested changes May 8, 2024

View reviewed changes

tangledbytes force-pushed the utkarsh/feat/dmapi-based-glacier branch from c92287a to b9278f9 Compare May 28, 2024 13:32

guymguym requested changes May 29, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NC | NSFS] add support for DMAPI xattr based GLACIER storage class #8028

[NC | NSFS] add support for DMAPI xattr based GLACIER storage class #8028

tangledbytes commented May 8, 2024

guymguym May 8, 2024

guymguym May 8, 2024

guymguym May 8, 2024

guymguym May 8, 2024

tangledbytes May 9, 2024

guymguym May 9, 2024

guymguym May 8, 2024

guymguym May 8, 2024

guymguym May 8, 2024

guymguym May 29, 2024

guymguym May 29, 2024

	if (GlacierBackend.storage_class_from_xattr(xattr, config.NSFS_GLACIER_USE_DMAPI) !== s3_utils.STORAGE_CLASS_GLACIER) {
	const storage_class = GlacierBackend.storage_class_from_xattr(xattr, config.NSFS_GLACIER_USE_DMAPI);
	if (storage_class !== s3_utils.STORAGE_CLASS_GLACIER) {

	state: 'RESTORED',
	state: GlacierBackend.RESTORE_STATUS_RESTORED,

		@@ -39,6 +40,20 @@ class GlacierBackend {

		static STORAGE_CLASS_XATTR = 'user.storage_class';

		/**

[NC | NSFS] add support for DMAPI xattr based GLACIER storage class #8028

Are you sure you want to change the base?

[NC | NSFS] add support for DMAPI xattr based GLACIER storage class #8028

Conversation

tangledbytes commented May 8, 2024

Explain the changes

Testing Instructions:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment