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

rgw: store torrent file for GetObjectTorrent in object attrs instead of omap #50168

Merged
merged 10 commits into from Mar 11, 2023
Merged

rgw: store torrent file for GetObjectTorrent in object attrs instead of omap #50168

merged 10 commits into from Mar 11, 2023

Conversation

cbodley
Copy link
Contributor

@cbodley cbodley commented Feb 19, 2023 •
edited

when rgw_torrent_flag is enabled, new object uploads store the torrent in RGW_ATTR_TORRENT instead of the head object's omap. omap doesn't work for EC pools, and required a separate (non-atomic) write after the head object was written

Fixes: https://tracker.ceph.com/issues/23838

a new get_torrent_info() function was added to rgw::sal::Object for this, with a default implementation in rgw::sal::StoreObject that reads directly from attrs. for backward compatibility, RadosObject will fall back to omap if StoreObject doesn't find RGW_ATTR_TORRENT

this reimplements class seed as a rgw::putobj::Pipe for RGWPutObj and as a free function rgw_read_torrent_file() for RGWGetObj, then cleans up the header dependencies in general

a new rgw_torrent_max_size option was added, with the same 5G default that aws uses, to prevent RGWPutObj_Torrent from buffering too many sha hashes

the RGWPutObj::get_torrent_filter() function requires RGWPutObj_Torrent to be movable, but its ceph::crypto::SHA1 member crashed after move. that's why the commit crypto: enable move construct/assign for OpenSSLDigest was necessary

for testing, the rgw/verify suite enables rgw_torrent_flag by default. a test case for GetObjectTorrent was added in ceph/s3-tests#495

my real motivation for this project was to finally get the rados-specific omap APIs out of sal::Object

Show available Jenkins commands
  • jenkins retest this please
  • jenkins test classic perf
  • jenkins test crimson perf
  • jenkins test signed
  • jenkins test make check
  • jenkins test make check arm64
  • jenkins test submodules
  • jenkins test dashboard
  • jenkins test dashboard cephadm
  • jenkins test api
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox
  • jenkins test windows

@cbodley cbodley requested a review from a team as a code owner February 19, 2023 17:41
@cbodley
Copy link
Contributor Author

cbodley commented Feb 19, 2023

cc @robbat2 since you were involved with torrent issues in the past

@cbodley cbodley force-pushed the wip-23838 branch 2 times, most recently from 5d5795b to 4364c9c Compare February 19, 2023 23:20
@github-actions github-actions bot added the tests label Feb 19, 2023
mattbenjamin
mattbenjamin reviewed Feb 20, 2023
View reviewed changes
src/common/ceph_crypto.cc
@@ -203,6 +204,21 @@ ssl::OpenSSLDigest::~OpenSSLDigest() {
}
}

ssl::OpenSSLDigest::OpenSSLDigest(OpenSSLDigest&& o) noexcept
Copy link
Contributor

Choose a reason for hiding this comment

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

you fixed a real howler here, well done

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'm hoping that nobody's tried copying/moving these before

dang
dang approved these changes Feb 20, 2023
View reviewed changes
Copy link
Contributor

@dang dang left a comment

Choose a reason for hiding this comment

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

Nice cleanup.

src/rgw/driver/rados/rgw_sal_rados.cc
return -ENOENT;
}
bl = std::move(result.begin()->second);
return 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this store it in the attribute to be future proof? Or are we just going to leave it in the omap for old object?

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 think we'll have to support this omap fallback forever, even if we do add conversion logic here. so i don't think it's worth paying for an object write to convert stuff during GetObj

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, sounds good.

@cbodley
rgw/torrent: bencoder uses string_view
53d2597 
Signed-off-by: Casey Bodley <cbodley@redhat.com>
@cbodley
rgw/torrent: add a PutObj filter for torrent info
f4bd49a 
Signed-off-by: Casey Bodley <cbodley@redhat.com>
@cbodley
crypto: enable move construct/assign for OpenSSLDigest
9fb9295 
copy and move operations were not deleted, so the compiler-generated
move operations were implemented as copies. the destructor of the
copied-from instance would destroy the handles that were still in use
by the copied-to instance, resulting in use-after-free or double-free

to prevent this, add noexcept move operations that leave the moved-from
instance empty. this also prevents the compiler from generating the
problematic copy operations

Signed-off-by: Casey Bodley <cbodley@redhat.com>
@cbodley
rgw/torrent: PutObj uses the filter to write RGW_ATTR_TORRENT
a89bb54 
Signed-off-by: Casey Bodley <cbodley@redhat.com>
@cbodley
rgw/sal: add Object::get_torrent_info()
2419880 
although torrent info is moving into RGW_ATTR_TORRENT, we'll still need
to support the rados backend's omap fallback.. forever. expose a new
Object API specifically for torrent info so we can remove all of the
omap-specific ones

Signed-off-by: Casey Bodley <cbodley@redhat.com>
@cbodley
rgw: add free function rgw_read_torrent_file() for GetObj
adbf1ea 
Signed-off-by: Casey Bodley <cbodley@redhat.com>
@cbodley
rgw: rgw_op.h doesn't include rgw_torrent.h
8c84c67 
Signed-off-by: Casey Bodley <cbodley@redhat.com>
@cbodley
rgw/torrent: remove unused 'class seed' and move stuff to source
174207b 
Signed-off-by: Casey Bodley <cbodley@redhat.com>
@cbodley
qa/rgw: verify suite enables torrents
ad3759a 
Signed-off-by: Casey Bodley <cbodley@redhat.com>
@github-actions
Copy link

github-actions bot commented Mar 2, 2023

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

@cbodley
Copy link
Contributor Author

cbodley commented Mar 10, 2023

this is causing test failures in qa/workunits/rgw/test_rgw_datacache.py because the bencoded torrent xattr in radosgw-admin object stat can't be decoded as utf-8

@cbodley cbodley removed the needs-qa label Mar 10, 2023
@cbodley
rgw/admin: hide bencoded torrent data from 'object stat'
8b637c4 
Signed-off-by: Casey Bodley <cbodley@redhat.com>
@cbodley
Copy link
Contributor Author

cbodley commented Mar 11, 2023

passed qa in https://pulpito.ceph.com/cbodley-2023-03-10_19:16:19-rgw-wip-cbodley-testing-distro-default-smithi/

@cbodley cbodley merged commit f6a49ae into ceph:main Mar 11, 2023
3 checks passed
@cbodley cbodley deleted the wip-23838 branch March 11, 2023 20:09
@cbodley cbodley mentioned this pull request Mar 14, 2023
Merged
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dang dang dang approved these changes

@mattbenjamin mattbenjamin mattbenjamin left review comments

Assignees
No one assigned
Labels
bug-fix cleanup common needs-qa rgw tests wip-cbodley-testing
Projects
None yet
Milestone
No milestone
3 participants
@cbodley @dang @mattbenjamin