Skip to content

Commit

Permalink
Merge pull request #8502 from nadavMiz/versioning-concurrency-conditions
Browse files Browse the repository at this point in the history 
NSFS | versioning | remove seperation between GPFS and POSIX errors for concurrency retries
  • Loading branch information
@nadavMiz
nadavMiz authored Nov 4, 2024
2 parents 07f1582 + d095c3a commit aca3c0b
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 16 deletions.
22 changes: 9 additions & 13 deletions src/sdk/namespace_fs.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -934,7 +934,6 @@ class NamespaceFS {
let stat;
let isDir;
let retries = (this._is_versioning_enabled() || this._is_versioning_suspended()) ? config.NSFS_RENAME_RETRIES : 0;
const is_gpfs = native_fs_utils._is_gpfs(fs_context);
try {
for (;;) {
try {
Expand Down Expand Up @@ -962,7 +961,7 @@ class NamespaceFS {
} catch (err) {
dbg.warn(`NamespaceFS.read_object_md: retrying retries=${retries} file_path=${file_path}`, err);
retries -= 1;
if (retries <= 0 || !native_fs_utils.should_retry_link_unlink(is_gpfs, err)) throw err;
if (retries <= 0 || !native_fs_utils.should_retry_link_unlink(err)) throw err;
await P.delay(get_random_delay(config.NSFS_RANDOM_DELAY_BASE, 0, 50));
}
}
Expand Down Expand Up @@ -1002,7 +1001,6 @@ class NamespaceFS {
try {
await this._load_bucket(params, fs_context);
let retries = (this._is_versioning_enabled() || this._is_versioning_suspended()) ? config.NSFS_RENAME_RETRIES : 0;
const is_gpfs = native_fs_utils._is_gpfs(fs_context);
let stat;
for (;;) {
try {
Expand Down Expand Up @@ -1033,7 +1031,7 @@ class NamespaceFS {
file = null;
}
retries -= 1;
if (retries <= 0 || !native_fs_utils.should_retry_link_unlink(is_gpfs, err)) {
if (retries <= 0 || !native_fs_utils.should_retry_link_unlink(err)) {
new NoobaaEvent(NoobaaEvent.OBJECT_GET_FAILED).create_event(params.key,
{bucket_path: this.bucket_path, object_name: params.key}, err);
throw err;
Expand Down Expand Up @@ -1526,7 +1524,7 @@ class NamespaceFS {
break;
} catch (err) {
retries -= 1;
const should_retry = native_fs_utils.should_retry_link_unlink(is_gpfs, err);
const should_retry = native_fs_utils.should_retry_link_unlink(err);
dbg.warn(`NamespaceFS._move_to_dest_version error: retries=${retries} should_retry=${should_retry}` +
` new_ver_tmp_path=${new_ver_tmp_path} latest_ver_path=${latest_ver_path}`, err);
if (!should_retry || retries <= 0) throw err;
Expand Down Expand Up @@ -2837,7 +2835,6 @@ class NamespaceFS {
*/
async _delete_single_object_versioned(fs_context, key, version_id) {
let retries = config.NSFS_RENAME_RETRIES;
const is_gpfs = native_fs_utils._is_gpfs(fs_context);
const latest_version_path = this._get_file_path({ key });
for (;;) {
let file_path;
Expand Down Expand Up @@ -2874,11 +2871,11 @@ class NamespaceFS {
// there are a few concurrency scenarios that might happen we should retry for -
// 1. the version id is the latest, concurrent put will might move the version id from being the latest to .versions/ -
// will throw safe unlink failed on non matching fd (on GPFS) or inode/mtime (on POSIX).
// 2. the version id is the second latest and stays under .versions/ - on concurrent delete of the latest,
// 2. the version id is the second latest and stays under .versions/ - on concurrent delete of the latest,
// the version id might move to be the latest and we will get ENOENT
// 3. concurrent delete of this version - will get ENOENT, doing a retry will return successfully
// 3. concurrent delete of this version - will get ENOENT, doing a retry will return successfully
// after we will see that the version was already deleted
if (retries <= 0 || !native_fs_utils.should_retry_link_unlink(is_gpfs, err)) throw err;
if (retries <= 0 || !native_fs_utils.should_retry_link_unlink(err)) throw err;
await P.delay(get_random_delay(config.NSFS_RANDOM_DELAY_BASE, 0, 50));
} finally {
if (gpfs_options) await this._close_files_gpfs(fs_context, gpfs_options.delete_version, undefined, true);
Expand Down Expand Up @@ -3053,7 +3050,7 @@ class NamespaceFS {
} catch (err) {
dbg.warn(`NamespaceFS._delete_latest_version error: retries=${retries} latest_ver_path=${latest_ver_path}`, err);
retries -= 1;
if (retries <= 0 || !native_fs_utils.should_retry_link_unlink(is_gpfs, err)) throw err;
if (retries <= 0 || !native_fs_utils.should_retry_link_unlink(err)) throw err;
await P.delay(get_random_delay(config.NSFS_RANDOM_DELAY_BASE, 0, 50));
} finally {
if (gpfs_options) await this._close_files_gpfs(fs_context, gpfs_options.delete_version, undefined, true);
Expand All @@ -3069,9 +3066,8 @@ class NamespaceFS {

// We can have only one versioned object with null version ID per key.
// It can be latest version, old version in .version/ directory or delete marker
// This function removes an object version or delete marker with a null version ID inside .version/ directory
// This function removes an object version or delete marker with a null version ID inside .version/ directory
async _delete_null_version_from_versions_directory(key, fs_context) {
const is_gpfs = native_fs_utils._is_gpfs(fs_context);
const null_versioned_path = this._get_version_path(key, NULL_VERSION_ID);
await this._check_path_in_bucket_boundaries(fs_context, null_versioned_path);
let gpfs_options;
Expand All @@ -3091,7 +3087,7 @@ class NamespaceFS {
} catch (err) {
dbg.warn(`NamespaceFS._delete_null_version_from_versions_directory error: retries=${retries} null_versioned_path=${null_versioned_path}`, err);
retries -= 1;
if (retries <= 0 || !native_fs_utils.should_retry_link_unlink(is_gpfs, err)) throw err;
if (retries <= 0 || !native_fs_utils.should_retry_link_unlink(err)) throw err;
await P.delay(get_random_delay(config.NSFS_RANDOM_DELAY_BASE, 0, 50));
} finally {
if (gpfs_options) await this._close_files_gpfs(fs_context, gpfs_options.delete_version, undefined, true);
Expand Down
6 changes: 3 additions & 3 deletions src/util/native_fs_utils.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -268,11 +268,11 @@ async function safe_unlink_gpfs(fs_context, to_delete_path, to_delete_file, dir_
}
}

function should_retry_link_unlink(is_gpfs, err) {
function should_retry_link_unlink(err) {
const should_retry_general = ['ENOENT', 'EEXIST', 'VERSION_MOVED', 'MISMATCH_VERSION'].includes(err.code);
const should_retry_gpfs = [gpfs_link_unlink_retry_err, gpfs_unlink_retry_catch].includes(err.code);
const should_retry_posix = [posix_link_retry_err, posix_unlink_retry_err].includes(err.message);
return should_retry_general || (is_gpfs ? should_retry_gpfs : should_retry_posix);
return should_retry_general || should_retry_gpfs || should_retry_posix;
}

////////////////////////
Expand Down Expand Up @@ -434,7 +434,7 @@ async function update_config_file(fs_context, schema_dir, config_path, config_da
break;
} catch (err) {
retries -= 1;
if (retries <= 0 || !should_retry_link_unlink(is_gpfs, err)) throw err;
if (retries <= 0 || !should_retry_link_unlink(err)) throw err;
dbg.warn(`native_fs_utils.update_config_file: Retrying failed move to dest retries=${retries}` +
` source_path=${open_path} dest_path=${config_path}`, err);
if (is_gpfs) {
Expand Down

0 comments on commit aca3c0b

Please sign in to comment.