RGW: pubsub publish commit with etag populated by AliMasarweh · Pull Request #54569 · ceph/ceph

AliMasarweh · 2023-11-20T11:11:31Z

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

Contribution Guidelines

To sign and title your commits, please refer to Submitting Patches to Ceph.
If you are submitting a fix for a stable branch (e.g. "quincy"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.
When filling out the below checklist, you may click boxes directly in the GitHub web UI. When entering or editing the entire PR message in the GitHub web UI editor, you may also select a checklist item by adding an x between the brackets: [x]. Spaces and capitalization matter when checking off items this way.

Checklist

Tracker (select at least one)
- References tracker ticket
- Very recent bug; references commit where it was introduced
- New feature (ticket optional)
- Doc update (no ticket needed)
- Code cleanup (no ticket needed)
Component impact
- Affects Dashboard, opened tracker ticket
- Affects Orchestrator, opened tracker ticket
- No impact that needs to be tracked
Documentation (select at least one)
- Updates relevant documentation
- No doc update is appropriate
Tests (select at least one)
- Includes unit test(s)
- Includes integration test(s)
- Includes bug reproducer
- No tests

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
jenkins test rook e2e

src/rgw/rgw_op.h

yuvalif · 2023-11-20T11:26:01Z

can you please add a test for etag in "MPU complete" to the test suite?

yuvalif · 2023-11-20T11:28:47Z

please add:

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

to the commit message

src/rgw/rgw_op.cc

AliMasarweh · 2023-11-28T14:02:46Z

I think we still have an issue with etag not being populated in the main branch
So I need to fix the original issue before proceeding to work on the test

src/rgw/rgw_op.cc

yuvalif · 2023-11-30T09:13:21Z

please test against the rgw suite, as there are changes here unrelated to notifications

src/rgw/rgw_op.cc

yuvalif · 2023-12-04T14:57:53Z

src/rgw/driver/rados/rgw_sal_rados.cc

  uint64_t min_part_size = cct->_conf->rgw_multipart_min_part_size;
  auto etags_iter = part_etags.begin();
-  rgw::sal::Attrs attrs = target_obj->get_attrs();
+  rgw::sal::Attrs& attrs = target_obj->get_attrs();


@cbodley can you please see if this change has other implication?

it's hard to tell. are we mutating the attrs? if not these should be const-refs instead

yes we are mutating the attrs at line 2544:

attrs[RGW_ATTR_ETAG] = etag_bl; if (compressed) { // write compression attribute to full object bufferlist tmp; encode(cs_info, tmp); attrs[RGW_ATTR_COMPRESSION] = tmp; }

AliMasarweh · 2023-12-05T11:29:59Z

this looks like a successful teuthology run:
https://pulpito.ceph.com/alimasa-2023-12-04_15:28:22-rgw-wip-alimasa-multi-pubsub-etag-distro-default-smithi/

AliMasarweh · 2023-12-05T11:30:19Z

jenkins test windows

yuvalif · 2023-12-05T15:05:17Z

this looks like a successful teuthology run: https://pulpito.ceph.com/alimasa-2023-12-04_15:28:22-rgw-wip-alimasa-multi-pubsub-etag-distro-default-smithi/

some failures are expected, but there is a crash there in one of the tests:
http://qa-proxy.ceph.com/teuthology/alimasa-2023-12-04_15:28:22-rgw-wip-alimasa-multi-pubsub-etag-distro-default-smithi/7477167/remote/smithi104/crash/posted/2023-12-04T15%3A53%3A44.284483Z_8e51245f-1feb-4d38-b2f3-0c816e6036ce/

Signed-off-by: Ali Masarwa <[email protected]>

AliMasarweh · 2023-12-16T09:14:51Z

I think this is a successful teuthology run:
https://pulpito.ceph.com/alimasa-2023-12-15_02:48:26-rgw-wip-alimasa-multi-pubsub-etag-distro-default-smithi/

3 failures unrelated to my change:

rgw multisite test failures 
Command failed on smithi088 with status 100: 'DEBIAN_FRONTEND=noninteractive sudo -E apt-get -y --force-yes install python-dev' 
rgw multisite test failures

can I merge?

yuvalif · 2023-12-17T08:34:15Z

I think this is a successful teuthology run: https://pulpito.ceph.com/alimasa-2023-12-15_02:48:26-rgw-wip-alimasa-multi-pubsub-etag-distro-default-smithi/

3 failures unrelated to my change:
rgw multisite test failures 
Command failed on smithi088 with status 100: 'DEBIAN_FRONTEND=noninteractive sudo -E apt-get -y --force-yes install python-dev' 
rgw multisite test failures 
can I merge?

yes. please merge once "make check" passes

AliMasarweh · 2023-12-17T08:58:21Z

jenkins test make check

AliMasarweh · 2023-12-17T08:58:27Z

jenkins test make check arm64

This version refreshes object attrs after upload completes, which seems to be the smallest available fix. Appears to return the correct value for ETag. For 7.1, we could replace this fix with a validated backport of (at least) ceph/ceph#54569. This is a more intrusive change and original author should do the backport. Resolves: rhbz#2266579 Signed-off-by: matt benjamin <[email protected]>

cbodley · 2024-05-03T12:57:10Z

src/rgw/rgw_op.cc

-  // remove the upload meta object ; the meta object is not versioned
-  // when the bucket is, as that would add an unneeded delete marker
-  int r = meta_obj->delete_object(this, y, true /* prevent versioning */);


before this moved to RGWCompleteMultipart::complete(), it was only being called on success. but complete() is called on errors too, so the change caused a regression in https://tracker.ceph.com/issues/65746

cbodley · 2024-05-03T12:59:35Z

src/rgw/rgw_op.cc

+  int r = serializer->unlock();
+  if (r < 0) {


this unlock() call wasn't here before, and i'm not sure why it was added. in the successful path where we call meta_obj->delete_object(), we shouldn't do a separate unlock() because delete_object() deletes the lock implicitly (that's why it uses serializer->clear_locked())

cbodley · 2024-05-03T13:16:14Z

src/rgw/rgw_op.cc

  // make reservation for notification if needed
-  std::unique_ptr<rgw::sal::Notification> res
-    = driver->get_notification(meta_obj.get(), nullptr, s, rgw::notify::ObjectCreatedCompleteMultipartUpload, y, &s->object->get_name());
+  res = driver->get_notification(meta_obj.get(), nullptr, s, rgw::notify::ObjectCreatedCompleteMultipartUpload, y,


it seems wrong for the Notification to point at the meta_obj rather than the target_obj. that's probably why you were seeing lifetime issues around it?

cbodley · 2024-05-03T15:43:50Z

@AliMasarweh @yuvalif please see fix/cleanup in #57257

This version refreshes object attrs after upload completes, which seems to be the smallest available fix. Appears to return the correct value for ETag. For 7.1, we could replace this fix with a validated backport of (at least) ceph#54569. This is a more intrusive change and original author should do the backport. Resolves: rhbz#2266579 Resolves: rhbz#2271117 Signed-off-by: matt benjamin <[email protected]>

AliMasarweh requested a review from a team as a code owner November 20, 2023 11:11

github-actions bot added the rgw label Nov 20, 2023

yuvalif reviewed Nov 20, 2023

View reviewed changes

src/rgw/rgw_op.h Outdated Show resolved Hide resolved

AliMasarweh force-pushed the wip-alimasa-multi-pubsub-etag branch from c274296 to a22bb01 Compare November 21, 2023 12:26

yuvalif approved these changes Nov 21, 2023

View reviewed changes

yuvalif added needs-qa needs-test labels Nov 22, 2023

yuvalif reviewed Nov 23, 2023

View reviewed changes

src/rgw/rgw_op.cc Show resolved Hide resolved

cbodley removed the needs-qa label Nov 23, 2023

AliMasarweh force-pushed the wip-alimasa-multi-pubsub-etag branch from a22bb01 to a7d40a2 Compare November 28, 2023 13:59

AliMasarweh force-pushed the wip-alimasa-multi-pubsub-etag branch from a7d40a2 to 2761671 Compare November 29, 2023 12:52

yuvalif reviewed Nov 30, 2023

View reviewed changes

src/rgw/rgw_op.cc Outdated Show resolved Hide resolved

yuvalif added the needs-qa label Nov 30, 2023

yuvalif requested a review from cbodley November 30, 2023 13:56

AliMasarweh force-pushed the wip-alimasa-multi-pubsub-etag branch from 2761671 to 81187ea Compare December 4, 2023 08:54

github-actions bot added the tests label Dec 4, 2023

AliMasarweh force-pushed the wip-alimasa-multi-pubsub-etag branch from 81187ea to 437f699 Compare December 4, 2023 11:01

yuvalif reviewed Dec 4, 2023

View reviewed changes

src/rgw/rgw_op.cc Outdated Show resolved Hide resolved

AliMasarweh force-pushed the wip-alimasa-multi-pubsub-etag branch 2 times, most recently from 1564aa2 to 4e7f375 Compare December 4, 2023 12:58

yuvalif reviewed Dec 4, 2023

View reviewed changes

AliMasarweh force-pushed the wip-alimasa-multi-pubsub-etag branch 2 times, most recently from c45aa3e to 5900adb Compare December 11, 2023 12:51

AliMasarweh force-pushed the wip-alimasa-multi-pubsub-etag branch from 5900adb to 66ba85c Compare December 11, 2023 12:52

RGW:pubsub publish commit with etag populated

18c202c

Signed-off-by: Ali Masarwa <[email protected]>

AliMasarweh force-pushed the wip-alimasa-multi-pubsub-etag branch from 66ba85c to 18c202c Compare December 14, 2023 09:49

yuvalif merged commit 02b3d4c into ceph:main Dec 17, 2023

yuvalif mentioned this pull request Mar 25, 2024

reef: RGW: pubsub publish commit with etag populated #56453

Merged

cbodley reviewed May 3, 2024

View reviewed changes

cbodley mentioned this pull request May 3, 2024

rgw: fix CompleteMultipart error handling regression #57257

Merged

Comments

Conversation

AliMasarweh commented Nov 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution Guidelines

Checklist

Uh oh!

Uh oh!

yuvalif commented Nov 20, 2023

Uh oh!

yuvalif commented Nov 20, 2023

Uh oh!

Uh oh!

AliMasarweh commented Nov 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

yuvalif commented Nov 30, 2023

Uh oh!

Uh oh!

yuvalif Dec 4, 2023

Choose a reason for hiding this comment

Uh oh!

cbodley Dec 5, 2023

Choose a reason for hiding this comment

Uh oh!

AliMasarweh Dec 6, 2023

Choose a reason for hiding this comment

Uh oh!

AliMasarweh commented Dec 5, 2023

Uh oh!

AliMasarweh commented Dec 5, 2023

Uh oh!

yuvalif commented Dec 5, 2023

Uh oh!

AliMasarweh commented Dec 16, 2023

Uh oh!

yuvalif commented Dec 17, 2023

Uh oh!

AliMasarweh commented Dec 17, 2023

Uh oh!

AliMasarweh commented Dec 17, 2023

Uh oh!

cbodley May 3, 2024

Choose a reason for hiding this comment

Uh oh!

cbodley May 3, 2024

Choose a reason for hiding this comment

Uh oh!

cbodley May 3, 2024

Choose a reason for hiding this comment

Uh oh!

cbodley commented May 3, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

AliMasarweh commented Nov 20, 2023 •

edited

Loading

AliMasarweh commented Nov 28, 2023 •

edited

Loading