rgw/sync-policy: Support disabling per-bucket replication #49803

soumyakoduri · 2023-01-20T10:57:44Z

When the zones replicate, allow disabling replication for specific buckets using sync policy.

These are the semantics to be followed while resolving the policy conflicts -

zonegroup	bucket	Result
enabled	enabled	enabled
enabled	allowed	enabled
enabled	forbidden	disabled
allowed	enabled	enabled
allowed	allowed	disabled
allowed	forbidden	disabled
forbidden	enabled	disabled
forbidden	allowed	disabled
forbidden	forbidden	disabled

In case multiple group policies are set to reflect for any sync pair (<source-zone,source-bucket>, <dest-zone,dest-bucket>), the following rules are applied in the order -

Even if one policy status is FORBIDDEN, the sync will be disabled
Atleast one policy should be ENABLED for the sync to be allowed.

Fixes: https://tracker.ceph.com/issues/58518
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

soumyakoduri · 2023-01-20T11:14:14Z

@yehudasa @cbodley @mattbenjamin .. please comment if the rule semantics mentioned in this PR seem correct to be followed to resolve the sync policies.

The changes in this commit seem to work to disable bucket replication. But I will test all other cases too and confirm.

mattbenjamin · 2023-01-20T14:44:37Z

@soumyakoduri the truth table appears to make sense; does this address the actual issue Daniel Parkes reported, or is it a fix for issues you found while debugging that?

yehudasa · 2023-01-20T15:52:16Z

@soumyakoduri the table makes sense. Is it working at the <zone, bucket> granularity? E.g., can disable sync to/from a bucket at a specific zone. Also, is it possible to disable bucket as source, but not as target (and vice versa)? Will need to test all these cases.

soumyakoduri · 2023-01-20T16:00:13Z

@soumyakoduri the table makes sense. Is it working at the <zone, bucket> granularity? E.g., can disable sync to/from a bucket at a specific zone. Also, is it possible to disable bucket as source, but not as target (and vice versa)? Will need to test all these cases.

Thanks a lot Yehuda. yes..I will test these cases too and update . Hopefully will try to include them as part of #38238 as well.

soumyakoduri · 2023-01-20T16:03:39Z

@soumyakoduri the truth table appears to make sense; does this address the actual issue Daniel Parkes reported, or is it a fix for issues you found while debugging that?

This PR is to specifically address https://bugzilla.redhat.com/show_bug.cgi?id=2132554 . But since the changes affect all the sync-policy use-cases, as discussed in the refactoring call, wanted to define and confirm the semantics for testing and any future reference.

soumyakoduri · 2023-02-21T13:05:30Z

I have addressed few bugs and outlined the testcases I tested/plan to test here - https://docs.google.com/document/d/19oBQA-bYxLBR4BnekA2DTwJJaTFvjAfrqAk9G3RGU0I/edit#heading=h.4qac9dpc76m

Please check if that covers all the cases @yehudasa

soumyakoduri · 2023-02-21T14:50:15Z

@yehudasa I found another strange behaviour with sync policy -

Out of 3 zones (zone1, zone2, zone3) within a zonegroup, if a policy is configured at zonegroup level to allow symmetrical sync between only zone1, zone2, I assume the sync shouldn't happen from zone1/zone2 <-> zone3. But that doesn't seem to be the case.

I see objects getting sync'ed from zone1, zone2 to zone3 but strangely not viceversa (i.e, from zone3 -> zone2,zone1 is restricted as expected but zone1, zone2 -> zone3 is not disabled).

Please provide your inputs on the same.

soumyakoduri · 2023-02-22T06:35:24Z

@yehudasa I found another strange behaviour with sync policy -

Out of 3 zones (zone1, zone2, zone3) within a zonegroup, if a policy is configured at zonegroup level to allow symmetrical sync between only zone1, zone2, I assume the sync shouldn't happen from zone1/zone2 <-> zone3. But that doesn't seem to be the case.

I see objects getting sync'ed from zone1, zone2 to zone3 but strangely not viceversa (i.e, from zone3 -> zone2,zone1 is restricted as expected but zone1, zone2 -> zone3 is not disabled).

Please provide your inputs on the same.

I raised https://tracker.ceph.com/issues/58822 for the same.

soumyakoduri · 2023-02-28T09:41:52Z

@yehudasa most of the cases listed (https://docs.google.com/document/d/19oBQA-bYxLBR4BnekA2DTwJJaTFvjAfrqAk9G3RGU0I/edit#) seem to be working at least for two-zones. With 3 zones, I am hitting the issue mentioned in https://tracker.ceph.com/issues/58822.

I will request QE also to test all the cases in detail (provided we have fix for #58822 as well).

Please review the latest changes and let me know your comments.

soumyakoduri · 2023-03-07T02:08:16Z

@yehudasa kindly review the changes.

yehudasa

@soumyakoduri see my comments

yehudasa · 2023-03-08T15:07:23Z

src/rgw/driver/rados/rgw_bucket_sync.cc

@@ -482,6 +488,30 @@ void RGWBucketSyncFlowManager::pipe_set::insert(const rgw_sync_bucket_pipe& pipe
  handlers.insert(h);
 }

+void RGWBucketSyncFlowManager::pipe_set::remove_all() {
+  pipe_map.erase(pipe_map.begin(), pipe_map.end());


@soumyakoduri any reason why you're not calling pipe_map.clear() instead?

Thanks! Addressed it. Also this routine is currently not used anywhere (in this fix), but just added as a helper routine if any need arise later.

yehudasa · 2023-03-08T15:23:29Z

src/rgw/driver/rados/rgw_bucket_sync.cc

@@ -494,14 +524,15 @@ bool RGWBucketSyncFlowManager::allowed_data_flow(const rgw_zone_id& source_zone,
                                                 bool check_activated) const
 {
  bool found = false;
-  bool found_activated = false;
+  bool found_activated = true;


this would return true if flow_groups are empty. However, I can't find anywhere were it's even called with check_activated true, so maybe a moot point. Am I missing something?

Right. It is not used anywhere at the moment with check_activated set to true.

But while working on this fix, I assumed it will be set (if ever in future) only for the child policies.. So in case if any child/bucket policy is missing the flow-group, it should inherit from parent policy and hence should return "true" in such case.

Since its not needed for this fix, will take it up later as it may need further discussion.

yehudasa · 2023-03-08T15:52:51Z

src/rgw/driver/rados/rgw_bucket_sync.cc

-      source_pipes->insert(pipe);
+      if (is_forbidden) {
+        ldpp_dout(dpp, 20) << __func__ << "(): flow manager (bucket=" << effective_bucket_key << "): removing source pipe: " << pipe << dendl;
+        source_pipes->disable(pipe);


@soumyakoduri do we have ordering issue, where we can enable stuff after we disabled that depending on the flow_group order?

yes..while testing I noticed it.

If there are two policies set, for eg.,

policy 1 - FORBIDDEN - <src_pipe,dest_pipe>

policy 2 - ENABLED - <src_pipe, dest_pipe>
policy2 is processed last in the order and thus granted the access.

To avoid that, have included disabled_pipes to store the restricted <pipe_set>s if any and match with the ones being inserted.

soumyakoduri · 2023-03-10T05:49:13Z

jenkins test make check arm64

soumyakoduri · 2023-03-10T05:49:45Z

jenkins test windows

soumyakoduri · 2023-03-21T14:37:46Z

@yehudasa I addressed your comments and an issue (found during testing) with removing an element in pipe_map . Kindly review the changes.

yehudasa

lgtm

When the zones replicate, allow disabling replication for specific buckets using sync policy. These are the semantics to be followed while resolving the policy conflicts - ================================================== zonegroup bucket Result ================================================== enabled enabled enabled allowed enabled forbidden disabled allowed enabled enabled allowed disabled forbidden disabled forbidden enabled disabled allowed disabled forbidden disabled In case multiple group policies are set to reflect for any sync pair (<source-zone,source-bucket>, <dest-zone,dest-bucket>), the following rules are applied in the order - 1) Even if one policy status is FORBIDDEN, the sync will be disabled 2) Atleast one policy should be ENABLED for the sync to be allowed. Various cases tested are outlined here - https://docs.google.com/document/d/19oBQA-bYxLBR4BnekA2DTwJJaTFvjAfrqAk9G3RGU0I/edit#heading=h.4qac9dpc76m Signed-off-by: Soumya Koduri <skoduri@redhat.com>

soumyakoduri · 2023-04-05T09:34:11Z

lgtm

Thanks Yehuda.

teuthology run: #49803

soumyakoduri · 2023-04-05T17:20:15Z

jenkins test make check arm64

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

soumyakoduri force-pushed the wip-skoduri-sync-flow branch from 4c731fe to 6931020 Compare January 20, 2023 11:10

soumyakoduri requested review from yehudasa, cbodley and mattbenjamin January 20, 2023 11:10

soumyakoduri force-pushed the wip-skoduri-sync-flow branch from 6931020 to 3b29a79 Compare January 20, 2023 11:16

soumyakoduri force-pushed the wip-skoduri-sync-flow branch from 3b29a79 to 921377a Compare February 20, 2023 13:26

soumyakoduri requested a review from a team as a code owner February 20, 2023 13:26

soumyakoduri removed the request for review from a team February 21, 2023 02:54

soumyakoduri force-pushed the wip-skoduri-sync-flow branch from 921377a to f2022a0 Compare February 21, 2023 13:04

soumyakoduri changed the title ~~[WIP] rgw/sync-policy: Support disabling per-bucket replication~~ rgw/sync-policy: Support disabling per-bucket replication Feb 28, 2023

soumyakoduri force-pushed the wip-skoduri-sync-flow branch from f2022a0 to f5cc8d1 Compare March 7, 2023 02:10

yehudasa reviewed Mar 8, 2023

View reviewed changes

soumyakoduri force-pushed the wip-skoduri-sync-flow branch from f5cc8d1 to 561a461 Compare March 9, 2023 13:08

soumyakoduri force-pushed the wip-skoduri-sync-flow branch from 561a461 to fb98020 Compare March 18, 2023 12:47

yehudasa approved these changes Apr 3, 2023

View reviewed changes

soumyakoduri force-pushed the wip-skoduri-sync-flow branch from fb98020 to aeb3a74 Compare April 3, 2023 14:16

cbodley merged commit 7d4c7b6 into ceph:main Apr 6, 2023
10 of 11 checks passed

This was referenced May 24, 2023

[reef] RGW backports #51717

Merged

[quincy] RGW backports #51806

Open

rgw/sync-policy: Support disabling per-bucket replication #49803

rgw/sync-policy: Support disabling per-bucket replication #49803

soumyakoduri commented Jan 20, 2023 •

edited

soumyakoduri commented Jan 20, 2023

mattbenjamin commented Jan 20, 2023

yehudasa commented Jan 20, 2023

soumyakoduri commented Jan 20, 2023

soumyakoduri commented Jan 20, 2023

soumyakoduri commented Feb 21, 2023 •

edited

soumyakoduri commented Feb 21, 2023

soumyakoduri commented Feb 22, 2023

soumyakoduri commented Feb 28, 2023

soumyakoduri commented Mar 7, 2023

yehudasa left a comment

yehudasa Mar 8, 2023

soumyakoduri Mar 9, 2023

yehudasa Mar 8, 2023

soumyakoduri Mar 9, 2023

yehudasa Mar 8, 2023

soumyakoduri Mar 9, 2023

soumyakoduri commented Mar 10, 2023

soumyakoduri commented Mar 10, 2023

soumyakoduri commented Mar 21, 2023

yehudasa left a comment

soumyakoduri commented Apr 5, 2023

soumyakoduri commented Apr 5, 2023

rgw/sync-policy: Support disabling per-bucket replication #49803

rgw/sync-policy: Support disabling per-bucket replication #49803

Conversation

soumyakoduri commented Jan 20, 2023 • edited

Checklist

soumyakoduri commented Jan 20, 2023

mattbenjamin commented Jan 20, 2023

yehudasa commented Jan 20, 2023

soumyakoduri commented Jan 20, 2023

soumyakoduri commented Jan 20, 2023

soumyakoduri commented Feb 21, 2023 • edited

soumyakoduri commented Feb 21, 2023

soumyakoduri commented Feb 22, 2023

soumyakoduri commented Feb 28, 2023

soumyakoduri commented Mar 7, 2023

yehudasa left a comment

Choose a reason for hiding this comment

yehudasa Mar 8, 2023

Choose a reason for hiding this comment

soumyakoduri Mar 9, 2023

Choose a reason for hiding this comment

yehudasa Mar 8, 2023

Choose a reason for hiding this comment

soumyakoduri Mar 9, 2023

Choose a reason for hiding this comment

yehudasa Mar 8, 2023

Choose a reason for hiding this comment

soumyakoduri Mar 9, 2023

Choose a reason for hiding this comment

soumyakoduri commented Mar 10, 2023

soumyakoduri commented Mar 10, 2023

soumyakoduri commented Mar 21, 2023

yehudasa left a comment

Choose a reason for hiding this comment

soumyakoduri commented Apr 5, 2023

soumyakoduri commented Apr 5, 2023

soumyakoduri commented Jan 20, 2023 •

edited

soumyakoduri commented Feb 21, 2023 •

edited