Skip to content

Comments

rgw: Added Cosmetic change in etag of copyObjectResult#47527

Merged
cbodley merged 1 commit intoceph:mainfrom
shraddhaghatol:shr/etag_cosmetic_change
Sep 2, 2022
Merged

rgw: Added Cosmetic change in etag of copyObjectResult#47527
cbodley merged 1 commit intoceph:mainfrom
shraddhaghatol:shr/etag_cosmetic_change

Conversation

@shraddhaghatol
Copy link

@shraddhaghatol shraddhaghatol commented Aug 10, 2022 •
edited
Loading

Problem:
Observed discrepancy with AWS for CopyObject API's response.
In ceph-RADOS, "etag" field was not included in ("/ /")

Resolution:
Included etag field of CopyObject API's response in ("/ /") to make it AWS compliant

Signed-off-by: shraddhaghatol [email protected]

Contribution Guidelines

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

@github-actions github-actions bot added the rgw label Aug 10, 2022
@shraddhaghatol shraddhaghatol force-pushed the shr/etag_cosmetic_change branch from 63b4c9a to 65c8e7f Compare August 10, 2022 08:24
@shraddhaghatol
Copy link
Author

shraddhaghatol commented Aug 10, 2022

@alimaredia @cbodley @dang Could you please review this PR.

@shraddhaghatol shraddhaghatol marked this pull request as ready for review August 10, 2022 08:36
@cdeshmukh
Copy link

cdeshmukh commented Aug 10, 2022

@cbodley @alimaredia failures seen for 2 checks against this PR doesnt seem to be related to PR changes itself, can you please check & suggest how to correct those

ceph windows tests
apt-get error: The repository 'https://download.ceph.com/debian-pacific jammy Release' does not have a Release file

make check (arm64)
apt-get error: Malformed line 1 in source list /etc/apt/sources.list.d/libboost.list (type)

@cdeshmukh cdeshmukh mentioned this pull request Aug 10, 2022
Merged
14 tasks
cbodley
cbodley reviewed Aug 11, 2022
View reviewed changes
src/rgw/rgw_op.cc Outdated
this,
s->yield);

etag = "\"" + etag + "\"";
Copy link
Contributor

@cbodley cbodley Aug 11, 2022

Choose a reason for hiding this comment

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

we format the response in https://github.com/ceph/ceph/blob/5ab5ff46/src/rgw/rgw_rest_s3.cc#L3470-L3478. please add quotes to the etag like other requests do (ex. https://github.com/ceph/ceph/blob/5ab5ff46/src/rgw/rgw_rest_s3.cc#L1737):

-      s->formatter->dump_string("ETag", std::move(etag));
+      s->formatter->dump_format("ETag", "\"%s\"", etag.c_str());

Copy link
Author

@shraddhaghatol shraddhaghatol Aug 16, 2022

Choose a reason for hiding this comment

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

Sure. Will do the change.

Copy link
Author

@shraddhaghatol shraddhaghatol Aug 22, 2022

Choose a reason for hiding this comment

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

@cbodley Added change suggested by you.

@shraddhaghatol shraddhaghatol force-pushed the shr/etag_cosmetic_change branch from 65c8e7f to 0a72f8c Compare August 12, 2022 13:59
@tchaikov
Copy link
Contributor

tchaikov commented Aug 16, 2022

jenkins test make check

@shraddhaghatol shraddhaghatol force-pushed the shr/etag_cosmetic_change branch 2 times, most recently from d405f05 to 01c0758 Compare August 22, 2022 13:12
@shraddhaghatol shraddhaghatol force-pushed the shr/etag_cosmetic_change branch from 01c0758 to 4a11712 Compare August 22, 2022 13:19
@shraddhaghatol
Copy link
Author

shraddhaghatol commented Aug 25, 2022

@cbodley saw your approval. Could you please share the plan to merge this PR.

@cdeshmukh
Copy link

cdeshmukh commented Aug 29, 2022

@cbodley @alimaredia can we merge this please? Am not sure of issues failing in PR checklist & if they apply to this one.

@cbodley
Copy link
Contributor

cbodley commented Aug 29, 2022

@cbodley saw your approval. Could you please share the plan to merge this PR.

@shraddhaghatol the needs-qa tag means it's ready for testing. we occasionally batch up PRs with this tag and run them through teuthology for validation

if you or @cdeshmukh would like to help test this yourself, the first step is to request vpn access to the sepia lab: https://wiki.sepia.ceph.com/doku.php?id=vpnaccess. when you file a ticket, feel free to link to this PR - Ali and I are both happy to vouch for you

you can read more about the integration tests in https://docs.ceph.com/en/latest/dev/developer_guide/testing_integration_tests/tests-integration-testing-teuthology-intro/#how-to-run-integration-tests. once you have vpn access, we can share the exact command we use to schedule the rgw suite of tests

@ivancich
Copy link
Member

ivancich commented Aug 31, 2022

Since you checked off "references tracker ticket" please provide a link to that ticket in the description, @shraddhaghatol .

@shraddhaghatol
Copy link
Author

shraddhaghatol commented Sep 1, 2022

Since you checked off "references tracker ticket" please provide a link to that ticket in the description, @shraddhaghatol .

@ivancich Added this check accidently. Removed it now.

@cbodley
Copy link
Contributor

cbodley commented Sep 1, 2022

@shraddhaghatol @cdeshmukh i've added this to my wip-cbodley-testing branch with 3 other PRs. i pushed that branch to the ceph-ci repo at https://github.com/ceph/ceph-ci/commits/wip-cbodley-testing, which triggers CI to build packages/containers that will show up under https://shaman.ceph.com/builds/ceph/wip-cbodley-testing/. once the builds are done, i'll schedule the rgw suite with a command like this:

$ teuthology-suite -s rgw -m smithi -c wip-cbodley-testing --subset 1/5 -p 75

test results will eventually show up under https://pulpito.ceph.com/?suite=rgw. i'll follow up with a link to the results, and merge if there are no regressions

@cbodley
Copy link
Contributor

cbodley commented Sep 1, 2022

jenkins test api

@cdeshmukh
Copy link

cdeshmukh commented Sep 1, 2022

if you or @cdeshmukh would like to help test this yourself, the first step is to request vpn access to the sepia lab:

thanks @cbodley for the suggestion, let us get acquainted with teuthology first and may be in sometime future we can plan for PRs coming from cortx-rgw, so not for this PR at least.

@cbodley
Copy link
Contributor

cbodley commented Sep 1, 2022

thanks @cbodley for the suggestion, let us get acquainted with teuthology first and may be in sometime future we can plan for PRs coming from cortx-rgw, so not for this PR at least.

sounds good! we welcome your contributions either way ❤️

@cbodley
Copy link
Contributor

cbodley commented Sep 2, 2022

passed qa in https://pulpito.ceph.com/cbodley-2022-09-01_15:25:15-rgw-wip-cbodley-testing-distro-default-smithi/

@cbodley cbodley merged commit 9f870f8 into ceph:main Sep 2, 2022
@cdeshmukh
Copy link

cdeshmukh commented Sep 2, 2022

thanks @cbodley

@VVoidV VVoidV mentioned this pull request Oct 24, 2024
Merged
14 tasks
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

+1 more reviewer

@cdeshmukh cdeshmukh cdeshmukh approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

needs-qa rgw wip-cbodley-testing

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@shraddhaghatol @cdeshmukh @tchaikov @cbodley @ivancich