mgr/rest: Trim requests array and limit size by NitzanMordhai · Pull Request #54984 · ceph/ceph

NitzanMordhai · 2023-12-21T09:43:17Z

Presently, the requests array in the REST module has the potential to grow indefinitely, leading to excessive memory consumption, particularly when dealing with lengthy and intricate request results.

To address this issue, a limit will be imposed on the requests array within the REST module.
This limitation will be governed by the mgr/restful/x/max_requests configuration parameter specific to the REST module.

I ran a few thousand pg dump commands with and without the trimming of the requests array, there is a big improvement with the trimming when the cluster returns a large output from the pg dump command.

with the trimming:
image

without the trimming:
image

Fixes: https://tracker.ceph.com/issues/59580

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

NitzanMordhai · 2023-12-21T09:55:08Z

@tchaikov @athanatos this is a split PR from #54634 like you suggested.
the 500 max_requests default size was set because of the large outputs that the users mentioned in the tracker that "pg dump" caused the mgr to OOM.
with trimmed requests, we are limiting the users getting older request results but on the other hand we not letting it grow un boundary ( We can also change that to limit by capacity instead of size, that way we can keep large amount of small requests)

tchaikov · 2023-12-25T08:56:02Z

i think there is a defect in the design of the restful mgr module. which allows client to send requests to the cluster using RESTful API, and client can track the requests' status using their ids. and the mgr module deletes the finished requests upon request of client, or delete the specified one upon request of client.

but if client fails to do so, the requests are accumulated in the mgr module, no matter if they are finished or not.

@NitzanMordhai i'd suggest

mark the restful mgr module deprecated, and encourage user switch to ceph_api. previous Ernesto sent a mail for this purpose 3 years ago. see https://lists.ceph.io/hyperkitty/list/ceph-users@ceph.io/thread/LBKLNXH7UQL7TLFU5G52Y2SYVME4RS6P/ . in order to deprecate this module, we can
- add corresponding document in doc/mgr/restful.rst.
- move this module out of mgr-modules-core into a dedicated package. (i can help on this)
- note in the PendingReleaseNotes regarding the change on packaging (i can help on this also)
- reply on the tracker ticket and mail, to explain that this leakage is due to a deprecated mgr module.
to address this very issue while preserving the existing behavior of tracking requests sent to cluster via restful mgr module to a certain degree
- add the max_requests option as implemented in this change. and note it in PendingReleaseNotes.
- when the number of requests exceeds the number of max_requets, trim the requests with the configured max_requets. and print a logging message with the info level if only the finished ones are trimmed., if unfinished requests are trimmed, print an error message in the logging message.
- please also note the rationales in the commit messages in details.

i think the title of "Trim results array and limit size" is a little bit misleading, it's a "request" list, not a "results" array.

and the option should be documented, but you checked "No doc update is appropriate". i doubt this.

NitzanMordhai · 2024-01-04T18:32:25Z

@tchaikov thanks for the review, I'm going to fix some of the things, but since we haven't got any respond yet on the user-list (3 years past and yet, we have some users who use that) maybe it will be good idea to resent that message and announce on the deprecation

tchaikov · 2024-01-08T13:41:42Z

@NitzanMordhai yeah, IMHO, 3 years is long enough to make a breaking change. and the first step is just to move the module into its own module instead of removing it. this should help users to understand the status of this module.

tchaikov · 2024-02-05T02:20:54Z

please ping me if this change is ready for another review.

NitzanMordhai · 2024-02-14T08:29:40Z

please ping me if this change is ready for another review.

@tchaikov thank you very much for your reviews, can we proceed with that fix and deprecate the rest module in another PR? Currently, the deprecation is not announced in changelog, I'll create another PR for the deprecation process - will you help me with that?

tchaikov · 2024-02-17T03:48:16Z

please ping me if this change is ready for another review.

@tchaikov thank you very much for your reviews, can we proceed with that fix and deprecate the rest module in another PR? Currently, the deprecation is not announced in changelog, I'll create another PR for the deprecation process - will you help me with that?

@NitzanMordhai Nitzan, sure. we can do this in parallel. but this PR also needs more work, AFAICT. as it does address the "unbounded requests queue" issue. what exactly do you want from my end?

NitzanMordhai · 2024-02-21T10:03:24Z

@NitzanMordhai Nitzan, sure. we can do this in parallel. but this PR also needs more work, AFAICT. as it does address the "unbounded requests queue" issue. what exactly do you want from my end?

@tchaikov i just updated the PR with your suggestions. can you please review it?

src/pybind/mgr/restful/module.py

tchaikov

sorry for the latency. left a comment.

NitzanMordhai · 2024-03-14T06:36:11Z

jenkins test api

tchaikov

could improve the document a little bit.

doc/mgr/restful.rst

tchaikov

the document does not look right.

tchaikov · 2024-03-30T14:41:30Z

@NitzanMordhai ping?

tchaikov · 2024-05-20T15:25:39Z

jenkins test make check

github-actions · 2024-07-21T09:01:26Z

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

NitzanMordhai · 2024-08-11T06:03:24Z

@yuriw can we add it to one of the tests?

rsacherer · 2024-08-11T19:49:29Z

Hello, would be good to see movement on this as I see people using the restful plugin (most of them using it for zabbix).

Thank you.

anthonyeleven · 2024-08-11T20:08:53Z

PendingReleaseNotes

  default json format produces a rather massive output in large clusters and
  isn't scalable. So we have removed the 'network_ping_times' section from
  the output. Details in the tracker: https://tracker.ceph.com/issues/57460
+* mgr/rest: The rest manager module will trim requests based on the 'max_requests' option.


anthonyeleven · 2024-08-11T20:09:15Z

PendingReleaseNotes

+* mgr/rest: The rest manager module will trim requests based on the 'max_requests' option.
+  Without this feature, and in the absence of manual deletion of old requests,
+  the accumulation of requests in the array can lead to Out Of Memory (OOM) issues, 
+  resulting in the termination of the manager.


resulting in the Manager crashing.

anthonyeleven · 2024-08-11T20:09:27Z

doc/mgr/restful.rst

 If the address it not configured, the *restful* will bind to ``::``,
 which corresponds to all available IPv4 and IPv6 addresses.

+Configuring the max_request


anthonyeleven · 2024-08-11T20:10:10Z

doc/mgr/restful.rst

+---------------------------
+
+The maximum request size can be configured via the configuration key
+facility::


via a central configuration option::

NitzanMordhai · 2024-08-19T06:15:53Z

Rados approved: https://tracker.ceph.com/projects/rados/wiki/MAIN#httpstrackercephcomissues67523

yuriw · 2024-08-19T14:23:19Z

jenkins test api

yuriw · 2024-08-19T14:23:46Z

@NitzanMordhai pls merge when all checks passed
ref: https://tracker.ceph.com/issues/67523

Presently, the requests array in the REST module has the potential to grow indefinitely, leading to excessive memory consumption, particularly when dealing with lengthy and intricate request results. To address this issue, a limit will be imposed on the requests array within the REST module. This limitation will be governed by the `mgr/restful/x/max_requests` configuration parameter specific to the REST module. when submit_request called we will check request array if exceed max_request option if it does we will check if the future trimmed request finished and log error message in case we are trimming un-finished requests. Fixes: https://tracker.ceph.com/issues/59580 Signed-off-by: Nitzan Mordechai <nmordech@redhat.com>

…max_request option Signed-off-by: Nitzan Mordechai <nmordech@redhat.com>

Signed-off-by: Nitzan Mordechai <nmordech@redhat.com>

NitzanMordhai · 2024-08-20T12:01:03Z

jenkins test make check

NitzanMordhai requested review from athanatos and tchaikov December 21, 2023 09:43

github-actions bot added the pybind label Dec 21, 2023

NitzanMordhai mentioned this pull request Dec 21, 2023

mgr/BaseMgrModule: Optimize CPython Call in Finish Function #54634

Merged

14 tasks

tchaikov removed their request for review February 5, 2024 02:20

NitzanMordhai force-pushed the wip-nitzan-restful-un-boundary-keep-requests branch from 01344a6 to d8a58ec Compare February 21, 2024 10:02

NitzanMordhai requested a review from a team as a code owner February 21, 2024 10:02

github-actions bot added documentation mgr labels Feb 21, 2024

NitzanMordhai changed the title ~~mgr/rest: Trim results array and limit size~~ mgr/rest: Trim requests array and limit size Feb 22, 2024

NitzanMordhai requested a review from tchaikov February 22, 2024 07:37

tchaikov reviewed Mar 7, 2024

View reviewed changes

src/pybind/mgr/restful/module.py Outdated Show resolved Hide resolved

tchaikov requested changes Mar 7, 2024

View reviewed changes

NitzanMordhai force-pushed the wip-nitzan-restful-un-boundary-keep-requests branch from d8a58ec to 738f485 Compare March 12, 2024 13:19

NitzanMordhai requested a review from tchaikov March 12, 2024 13:20

tchaikov requested changes Mar 15, 2024

View reviewed changes

doc/mgr/restful.rst Outdated Show resolved Hide resolved

NitzanMordhai force-pushed the wip-nitzan-restful-un-boundary-keep-requests branch from 738f485 to d6e99c0 Compare March 18, 2024 08:54

NitzanMordhai requested a review from tchaikov March 18, 2024 08:54

tchaikov requested changes Mar 19, 2024

View reviewed changes

NitzanMordhai force-pushed the wip-nitzan-restful-un-boundary-keep-requests branch from d6e99c0 to e12ba7d Compare March 31, 2024 06:12

tchaikov mentioned this pull request May 20, 2024

mgr/{restful,zabbix}: remove deprecated module #57299

Merged

14 tasks

NitzanMordhai closed this May 22, 2024

NitzanMordhai reopened this May 22, 2024

github-actions bot added the stale label Jul 21, 2024

NitzanMordhai force-pushed the wip-nitzan-restful-un-boundary-keep-requests branch from 208aed8 to 7ba09b0 Compare August 11, 2024 06:02

github-actions bot removed the stale label Aug 11, 2024

anthonyeleven reviewed Aug 11, 2024

View reviewed changes

ljflores added core wip-yuri-testing labels Aug 12, 2024

yuriw added TESTED ready-to-merge and removed wip-yuri-testing labels Aug 19, 2024

NitzanMordhai added 3 commits August 20, 2024 10:29

PendingReleaseNotes: Adding note about rest module change and adding …

7b251ca

…max_request option Signed-off-by: Nitzan Mordechai <nmordech@redhat.com>

doc/mgr/restful: update max_request config

dd7e8bb

Signed-off-by: Nitzan Mordechai <nmordech@redhat.com>

NitzanMordhai force-pushed the wip-nitzan-restful-un-boundary-keep-requests branch from 7ba09b0 to dd7e8bb Compare August 20, 2024 10:35

NitzanMordhai requested a review from anthonyeleven August 20, 2024 10:36

NitzanMordhai merged commit b9d6a00 into ceph:main Aug 20, 2024

NitzanMordhai deleted the wip-nitzan-restful-un-boundary-keep-requests branch August 20, 2024 16:16

This was referenced Aug 21, 2024

quincy: mgr/rest: Trim requests array and limit size #59370

Merged

reef: mgr/rest: Trim requests array and limit size #59371

Merged

squid: mgr/rest: Trim requests array and limit size #59372

Merged

Conversation

NitzanMordhai commented Dec 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution Guidelines

Checklist

Uh oh!

NitzanMordhai commented Dec 21, 2023

Uh oh!

tchaikov commented Dec 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NitzanMordhai commented Jan 4, 2024

Uh oh!

tchaikov commented Jan 8, 2024

Uh oh!

tchaikov commented Feb 5, 2024

Uh oh!

NitzanMordhai commented Feb 14, 2024

Uh oh!

tchaikov commented Feb 17, 2024

Uh oh!

NitzanMordhai commented Feb 21, 2024

Uh oh!

Uh oh!

tchaikov left a comment

Choose a reason for hiding this comment

Uh oh!

NitzanMordhai commented Mar 14, 2024

Uh oh!

tchaikov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tchaikov left a comment

Choose a reason for hiding this comment

Uh oh!

tchaikov commented Mar 30, 2024

Uh oh!

tchaikov commented May 20, 2024

Uh oh!

github-actions bot commented Jul 21, 2024

Uh oh!

NitzanMordhai commented Aug 11, 2024

Uh oh!

rsacherer commented Aug 11, 2024

Uh oh!

anthonyeleven Aug 11, 2024

Choose a reason for hiding this comment

Uh oh!

anthonyeleven Aug 11, 2024

Choose a reason for hiding this comment

Uh oh!

anthonyeleven Aug 11, 2024

Choose a reason for hiding this comment

Uh oh!

anthonyeleven Aug 11, 2024

Choose a reason for hiding this comment

Uh oh!

NitzanMordhai commented Aug 19, 2024

Uh oh!

yuriw commented Aug 19, 2024

Uh oh!

yuriw commented Aug 19, 2024

Uh oh!

NitzanMordhai commented Aug 20, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

NitzanMordhai commented Dec 21, 2023 •

edited

Loading

tchaikov commented Dec 25, 2023 •

edited

Loading