Skip to content
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.

Sign up for GitHub

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

Jump to bottom

NSFS | fix copy_object issues #8355

Merged
merged 1 commit into from
Sep 17, 2024
Merged

NSFS | fix copy_object issues #8355

merged 1 commit into from
Sep 17, 2024

Conversation

nadavMiz
Copy link
Contributor

@nadavMiz nadavMiz commented Sep 12, 2024 •
edited
Loading

Explain the changes

  1. change _upload_stream to call read_object_stream directly during fallback copy instead of piping the stream to passthroughStream
  2. add content_type to multipart upload
  3. always copy xattr during copy object
  4. remove old copy for source xattr and change copy_object tests
  5. enable copy_object ceph_tests

Issues: Fixed #8315

Testing Instructions:

  1. run ceph_test: s3tests_boto3/functional/test_s3.py::test_object_copy_versioning_multipart_upload
  • Doc added/updated
  • Tests added

romayalon
romayalon reviewed Sep 12, 2024
View reviewed changes
src/sdk/namespace_fs.js Show resolved Hide resolved
src/sdk/namespace_fs.js Show resolved Hide resolved
@pull-request-size pull-request-size bot added size/M and removed size/S labels Sep 12, 2024
guymguym
guymguym requested changes Sep 12, 2024
View reviewed changes
src/sdk/namespace_fs.js Show resolved Hide resolved
src/sdk/object_sdk.js Show resolved Hide resolved
src/sdk/object_sdk.js Show resolved Hide resolved
src/test/system_tests/ceph_s3_tests/s3-tests-lists/nsfs_s3_tests_pending_list.txt Show resolved Hide resolved
src/sdk/namespace_fs.js Show resolved Hide resolved
@nadavMiz nadavMiz force-pushed the fix_copy_object branch 4 times, most recently from 04322a4 to 71c4943 Compare September 16, 2024 15:04
@pull-request-size pull-request-size bot added size/L and removed size/M labels Sep 16, 2024
guymguym
guymguym approved these changes Sep 16, 2024
View reviewed changes
Copy link
Member

@guymguym guymguym left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nadavMiz I think it looks OK, but due to the sensitivity I still pointed two things for @romayalon and @nimrod-becker to notice, so would be good to check with them

src/sdk/namespace_fs.js
@@ -1373,13 +1369,6 @@ class NamespaceFS {
return upload_info;
}

async _get_copy_source_xattr(params, fs_context, fs_xattr) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@romayalon can you please review this removal - this is based on the fact that we already handle this for all cases in object_sdk fix_copy_source_params (look for source_md.xattr).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have reviewed it with @romayalon seems like this was added by mistake

src/sdk/object_sdk.js
Comment on lines +684 to +689
if (target_ns instanceof NamespaceFS) {
params.source_ns = actual_source_ns;
params.source_params = source_params;
} else {
//this._populate_nsfs_copy_fallback({ source_params, source_ns, params });
throw new Error('TODO fix _populate_nsfs_copy_fallback');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nimrod-becker @romayalon please notice that in this PR we only fixed the copy between nsfs and another nsfs bucket, but copy between nsfs and other namespace types is throwing for now - it should be fixed in a followup PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have opened a follow up issue #8372

@nadavMiz nadavMiz mentioned this pull request Sep 17, 2024
Open
@nadavMiz
NSFS | fix copy_object issues
b744060 
Signed-off-by: nadav mizrahi <nadav.mizrahi16@gmail.com>
@nadavMiz nadavMiz merged commit fb9c96c into noobaa:master Sep 17, 2024
10 checks passed
@romayalon romayalon mentioned this pull request Sep 23, 2024
Merged
2 tasks
@romayalon romayalon mentioned this pull request Oct 7, 2024
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@romayalon romayalon romayalon approved these changes

@guymguym guymguym guymguym approved these changes

Assignees
No one assigned
Labels
size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

NSFS | S3 | Versioning: CopyObject after multipart upload of a versioned object leads to invalid data content
3 participants
@nadavMiz @guymguym @romayalon