rgw/lifecycle-notification: Do not block lc processing for notification errors. #57079

kchheda3 · 2024-04-24T16:28:26Z

Fixes --> https://tracker.ceph.com/issues/66018
Currently if there is any error while calling publish_reserve the lc processing is cancelled for that object. This is different from behavior we have for replication events where the notification errors are not blocking replication. On similar note, lc being internal ceph processing,
do not block lc if there are notification error's.

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/driver/rados/rgw_cr_rados.cc

yuvalif · 2024-05-09T10:23:12Z

src/rgw/rgw_lc.cc

 static std::string lc_id = "rgw lifecycle";
 static std::string lc_req_id = "0";

+static void may_be_send_notification(const DoutPrefixProvider *dpp,


nit: I think we can change to send_notification() sending is not conditional, just could fail.
the fact that it does not return the error code mean that the failure should be ignored.

cbodley · 2024-05-13T17:50:13Z

merge conflict from #57356. also note that #57377 is hiding RGWObjState altogether. please just pass the etag to send_notification()

kchheda3 · 2024-05-13T18:53:53Z

merge conflict from #57356. also note that #57377 is hiding RGWObjState altogether. please just pass the etag to send_notification()

done

soumyakoduri · 2024-05-14T10:59:44Z

src/rgw/rgw_lc.cc

-    if (publish_ret < 0) {
-      ldpp_dout(dpp, 5) << "WARNING: notify publish_commit failed, with error: " << publish_ret << dendl;
-    }
+    send_notification(dpp, driver, obj.get(), oc.bucket, etag, obj_state->size,


As mentioned by Casey in #57356 (comment), obj_state may not be valid post transition/deletion., esp. after cloud transition which can reset obj_size to '0' or delete the object.

Similar to etag, we would need to read obj_state->size before any LC operation.

obj_state->size

@soumyakoduri is obj_state->size an attribute as well like etag ?

obj_state->size

@soumyakoduri is obj_state->size an attribute as well like etag ?

done, started using the obj->get_obj_size(), and verified the size is populated.

soumyakoduri · 2024-05-14T16:38:43Z

src/rgw/rgw_lc.cc

+    return;
+  }
+  ret =
+      notify->publish_commit(dpp, obj->get_obj_size(), ceph::real_clock::now(),


Probably this will get replaced with new SAL APIs to fetch right obj_state & size from the backend store.

Are there any test-cases to check notifications with LC expiration & cloud-transition? Post transition to cloud, either the object's HEAD is retained with data erased (so the size will be '0' bytes) or it is deleted (same as the case of LC expiration), in which in case get_obj_size() may return ENOENT if that call is passed to backend store.

Request @cbodley @dang to comment.

I have not made any changes as part of this PR to use sal object, I have just reorganised the code
If a future PR is gonna change this behaviour then should this PR be blocked for that ?

cc @yuvalif

the bucket notification tests do include some coverage for lifecycle, but not cloud transition:
https://github.com/ceph/ceph/blob/a052683/src/test/rgw/bucket_notification/test_bn.py#L1720-L1913

the rgw/notifications teuthology suite is what runs this

in which in case get_obj_size() may return ENOENT if that call is passed to backend store.

similar to Object::get_attrs(), get_obj_size() is just returning the cached object state, so shouldn't ever go to the backend. some mutations may update that cached state, but i don't think delete_obj() does. callers probably shouldn't expect getters like get_obj_size() to return sensible answers after deletion

in which in case get_obj_size() may return ENOENT if that call is passed to backend store.

similar to Object::get_attrs(), get_obj_size() is just returning the cached object state, so shouldn't ever go to the backend. some mutations may update that cached state, but i don't think delete_obj() does. callers probably shouldn't expect getters like get_obj_size() to return sensible answers after deletion

@cbodley so you suggesting, instead of calling the get_obj_size after the object is removed (lc'd), just get the size prior of deletion ?

like was done for etag, yeah

soumyakoduri

Rest all looks good to me.

src/rgw/rgw_lc.cc

kchheda3 · 2024-06-04T15:54:45Z

having trouble getting a clean qa run: https://pulpito.ceph.com/cbodley-2024-05-29_22:01:03-rgw-wip-cbodley-testing-distro-default-smithi/

lots of valgrind errors which are showing up on other branches too, and a test_d4n.sh failure that didn't go away with a --rerun

valgrind errors are related to get_monmap_and_config() and the single test_d4n failure is due to failed to remove '/home/ubuntu/cephtest': Directory not empty
so the failures are unrelated to the changes ?

cbodley · 2024-06-04T15:59:39Z

valgrind errors are related to get_monmap_and_config() and the single test_d4n failure is due to failed to remove '/home/ubuntu/cephtest': Directory not empty so the failures are unrelated to the changes ?

there were other prs in the test batch so it's hard to tell whether the d4n failure is related:

2024-05-29T22:24:47.627 INFO:tasks.workunit.client.0.smithi107.stdout:[==========] Running 11 tests from 2 test suites.
2024-05-29T22:24:47.627 INFO:tasks.workunit.client.0.smithi107.stdout:[----------] Global test environment set-up.
2024-05-29T22:24:47.629 INFO:tasks.workunit.client.0.smithi107.stdout:[----------] 5 tests from ObjectDirectoryFixture
2024-05-29T22:24:47.629 INFO:tasks.workunit.client.0.smithi107.stdout:[ RUN      ] ObjectDirectoryFixture.SetYield
2024-05-29T22:24:47.657 INFO:tasks.workunit.client.0.smithi107.stdout:[       OK ] ObjectDirectoryFixture.SetYield (28 ms)
2024-05-29T22:24:47.657 INFO:tasks.workunit.client.0.smithi107.stdout:[ RUN      ] ObjectDirectoryFixture.GetYield
2024-05-29T22:24:47.682 INFO:tasks.workunit.client.0.smithi107.stdout:[       OK ] ObjectDirectoryFixture.GetYield (25 ms)
2024-05-29T22:24:47.682 INFO:tasks.workunit.client.0.smithi107.stdout:[ RUN      ] ObjectDirectoryFixture.CopyYield
2024-05-29T22:24:47.683 INFO:tasks.workunit.client.0.smithi107.stdout:./src/test/rgw/test_d4n_directory.cc:207: Failure
2024-05-29T22:24:47.683 INFO:tasks.workunit.client.0.smithi107.stdout:Expected equality of these values:
2024-05-29T22:24:47.683 INFO:tasks.workunit.client.0.smithi107.stdout:  0
2024-05-29T22:24:47.683 INFO:tasks.workunit.client.0.smithi107.stdout:  dir->copy(obj, "copyTestName", "copyBucketName", yield)
2024-05-29T22:24:47.684 INFO:tasks.workunit.client.0.smithi107.stdout:    Which is: -22
2024-05-30T01:24:47.605 DEBUG:teuthology.orchestra.run:got remote process result: 124

trying to run against main for comparison

cbodley · 2024-06-06T14:39:14Z

https://pulpito.ceph.com/cbodley-2024-06-05_13:55:19-rgw-main-distro-default-smithi/ against main shows the same valgrind and d4n failures, now tracked in https://tracker.ceph.com/issues/66336 and https://tracker.ceph.com/issues/66365

there was also a notification job with these failures:

FAIL: test persistent topic stats
FAIL: test that when object is deleted due to lifecycle policy, notification is sent on master

@yuvalif i'm guessing those are unrelated issues with the mock http server? (note this batch included changes to persistent topics from #57536)

cbodley · 2024-06-06T14:39:27Z

The following tests FAILED:
34 - run-rbd-unit-tests-127.sh (Failed)

cbodley · 2024-06-06T14:39:32Z

jenkins test make check

yuvalif · 2024-06-06T14:48:36Z

https://pulpito.ceph.com/cbodley-2024-06-05_13:55:19-rgw-main-distro-default-smithi/ against main shows the same valgrind and d4n failures, now tracked in https://tracker.ceph.com/issues/66336 and https://tracker.ceph.com/issues/66365

there was also a notification job with these failures:

FAIL: test persistent topic stats
FAIL: test that when object is deleted due to lifecycle policy, notification is sent on master

@yuvalif i'm guessing those are unrelated issues with the mock http server? (note this batch included changes to persistent topics from #57536)

the persistent topic stats test is failing unrelated to the change here.
however, lifecycle test failure is uncommon. since this PR is changing this area of the code, it is probably worth investigating.
also note that with the latest test change (#57550), the lifecycle tests are going to be executed against http, kafka and amqp backends. if they fail only with an http backend then we can probbaly consider it as a pass

cbodley · 2024-06-06T16:15:34Z

however, lifecycle test failure is uncommon. since this PR is changing this area of the code, it is probably worth investigating.
also note that with the latest test change (#57550), the lifecycle tests are going to be executed against http, kafka and amqp backends. if they fail only with an http backend then we can probbaly consider it as a pass

ok, i'll push a rebase of this and #57536 and rerun against that

github-actions · 2024-06-11T14:23:30Z

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

cbodley · 2024-06-11T15:13:58Z

rerunning dead jobs from https://pulpito.ceph.com/cbodley-2024-06-10_12:56:39-rgw-wip-cbodley-testing-distro-default-smithi/

cbodley · 2024-06-12T13:32:02Z

re-rerun looks good enough: https://pulpito.ceph.com/cbodley-2024-06-11_19:40:35-rgw-wip-cbodley-testing-distro-default-smithi/

@kchheda3 if the rebase is trivial, we don't need to qa again

…on errors. Currently if there is any error while calling publish_reserve the lc processing is cancelled for that object. This is different from behavior we have for replication events where the notification errors are not blocking replication. On similar note, lc being internal ceph processing, notification error's should not block the lc processing. Signed-off-by: kchheda3 <kchheda3@bloomberg.net>

…s for processing for object tag decode errors. This was the behavior prior to ceph#55795, however while refactoring this behavior was changed, hence revert back the logic to not send the events if the obj_tag decoding fails. Signed-off-by: kchheda3 <kchheda3@bloomberg.net>

kchheda3 · 2024-06-12T18:12:55Z

re-rerun looks good enough: https://pulpito.ceph.com/cbodley-2024-06-11_19:40:35-rgw-wip-cbodley-testing-distro-default-smithi/

@kchheda3 if the rebase is trivial, we don't need to qa again

was a trivial rebase, rename of event Noncurrent to NonCurrent

cbodley · 2024-06-13T13:24:25Z

FAIL: test_get_indiv_flag (tasks.mgr.dashboard.test_osd.OsdFlagsTest)

cbodley · 2024-06-13T13:24:28Z

jenkins test api

cbodley · 2024-06-13T13:24:36Z

The following tests FAILED:
203 - unittest_osdmap (Subprocess aborted)

cbodley · 2024-06-13T13:24:39Z

jenkins test make check

cbodley · 2024-06-19T13:59:33Z

jenkins test make check

cbodley · 2024-06-19T13:59:36Z

jenkins test api

cbodley · 2024-06-19T15:11:54Z

jenkins pls 🙏

kchheda3 requested a review from a team as a code owner April 24, 2024 16:28

kchheda3 requested review from mattbenjamin and yuvalif April 24, 2024 16:28

kchheda3 self-assigned this Apr 24, 2024

github-actions bot added the rgw label Apr 24, 2024

kchheda3 requested a review from BBoozmen April 24, 2024 16:28

yuvalif reviewed May 9, 2024

View reviewed changes

src/rgw/driver/rados/rgw_cr_rados.cc Show resolved Hide resolved

yuvalif reviewed May 9, 2024

View reviewed changes

kchheda3 force-pushed the wip-lc-notification branch from 1607dff to e1f2af9 Compare May 10, 2024 01:52

yuvalif approved these changes May 10, 2024

View reviewed changes

yuvalif added the needs-qa label May 10, 2024

cbodley added needs-rebase and removed needs-qa labels May 13, 2024

kchheda3 force-pushed the wip-lc-notification branch from e1f2af9 to ff71cea Compare May 13, 2024 18:53

github-actions bot removed the needs-rebase label May 13, 2024

cbodley mentioned this pull request May 13, 2024

rgw/cloud-transition: fix the crash with publish_commit #57356

Merged

14 tasks

soumyakoduri reviewed May 14, 2024

View reviewed changes

kchheda3 force-pushed the wip-lc-notification branch 2 times, most recently from c810e78 to f9148ef Compare May 14, 2024 14:58

soumyakoduri reviewed May 14, 2024

View reviewed changes

kchheda3 force-pushed the wip-lc-notification branch from f9148ef to 745beae Compare May 14, 2024 18:59

kchheda3 requested review from cbodley and soumyakoduri May 15, 2024 17:28

kchheda3 closed this May 15, 2024

kchheda3 deleted the wip-lc-notification branch May 15, 2024 17:40

kchheda3 restored the wip-lc-notification branch May 15, 2024 17:40

kchheda3 reopened this May 15, 2024

soumyakoduri reviewed May 15, 2024

View reviewed changes

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

github-actions bot added the needs-rebase label Jun 11, 2024

cbodley added TESTED and removed needs-qa wip-cbodley-testing labels Jun 12, 2024

kchheda3 added 2 commits June 12, 2024 14:09

kchheda3 force-pushed the wip-lc-notification branch from 9271b3f to 02295f9 Compare June 12, 2024 18:11

github-actions bot removed the needs-rebase label Jun 12, 2024

cbodley added the ready-to-merge label Jun 12, 2024

cbodley merged commit ef20848 into ceph:main Jun 20, 2024

kchheda3 deleted the wip-lc-notification branch June 20, 2024 14:20

kchheda3 mentioned this pull request Jun 21, 2024

squid: rgw/lifecycle-notification: Do not block lc processing for notification errors. #58197

Merged

rgw/lifecycle-notification: Do not block lc processing for notification errors. #57079

rgw/lifecycle-notification: Do not block lc processing for notification errors. #57079

Uh oh!

Conversation

kchheda3 commented Apr 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution Guidelines

Checklist

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbodley commented May 13, 2024

Uh oh!

kchheda3 commented May 13, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbodley May 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soumyakoduri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kchheda3 commented Jun 4, 2024

Uh oh!

cbodley commented Jun 4, 2024

Uh oh!

cbodley commented Jun 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cbodley commented Jun 6, 2024

Uh oh!

cbodley commented Jun 6, 2024

Uh oh!

yuvalif commented Jun 6, 2024

Uh oh!

cbodley commented Jun 6, 2024

Uh oh!

github-actions bot commented Jun 11, 2024

Uh oh!

cbodley commented Jun 11, 2024

Uh oh!

cbodley commented Jun 12, 2024

Uh oh!

kchheda3 commented Jun 12, 2024

Uh oh!

cbodley commented Jun 13, 2024

Uh oh!

cbodley commented Jun 13, 2024

Uh oh!

cbodley commented Jun 13, 2024

Uh oh!

cbodley commented Jun 13, 2024

Uh oh!

cbodley commented Jun 19, 2024

Uh oh!

cbodley commented Jun 19, 2024

Uh oh!

cbodley commented Jun 19, 2024

kchheda3 commented Apr 24, 2024 •

edited

Loading

cbodley May 14, 2024 •

edited

Loading

cbodley commented Jun 6, 2024 •

edited

Loading