From 1c5964a6f45f09dc31c6f356b8566aadd1750cbf Mon Sep 17 00:00:00 2001 From: shirady <57721533+shirady@users.noreply.github.com> Date: Wed, 19 Feb 2025 09:28:12 +0200 Subject: [PATCH] NSFS | Fix Bug | Race between list object and delete object during operation 1. Replace the function stat_ignore_eacces with stat_if_exists and add ignore of ENOENT and ENOTDIR (in addition to EACCES with a flag of config.EACCES_IGNORE_ENTRY). 2. Call stat on the entry_path one time and save is instead of calling it in concurrency so that we can be sure that the value was not deleted from the result array. Move the stat call before results.push(r) and results.splice(pos, 0, r) as they both add a result to the array. 3. Add a comment before the check_access, see also in PR description of Added check_access() call to list objects #6576. This change is continuing PR #8751 by adding a case to ignore a stat failure entry. Signed-off-by: shirady <57721533+shirady@users.noreply.github.com> --- config.js | 3 + src/native/fs/fs_napi.cpp | 5 + src/sdk/namespace_fs.js | 28 ++-- .../jest_tests/test_nsfs_concurrency.test.js | 124 ++++++++++++++++-- src/util/native_fs_utils.js | 36 +++-- 5 files changed, 160 insertions(+), 36 deletions(-) diff --git a/config.js b/config.js index 8b2fcea0ca..1755b978cd 100644 --- a/config.js +++ b/config.js @@ -913,6 +913,9 @@ config.ANONYMOUS_ACCOUNT_NAME = 'anonymous'; config.NFSF_UPLOAD_STREAM_MEM_THRESHOLD = 8 * 1024 * 1024; +// we want to change our handling related to EACCESS error +config.NSFS_LIST_IGNORE_ENTRY_ON_EACCES = true; + //////////////////////////// // NSFS NON CONTAINERIZED // //////////////////////////// diff --git a/src/native/fs/fs_napi.cpp b/src/native/fs/fs_napi.cpp index b5195eb5b8..d1d56d3ab6 100644 --- a/src/native/fs/fs_napi.cpp +++ b/src/native/fs/fs_napi.cpp @@ -752,6 +752,11 @@ struct FSWrapWorker : public FSWorker /** * Stat is an fs op + * + * Note: this stat operation contains the system call of open. + * Currently, we use it in list objects, but might want to create a different stat call + * (or add changes inside this) to avoid permission check during list objects + * while we stat each file (to avoid EACCES error) */ struct Stat : public FSWorker { diff --git a/src/sdk/namespace_fs.js b/src/sdk/namespace_fs.js index 9f812d19ee..54b9cca5aa 100644 --- a/src/sdk/namespace_fs.js +++ b/src/sdk/namespace_fs.js @@ -718,11 +718,17 @@ class NamespaceFS { if (!delimiter && r.common_prefix) { await process_dir(r.key); } else { - if (pos < results.length) { - results.splice(pos, 0, r); - } else { - const stat = await native_fs_utils.stat_ignore_eacces(this.bucket_path, r, fs_context); - if (stat) { + const entry_path = path.join(this.bucket_path, r.key); + // If entry is outside of bucket, returns stat of symbolic link + const use_lstat = !(await this._is_path_in_bucket_boundaries(fs_context, entry_path)); + const stat = await native_fs_utils.stat_if_exists(fs_context, entry_path, + use_lstat, config.NSFS_LIST_IGNORE_ENTRY_ON_EACCES); + if (stat) { + r.stat = stat; + // add the result only if we have the stat information + if (pos < results.length) { + results.splice(pos, 0, r); + } else { results.push(r); } } @@ -764,6 +770,11 @@ class NamespaceFS { await insert_entry_to_results_arr(r); }; + // our current mechanism - list the files and skipping inaccessible directory (invisible in the list). + // We use this check_access in case the directory is not accessible inside a bucket. + // In a directory if we don’t have access to the directory, we want to skip the directory and its sub directories from the list. + // We did it outside to avoid undefined values in the cache. + // Note: It is not the same case as a file without permission. if (!(await this.check_access(fs_context, dir_path))) return; try { if (list_versions) { @@ -885,13 +896,6 @@ class NamespaceFS { const prefix_dir_key = prefix.slice(0, prefix.lastIndexOf('/') + 1); await process_dir(prefix_dir_key); - await Promise.all(results.map(async r => { - if (r.common_prefix) return; - const entry_path = path.join(this.bucket_path, r.key); - //If entry is outside of bucket, returns stat of symbolic link - const use_lstat = !(await this._is_path_in_bucket_boundaries(fs_context, entry_path)); - r.stat = await nb_native().fs.stat(fs_context, entry_path, { use_lstat }); - })); const res = { objects: [], common_prefixes: [], diff --git a/src/test/unit_tests/jest_tests/test_nsfs_concurrency.test.js b/src/test/unit_tests/jest_tests/test_nsfs_concurrency.test.js index b631a13968..b1ccfca1df 100644 --- a/src/test/unit_tests/jest_tests/test_nsfs_concurrency.test.js +++ b/src/test/unit_tests/jest_tests/test_nsfs_concurrency.test.js @@ -6,7 +6,7 @@ const P = require('../../../util/promise'); const fs_utils = require('../../../util/fs_utils'); const NamespaceFS = require('../../../sdk/namespace_fs'); const buffer_utils = require('../../../util/buffer_utils'); -const { TMP_PATH } = require('../../system_tests/test_utils'); +const { TMP_PATH, TEST_TIMEOUT } = require('../../system_tests/test_utils'); const { crypto_random_string } = require('../../../util/string_utils'); const endpoint_stats_collector = require('../../../sdk/endpoint_stats_collector'); @@ -27,18 +27,20 @@ function make_dummy_object_sdk(nsfs_config, uid, gid) { } const DUMMY_OBJECT_SDK = make_dummy_object_sdk(true); + +const tmp_fs_path = path.join(TMP_PATH, 'test_nsfs_concurrency'); + +const nsfs = new NamespaceFS({ + bucket_path: tmp_fs_path, + bucket_id: '1', + namespace_resource_id: undefined, + access_mode: undefined, + versioning: 'DISABLED', + force_md5_etag: false, + stats: endpoint_stats_collector.instance(), +}); + describe('test nsfs concurrency', () => { - const tmp_fs_path = path.join(TMP_PATH, 'test_nsfs_concurrency'); - - const nsfs = new NamespaceFS({ - bucket_path: tmp_fs_path, - bucket_id: '1', - namespace_resource_id: undefined, - access_mode: undefined, - versioning: 'DISABLED', - force_md5_etag: false, - stats: endpoint_stats_collector.instance(), - }); beforeEach(async () => { await fs_utils.create_fresh_path(tmp_fs_path); @@ -68,5 +70,101 @@ describe('test nsfs concurrency', () => { } await P.delay(5000); expect(res_etags).toHaveLength(15); - }, 6000); + }, TEST_TIMEOUT); + + test_list_and_delete({ + test_name: 'list objects and delete an object during it - random deletion', + bucket_name: 'bucket1', + num_of_objects_to_upload: 5, + expected_num_of_object_in_list: 4, + key_to_delete: `my-key-${random_integer(1, 5)}`, + iterations: 5, + }); + + test_list_and_delete({ + test_name: 'list objects and delete an object during it - delete the last object', + bucket_name: 'bucket2', + num_of_objects_to_upload: 1000, + expected_num_of_object_in_list: 999, + key_to_delete: `my-key-1000`, + iterations: 1 + }); }); + + /** + * @param {{ + * test_name: string, + * bucket_name: string, + * num_of_objects_to_upload: number, + * expected_num_of_object_in_list: number, + * key_to_delete?: string, + * iterations?: number, + * }} params + */ + function test_list_and_delete({ + test_name, + bucket_name, + num_of_objects_to_upload, + expected_num_of_object_in_list, + key_to_delete, + iterations = 1 + }) { + + it(test_name, async () => { + await _upload_objects(bucket_name, num_of_objects_to_upload); + if (key_to_delete === undefined) { + key_to_delete = `my-key-${random_integer(1, expected_num_of_object_in_list)}`; + console.log('test_list_and_delete: key_to_delete', key_to_delete); + } + + console.log(`test_list_and_delete: ${test_name} ${bucket_name} num_of_objects_to_upload: ${num_of_objects_to_upload}, + expected_num_of_object_in_list ${expected_num_of_object_in_list} key_to_delete ${key_to_delete}`); + + for (let i = 0; i < iterations; ++i) { + nsfs.list_objects({ bucket: bucket_name }, DUMMY_OBJECT_SDK) + .catch(err => { + console.log('error during list_objects', err); + throw err; + }).then(res => { + console.log('list was successful'); + }); + nsfs.delete_object({ bucket: bucket_name, key: key_to_delete }, DUMMY_OBJECT_SDK) + .catch(err => { + console.log('delete_object got an error', err); + throw err; + }).then(res => { + console.log('delete_object during list objects was successful'); + }); + await P.delay(5000); + // up to this point if it was successful, the race between the delete object and list object went fine. + } + }, TEST_TIMEOUT); +} + +/** + * _upload_objects uploads number_of_versions of objects in bucket + * note: this function is not concurrent, it's a helper function for preparing a bucket with a couple of objects + * @param {string} bucket + * @param {number} number_of_objects + */ +async function _upload_objects(bucket, number_of_objects) { + const keys_names = []; + for (let i = 0; i < number_of_objects; i++) { + const key_name = `my-key-${i + 1}`; + const random_data = Buffer.from(String(crypto_random_string(7))); + const body = buffer_utils.buffer_to_read_stream(random_data); + await nsfs.upload_object({ bucket: bucket, key: key_name, source_stream: body }, DUMMY_OBJECT_SDK); + keys_names.push(key_name); + } + return keys_names; +} + +/** + * randomInteger between min (included) and max (included) + * // copied from: https://stackoverflow.com/questions/4959975/generate-random-number-between-two-numbers-in-javascript + * @param {number} min + * @param {number} max + */ +function random_integer(min, max) { + return Math.floor(Math.random() * (max - min + 1)) + min; + } diff --git a/src/util/native_fs_utils.js b/src/util/native_fs_utils.js index aa4733ffb9..155f2d446e 100644 --- a/src/util/native_fs_utils.js +++ b/src/util/native_fs_utils.js @@ -303,6 +303,30 @@ async function stat_ignore_enoent(fs_context, file_path) { } } +/** + * stat_if_exists execute stat on entry_path and ignores on certain error codes. + * @param {nb.NativeFSContext} fs_context + * @param {string} entry_path + * @param {boolean} use_lstat + * @param {boolean} should_ignore_eacces + * @returns {Promise} + */ +async function stat_if_exists(fs_context, entry_path, use_lstat, should_ignore_eacces) { + try { + return await nb_native().fs.stat(fs_context, entry_path, { use_lstat }); + } catch (err) { + // we might want to expand the error list due to permission/structure + // change (for example: ELOOP, ENAMETOOLONG) or other reason (EPERM) - need to be decided + if ((err.code === 'EACCES' && should_ignore_eacces) || + err.code === 'ENOENT' || err.code === 'ENOTDIR') { + dbg.log0('stat_if_exists: Could not access file entry_path', + entry_path, 'error code', err.code, ', skipping...'); + } else { + throw err; + } + } +} + //////////////////////// /// NON CONTAINERIZED // //////////////////////// @@ -688,16 +712,6 @@ function translate_error_codes(err, entity) { return err; } -async function stat_ignore_eacces(bucket_path, result, fs_context) { - const entry_path = path.join(bucket_path, result.key); - try { - return await nb_native().fs.stat(fs_context, entry_path); - } catch (err) { - if (err.code !== 'EACCES') throw err; - dbg.log0('NamespaceFS: check_stats_for_object : couldnt access file entry_path', entry_path, ", skipping..."); - } -} - exports.get_umasked_mode = get_umasked_mode; exports._make_path_dirs = _make_path_dirs; exports._create_path = _create_path; @@ -708,6 +722,7 @@ exports.finally_close_files = finally_close_files; exports.get_user_by_distinguished_name = get_user_by_distinguished_name; exports.get_config_files_tmpdir = get_config_files_tmpdir; exports.stat_ignore_enoent = stat_ignore_enoent; +exports.stat_if_exists = stat_if_exists; exports._is_gpfs = _is_gpfs; exports.safe_move = safe_move; @@ -739,4 +754,3 @@ exports.get_bucket_tmpdir_full_path = get_bucket_tmpdir_full_path; exports.get_bucket_tmpdir_name = get_bucket_tmpdir_name; exports.entity_enum = entity_enum; exports.translate_error_codes = translate_error_codes; -exports.stat_ignore_eacces = stat_ignore_eacces;