Skip to content
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.

Sign up for GitHub

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

Jump to bottom

rgw/multisite: add x-rgw-replicated-at header to replicated objects #55503

Merged
merged 2 commits into from Mar 15, 2024
Merged

rgw/multisite: add x-rgw-replicated-at header to replicated objects #55503

merged 2 commits into from Mar 15, 2024

Conversation

smanjara
Copy link
Contributor

@smanjara smanjara commented Feb 8, 2024

fixes: https://tracker.ceph.com/issues/64365

Contribution Guidelines

  • To sign and title your commits, please refer to Submitting Patches to Ceph.

  • If you are submitting a fix for a stable branch (e.g. "quincy"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.

  • When filling out the below checklist, you may click boxes directly in the GitHub web UI. When entering or editing the entire PR message in the GitHub web UI editor, you may also select a checklist item by adding an x between the brackets: [x]. Spaces and capitalization matter when checking off items this way.

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)
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
  • jenkins test rook e2e

@smanjara smanjara requested a review from a team as a code owner February 8, 2024 17:33
@github-actions github-actions bot added the rgw label Feb 8, 2024
cbodley
cbodley reviewed Feb 8, 2024
View reviewed changes
Copy link
Contributor

@cbodley cbodley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks great

src/rgw/driver/rados/rgw_rados.cc Outdated Show resolved Hide resolved
@smanjara
Copy link
Contributor Author

smanjara commented Feb 8, 2024

probably we don't need a nanosecond precision if we expect a user to take the difference between user.rgw.amz-replicated-at and user.rgw.x-amz-date

@cbodley
Copy link
Contributor

cbodley commented Feb 8, 2024

probably we don't need a nanosecond precision if we expect a user to take the difference between user.rgw.amz-replicated-at and user.rgw.x-amz-date

from rgw_rest_s3.cc, it looks like we're using dump_time() for LastModified, which calls through rgw_to_iso8601() for the formatting. probably better to use the same for replicated-at

cbodley
cbodley reviewed Feb 8, 2024
View reviewed changes
src/rgw/rgw_rest_s3.cc Outdated Show resolved Hide resolved
@smanjara smanjara changed the title rgw/multisite: add x-amx-replicated-at header to replicated objects rgw/multisite: add x-rgw-replicated-at header to replicated objects Feb 8, 2024
@smanjara
Copy link
Contributor Author

smanjara commented Feb 8, 2024

@mattbenjamin could you describe what you had in mind? or may be refactoring call is ok too if you want to discuss it there.

@mattbenjamin
Copy link
Contributor

this completely addresses getting the correct replication time from the source zone to its destination; I think Bloomberg framed the conversation as being about propagating the results back to the origin--which has several problems, though I can imagine a way to get the information there using the replication log; We should get them to do a code review of the current PR

cbodley
cbodley reviewed Feb 13, 2024
View reviewed changes
src/rgw/rgw_admin.cc Outdated Show resolved Hide resolved
@cbodley cbodley added this to the squid milestone Feb 14, 2024
@cbodley
Copy link
Contributor

cbodley commented Feb 14, 2024

would you mind adding a note to PendingReleaseNotes to advertise this new feature? something like

RGW: GetObject and HeadObject requests now return a `x-rgw-replicated-at` header for replicated objects. This timestamp can be compared against the `Last-Modified` header to determine how long the object took to replicate.

rgw/multisite: handle object stat output of attrs
626179f 
"user.rgw.replicated-at"

Signed-off-by: Shilpa Jagannath <smanjara@redhat.com>
@smanjara
Copy link
Contributor Author

would you mind adding a note to PendingReleaseNotes to advertise this new feature? something like

done

@cbodley
Copy link
Contributor

cbodley commented Feb 14, 2024

can you think of an easy test case we could write to exercise this?

@cbodley
Copy link
Contributor

cbodley commented Feb 22, 2024

@smanjara i included this in a test batch that passed qa (https://pulpito.ceph.com/cbodley-2024-02-20_00:28:35-rgw-wip-cbodley-testing-distro-default-smithi/ with rerun https://pulpito.ceph.com/cbodley-2024-02-21_19:22:22-rgw-wip-cbodley-testing-distro-default-smithi/)

@cbodley cbodley merged commit 694f19b into ceph:main Mar 15, 2024
10 of 11 checks passed
@smanjara
Copy link
Contributor Author

@smanjara i included this in a test batch that passed qa (https://pulpito.ceph.com/cbodley-2024-02-20_00:28:35-rgw-wip-cbodley-testing-distro-default-smithi/ with rerun https://pulpito.ceph.com/cbodley-2024-02-21_19:22:22-rgw-wip-cbodley-testing-distro-default-smithi/)

thank you!

@cbodley cbodley mentioned this pull request Mar 15, 2024
Merged
cbodley
cbodley reviewed Apr 8, 2024
View reviewed changes
src/rgw/rgw_rest_s3.cc
try {
ceph::real_time replicated_time;
decode(replicated_time, i->second);
dump_time(s, "x-rgw-replicated-at", replicated_time);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@smanjara i think we want to use dump_time_header() here instead? dump_time() is calling s->formatter->dump_string() to format this as json/xml

i'm tracking a regression here in https://tracker.ceph.com/issues/65373

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix in #56765

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cbodley cbodley cbodley approved these changes

Assignees
No one assigned
Labels
documentation rgw
Projects
None yet
Milestone
squid
4 participants
@smanjara @cbodley @mattbenjamin @yuriw