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

jewel: rgw: multisite: fixes for meta sync across periods #15556

Merged
merged 14 commits into from Sep 12, 2017

Conversation

Projects
None yet
5 participants
@cbodley
Contributor

cbodley commented Jun 7, 2017

backport for http://tracker.ceph.com/issues/19847 (including related changes to test_multi.py from #13067)

cbodley added some commits Jun 8, 2016

rgw: add empty_on_enoent flag to RGWSimpleRadosReadCR
RGWSimpleRadosReadCR won't currently fail with ENOENT, but instead
passes an empty object to handle_data(). add an empty_on_enoent flag to
the constructor, defaulting to true, to make this behavior optional for
callers that do want to fail on ENOENT

Signed-off-by: Casey Bodley <cbodley@redhat.com>
(cherry picked from commit c5c95e7)
rgw: add == and != operators for period history cursor
RGWMetaSyncCR was using operator== but it always returned true!

Signed-off-by: Casey Bodley <cbodley@redhat.com>
(cherry picked from commit 09c847f)
rgw: RGWBackoffControlCR only retries until success
Signed-off-by: Casey Bodley <cbodley@redhat.com>
(cherry picked from commit a898fb7)
rgw: fix marker comparison to detect end of mdlog period
Fixes: http://tracker.ceph.com/issues/18639

Signed-off-by: Casey Bodley <cbodley@redhat.com>
(cherry picked from commit 7c23713)
rgw: clean up RGWInitDataSyncStatusCoroutine
RGWInitDataSyncStatusCoroutine operates on a given rgw_data_sync_status
pointer, which saves us from having to read it back from rados

Signed-off-by: Casey Bodley <cbodley@redhat.com>
(cherry picked from commit 69be410)

Conflicts:
	src/rgw/rgw_data_sync.cc: rgw_pool, rgw_raw_obj
rgw: store realm epoch with sync status markers
sync status markers can't be compared between periods, so we need to
record the current period's realm epoch with its markers. when the
rgw_meta_sync_info.realm_epoch is more recent than the marker's
realm_epoch, we must treat the marker as empty

Signed-off-by: Casey Bodley <cbodley@redhat.com>
(cherry picked from commit 574ff5f)

Conflicts:
	src/rgw/rgw_sync.cc: rgw_pool
rgw: change metadata read_sync_status interface
makes the same change to read_sync_status() in RGWMetaSyncStatusManager,
needed to support multiple concurrent readers for the rest interface

Signed-off-by: Casey Bodley <cbodley@redhat.com>
(cherry picked from commit 0372df2)
rgw: use RGWShardCollectCR for RGWReadSyncStatusCoroutine
Signed-off-by: Casey Bodley <cbodley@redhat.com>
(cherry picked from commit a4bf014)

Conflicts:
	src/rgw/rgw_sync.cc: rgw_pool, rgw_raw_obj
rgw: period commit uses sync status markers
Signed-off-by: Casey Bodley <cbodley@redhat.com>
(cherry picked from commit bb49e2f)

Conflicts:
	src/rgw/rgw_rados.cc: RGWPeriod::update removed
rgw: remove rgw_realm_reconfigure_delay
when the master zone is changed, this config variable was increasing the
window of time where the old master zone would continue to handle
requests to modify metadata. those changes would not be reflected by the
new metadata master zone, and would be lost to the cluster

it was an attempt to optimize for the unlikely case of multiple period
changes in a short period of time, but the logic in reload() handles this
case correctly as is

Signed-off-by: Casey Bodley <cbodley@redhat.com>
(cherry picked from commit f422d4f)
rgw: require --yes-i-really-mean-it to promote zone with stale metadata
if a zone is promoted to master before it has a chance to sync from the
previous master zone, any metadata entries after its sync position will
be lost

print an error if 'period commit' is trying to promote a zone that is
more than one period behind the current master, and only allow the
commit to proceed if the --yes-i-really-mean-it flag is provided

Signed-off-by: Casey Bodley <cbodley@redhat.com>
(cherry picked from commit 721e3d6)
test/rgw: meta checkpoint compares realm epoch
avoid marker comparisons between different periods

Signed-off-by: Casey Bodley <cbodley@redhat.com>
(cherry picked from commit 20df35a)
test/rgw: fixes for test_multi_period_incremental_sync()
test was only creating objects in subsequent periods, which wasn't
adding any entries to the mdlog. this wasn't correctly testing
incremental metadata sync across periods

Signed-off-by: Casey Bodley <cbodley@redhat.com>
(cherry picked from commit 2976fd3)
test/rgw: wait for realm reload after set_master_zone
Signed-off-by: Casey Bodley <cbodley@redhat.com>
(cherry picked from commit 5cc99db)

@cbodley cbodley added this to the jewel milestone Jun 7, 2017

@smithfarm

This comment has been minimized.

Show comment
Hide comment
@smithfarm

smithfarm Jun 19, 2017

Contributor

@cbodley Please rebase to get a new Jenkins run.

Contributor

smithfarm commented Jun 19, 2017

@cbodley Please rebase to get a new Jenkins run.

@smithfarm

This comment has been minimized.

Show comment
Hide comment
@smithfarm

smithfarm Jun 19, 2017

Contributor

Jenkins re-test this please

Contributor

smithfarm commented Jun 19, 2017

Jenkins re-test this please

@cbodley

This comment has been minimized.

Show comment
Hide comment
@cbodley

cbodley Jun 21, 2017

Contributor

@smithfarm looks ok now?

Contributor

cbodley commented Jun 21, 2017

@smithfarm looks ok now?

@smithfarm

This comment has been minimized.

Show comment
Hide comment
@smithfarm

smithfarm Jun 21, 2017

Contributor

@cbodley Yeah.

Contributor

smithfarm commented Jun 21, 2017

@cbodley Yeah.

@smithfarm smithfarm changed the title from jewel: rgw multisite: fixes for meta sync across periods to jewel: rgw: multisite: fixes for meta sync across periods Jul 12, 2017

"--yes-i-really-mean-it." << std::endl;
return -EINVAL;
}
// empty sync status markers - other zones will skip this period during

This comment has been minimized.

@amitkumar50

amitkumar50 Aug 16, 2017

Contributor

@cbodley I believe It would be even better if we start comment with capital letter something as:
// Empty sync status markers - other zones will skip this period during

@amitkumar50

amitkumar50 Aug 16, 2017

Contributor

@cbodley I believe It would be even better if we start comment with capital letter something as:
// Empty sync status markers - other zones will skip this period during

This comment has been minimized.

@cbodley

cbodley Aug 16, 2017

Contributor

@amitkumar50 this is a backport pr, the original change has already merged into master. and why are we getting so picky about language/formatting in code comments?

@cbodley

cbodley Aug 16, 2017

Contributor

@amitkumar50 this is a backport pr, the original change has already merged into master. and why are we getting so picky about language/formatting in code comments?

This comment has been minimized.

@amitkumar50

amitkumar50 Aug 16, 2017

Contributor

@cbodley Thanks for response and backport and really appreciate your work. Its not picky, But i believe good practice to start sentence with Capital letter.

@amitkumar50

amitkumar50 Aug 16, 2017

Contributor

@cbodley Thanks for response and backport and really appreciate your work. Its not picky, But i believe good practice to start sentence with Capital letter.

@smithfarm

This comment has been minimized.

Show comment
Hide comment
@smithfarm

smithfarm Aug 16, 2017

Contributor

@amitkumar50 As @cbodley said, backports are cherry-picked from master, so any changes would have to go into master, first.

Code comments are not public-facing and we should "fix" them only if they contain factual errors. What you are suggesting here falls into the category of "wordsmithing" - for example, a full sentence should start with a capital letter and end with a period/fullstop. This would be fine for public-facing documentation, but in code comments it only serves to muddle the git history IMHO.

Contributor

smithfarm commented Aug 16, 2017

@amitkumar50 As @cbodley said, backports are cherry-picked from master, so any changes would have to go into master, first.

Code comments are not public-facing and we should "fix" them only if they contain factual errors. What you are suggesting here falls into the category of "wordsmithing" - for example, a full sentence should start with a capital letter and end with a period/fullstop. This would be fine for public-facing documentation, but in code comments it only serves to muddle the git history IMHO.

@mattbenjamin

This comment has been minimized.

Show comment
Hide comment
@mattbenjamin
Contributor

mattbenjamin commented Aug 16, 2017

@smithfarm

This comment has been minimized.

Show comment
Hide comment
@smithfarm

smithfarm Aug 29, 2017

Contributor

@cbodley This passed an rgw run at http://tracker.ceph.com/issues/20613#note-29

It also passed a rados run at http://tracker.ceph.com/issues/20613#note-25 - this run initially had some odd rgw-related errors, but they were not reproducible.

You can't review your own PR - who would be a good person to review it, do you think?

Contributor

smithfarm commented Aug 29, 2017

@cbodley This passed an rgw run at http://tracker.ceph.com/issues/20613#note-29

It also passed a rados run at http://tracker.ceph.com/issues/20613#note-25 - this run initially had some odd rgw-related errors, but they were not reproducible.

You can't review your own PR - who would be a good person to review it, do you think?

@cbodley

This comment has been minimized.

Show comment
Hide comment
@cbodley

cbodley Aug 29, 2017

Contributor

You can't review your own PR - who would be a good person to review it, do you think?

requesting review from @yehudasa, who reviewed the original #13070. in the meantime, i'll build and verify with the updated multisite tests

Contributor

cbodley commented Aug 29, 2017

You can't review your own PR - who would be a good person to review it, do you think?

requesting review from @yehudasa, who reviewed the original #13070. in the meantime, i'll build and verify with the updated multisite tests

@cbodley cbodley requested a review from yehudasa Aug 29, 2017

@cbodley

This comment has been minimized.

Show comment
Hide comment
@cbodley

cbodley Aug 29, 2017

Contributor

test_multi.py, including updated test_multi_period_incremental_sync(), still passing 👍

Contributor

cbodley commented Aug 29, 2017

test_multi.py, including updated test_multi_period_incremental_sync(), still passing 👍

@smithfarm

This comment has been minimized.

Show comment
Hide comment
@smithfarm

smithfarm Sep 2, 2017

Contributor

@yehudasa This passed another rgw suite at http://tracker.ceph.com/issues/20613#note-43

Waiting for your imprimatur.

Contributor

smithfarm commented Sep 2, 2017

@yehudasa This passed another rgw suite at http://tracker.ceph.com/issues/20613#note-43

Waiting for your imprimatur.

@smithfarm

This comment has been minimized.

Show comment
Hide comment
@smithfarm

smithfarm Sep 7, 2017

Contributor

This passed another rgw suite at http://tracker.ceph.com/issues/20613#note-53

Who can review?

Contributor

smithfarm commented Sep 7, 2017

This passed another rgw suite at http://tracker.ceph.com/issues/20613#note-53

Who can review?

@smithfarm smithfarm merged commit a683b04 into ceph:jewel Sep 12, 2017

5 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
Unmodified Submodules submodules for project are unmodified
Details
default Build finished.
Details
make check make check succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment