-
Notifications
You must be signed in to change notification settings - Fork 76
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[NC | NSFS] add support for DMAPI xattr based GLACIER storage class #8028
base: master
Are you sure you want to change the base?
[NC | NSFS] add support for DMAPI xattr based GLACIER storage class #8028
Conversation
1af7dee
to
c92287a
Compare
@@ -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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
split long line -
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) { |
expiry.setDate(expiry.getDate() + config.S3_RESTORE_REQUEST_MAX_DAYS); | ||
|
||
return { | ||
state: 'RESTORED', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use constant -
state: 'RESTORED', | |
state: GlacierBackend.RESTORE_STATUS_RESTORED, |
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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);
*/ | ||
static storage_class_from_xattr(xattr, use_dmapi) { | ||
if ( | ||
use_dmapi && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
use_dmapi && | |
config.NSFS_GLACIER_USE_DMAPI && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using global configs makes testing quite tricky that's why I deliberately added it as a parameter wherever I could.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) {
|
||
if (config.NSFS_GLACIER_USE_DMAPI) { | ||
if (xattr[GlacierBackend.GPFS_DMAPI_XATTR_TAPE_PREMIG]) { | ||
const expiry = new Date(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this expiry might deserve a distinct name, like
const expiry = new Date(); | |
const premig_expiry = new Date(); |
@@ -39,6 +40,20 @@ class GlacierBackend { | |||
|
|||
static STORAGE_CLASS_XATTR = 'user.storage_class'; | |||
|
|||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indentation
src/sdk/namespace_fs.js
Outdated
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; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about extracting this code to a function in GlacierBackend
c92287a
to
b9278f9
Compare
src/native/fs/fs_napi.cpp
Outdated
const static std::vector<std::string> GPFS_XATTRS{ | ||
GPFS_ENCRYPTION_XATTR_NAME, | ||
GPFS_DMAPI_XATTR_TAPE_INDICATOR, | ||
GPFS_DMAPI_XATTR_TAPE_PREMIG, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, PTAL.
@@ -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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, done.
b9278f9
to
b57d9dd
Compare
Signed-off-by: Utkarsh Srivastava <[email protected]> improve restore check Signed-off-by: Utkarsh Srivastava <[email protected]> improve restore tests Signed-off-by: Utkarsh Srivastava <[email protected]> improve restore tests Signed-off-by: Utkarsh Srivastava <[email protected]> remove unused deps Signed-off-by: Utkarsh Srivastava <[email protected]> fix race in the test Signed-off-by: Utkarsh Srivastava <[email protected]> make linter happy Signed-off-by: Utkarsh Srivastava <[email protected]> address review comments Signed-off-by: Utkarsh Srivastava <[email protected]> rename glacier backends and restructure Signed-off-by: Utkarsh Srivastava <[email protected]> replace restore object external managed entity test with direct testing of is_externally_managed Signed-off-by: Utkarsh Srivastava <[email protected]> fix linting issue Signed-off-by: Utkarsh Srivastava <[email protected]>
b57d9dd
to
fb773a6
Compare
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