rgw/archive: Disable logging for archive zone #50676

soumyakoduri · 2023-03-26T17:16:18Z

Since archive zone is not used as source for the incremental sync, disable its data logging. In addition, correct bucket sync status cmd ouput for the zones with archive zone listed as source.

Fixes: https://tracker.ceph.com/issues/59106
Signed-off-by: Soumya Koduri skoduri@redhat.com

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

mattbenjamin

this looks plausible to me; can you coordinate with me on downstream backports for testing?

soumyakoduri · 2023-03-27T07:45:16Z

this looks plausible to me; can you coordinate with me on downstream backports for testing?

sure.. Also please note that this change doesn't handle deleting the logs (if any created in the old release or mixed-mode clusters). They need to be cleaned up manually.

soumyakoduri · 2023-03-27T07:59:55Z

@mattbenjamin @cbodley Other thing I noticed while working on this PR is that archive zone creates multiple versions of the same object depending on the number of zones configured.

For eg., if there are two zones z1, z2 (apart from the archive-zone) configured, when an object ('test-object') is created, archive zone seems to be replicating the object from both the zones and storing two separate copies (though with different version-ids) .. This seems incorrect and a bug to me. Based on etag/src_mtime, it should have ideally filtered the objects already sync'ed from other zones.

Can you please confirm.

cbodley · 2023-03-27T13:26:39Z

Based on etag/src_mtime, it should have ideally filtered the objects already sync'ed from other zones.

in normal sync, the destination zone sends its current object mtime in the If-Modified-Since header to avoid duplicating sync from multiple source zones

it looks like that logic is broken by RGWArchiveDataSyncModule::sync_object(), which generates a random version id for the destination object; so RGWObjFetchCR would try to look up the mtime for that version and fail, so wouldn't be able to send a If-Modified-Since header

cbodley · 2023-03-27T13:47:04Z

src/rgw/rgw_zone.cc

+    if (zone.tier_type == "archive") {
+      zone.log_data = false;


ok, but two comments:

in existing deployments, this wouldn't take effect until a radosgw-admin zone modify calls this function

ideally this would use RGWSyncModulesManager::supports_data_export(zone.tier_type) so we do the right thing for other sync modules like elasticsearch and cloud sync too

unfortunately, i think that sync module stuff is specific to rados; so maybe this should all be in RGWSyncPolicyCompat::convert_old_sync_config(), where we have access to the 'sync module service'?

* in existing deployments, this wouldn't take effect until a `radosgw-admin zone modify` calls this function

Right..Thanks! Initially I tried doing this check as part of RGWSI_Zone::do_start() using RGWSyncModulesManager::supports_data_export() but that doesn't update the zone params stored at the backend.
So for existing deployments, do you suggest we need these changes in RGWSI_Zone::do_start() as well (in case radosgw-admin zone modify is not executed)? Or can we update release notes to execute the cmd post upgrade?

* ideally this would use `RGWSyncModulesManager::supports_data_export(zone.tier_type)` so we do the right thing for other sync modules like elasticsearch and cloud sync too

unfortunately, i think that sync module stuff is specific to rados; so maybe this should all be in RGWSyncPolicyCompat::convert_old_sync_config(), where we have access to the 'sync module service'?

Ok. I observed that at times RGWSyncModulesManager may not be initialized by the time we want to use supports_data_export(..).. I will recheck in this code-path and update.

what do you think about leaving the zone's log_data flag alone, and relying exclusively on RGWBucketSyncPolicyHandler::bucket_exports_data() to do the right thing? that would make things easier on existing users

these services are a mess of circular dependencies :( from RGWServices_Def::init(), it looks like the order is zone_svc -> sync_modules_svc -> bucket_sync_sobj

what do you think about leaving the zone's log_data flag alone, and relying exclusively on RGWBucketSyncPolicyHandler::bucket_exports_data() to do the right thing? that would make things easier on existing users

yes..I initially started with those changes and it helps to disable the logging internally. But bucket sync status and zone_params->log_data still shows incorrect values, which did not seem right. If that is acceptable, I will redo those changes.

these services are a mess of circular dependencies :( from RGWServices_Def::init(), it looks like the order is zone_svc -> sync_modules_svc -> bucket_sync_sobj

Thanks for confirming.

for bucket sync status, i see that bucket_sync_status() is using RGWBucketSyncPolicyHandler::bucket_imports_data() to tell whether the bucket has any sources. but bucket_source_sync_status() is using RGWZone::syncs_from(zone_name) which only consults the sync_from/sync_from_all fields. maybe we should use bucket_exports_data() there instead?

right. log_index_operation() in cls/rgw/cls_rgw.cc runs on the osd side, and bool log_op is a parameter for the bucket index operations in cls/rgw/cls_rgw_client.h; so we'd use bucket_exports_data() on the rgw side to determine what to pass for log_op

Got it. Thanks!

@cbodley @smanjara The changes updated now disables both datalog and bilog for the archive zone. Kindly review.

i was hoping that we could add the per-bucket control as well here (using bucket_exports_data() instead of need_to_log_data()), but this looks correct 👍

@smanjara should we follow up on the per-bucket part later?

@cbodley sure we could follow up with another pr.

soumyakoduri · 2023-03-27T14:09:55Z

Based on etag/src_mtime, it should have ideally filtered the objects already sync'ed from other zones.

in normal sync, the destination zone sends its current object mtime in the If-Modified-Since header to avoid duplicating sync from multiple source zones

it looks like that logic is broken by RGWArchiveDataSyncModule::sync_object(), which generates a random version id for the destination object; so RGWObjFetchCR would try to look up the mtime for that version and fail, so wouldn't be able to send a If-Modified-Since header

Thanks! Will file a tracker to fix it then.

smanjara · 2023-03-27T16:51:51Z

Based on etag/src_mtime, it should have ideally filtered the objects already sync'ed from other zones.

in normal sync, the destination zone sends its current object mtime in the If-Modified-Since header to avoid duplicating sync from multiple source zones
it looks like that logic is broken by RGWArchiveDataSyncModule::sync_object(), which generates a random version id for the destination object; so RGWObjFetchCR would try to look up the mtime for that version and fail, so wouldn't be able to send a If-Modified-Since header

Thanks! Will file a tracker to fix it then.

thanks soumya. seems to be coming from my change in bcea963

soumyakoduri · 2023-03-27T18:59:26Z

thanks soumya. seems to be coming from my change in bcea963

Thanks Shilpa. Raised https://tracker.ceph.com/issues/59184 for the same.

cbodley · 2023-04-10T19:33:34Z

src/rgw/rgw_admin.cc

+  if (!zone.syncs_from(source.name) ||
+     (!static_cast<rgw::sal::RadosStore*>(driver)->svc()->zone->zone_syncs_from(static_cast<rgw::sal::RadosStore*>(driver)->svc()->zone->get_zone(), source))) {


if (!driver->svc()->zone->zone_syncs_from(zone, source)) {

driver is already a RadosStore* so we don't need the casts

RGWSI_Zone::zone_syncs_from() already checks target_zone.syncs_from(source_zone)

driver->svc()->zone->get_zone() is just zone here

thanks! addressed it.

soumyakoduri · 2023-04-11T07:54:51Z

jenkins test windows

Zones (such as archive zone) which do not export data should have sync logging disabled. Fixes# https://tracker.ceph.com/issues/59106 Signed-off-by: Soumya Koduri <skoduri@redhat.com>

Signed-off-by: Soumya Koduri <skoduri@redhat.com>

soumyakoduri · 2023-05-04T03:03:45Z

jenkins test make check

soumyakoduri · 2023-05-04T03:03:55Z

jenkins test make check arm64

soumyakoduri · 2023-05-04T03:04:33Z

http://pulpito.front.sepia.ceph.com/soumyakoduri-2023-05-03_01:30:22-rgw-wip-skoduri-archive-distro-default-smithi

soumyakoduri · 2023-05-04T10:47:41Z

jenkins test make check arm64

soumyakoduri requested a review from a team as a code owner March 26, 2023 17:16

github-actions bot added the rgw label Mar 26, 2023

soumyakoduri requested review from yehudasa, cbodley and mattbenjamin and removed request for a team March 26, 2023 17:16

mattbenjamin approved these changes Mar 26, 2023

View reviewed changes

cbodley reviewed Mar 27, 2023

View reviewed changes

mattbenjamin requested a review from smanjara March 27, 2023 13:50

soumyakoduri force-pushed the wip-skoduri-archive branch from 51a3352 to c679ff1 Compare April 10, 2023 14:54

cbodley approved these changes Apr 10, 2023

View reviewed changes

soumyakoduri force-pushed the wip-skoduri-archive branch from e0d6d45 to 8985267 Compare April 11, 2023 04:13

cbodley added the needs-qa label Apr 11, 2023

soumyakoduri force-pushed the wip-skoduri-archive branch from 8985267 to fff109d Compare April 23, 2023 11:15

soumyakoduri added 2 commits May 2, 2023 21:56

rgw: Disable logging for datalog and bilog if zone doesn't export data

85f0cb5

Zones (such as archive zone) which do not export data should have sync logging disabled. Fixes# https://tracker.ceph.com/issues/59106 Signed-off-by: Soumya Koduri <skoduri@redhat.com>

rgw/archive: Correct the bucket sync status cmd output

d7af6c2

Signed-off-by: Soumya Koduri <skoduri@redhat.com>

soumyakoduri force-pushed the wip-skoduri-archive branch from fff109d to d7af6c2 Compare May 2, 2023 16:26

soumyakoduri merged commit 9d61a2e into ceph:main May 4, 2023
10 of 11 checks passed

soumyakoduri mentioned this pull request May 23, 2023

[reef] RGW backports #51717

Merged

14 tasks

soumyakoduri mentioned this pull request May 29, 2023

[quincy] RGW backports #51806

Open

14 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rgw/archive: Disable logging for archive zone #50676

rgw/archive: Disable logging for archive zone #50676

soumyakoduri commented Mar 26, 2023 •

edited

mattbenjamin left a comment

soumyakoduri commented Mar 27, 2023

soumyakoduri commented Mar 27, 2023 •

edited

cbodley commented Mar 27, 2023

cbodley Mar 27, 2023

soumyakoduri Mar 27, 2023 •

edited

cbodley Mar 27, 2023

soumyakoduri Mar 27, 2023

cbodley Mar 27, 2023

cbodley Apr 6, 2023

soumyakoduri Apr 6, 2023

soumyakoduri Apr 10, 2023

cbodley Apr 10, 2023

smanjara Apr 10, 2023

soumyakoduri commented Mar 27, 2023

smanjara commented Mar 27, 2023

soumyakoduri commented Mar 27, 2023

cbodley Apr 10, 2023

soumyakoduri Apr 11, 2023

soumyakoduri commented Apr 11, 2023

soumyakoduri commented May 4, 2023

soumyakoduri commented May 4, 2023

soumyakoduri commented May 4, 2023

soumyakoduri commented May 4, 2023

		if (!zone.syncs_from(source.name) \|\|
		(!static_cast<rgw::sal::RadosStore>(driver)->svc()->zone->zone_syncs_from(static_cast<rgw::sal::RadosStore>(driver)->svc()->zone->get_zone(), source))) {

rgw/archive: Disable logging for archive zone #50676

rgw/archive: Disable logging for archive zone #50676

Conversation

soumyakoduri commented Mar 26, 2023 • edited

Checklist

mattbenjamin left a comment

Choose a reason for hiding this comment

soumyakoduri commented Mar 27, 2023

soumyakoduri commented Mar 27, 2023 • edited

cbodley commented Mar 27, 2023

Choose a reason for hiding this comment

soumyakoduri Mar 27, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soumyakoduri commented Mar 27, 2023

smanjara commented Mar 27, 2023

soumyakoduri commented Mar 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soumyakoduri commented Apr 11, 2023

soumyakoduri commented May 4, 2023

soumyakoduri commented May 4, 2023

soumyakoduri commented May 4, 2023

soumyakoduri commented May 4, 2023

soumyakoduri commented Mar 26, 2023 •

edited

soumyakoduri commented Mar 27, 2023 •

edited

soumyakoduri Mar 27, 2023 •

edited