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.

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

rgw: RGWRESTOp no longer tracks separate error code #37052

Merged
merged 1 commit into from
Sep 11, 2020

Conversation

cbodley
Copy link
Contributor

@cbodley cbodley commented Sep 8, 2020

RGWOp_Bucket_Remove::execute() was storing failures from bucket->remove_bucket() in op_ret, but left http_ret=0 so we responded to the client with '200 OK'

to avoid bugs like this, remove the extra http_ret variable and only use the op_ret from RGWOp

Checklist

  • References tracker ticket
  • Updates documentation if necessary
  • Includes tests for new functionality or reproducer for bug

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 api
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox

@cbodley
Copy link
Contributor Author

cbodley commented Sep 8, 2020

@mkogan1 this should at least make the admin api for bucket delete return the correct error

Copy link
Contributor

@dang dang left a comment

Choose a reason for hiding this comment

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

This seems like a good idea to me. I was just looking at this today.

@mattbenjamin
Copy link
Contributor

mattbenjamin commented Sep 8, 2020

can the correct http error code really be inferred from op_ret--esp. in rgw_process(), where there is no context--in all cases though? I've assumed some kind of revised model is maybe needed on RGWOp to deal with this
@cbodley @dang

@cbodley
Copy link
Contributor Author

cbodley commented Sep 8, 2020

can the correct http error code really be inferred from op_ret--esp. in rgw_process(), where there is no context--in all cases though? I've assumed some kind of revised model is maybe needed on RGWOp to deal with this

@mattbenjamin the result we send back to clients is stored in req_state::err. ops call set_req_state_err(s, op_ret); to assign this

note that this change is limited to admin APIs, since they're the only ones that derive from class RGWRESTOp, and just makes them consistent with other RGWOps

@cbodley
Copy link
Contributor Author

cbodley commented Sep 8, 2020

the api test failure shows this working. the -ECANCELED from RGWOp_Bucket_Remove is correctly being returned to the client as a 500 error:

2020-09-08 18:55:45,267.267 INFO:__main__:Traceback (most recent call last):
2020-09-08 18:55:45,267.267 INFO:__main__:  File "/home/jenkins-build/build/workspace/ceph-api/qa/tasks/mgr/dashboard/test_rgw.py", line 285, in test_all
2020-09-08 18:55:45,267.267 INFO:__main__:    self.assertStatus(204)
2020-09-08 18:55:45,267.267 INFO:__main__:  File "/home/jenkins-build/build/workspace/ceph-api/qa/tasks/mgr/dashboard/helper.py", line 392, in assertStatus
2020-09-08 18:55:45,267.267 INFO:__main__:    self.assertEqual(self._resp.status_code, status)
2020-09-08 18:55:45,268.268 INFO:__main__:AssertionError: 500 != 204

@cbodley
Copy link
Contributor Author

cbodley commented Sep 9, 2020

RGWOp_Bucket_Remove::execute() was storing failures from
bucket->remove_bucket() in op_ret, but left http_ret=0 so we responded
to the client with '200 OK'

to avoid bugs like this, remove the extra http_ret variable and only use
the op_ret from RGWOp

Signed-off-by: Casey Bodley <cbodley@redhat.com>
@cbodley
Copy link
Contributor Author

cbodley commented Sep 10, 2020

rebased over conflict with cls_fifo

@cbodley cbodley merged commit cd6e535 into ceph:master Sep 11, 2020
@cbodley cbodley deleted the wip-rgw-rest-op-err branch September 11, 2020 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants