rgw: disable RGWDataChangesLog::add_entry() when log_data is off

[cbodley]

this restores a check for RGWZone::log_data in add_entry(). with per-bucket replication, this check was replaced by a call to RGWBucketSyncPolicyHandler::bucket_exports_data()

this call has to consult two rados objects, bucket.sync-source-hints.<bucketname> and bucket.sync-target-hints.<bucketname>

but if the zone is not configured for multisite, we should avoid these extra object reads and return early

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

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]

@yehudasa i know that someday we'll be able to support sync between buckets in the same zone, but for now i don't think it's necessary to consult these sync policy objects outside of multisite?

[yehudasa]

It is likely that you will break certain cases in bucket level sync this way. The "configured for multisite" hammer is too loosely defined and too big IMO.

[cbodley]

It is likely that you will break certain cases in bucket level sync this way.

can you be more specific?

The "configured for multisite" hammer is too loosely defined and too big IMO.

this particular hammer is RGWZone::log_data, which is meant to control whether we're writing to the datalog/bilogs. if that's false, then nothing in per-bucket replication should ever cause us to write datalogs - we'd accumulate data that never gets trimmed

[cbodley]

ping @yehudasa

[yehudasa]

The idea is not to need to configure sync in multiple places. How about you change that flag to a 3 state, and by default it's not defined (in which case it relies exclusively on sync policy). If true/false would mean that user explicitly had set it. In the case of sync policy, can check on init whether zone allows sync out, if it does not then can change flag to false.

[cbodley]

The idea is not to need to configure sync in multiple places.

thanks @yehudasa. i'm still fuzzy on sync policy, can you please check my understanding here?

• we have to read these per-bucket hints because the global sync policy is 'allowed' (as opposed to 'enabled' or 'forbidden')
• global sync defaults to 'allowed' in single-zone configurations

i'm guessing that 'allowed' is the default, so you can attach a new zone later without having to update this zone's policy? if that's the intent, shouldn't we at least make this 'allowed' behavior conditional on the presence of other zones?

the 'presence of other zones' is essentially all that log_data tracks. at some point, we stopped trusting our users with control over it, so now RGWZoneGroup::post_process_params() overwrites log_data = zones.size() > 1 whenever you make changes to the zonegroup. so log_data doesn't require any extra configuration, it's managed automatically when you add/remove zones

if i am understanding the intent of the 'allowed' sync policy here, then it seems to compose just fine with the log_data change in this PR

How about you change that flag to a 3 state, and by default it's not defined (in which case it relies exclusively on sync policy). If true/false would mean that user explicitly had set it. In the case of sync policy, can check on init whether zone allows sync out, if it does not then can change flag to false.

i'm not sure what you're suggesting with the third state for log_data. in this new state, what part of sync policy is it using to determine whether 'zone allows sync out'? if sync policy can already tell us that, couldn't RGWBucketSyncPolicyHandler::bucket_exports_data() use that to avoid reading these per-bucket hints? or does that decision depend on the per-bucket hints?

[yehudasa]

The idea is not to need to configure sync in multiple places.

thanks @yehudasa. i'm still fuzzy on sync policy, can you please check my understanding here?

• we have to read these per-bucket hints because the global sync policy is 'allowed' (as opposed to 'enabled' or 'forbidden')
• global sync defaults to 'allowed' in single-zone configurations

i'm guessing that 'allowed' is the default, so you can attach a new zone later without having to update this zone's policy? if that's the intent, shouldn't we at least make this 'allowed' behavior conditional on the presence of other zones?

the 'presence of other zones' is essentially all that log_data tracks. at some point, we stopped trusting our users with control over it, so now RGWZoneGroup::post_process_params() overwrites log_data = zones.size() > 1 whenever you make changes to the zonegroup. so log_data doesn't require any extra configuration, it's managed automatically when you add/remove zones

if i am understanding the intent of the 'allowed' sync policy here, then it seems to compose just fine with the log_data change in this PR

How about you change that flag to a 3 state, and by default it's not defined (in which case it relies exclusively on sync policy). If true/false would mean that user explicitly had set it. In the case of sync policy, can check on init whether zone allows sync out, if it does not then can change flag to false.

i'm not sure what you're suggesting with the third state for log_data. in this new state, what part of sync policy is it using to determine whether 'zone allows sync out'? if sync policy can already tell us that, couldn't RGWBucketSyncPolicyHandler::bucket_exports_data() use that to avoid reading these per-bucket hints? or does that decision depend on the per-bucket hints?

The decision depends on it iirc. A bucket policy by itself cannot know that there is/isn't another bucket that has a policy that fetches data from it, that's why we need the hints (that point at other buckets that may sync from us).
As a side note, iirc, one thing that the hints allow is to deal with resharded bucket as the newly sharded bucket instance can get a hint that points it to the older bucket instance.

[cbodley]

ok, so back to RGWDataChangesLog::add_entry(). even if there is a policy that covers writes to this bucket, why should we write to the datalog/bilog if we're the only zone in our zonegroup?

if there's no other zone attached, then nobody is going to read those logs. if another zone attaches later, it must start with a full sync and skip past these entries anyway

isn't log_data doing the right thing here?

[cbodley]

passed qa in https://pulpito.ceph.com/cbodley-2022-03-25_20:30:36-rgw-wip-cbodley-testing-distro-default-smithi/

[cbodley]

do you have any suggestions here @yehudasa?

[cbodley]

jenkins test make check

[yuvalif]

passed qa in https://pulpito.ceph.com/cbodley-2022-03-25_20:30:36-rgw-wip-cbodley-testing-distro-default-smithi/

do we have bucket sync policy tests in teuthology?

[cbodley]

passed qa in https://pulpito.ceph.com/cbodley-2022-03-25_20:30:36-rgw-wip-cbodley-testing-distro-default-smithi/

do we have bucket sync policy tests in teuthology?

no, i merged #31686 for octopus under the condition that tests would follow, but they didn't

[yehudasa]

passed qa in https://pulpito.ceph.com/cbodley-2022-03-25_20:30:36-rgw-wip-cbodley-testing-distro-default-smithi/

do we have bucket sync policy tests in teuthology?

no, i merged #31686 for octopus under the condition that tests would follow, but they didn't

Note this PR that is still pending:
#38238

[yuvalif]

passed qa in https://pulpito.ceph.com/cbodley-2022-03-25_20:30:36-rgw-wip-cbodley-testing-distro-default-smithi/

do we have bucket sync policy tests in teuthology?

no, i merged #31686 for octopus under the condition that tests would follow, but they didn't

Note this PR that is still pending: #38238

this is from the end of 2020 !
what is blocking that PR?