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.

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 copy_object issues #8355

Merged
merged 1 commit into from
Sep 17, 2024
Merged

Conversation

nadavMiz
Copy link
Contributor

@nadavMiz nadavMiz commented Sep 12, 2024

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

src/sdk/namespace_fs.js Show resolved Hide resolved
src/sdk/namespace_fs.js Show resolved Hide resolved
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/sdk/namespace_fs.js Show resolved Hide resolved
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

@@ -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

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

Signed-off-by: nadav mizrahi <[email protected]>
@nadavMiz nadavMiz merged commit fb9c96c into noobaa:master Sep 17, 2024
10 checks passed
@romayalon romayalon mentioned this pull request Oct 7, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
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