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
rgw: notifications on object replication #43371
Conversation
|
This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved |
src/rgw/rgw_data_sync.cc
Outdated
| * we need to fetch info about source object, so that we can determine | ||
| * the correct policy configuration. This can happen if there are multiple | ||
| * policy rules, and some depend on the object tagging */ | ||
| yield call(new RGWStatRemoteObjCR(sync_env->async_rados, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so you're making this Stat request unconditional? my vague understanding is that need_more_info is very rarely needed at the moment, so this could be an expensive change. this Stat is also inherently racy, because the remote could overwrite the object between our calls to Stat and FetchRemoteObj
whatever info you need from this Stat request, you should get from FetchRemoteObj instead
src/rgw/rgw_data_sync.cc
Outdated
| int ret = rgw::notify::publish_reserve(dpp, rgw::notify::ObjectSyncedCreate, notify_res, &obj_tags); | ||
| if (ret < 0) { | ||
| ldpp_dout(dpp, 1) << "ERROR: reserving notification failed, with error: " << ret << dendl; | ||
| // no need to return, the sync already happened | ||
| } else { | ||
| ret = rgw::notify::publish_commit(&obj, src_size, src_mtime, src_etag, 0/* version id */, rgw::notify::ObjectSyncedCreate, notify_res, dpp); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these are blocking calls to librados, which we shouldn't use in coroutines. blocking here means that sync can't make progress with other buckets/objects in the meantime
it would probably be easier to trigger this stuff from RGWRados::fetch_remote_obj(), which is already being called synchronously via the RGWAsyncRadosProcessor thread pool
src/rgw/rgw_data_sync.cc
Outdated
| @@ -2441,6 +2442,33 @@ class RGWObjFetchCR : public RGWCoroutine { | |||
| } | |||
| return set_cr_error(retcode); | |||
| } | |||
|
|
|||
| // notify that object has synced to this zone | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note that a call to FetchRemoteObj doesn't actually mean that we synced the object. it sends a GET request with the If-Modified-Since header so that it only transfers the object if it's newer than our local copy. if not, we just get ERR_NOT_MODIFIED and don't have to do anything
you can find some extra logic in RGWAsyncFetchRemoteObj::_send_request() for perf counters that tries to detect whether or not it actually transferred anything
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @cbodley !
@liavt, moving the notification code to RGWAsyncFetchRemoteObj::_send_request() would simplify the code, on top of resolving the issues in the PR.
it already has: RGWObjectCtx, rgw::sal::Attrs attr, rgw::sal::RadosBucket and rgw::sal::RadosObject available.
in addition, you can use the value of bytes_transferred to verify that the object was indeed synced
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved the notification code to RGWAsyncFetchRemoteObj::_send_request() and reverted changes to the coroutine, including the one for need_more_info
Commit: 6211ee4
|
@liavt please don't forget to sign your commits (use: |
|
This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days. |
|
@liavt hi; the original lc notifications pr has merged, please try to rebase this on master |
|
I removed Matt's commit and rebased the pull request, it still requires testing however (and will be squashed and signed after everything is done) |
|
thanks guys, yay. Just fyi, this should have a tracker ticket, as well :) |
this is great! when i tried that (several months ago), it was eventually crashing, with something that seemed unrelated to the code changed here :-( run: to get the pid of the RGW you want to attach to. note that, for some unknown reason, after a while, gdb will stop on a "read()" function (even if no breakpoint is set). |
|
regarding the rebase, i don't think we should see this commit here: 08e7383
|
The rebase should be fixed, the extra commit is gone |
| @@ -713,20 +712,22 @@ int RGWAsyncFetchRemoteObj::_send_request(const DoutPrefixProvider *dpp) | |||
| } | |||
| } | |||
|
|
|||
| // NOTE: we create a mutable copy of bucket.get_tenant as the get_notification function expects a std::string&, not const | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think you can just change the function to use const std::string&
nothing should change the tenant inside the function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing the function to use const std::string& has a lot of other side effects and changes to other code, namely the class reservation_t from rgw_notify.h will need to be updated to have a const tenant, as well as any class that deals with notifications (which is a couple)
Ideally I think it should be a separate PR if anything as it touches a lot of unrelated files, and it isn't necessarily required for this one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree. making all of them const is a good thing.
but lets keep it out of scope of this PR
src/rgw/rgw_cr_rados.cc
Outdated
| ceph::make_timespan(g_conf()->rgw_op_thread_timeout), | ||
| ceph::make_timespan(g_conf()->rgw_op_thread_suicide_timeout), | ||
| &m_tp) { | ||
| ceph::make_timespan(g_conf()->rgw_op_thread_timeout), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: please avoid indentation changes in existing code in the file. here as well as lines 77, 78, 112, 130, 667
src/rgw/rgw_notify_event_type.cc
Outdated
| @@ -42,6 +42,14 @@ namespace rgw::notify { | |||
| return "s3:ObjectLifecycle:Transition:Current"; | |||
| case ObjectTransitionNoncurrent: | |||
| return "s3:ObjectLifecycle:Transition:Noncurrent"; | |||
| case ObjectSynced: | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: seems like indentation is off
(we use 2 spaces and expand tabs)
src/rgw/rgw_cr_rados.cc
Outdated
| std::string tenant(bucket.get_tenant()); | ||
|
|
||
| std::unique_ptr<rgw::sal::Notification> notify | ||
| = store->get_notification(dpp, &dest_obj, &src_obj, &obj_ctx, rgw::notify::ObjectSyncedCreate, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please use nullptr instead of src_obj
names are confusing, but for this function "src" and "dest" mean that an object is copied to another object (which is not the case here).
src/rgw/rgw_cr_rados.cc
Outdated
| ldpp_dout(dpp, 0) << "sending sync notification" << dendl; | ||
|
|
||
| // send notification that object was succesfully synced | ||
| std::string user_id = "0"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably better to have "req_id" = "0" and "user_id" = "rgw sync"
src/rgw/rgw_cr_rados.cc
Outdated
| = store->get_notification(dpp, &dest_obj, &src_obj, &obj_ctx, rgw::notify::ObjectSyncedCreate, | ||
| &bucket, user_id, | ||
| tenant, | ||
| user_id, null_yield); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be "req_id" and not "user_id"
src/rgw/rgw_cr_rados.cc
Outdated
| ldpp_dout(dpp, 1) << "ERROR: reserving notification failed, with error: " << ret << dendl; | ||
| // no need to return, the sync already happened | ||
| } else { | ||
| ret = rgw::notify::publish_commit(&src_obj, src_obj.get_obj_size(), src_mtime, "" /* etag */, "0"/* version id */, rgw::notify::ObjectSyncedCreate, notify_res, dpp); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you not get the realm etag and version id here?
it looks like you could get the etag from the string *petag argument of fetch_remote_obj()
dest_obj.get_instance() should give you the version id
src/rgw/rgw_cr_rados.cc
Outdated
| } else { | ||
| // r >= 0 | ||
| if (bytes_transferred) { | ||
| ldpp_dout(dpp, 0) << "sending sync notification" << dendl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we drop this log message? in most cases, notifications for sync won't be enabled on the bucket, so this message would be misleading
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
It was here for debugging right now but it can be removed once the commit is finalized for the PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good
|
@liavt I think the code is ready! next steps for this PR:
next steps for other PRs:
|
|
jenkins test make check |
|
jenkins test docs |
|
teuthology run has 6 failures: however, bucket notification test is failing due to RGW crash: the backtrace does not seem related to the code changes, however, it is probably due to something in this PR. |
|
The pulpito links do not seem to work for me, the requests time out Regarding the backtrace, it doesn't seem like that passes through anything changed by the PR. Do you know if there is a way to reproduce this locally or have an area of the code that could be affected by the changes? |
sorry, can you try this link (the above use the sepia VPN)? you can run the bucket notification test locally to see why it crashes. the directory holding the test code and instructions is here: |
| = store->get_notification(dpp, &dest_obj, nullptr, &obj_ctx, rgw::notify::ObjectSyncedCreate, | ||
| &dest_bucket, user_id, | ||
| tenant, | ||
| req_id, null_yield); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using null_yield here may have a negative impact on sync performance.
in such a case, the notification sending code would just block using a mutex until the ack for the notification is received.
this needs more investigation since we are already inside a coroutine, there should be a way to yield and do async waits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this needs more investigation since we are already inside a coroutine, there should be a way to yield and do async waits.
unfortunately these are two different kinds of coroutines. they could potentially work together like you describe (i.e. a RGWCoroutine could spawn::spawn() a yield context and wait on it), but that would be a big project. that could be a stepping stone on the way to a better multisite, but we might also be able to avoid it - that's worth discussion
but classes like RGWAsyncFetchRemoteObj that inherit from RGWAsyncRadosRequest are actually being handed off to a thread pool in RGWAsyncRadosProcessor to run synchronously. fetch_remote_obj() itself is blocking, so this blocking null_yield is harmless
|
can still see crashes here: http://qa-proxy.ceph.com/teuthology/yuvalif-2022-04-07_08:28:12-rgw:notifications-wip-yuval-sync-notifications-distro-basic-smithi/6780814/remote/smithi135/crash/posted/ crash here makes more sense (even though it is not in the code introduced in this PR). from the log: looks like an AMQP ack was received, triggering the call to |
|
after another rebase, notification tests are not crashing anymore: @liavt please rebase and force push the rebased code |
|
multisite tests are also passing:
|
src/rgw/rgw_cr_rados.cc
Outdated
| std::string tenant(dest_bucket.get_tenant()); | ||
|
|
||
| std::unique_ptr<rgw::sal::Notification> notify | ||
| = store->get_notification(dpp, &dest_obj, nullptr, &obj_ctx, rgw::notify::ObjectSyncedCreate, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this does not compile now:
error: no matching function for call to ‘rgw::sal::RadosStore::get_notification(...
the function changes to:
rgw_sal_rados.h:97:43: note: candidate: ‘virtual std::unique_ptr<rgw::sal::Notification> rgw::sal::RadosStore::get_notification(const DoutPrefixProvider*, rgw::sal::Object*, rgw::sal::Object*, rgw::notify::EventType, rgw::sal::Bucket*, std::string&, std::string&, std::string&, optional_yield)’
I think you should remove: obj_ctx
Signed-off-by: liavt <liav.turkia@gmail.com>
|
@yuvalif I removed |
|
notification tests are passing after rebase (error exist in baseline as well): |
|
full test suite is passing: http://pulpito.front.sepia.ceph.com/yuvalif-2022-04-27_14:28:25-rgw-wip-yuval-syn-notifications-distro-basic-smithi/ |
|
Can one of the admins verify this patch? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
src/rgw/rgw_notify_event_type.cc
Outdated
| @@ -30,6 +30,14 @@ namespace rgw::notify { | |||
| return "s3:ObjectLifecycle:NoncurrentExpiration"; | |||
| case ObjectDeleteMarkerExpiration: | |||
| return "s3:ObjectLifecycle:DeleteMarkerExpiration"; | |||
| case ObjectSynced: | |||
| return "s3:ObjectSynced:*"; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did use s3:ObjectLifecycle in my lifecycle notifications change, but should we possibly use rgw: here (and maybe there, I don't know--it's a little less clear given that the lifecycle features are AWS compatible)
|
remaining work on the feature: https://gist.github.com/yuvalif/0db188fc63db40af3229f7bd63407bfb |
|
@yuvalif hi Yuval, in future, could you please add "Reviewed-by" lines in the merge commit when merging pull requests? |
Isn't that something we can rely on github to do, since we have approving github reviewers? |
Optionally publishes notifications upon object replication between zones in the same zone group.
New notification types were added to facilitate this change.
Uses commits and API from #39192
Fixes: no ticket
Signed-off-by: Liav Turkia liav.turkia@gmail.com
Checklist
Show available Jenkins commands
jenkins retest this pleasejenkins test classic perfjenkins test crimson perfjenkins test signedjenkins test make checkjenkins test make check arm64jenkins test submodulesjenkins test dashboardjenkins test dashboard cephadmjenkins test apijenkins test docsjenkins render docsjenkins test ceph-volume alljenkins test ceph-volume tox