rgw/s3select : fix for error flow. add an option to disable s3select-request. by galsalomon66 · Pull Request #56834 · ceph/ceph

galsalomon66 · 2024-04-11T16:39:21Z

-- add the option to reject s3select-requests
ceph config set client.rgw.8000 rgw_s3select_disable true

-- in some cases the error message does not return to client, connection got broken (invalid chunk length)

https://bugzilla.redhat.com/show_bug.cgi?id=2252396
https://bugzilla.redhat.com/show_bug.cgi?id=2275323
ceph/s3-tests#561 NOTE: the s3-tests PR must merged with this PR

https://tracker.ceph.com/issues/65828

Contribution Guidelines

To sign and title your commits, please refer to Submitting Patches to Ceph.
If you are submitting a fix for a stable branch (e.g. "quincy"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.
When filling out the below checklist, you may click boxes directly in the GitHub web UI. When entering or editing the entire PR message in the GitHub web UI editor, you may also select a checklist item by adding an x between the brackets: [x]. Spaces and capitalization matter when checking off items this way.

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)
- Includes unit test(s)
- Includes integration test(s)
- Includes bug reproducer
- No tests

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
jenkins test rook e2e

galsalomon66 · 2024-04-19T12:54:30Z

https://pulpito.ceph.com/gsalomon-2024-04-19_04:12:34-rgw:verify-fix_error_message_flow-distro-default-smithi/
4 dead : not related to changes, all are centos.

galsalomon66 · 2024-04-20T04:54:46Z

https://pulpito.ceph.com/gsalomon-2024-04-19_21:46:08-rgw:verify-fix_error_message_flow-distro-default-smithi/

galsalomon66 · 2024-04-26T05:26:34Z

https://pulpito.ceph.com/gsalomon-2024-04-25_16:28:18-rgw:verify-fix_error_message_flow-distro-default-smithi/
all are green

galsalomon66 · 2024-04-26T12:01:21Z

https://pulpito.ceph.com/gsalomon-2024-04-26_07:23:10-rgw-fix_error_message_flow-distro-default-smithi/
70 passed
3 failed

mattbenjamin · 2024-05-06T16:56:12Z

  s3tests:
-    force-branch: ceph-master
+    force-branch: fix_non_handled_error_resonse
+    git_remote: https://github.com/galsalomon66/


galsalomon?

these lines already removed (back to ceph-master)

mattbenjamin · 2024-05-06T16:57:03Z

  services:
  - rgw
  with_legacy: true
+- name: rgw_s3select_disable


suggest rgw_disable_s3select

mattbenjamin · 2024-05-06T16:58:20Z

+- name: rgw_s3select_disable
+  type: bool
+  level: advanced
+  desc: disable the s3select operation


describe what will happen when disabled, e.g., will we return an error code such as invalid request?

mattbenjamin · 2024-05-06T16:59:42Z

-void aws_response_handler::send_error_response_rgw_formatter(const char* error_code,
-    const char* error_message,
-    const char* resource_id)
+std::string empty_error="--";


the diff is maybe confusing me, but is this a global variable? I think you can use a static constexpr std::string for this

I missed that.
will fix that to static/constexpr

yuvalif · 2024-05-06T17:22:02Z

please link to trackers not BZs
i don't understand how this is fixing the OOM issue. clearly, the s3-select request was not sent by mistake, and the intent of the user was to use the feature. how will an admin know that there are users that indend to send large files up front?
using the flag as a "kill switch" when they see RGW crashes (guessing the reason is s3-select OOM...) is, IMO, very wrong
would recommend adding a conf parameter on the maximum size of a file allowed for s3-select. this does not have to be very accurate (1GB? 5GB?), and the admin settign the value will be able to reason about the decision to not allowing for using the feature on very large files as it may crash the RGW

galsalomon66 · 2024-05-06T23:20:55Z

please link to trackers not BZs

i don't understand how this is fixing the OOM issue. clearly, the s3-select request was not sent by mistake, and the intent of the user was to use the feature. how will an admin know that there are users that indend to send large files up front?

using the flag as a "kill switch" when they see RGW crashes (guessing the reason is s3-select OOM...) is, IMO, very wrong

would recommend adding a conf parameter on the maximum size of a file allowed for s3-select. this does not have to be very accurate (1GB? 5GB?), and the admin settign the value will be able to reason about the decision to not allowing for using the feature on very large files as it may crash the RGW

adding @mattbenjamin

the "kill switch" could be "too dramatic"
as for Parquet-size-limitation switch, the OOM occurs not because of Parquet size but because of the Parquet chunk size
(the way Parquet is organized),
meaning, if there were much more row-groups (not just 6), it would "survive" in a low-end server.
i agree that the Parquet-size-limitation is more subtle.

in some cases the error message does not return to client, connection got broken (invalid chunk length) fix another broken connection all data-source to use same API for sending error-response add the option rgw_s3select_disable(boolean). upon turning-on this option, it rejects s3select-requests with an error-message editorial. rollback to ceph-master. the ceph/s3-tests#561 must be merged with ceph-PR Signed-off-by: Gal Salomon <gal.salomon@gmail.com>

ivancich

lgtm

cbodley · 2024-07-05T14:03:33Z

i very much agree with @yuvalif's sentiments above

admins need a way to control rgw's total memory usage so it doesn't keep crashing with OOM. rgw will process up to rgw_max_concurrent_requests (1024) at a time. each Get/PutObject request will buffer up to rgw_get_obj_window_size/rgw_put_obj_min_window_size (16M) of data. so the admin can tune these knobs to guarantee that a server saturated with requests will still fit in available memory. we need a way to fit SELECT requests into this model too

so how is it that a single SELECT request can kill the server with OOM? what data is it that's not bounded in size? is it state from the parquet parser? is it buffered results that we haven't written back to the client yet?

that said, users have requested a way to disable s3select (like in https://tracker.ceph.com/issues/61821) so i'm glad to see this. i just think there's more work to do here

galsalomon66 · 2024-07-07T05:03:08Z

@cbodley

yes,
this issue should resolved with a memory management solution, not with disable-flag.

row groups divide the object, enabling skipping this basic building block.
I.e, in case the row-group does not match the statement predicate, we can skip it.
the bigger the row-group the less possible that it does not match the predicate(thus, no skipping).

this 12GB object contains 6 groups, it is definitely not efficient and also caused the OOM.

the solution should be by fetching batches(pre-defined size) of that row group whatever its size, and not all in one batch, as currently done.

thus, filtering out big-size Parquet objects is not a solution.

on a side note,
the different readers are integrated into the GetObject, each of each works differently, the concept is to stream data not to pre-load it.
CSV is the most simple reader, while JSON and Parquet are more complex.

in the case of Parquet, the SQL engine(actually, the Parquet reader) issues range-requests according to meta-data,
while in the JSON case, the SQL statement defines how much JSON data to read (it should be a row, that row has no pre-defined end-of-row)

the combination of the SQL statement and data-profile could sometimes lead to behavior that was not anticipated in advance.

rgw/s3select : fix for error flow. add an option to disable s3select-request. Reviewed-by: J. Eric Ivancich <ivancich@redhat.com>

galsalomon66 · 2024-08-27T14:05:30Z

@cbodley @yuvalif

the following PR #59465 (WIP)
fix the memory usage consumption by limiting the buffer size internally

galsalomon66 requested a review from a team as a code owner April 11, 2024 16:39

github-actions Bot added the rgw label Apr 11, 2024

galsalomon66 force-pushed the fix_error_message_flow branch 2 times, most recently from e4ffbf0 to ce23c37 Compare April 15, 2024 07:34

galsalomon66 added the needs-qa label Apr 17, 2024

github-actions Bot added the common label Apr 22, 2024

galsalomon66 changed the title ~~rgw/s3select : fix for error flow.~~ rgw/s3select : fix for error flow. add an option to disable s3select-request. Apr 25, 2024

galsalomon66 force-pushed the fix_error_message_flow branch from 5bfac0a to ce55750 Compare April 28, 2024 14:23

galsalomon66 requested review from ivancich and yuvalif May 1, 2024 17:26

galsalomon66 mentioned this pull request May 2, 2024

rgw: set correct requestId and hostId on s3select error #56864

Merged

mattbenjamin reviewed May 6, 2024

View reviewed changes

galsalomon66 force-pushed the fix_error_message_flow branch from bbc9a49 to 91a60c7 Compare May 6, 2024 22:53

galsalomon66 force-pushed the fix_error_message_flow branch from 91a60c7 to 53ad57c Compare May 7, 2024 06:57

ivancich approved these changes May 10, 2024

View reviewed changes

cbodley merged commit 2f02bce into ceph:main Jul 5, 2024

cbodley mentioned this pull request Apr 24, 2025

Squid #62942

Closed

14 tasks

Conversation

galsalomon66 commented Apr 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution Guidelines

Checklist

Uh oh!

galsalomon66 commented Apr 19, 2024

Uh oh!

galsalomon66 commented Apr 20, 2024

Uh oh!

galsalomon66 commented Apr 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

galsalomon66 commented Apr 26, 2024

Uh oh!

mattbenjamin May 6, 2024

Choose a reason for hiding this comment

Uh oh!

galsalomon66 May 6, 2024

Choose a reason for hiding this comment

Uh oh!

mattbenjamin May 6, 2024

Choose a reason for hiding this comment

Uh oh!

mattbenjamin May 6, 2024

Choose a reason for hiding this comment

Uh oh!

mattbenjamin May 6, 2024

Choose a reason for hiding this comment

Uh oh!

galsalomon66 May 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yuvalif commented May 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

galsalomon66 commented May 6, 2024

Uh oh!

ivancich left a comment

Choose a reason for hiding this comment

Uh oh!

cbodley commented Jul 5, 2024

Uh oh!

galsalomon66 commented Jul 7, 2024

Uh oh!

galsalomon66 commented Aug 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

galsalomon66 commented Apr 11, 2024 •

edited

Loading

galsalomon66 commented Apr 26, 2024 •

edited

Loading

galsalomon66 May 6, 2024 •

edited

Loading

yuvalif commented May 6, 2024 •

edited

Loading

galsalomon66 commented Aug 27, 2024 •

edited

Loading