Check for either 400 or 416 response code for invalid range test#195
Check for either 400 or 416 response code for invalid range test#195vasukulkarni merged 1 commit intomasterfrom
Conversation
vasukulkarni
force-pushed
the
wip-fix-416
branch
from
091eb89 to
36a5631
Compare
| # on RHEL we get response 416, check bugzilla 1501005 | ||
| valid_status = [400, 416] | ||
| if not any(s == e.status for s in valid_status): | ||
| raise AssertionError("Invalid response " + str(status)) |
There was a problem hiding this comment.
this will accept 400 or 416 for e.status, but the e.reason and e.error_code checks below are still specific to 400 Bad Request - right?
There was a problem hiding this comment.
I haven't got past due to first assert, my guess was reason and error code should be same, I will know in new runs and update here.
There was a problem hiding this comment.
ok. for 416, i would expect e.reason to be Requested Range Not Satisfiable. for e.error_code, i don't think the requirements are well-defined, so it's probably best to ignore it
There was a problem hiding this comment.
I am bit surprised the tests worked here: http://pulpito.ceph.redhat.com/vasu-2017-11-03_14:24:45-rgw:multifs-luminous---basic-multi
Compared to one without fix:
http://pulpito.ceph.redhat.com/vasu-2017-11-01_18:16:45-rgw:multifs-luminous---basic-multi/
There was a problem hiding this comment.
huh checking the branch, it used ceph-master, :(, so it never ran that unit test
vasukulkarni
force-pushed
the
wip-fix-416
branch
from
36a5631 to
45ecf5d
Compare
|
@cbodley updated, going to retest with force-branch |
vasukulkarni
force-pushed
the
wip-fix-416
branch
2 times, most recently
from
f43128c to
626aa98
Compare
|
2017-11-03T16:36:42.374 INFO:teuthology.orchestra.run.clara003.stderr:s3tests.functional.test_s3.test_multipart_copy_invalid_range ... ok |
s3tests/functional/test_s3.py
Outdated
| eq(e.error_code, 'InvalidArgument') | ||
| # on RHEL we get response 416, check bugzilla 1501005 | ||
| valid_status = [400, 416] | ||
| if not any(s == e.status for s in valid_status): |
There was a problem hiding this comment.
if e.status not in valid_status:
will also work here
There was a problem hiding this comment.
better!, will update
s3tests/functional/test_s3.py
Outdated
| if not any(s == e.status for s in valid_status): | ||
| raise AssertionError("Invalid response " + str(status)) | ||
| valid_reason = ['Bad Request', 'Requested Range Not Satisfiable'] | ||
| if not any(r == e.reason for r in valid_reason): |
|
We need this commit for luminous branch as well now, seeing failures because of this http://pulpito.ceph.com/abhi-2017-11-05_16:46:58-rgw-wip-abhi-testing-2017-11-05-1320-distro-basic-smithi/ |
s3tests/functional/test_s3.py
Outdated
| eq(e.status, 400) | ||
| eq(e.reason, 'Bad Request') | ||
| eq(e.error_code, 'InvalidArgument') | ||
| # on RHEL we get response 416, check bugzilla 1501005 |
There was a problem hiding this comment.
not necessary to mention rhel or bugzillas, all versions of radosgw behave this way
There was a problem hiding this comment.
@cbodley if all versions behave same, do we need to check for 400 status code? can it be only 416 now? Not sure why it is 400?
There was a problem hiding this comment.
that's how it was submitted in #179, presumably because the referenced s3proxy change returned 400 there
Signed-off-by: Vasu Kulkarni <vasu@redhat.com>
vasukulkarni
force-pushed
the
wip-fix-416
branch
from
626aa98 to
e208a74
Compare
|
@cbodley @theanalyst updated |
|
@yehudasa can you please review/merge? we had last discussed this in #179 (comment) |
|
Tested this internally. |
3 participants
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1501005
Also needs backport to ceph-luminous.
Signed-off-by: Vasu Kulkarni vasu@redhat.com