-
Notifications
You must be signed in to change notification settings - Fork 80
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
NSFS | Fix Bug | Race Between List Object and Delete Object #8809
Merged
+160
−36
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
naveenpaul1
reviewed
Feb 19, 2025
nadavMiz
reviewed
Feb 19, 2025
nadavMiz
reviewed
Feb 19, 2025
romayalon
reviewed
Feb 20, 2025
5463962
to
4ff81c4
Compare
ddd6985
to
8ce3b9e
Compare
8ce3b9e
to
3f35b9f
Compare
romayalon
approved these changes
Mar 4, 2025
d444e64
to
6ca05a4
Compare
2 tasks
ac3aef0
to
1c5964a
Compare
guymguym
approved these changes
Mar 6, 2025
…eration 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 noobaa#6576. This change is continuing PR noobaa#8751 by adding a case to ignore a stat failure entry. Signed-off-by: shirady <[email protected]>
1c5964a
to
fe4115c
Compare
Backport comment - We would like the backport to be with |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Explain the changes
stat_ignore_eacces
withstat_if_exists
and add ignore ofENOENT
andENOTDIR
(in addition toEACCES
with a flag ofconfig.EACCES_IGNORE_ENTRY
).stat
on theentry_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 thestat
call beforeresults.push(r)
andresults.splice(pos, 0, r)
as they both add a result to the array.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.Issues: Fixed DFBUGS-1582
entry_path
(which is the path of the key in the bucket in the FS). If a concurrent delete happens, thisstat
will return an error (ENOENT
as the object was deleted). In this fix, we suggest that thestat
would not return any value and, as a result, would not appear in the list object result.GAPs - are mentioned in issue #8845 (discussions were documented here).
Testing Instructions:
Automatic Test:
Please run:
sudo npx jest test_nsfs_concurrency.test.js
For example we could see the lines:
Manual Testing instructions:
We will add code changes to simulate the problem:
Add the sleep function:
we will add the following lines:
sudo node src/cmd/manage_nsfs account add --name <account-name> --new_buckets_path /Users/buckets/ --access_key <access-key> --secret_key <secret-key> --uid <uid> --gid <gid>
Note: before creating the account need to give permission to the
new_buckets_path
:chmod 777 /Users/buckets/
sudo node src/cmd/nsfs --debug 5
alias nc-user-1-s3=‘AWS_ACCESS_KEY_ID=<access-key> AWS_SECRET_ACCESS_KEY=<secret-key> aws --no-verify-ssl --endpoint-url https://localhost:6443’
nc-user-1-s3 s3 ls; echo $?
nc-user-1-s3 s3 mb s3://bucket-race
(bucket-race
is the bucket name in this example)echo 'hello_world' | nc-user-1-s3 s3 cp - s3://bucket-race/hello_world1.txt
(repeat this and change the key name tohello_world2.txt
,hello_world3.txt
,hello_world4.txt
nc-user-1-s3 s3api list-objects-v2 --bucket bucket-race
Tab2: Delete an object while you see the "SDSD before stat" on the key:
nc-user-1-s3 s3api delete-object --bucket bucket-race --key hello_world3.txt
(for example)Expect to see the list without the deleted key, and also to see in the logs printings.
Before the change
It was:
An error occurred (NoSuchKey) when calling the ListObjectsV2 operation: The specified key does not exist.
and in the logs:
Logs before the change