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: polymorphic error codes #10690

Merged
merged 2 commits into from May 16, 2017

Conversation

Projects
None yet
6 participants
@mdw-at-linuxbox
Contributor

mdw-at-linuxbox commented Aug 11, 2016

[ Required for radosgw STS. See github.com/github.com:linuxbox2/linuxbox-ceph wip-rgw-sts-7 src/sts/rgw_rest_sts.cc sts_err &etc for usage. ]

make set_req_state_err a method of req_state

get rid of params.perr; it's never used

rgw - polymorphic error object.

polymorphic error object: this way sts, which has additional error fields,
and its own xml format for how errors should be produced, can overload
the base rgw_err class.

STL fixup: use real mappings for error mappings, not a linear search.
insubstantiate error mappings exactly once in src/rgw/rgw_common.cc, instead
of multiple copies from every source file that includes src/rgw/rgw_http_errors.h.

remove dump_errno(s, err) - it was never used, and the code looked bogus.

move rgw_http_error_to_errno from header to rgw_rest_client.cc,
the only place it's used.
move rgw_http_status_code and http_codes to rgw_rest.cc,
the only place it's used.

base class (rgw_err) implements s3 errors; swift & sts errors use
the base class and overlay their own additional error codes to
the ones recognized by the base class.

For a while, I was trying to just have a "generic" abort_early() that
used dump_more. Now, I have an op aware form that works the old way.
I would like the http routines to known less about "op" structures,
which is why I'm making this distinction.

Splitting up generic and s3 flavors of some things:
rgw_rest_init() - split into s3 and generic forms.
end_header: s3 vs generic forms: generic form uses dump_more.
abort_early: s3 vs generic forms: generic form uses dump_more, error_content.

And fixing some mistakes from the merge:
Fixup: should not have deleted dump_errno() local storage forms.
Fixup:RGWRESTFlusher - take a dump_more to make it generic.
Fixup: need RGWRESTOp::send_response()

Move swift_err up; use it in bulkdelete_respond
I Think I don't need s->prot_flags when using swift_err.

Signed-off-by: Marcus Watts mwatts@redhat.com

@cbodley cbodley added the rgw label Aug 11, 2016

@oritwas

This comment has been minimized.

Contributor

oritwas commented Aug 12, 2016

looks good.
Can you make err statically allocated?

@cbodley

This comment has been minimized.

Contributor

cbodley commented Sep 7, 2016

Can you make err statically allocated?

i agree that we really don't want to allocate an error code for every request. we already have polymorphism in the Handlers, so i think the formatting logic should be a part of that instead of the rgw_err itself

@mdw-at-linuxbox

This comment has been minimized.

Contributor

mdw-at-linuxbox commented Nov 14, 2016

Properly allocating a polymorphic err structure based on Handler means allocating req_state after calling rest->get_handler, which is a pretty big change. Right now, req_state is allocated way early, before much is known about the request that is being processed. So allocating it statically means doing some kind of union or other memory allocation trick.

I've pushed a new version of this change rebased on a current(ish) master. Since that version of master doesn't pass "make check", this version won't either.

However, this version should pass s3-tests and swifttests, including test_object_requestid_on_error and test_object_requestid_matchs_header_on_error.

@ghost

This comment has been minimized.

ghost commented Nov 17, 2016

jenkins test this please (no logs)

@ghost

This comment has been minimized.

ghost commented Nov 21, 2016

jenkins test this please (eio now ignored in master)

@cbodley

This comment has been minimized.

Contributor

cbodley commented Nov 21, 2016

in offline discussion, boost::variant<> was suggested to avoid new allocations for errors

@pritha-srivastava

This comment has been minimized.

Contributor

pritha-srivastava commented Dec 8, 2016

@cbodley : Please take a look.

if (s->format != RGW_FORMAT_HTML)
s->formatter->close_section();
}

This comment has been minimized.

@cbodley

cbodley Dec 8, 2016

Contributor

i think rgw_err::dump() is the only part of rgw_err that depends on having this req_state *s pointer. can we just add that as an argument to dump(), so that we don't need to store it inside of rgw_err and its variant types?

const string& get_err_message() const;
void set_err_message(const string& err_msg);
const string& get_err_code() const;
void set_err_code(const string& err_code);

This comment has been minimized.

@cbodley

cbodley Dec 8, 2016

Contributor

i'm not sure that the error message and code should be part of the rgw_err interface - that assumes that all error types, including sts, will have them - and if that is the case, they should be in rgw_err instead of the variant types

@mdw-at-linuxbox it would really help if you could share your sts subclass of rgw_err so we know exactly what the requirements are

This comment has been minimized.

@pritha-srivastava

pritha-srivastava Dec 8, 2016

Contributor

@cbodley : I agree that if err_code and message are common to all three, then it should be part of rgw_err, but then classes s3_err and swift_err will be empty?

This comment has been minimized.

@pritha-srivastava

pritha-srivastava Dec 8, 2016

Contributor

I think the code for STS is here: git@github.com:linuxbox2/linuxbox-ceph.git wip-rgw-sts-9 . I'll check that tomorrow.

This comment has been minimized.

@pritha-srivastava

pritha-srivastava Dec 9, 2016

Contributor

@mdw-at-linuxbox : I took a look at the above branch, but didnt find the STS subclass of rgw_err. Can you please point me to the correct repo/ branch.

This comment has been minimized.

@cbodley

cbodley Dec 9, 2016

Contributor

here are links to struct sts_err and sts_err::dump(), which is printing out both s3_code and message if they're present

@cbodley

This comment has been minimized.

Contributor

cbodley commented Dec 9, 2016

after reading through rgw_rest_sts.h, i'm going to suggest again that we extend our existing RGWHandler interface instead of making changes to rgw_err itself

i.e. add something like this to RGWHandler:

  /// use the given error code to initialize req_state::err, returning false
  /// if no mapping is found
  virtual bool set_error_code(int err_code) = 0;

  /// format the error in req_state::err for the response body
  virtual void dump_error(Formatter *f) = 0;

the RGWHandler_REST_S3 and RGWHandler_REST_SWIFT implementations would be pretty much identical, except that swift would include its own error mapping

for STS, the extra fields in struct sts_err (tsErrorType type and uuid_d request_id) could either go into req_state directly, or encapsulated in a plain-old-data version of struct sts_err

doesn't that sound a lot simpler?

@@ -29,6 +31,75 @@ PerfCounters *perfcounter = NULL;
const uint32_t RGWBucketInfo::NUM_SHARDS_BLIND_BUCKET(UINT32_MAX);
rgw_http_errors rgw_http_s3_errors({

This comment has been minimized.

@Liuchang0812

Liuchang0812 Dec 15, 2016

Contributor

hi, @mdw-at-linuxbox , s3 errors need "s3 code" and "s3 message" in MultiDelete request. FYI #12485
We should add a member called error_message to rgw_http_errors struct.

@@ -267,18 +269,21 @@ enum RGWObjCategory {
/** Store error returns for output at a different point in the program */
struct rgw_err {
rgw_err();
rgw_err(int http, const std::string &s3);
virtual ~rgw_err() { };

This comment has been minimized.

@cbodley

cbodley Dec 15, 2016

Contributor

don't need the virtual dtor anymore

@@ -1590,6 +1593,8 @@ class RGWHandler {
virtual int authorize() = 0;
virtual int postauth_init() = 0;
virtual int error_handler(int err_no, string *error_content);
virtual bool set_rgw_err(int err_no, bool& is_website_redirect, int& http_ret, string& code) { return false;}

This comment has been minimized.

@cbodley

cbodley Dec 15, 2016

Contributor

is_website_redirect appears to be an input parameter, right? if that's the case, it shouldn't be a reference

@@ -17,8 +18,7 @@
#include "rgw_rest_s3.h"
#include "rgw_swift_auth.h"
#include "rgw_cors_s3.h"
#include "rgw_http_errors.h"
#include "rgw_lib.h"
// #include "rgw_lib.h" // XXX mdw is this necessary?

This comment has been minimized.

@cbodley

cbodley Dec 20, 2016

Contributor

it looks like we can remove this line

@@ -1369,7 +1372,12 @@ struct req_state {
req_state(CephContext* _cct, RGWEnv* e, RGWUserInfo* u);
~req_state();
void set_req_state_err(int err_no, RGWHandler* handler);
void set_req_state_err(int err_no, const string &err_msg, RGWHandler* handler);

This comment has been minimized.

@cbodley

cbodley Dec 20, 2016

Contributor

i don't think we really need to move these inside of req_state - can we leave them as free functions?

This comment has been minimized.

@pritha-srivastava

pritha-srivastava Dec 21, 2016

Contributor

@mdw-at-linuxbox : Do you have any specific reason for keeping them within req_state, else I will move them out as free functions.

@@ -104,6 +105,8 @@ RGWOp() : s(nullptr), dialect_handler(nullptr), store(nullptr),
virtual uint32_t op_mask() { return 0; }
virtual int error_handler(int err_no, string *error_content);
boost::function<void()> dump_access_control_f();

This comment has been minimized.

@cbodley

cbodley Dec 20, 2016

Contributor

i don't understand the motivation for this boost::function part.. why can't end_header() continue to call dump_access_control() directly?

the commit message says I would like the http routines to known less about "op" structures, but Op and Handler are two of the most basic parts of our rest framework

This comment has been minimized.

@pritha-srivastava

pritha-srivastava Dec 21, 2016

Contributor

@mdw-at-linuxbox : Do you have a specific reason for the above comment?

This comment has been minimized.

@pritha-srivastava

pritha-srivastava May 4, 2017

Contributor

@cbodley : I am thinking of removing dump_access_control_f(), since I have already reverted to calling dump_access_control() directly. Let me know what you think.

err_no = new_err_no;
}
if (op) dump_more = op->dump_access_control_f();
abort_early(s, dump_more, error_content, err_no, handler);

This comment has been minimized.

@cbodley

cbodley Dec 20, 2016

Contributor

there is a check for if (handler != NULL) above - can you tell whether we have callers that will actually pass null?

and maybe add an assert(handler); before your calls to handler->set_rgw_err() and handler->dump() to make sure we aren't hitting that in teuthology

This comment has been minimized.

@pritha-srivastava

pritha-srivastava Dec 21, 2016

Contributor

abort_early is called from various points in process_request; one such place is when we do not get an appropriate manager/ handler for the incoming URI, in this case the handler passed to abort_early is NULL, hence the check.

We would have aborted the request early in case the handler is NULL, but just to be on the safer side, i'll add the assert.

@idealguo

This comment has been minimized.

idealguo commented Mar 24, 2017

how about the progress? there are some conflicts now

@oritwas

This comment has been minimized.

Contributor

oritwas commented Apr 3, 2017

@mdw-at-linuxbox , please rebase

@pritha-srivastava

This comment has been minimized.

Contributor

pritha-srivastava commented Apr 5, 2017

Rebased the code, now preparing to run it through teuthology.

@pritha-srivastava

This comment has been minimized.

Contributor

pritha-srivastava commented Apr 13, 2017

@cbodley: Can you please review the changes.

void clear();
bool is_clear() const;
bool is_err() const;
friend std::ostream& operator<<(std::ostream& oss, const rgw_err &err);
bool is_website_redirect;

This comment has been minimized.

@cbodley

cbodley May 2, 2017

Contributor

it doesn't look like this needs to be a member of rgw_err. it only appears to be used in set_req_state_err(), where it could just as easily be a local variable

@@ -1665,6 +1667,7 @@ struct req_init_state {
/* XXX why don't RGWRequest (or descendants) hold this state? */
class RGWRequest;
class RGWHandler;

This comment has been minimized.

@cbodley

cbodley May 2, 2017

Contributor

unused

@@ -20,6 +20,7 @@ extern std::map<std::string, std::string> rgw_to_http_attrs;
extern string camelcase_dash_http_attr(const string& orig);
extern string lowercase_dash_http_attr(const string& orig);
extern void rgw_rest_init(CephContext *cct);

This comment has been minimized.

@cbodley

cbodley May 2, 2017

Contributor

i don't see this overload used, other than being called by the other rgw_rest_init(CephContext *cct, RGWRados *store, RGWZoneGroup& zone_group) overload. could you revert those changes?

@@ -1093,7 +1087,7 @@ void RGWRESTFlusher::do_start(int ret)
set_req_state_err(s, ret); /* no going back from here */
dump_errno(s);
dump_start(s);
end_header(s, op);
end_header(s);

This comment has been minimized.

@cbodley

cbodley May 2, 2017

Contributor

why remove this op argument?

This comment has been minimized.

@pritha-srivastava

pritha-srivastava May 4, 2017

Contributor

This was by mistake. I double checked every end_header() call that I had modified.

@@ -2045,7 +2044,6 @@ void RGWPostObj_ObjStore_S3::send_response()
s->formatter->dump_string("Key", s->object.name);
s->formatter->close_section();
}
s->err.message = err_msg;

This comment has been minimized.

@cbodley

cbodley May 2, 2017

Contributor

should either keep this line, or add err_msg as an argument to set_req_state_err() below

const char *s3_code;
};
void rgw_get_errno_s3(struct rgw_http_error *e, int err_no);

This comment has been minimized.

@cbodley

cbodley May 2, 2017

Contributor

rgw_get_errno_s3() doesn't appear to be used anywhere, struct rgw_http_error is only used inside

This comment has been minimized.

@pritha-srivastava

pritha-srivastava May 4, 2017

Contributor

There is one call to rgw_get_errno_s3() in RGWDeleteMultiObj_ObjStore_S3::send_partial_response()

@cbodley

cbodley approved these changes May 5, 2017

@pritha-srivastava

This comment has been minimized.

Contributor

pritha-srivastava commented May 10, 2017

@cbodley : http://pulpito.ceph.com/prsrivas-2017-05-08_04:25:59-rgw-wip-rgw-polymorphic-errors---basic-smithi/ has the results of the teuthology run. All errors are unrelated to changes in this PR. I need to open a tracker issue for a valgrind issue related to RGWCoroutine().

Would you want me to squash all the commits before the code is merged?

@cbodley

This comment has been minimized.

Contributor

cbodley commented May 10, 2017

Would you want me to squash all the commits before the code is merged?

@pritha-srivastava yes please, as long as it preserves Marcus' initial commit message and sign-off

@pritha-srivastava

This comment has been minimized.

Contributor

pritha-srivastava commented May 11, 2017

@cbodley : I have squashed the commits. Not sure why this build failure repeatedly appears, the piece of code that is apparently causing the build to fail, is not there in the code at all. The builds in shaman for this branch are all fine.

@pritha-srivastava

This comment has been minimized.

Contributor

pritha-srivastava commented May 15, 2017

jenkins test this please

@cbodley

This comment has been minimized.

Contributor

cbodley commented May 15, 2017

@pritha-srivastava that error message does look super confusing, but it looks like the jenkins bot is building a rebased version of this branch. i was able to reproduce the same failure after rebasing

mdw-at-linuxbox and others added some commits Nov 3, 2015

polymorphic error codes.
make set_req_state_err a method of req_state

get rid of params.perr; it's never used

rgw - polymorphic error object.

polymorphic error object: this way sts, which has additional error fields,
and its own xml format for how errors should be produced, can overload
the base rgw_err class.

STL fixup: use real mappings for error mappings, not a linear search.
insubstantiate error mappings exactly once in src/rgw/rgw_common.cc, instead
of multiple copies from every source file that includes src/rgw/rgw_http_errors.h.

remove dump_errno(s, err) - it was never used, and the code looked bogus.

move rgw_http_error_to_errno from header to rgw_rest_client.cc,
the only place it's used.
move rgw_http_status_code and http_codes to rgw_rest.cc,
the only place it's used.

base class (rgw_err) implements s3 errors; swift & sts errors use
the base class and overlay their own additional error codes to
the ones recognized by the base class.

For a while, I was trying to just have a "generic" abort_early() that
used dump_more.  Now, I have an op aware form that works the old way.
I would like the http routines to known less about "op" structures,
which is why I'm making this distinction.

Splitting up generic and s3 flavors of some things:
rgw_rest_init() - split into s3 and generic forms.
end_header: s3 vs generic forms: generic form uses dump_more.
abort_early: s3 vs generic forms: generic form uses dump_more, error_content.

And fixing some mistakes from the merge:
Fixup: should not have deleted dump_errno() local storage forms.
Fixup:RGWRESTFlusher  - take a dump_more to make it generic.
Fixup: need RGWRESTOp::send_response()

Move swift_err up; use it in bulkdelete_respond
I Think I don't need s->prot_flags when using swift_err.

Signed-off-by: Marcus Watts <mwatts@redhat.com>
rgw: Searching for error codes and dumping output based on protocol f…
…lags.

The previous commit uses dynamic allocation to search for s3/ swift specific
error codes and dump the results. In order to avoid dynamic allocation,
we are making use of protocol flags to achieve the above.

Signed-off-by: Pritha Srivastava <prsrivas@redhat.com>
@pritha-srivastava

This comment has been minimized.

Contributor

pritha-srivastava commented May 16, 2017

jenkins test this please

@pritha-srivastava

This comment has been minimized.

Contributor

pritha-srivastava commented May 16, 2017

@cbodley : Finally, this branch built successfully!

@cbodley cbodley changed the title from polymorphic error codes. to rgw: polymorphic error codes May 16, 2017

@cbodley cbodley merged commit 978f1d3 into master May 16, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details

@liewegas liewegas deleted the wip-rgw-polymorphic-errors branch Aug 25, 2017

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