rgw: improve handling of Swift's error messages and limits #15369

rzarzynski · 2017-05-30T17:10:33Z

These patches improve Swift's error handling and bring support for several limits (mandatory in recent Tempest versions) which are enumerated in the /info. The number of changes have been minimised for the sake of backportability. It's expected they will be later (most likely after Luminous) supplemented with broader rework of the error handling infrastructure in RadosGW.

Fixes:

Best works with: #15273, #12704.

Derogates: #12109, #12089.

rzarzynski · 2017-05-30T17:10:58Z

src/common/config_opts.h

@@ -1438,6 +1438,7 @@ OPTION(rgw_put_obj_min_window_size, OPT_INT, 16 * 1024 * 1024)
 OPTION(rgw_put_obj_max_window_size, OPT_INT, 64 * 1024 * 1024)
 OPTION(rgw_max_put_size, OPT_U64, 5ULL*1024*1024*1024)
 OPTION(rgw_max_put_param_size, OPT_U64, 1 * 1024 * 1024) // max input size for PUT requests accepting json/xml params
+OPTION(rgw_max_attrs_num_in_req, OPT_U64, 0)


DOCUMENT ME.

rzarzynski · 2017-05-30T22:42:16Z

Both Tempest and s3-tests haven't found any regression here.

mattbenjamin · 2017-05-31T20:33:30Z

this looks good to me

mattbenjamin · 2017-05-31T20:27:06Z

src/rgw/rgw_op.cc

@@ -1526,7 +1526,7 @@ void RGWGetObj::execute()
  if (torrent.get_flag())
  {
    torrent.init(s, store);


mattbenjamin · 2017-05-31T20:31:27Z

src/rgw/rgw_rest_swift.cc

+  if (op_ret == -EFBIG) {
+    /* Handle the custom error message of exceeding maximum custom attribute
+     * (stored as xattr) size. */
+    const auto error_message = boost::str(


interesting, boost::format idiom

rzarzynski · 2017-06-12T12:54:05Z

The branch has been tested in following Teuthology runs:

Reference point:

http://pulpito.ceph.com/rzarzynski-2017-06-08_13:55:44-rgw-wip-rzarzynski-testing-master---basic-mira/.

The rgw multisite test failures don't seem to be related. The assertion on weak refs in OSD is well-known and appears in other, unrelated branches as well. In the log we can find:

2017-06-08T19:00:51.548 INFO:tasks.ceph.c1.osd.2.mira041.stderr:/home/jenkins-build/build/workspace/ceph-dev-new-build/ARCH/x86_64/AVAILABLE_ARCH/x86_64/AVAILABLE_DIST/centos7/DIST/centos7/MACHINE_SIZE/huge/release/12.0.2-2416-g3beddb9/rpm/el7/BUILD/ceph-12.0.2-2416-g3beddb9/src/common/shared_cache.hpp: 105: FAILED assert(weak_refs.empty())
2017-06-08T19:00:51.548 INFO:tasks.ceph.c1.osd.2.mira041.stderr:
2017-06-08T19:00:51.549 INFO:tasks.ceph.c1.osd.2.mira041.stderr: ceph version  12.0.2-2416-g3beddb9 (3beddb967cf6cbaaa6712bab86dbf3822cbe0843) luminous (dev)
2017-06-08T19:00:51.549 INFO:tasks.ceph.c1.osd.2.mira041.stderr: 1: (ceph::__ceph_assert_fail(char const*, char const*, int, char const*)+0x110) [0xafa850]
2017-06-08T19:00:51.549 INFO:tasks.ceph.c1.osd.2.mira041.stderr: 2: (()+0x5030da) [0x60b0da]
2017-06-08T19:00:51.549 INFO:tasks.ceph.c1.osd.2.mira041.stderr: 3: (OSDService::~OSDService()+0x17c) [0x594bac]
2017-06-08T19:00:51.549 INFO:tasks.ceph.c1.osd.2.mira041.stderr: 4: (OSD::~OSD()+0x133) [0x5e31a3]
2017-06-08T19:00:51.551 INFO:tasks.ceph.c1.osd.2.mira041.stderr: 5: (OSD::~OSD()+0x9) [0x5e37e9]
2017-06-08T19:00:51.551 INFO:tasks.ceph.c1.osd.2.mira041.stderr: 6: (main()+0x2f48) [0x4dc1b8]
2017-06-08T19:00:51.551 INFO:tasks.ceph.c1.osd.2.mira041.stderr: 7: (__libc_start_main()+0xf5) [0xd5bdb15]
2017-06-08T19:00:51.551 INFO:tasks.ceph.c1.osd.2.mira041.stderr: 8: (()+0x46d1d9) [0x5751d9]
2017-06-08T19:00:51.551 INFO:tasks.ceph.c1.osd.2.mira041.stderr: NOTE: a copy of the executable, or `objdump -rdS <executable>` is needed to interpret this.

rzarzynski · 2017-06-14T12:21:04Z

jenkins retest this please

rzarzynski · 2017-06-15T12:19:53Z

jenkins retest this please (154 - osd-dup.sh (Failed))

rzarzynski · 2017-06-21T13:10:22Z

jenkins retest this please

rzarzynski · 2017-06-26T12:25:06Z

src/rgw/rgw_rest_swift.cc

@@ -1666,7 +1666,10 @@ void RGWInfo_ObjStore_SWIFT::list_swift_data(Formatter& formatter,

  string ceph_version(CEPH_GIT_NICE_VER);
  formatter.dump_string("version", ceph_version);
-  formatter.dump_int("max_meta_name_length", 81);


This were causing failures in the container-related tests of Swift (e.g. test.functional.test_container:TestContainer.test_POST_bad_metadata). The patch rectifies that as /info returns 80 for max_meta_name_length now.

rzarzynski · 2017-07-28T17:05:53Z

jenkins retest this please (arm64 only)

Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>

Fixes: http://tracker.ceph.com/issues/17935 Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>

Fixes: http://tracker.ceph.com/issues/17938 Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>

…ocs. Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>

Fixes: http://tracker.ceph.com/issues/17936 Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>

rzarzynski · 2017-08-22T13:28:38Z

Rebased after a trivial merge conflict.

mdw-at-linuxbox · 2017-08-27T19:56:26Z

With "discover=true", the logic for "rgw_max_attrs_num_in_req" fails on tempest runs. There is a silent limit set by civetweb.h mg_request_info http_headers[64]. The actual max # of metadata items is slightly slower - larger #'s will cause extra http headers to drop - this typically causes swift authentication to fail (in tempest).

Setting a sufficiently small number (say, 32) causes different behavior. There's an assertion generated in src/rgw/rgw_rest_swift.cc in handle_metadata_errors when it tries to do this,
boost::format("Too many metadata items; max %lld") % s->cct->_conf->get_val("rgw_max_attrs_num_in_req"))
Boost says something about "wrong type or option does not exist". %lld is the wrong type for an int, which should probably be a size_t (it is elsewhere).

Ideally, we should have teuthology or unit tests for some of this behavior.

mdw-at-linuxbox · 2017-08-27T20:01:33Z

src/rgw/rgw_rest_swift.cc

+  } else if (op_ret == -E2BIG) {
+    const auto error_message = boost::str(
+      boost::format("Too many metadata items; max %lld")
+        % s->cct->_conf->get_val<int>("rgw_max_attrs_num_in_req"));


The here should almost certainly be <size_t> . Better yet would be to use "cct->_conf->rgw_max_attrs_num_in_req". The boost runtime code for get_val generates a lot of runtime code - and for this bug a very hairy stack backtrace.

Yeah, boost::format & co. isn't cheap. I hoped that having it outside of main execution paths isn't an issue.

I'm not sure whether we could use cct->_conf->rgw_max_attrs_num_in_req due to the recent changes in the configuration component. I guess it's allowed only for legacy options while rgw_max_attrs_num_in_req is a new one. Anyway, I will check this out.

Just pushed the updated branch. It looks I introduced the int bug with the rebase after the new configuration subsystem. Big thanks for pointing it out, Marcus! :-)

mdw-at-linuxbox · 2017-08-27T20:04:24Z

src/rgw/rgw_op.h

+        cct->_conf->get_val<size_t>("rgw_max_attrs_num_in_req");
+      if (rgw_max_attrs_num_in_req &&
+          ++valid_meta_count > rgw_max_attrs_num_in_req) {
+        return -E2BIG;


There's logic elsewhere in this commit that turns E2BIG into an error message. It would be much better to generate that error message here. That would result in simpler and easier to understand logic with less chance of breakage - less brittle!

I'm afraid there is a reason for not doing that. IIRC the rgw_get_request_metadata is a generic routine that is used by both our Swift and S3 implementations while the error message is specific to Swift.

mdw-at-linuxbox · 2017-08-27T20:10:15Z

src/common/options.cc

@@ -4163,6 +4163,10 @@ std::vector<Option> get_rgw_options() {
    .set_default(1 * 1024 * 1024)
    .set_description(""),

+    Option("rgw_max_attrs_num_in_req", Option::TYPE_UINT, Option::LEVEL_ADVANCED)
+    .set_default(0)
+    .set_description("The maximum number of metadata items that can be put via single request"),


I'm not sure where this goes, but there's a max limit on this value that's sized by civetweb.h mg_request_info http_headers[64]. The true limit is "somewhat less" than this number. It would be good to document this number somewhere. It would be good to emit a runtime warning at startup for values near this limit. It would be really good if civetweb failed requests in a more obvious fashion when this limit was exceeded. And it would be great if civetweb didn't have this limit in the first place (but given asio we might not want to bother.)

Also, why "default(0)"? ocata swift advertises 90 here (that means changing civetweb tho)

Thanks for pointing out this CivetWeb's limitation! Documenting it is definitely a very good idea.

Also, why "default(0)"? ocata swift advertises 90 here (that means changing civetweb tho)

I believe that's one of the compromises coming from being a multi-API program. The 0 value means do not enforce as in rgw_get_request_metadata we have:

if (rgw_max_attrs_num_in_req && ++valid_meta_count > rgw_max_attrs_num_in_req) { return -E2BIG; }

I see 2 factors driving this behavior:

the need to not break the level of S3 compliance we offer in the default configuration,

necessity to have the policy coherent between all APIs similarly to ACLs.

But it's not true we have a '0 = do not enforce' limit. What we currently have is a default "number slightly larger than 60 causes weird bad behavior" limit. Having a common default that causes non-weird deterministic behavior in s3 would still be an improvement over what we offer today.

If it's really the case that s3 doesn't expect a limit here than perhaps this should be a swift-only limit. And if we don't want to have any limit as a default, then the radosgw install directions need to call that out as something people caring about swift standards must set. But before any of this makes sense, we'd have to fix the civetweb restriction which requires we have a limit.

We have many different limits across the layers. Passing a check on particular layer doesn't guarantee success elsewhere.

I perceive CivetWeb as a component located on the front-end layer. As it can't deal properly with requests carrying huge number of metadata headers, it should at least properly error there (maybe by sending a 5xx?). I would like to avoid introducing a logic that crosses the layer boundary.

mdw-at-linuxbox · 2017-08-27T20:25:31Z

src/rgw/rgw_rest_swift.cc

+    - strlen(RGW_ATTR_PREFIX RGW_AMZ_META_PREFIX);
+  formatter.dump_int("max_meta_name_length", meta_name_limit);
+
+  const size_t meta_value_limit = g_conf->osd_max_attr_size;


Again not sure where to put this - but this really should have a default value (ocata swift advertises 256). Since this is an "osd" value and applies to all rados attributes, setting this to small values causes other interesting bits of rados to start breaking (and fixing that means restarting all osds). From an operational standpoint for rgw, I think the only sane way to set this is as an rgw specific value and have it set to a separate possibly lower value than the osd. So perhaps this should just be its own separate configuration item.

setting this to small values causes other interesting bits of rados to start breaking (and fixing that means restarting all osds).

The usage pattern of osd_max_attr_size in RadosGW assumed setting the option only in the RGW-dedicated section of ceph.conf. Does the mentioned problem occur then? If so, we might have an issue in OSDs as a misbehaving RADOS client shouldn't cause the need to restart it, I believe.

If I move this to the rgw only section I get more desirable behavior yes.

Note: naive people looking at an option that starts with "osd_" are likely to think it matters to the osd. And, oh it does, but not in a way that's useful to swift.

I'd prefer to see this be its own independent configuration name starting with "rgw_".

According to the s3 documentation, http://docs.aws.amazon.com/AmazonS3/latest/dev/UsingMetadata.html#object-metadata , there is also a s3 metadata size limit. I don't think we want to enforce that limit, but it appears to me if we advertise a default 2K per-meta value limit, we can please the tempest tests, provide a perfectly defensible swift size limit, and an equally defensible s3 experience.

Got it. Renaming the configurables to rgw_.

Fixes: http://tracker.ceph.com/issues/17934 Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>

Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>

The bug that is fixed in this patch has been responsible for failing the Tempest's test_delete_non_empty_container test case. The investigation has been made by: Marcus Watts <mwatts@redhat.com>. Fixes: http://tracker.ceph.com/issues/21169 Signed-off-by: Radoslaw Zarzynski <rzarzyns@redhat.com>

mattbenjamin · 2017-08-29T17:43:07Z

@rzarzynski can you schedule an RGW suite run on this, as well?

rzarzynski · 2017-08-29T17:45:50Z

@mattbenjamin: sure, I will schedule on https://github.com/rzarzynski/ceph/tree/wip-rgw-swift_api_testing which should have all the patches.

rzarzynski · 2017-08-29T20:01:54Z

Scheduled: http://pulpito.ceph.com/rzarzynski-2017-08-29_19:58:31-rgw-wip-rgw-swift_api_testing---basic-smithi.

rzarzynski · 2017-08-30T13:03:48Z

Rescheduled with higher priority: http://pulpito.ceph.com/rzarzynski-2017-08-30_12:11:05-rgw-wip-rgw-swift_api_testing---basic-smithi.

mdw-at-linuxbox · 2017-09-16T22:11:21Z

There is interest in getting this into master/luminous to fix tempest swift tests.

If there is a possibility of fixing some of the nits I'll address more above quickly that would be cool, otherwise I guess that better wait for some other clean up pass.

Signed-off-by: Radoslaw Zarzynski <rzarzyns@redhat.com>

rzarzynski · 2017-09-19T12:55:40Z

@mdw-at-linuxbox: pushed a commit changing the names. It needs testing. Though, I'm still unhappy as rgw_max_attr_name_len operates on attr names after the conversion to RADOS. I will work on that, maybe as a follow-up.

mattbenjamin · 2017-09-19T14:10:43Z

@rzarzynski @mdw-at-linuxbox I don't quite follow the meaning of "conversion to RADOS"; changing civetweb header limits seems less useful now, I'd rather focus on polishing the ASIO frontend? normalizing attrs with OSD settings is problematic in general, so I think there are no open issues for for this PR; as regards testing, does this need another suite run?

mattbenjamin

lgtm (updated for unique var name requested by marcus, checked by Yehuda, passed teuthology)

rzarzynski added bug-fix needs-doc rgw labels May 30, 2017

rzarzynski commented May 30, 2017

View reviewed changes

rzarzynski changed the title ~~rgw: improve handling Swift's error messages and limits~~ rgw: improve handling of Swift's error messages and limits May 30, 2017

rzarzynski force-pushed the wip-rgw-custom-errmsgs-2 branch from 7b4df18 to 9b63abf Compare May 30, 2017 20:23

This was referenced May 31, 2017

rgw: improve handling of custom attributes exceeding the limit on name length #12109

Closed

[DNM] rgw: improve handling of error messages #12089

Closed

rzarzynski added the cleanup label May 31, 2017

mattbenjamin requested a review from mdw-at-linuxbox May 31, 2017 20:26

mattbenjamin reviewed May 31, 2017

View reviewed changes

rzarzynski commented Jun 26, 2017

View reviewed changes

rzarzynski force-pushed the wip-rgw-custom-errmsgs-2 branch 2 times, most recently from 072ea64 to 9c622f1 Compare July 27, 2017 19:05

rzarzynski added 5 commits August 22, 2017 09:19

rgw: honor custom rgw_err::message in Swift's error handling.

e851614

Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>

rgw: Swift API returns 400 Bad Request on too long container names.

1845e41

Fixes: http://tracker.ceph.com/issues/17935 Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>

rgw: add support for max_meta_name_length of Swift API's /info.

593d656

Fixes: http://tracker.ceph.com/issues/17938 Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>

rgw: refactor rgw_get_request_metadata to reduce the number of dynall…

64b7923

…ocs. Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>

rgw: add support for max_meta_value_length of Swift API's /info.

06b1f25

Fixes: http://tracker.ceph.com/issues/17936 Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>

rzarzynski force-pushed the wip-rgw-custom-errmsgs-2 branch from 9c622f1 to da21be7 Compare August 22, 2017 13:27

mdw-at-linuxbox reviewed Aug 27, 2017

View reviewed changes

rzarzynski added 2 commits August 27, 2017 23:29

rgw: add support for max_meta_count of Swift API's /info.

9e53c14

Fixes: http://tracker.ceph.com/issues/17934 Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>

rgw: seed::get_torrent_file returns errors in the usual way.

21ad80a

Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>

rzarzynski force-pushed the wip-rgw-custom-errmsgs-2 branch from da21be7 to 21ad80a Compare August 27, 2017 21:29

rzarzynski force-pushed the wip-rgw-custom-errmsgs-2 branch from 70bc1b1 to 308c8d3 Compare August 29, 2017 10:43

rgw: rename the configurables for metadata limits to start with rgw_.

9b06985

Signed-off-by: Radoslaw Zarzynski <rzarzyns@redhat.com>

mattbenjamin approved these changes Sep 19, 2017

View reviewed changes

mattbenjamin merged commit 1456dbf into ceph:master Sep 19, 2017

rzarzynski mentioned this pull request Sep 20, 2017

luminous: rgw: improve handling of Swift's error messages and limits #17836

Closed

rgw: improve handling of Swift's error messages and limits #15369

rgw: improve handling of Swift's error messages and limits #15369

Conversation

rzarzynski commented May 30, 2017 • edited

Choose a reason for hiding this comment

rzarzynski commented May 30, 2017

mattbenjamin commented May 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rzarzynski commented Jun 12, 2017

rzarzynski commented Jun 14, 2017

rzarzynski commented Jun 15, 2017

rzarzynski commented Jun 21, 2017

Choose a reason for hiding this comment

rzarzynski commented Jul 28, 2017

rzarzynski commented Aug 22, 2017

mdw-at-linuxbox commented Aug 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rzarzynski Aug 27, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattbenjamin commented Aug 29, 2017

rzarzynski commented Aug 29, 2017

rzarzynski commented Aug 29, 2017

rzarzynski commented Aug 30, 2017

mdw-at-linuxbox commented Sep 16, 2017

rzarzynski commented Sep 19, 2017

mattbenjamin commented Sep 19, 2017 • edited

mattbenjamin left a comment

Choose a reason for hiding this comment

rzarzynski commented May 30, 2017 •

edited

rzarzynski Aug 27, 2017 •

edited

mattbenjamin commented Sep 19, 2017 •

edited