Skip to content

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

Merged
cbodley merged 2 commits intoceph:mainfrom
smanjara:wip-x-amz-replicated-at
Mar 15, 2024
Merged

rgw/multisite: add x-rgw-replicated-at header to replicated objects#55503
cbodley merged 2 commits intoceph:mainfrom
smanjara:wip-x-amz-replicated-at

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

@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
@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.

@smanjara smanjara force-pushed the wip-x-amz-replicated-at branch from 14674b8 to bc70da5 Compare February 12, 2024 21:25
cbodley
cbodley reviewed Feb 12, 2024
View reviewed changes
@mattbenjamin
Copy link
Contributor

mattbenjamin commented Feb 12, 2024

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

@smanjara smanjara force-pushed the wip-x-amz-replicated-at branch from bc70da5 to 9bd748d Compare February 13, 2024 16:07
cbodley
cbodley reviewed Feb 13, 2024
View reviewed changes
@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 smanjara force-pushed the wip-x-amz-replicated-at branch from 9bd748d to 626179f Compare February 14, 2024 22:56
@smanjara
Copy link
Contributor Author

smanjara commented Feb 14, 2024

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

done

cbodley
cbodley reviewed Feb 14, 2024
View reviewed changes
@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
@smanjara
Copy link
Contributor Author

smanjara commented Mar 15, 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/)

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

@cbodley cbodley Apr 8, 2024

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

@cbodley cbodley Apr 8, 2024

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

Development

Successfully merging this pull request may close these issues.

4 participants

@smanjara @cbodley @mattbenjamin @yuriw