mgr: Add one finisher thread per module #47893

kotreshhr · 2022-09-01T06:37:54Z

Adds a finisher thread for each module. Each module commands are
executed via corresponding finisher thread. There is generic
finisher thread via which all other commands like notify, config
change are run.

Fixes: https://tracker.ceph.com/issues/51177
Signed-off-by: Kotresh HR khiremat@redhat.com

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. "pacific"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.

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

vshankar · 2022-09-07T05:05:42Z

@kotreshhr @batrick I'm not against the approach taken, however, I would like to check if the point made here(*) is worth considering?

(*): https://tracker.ceph.com/issues/51177#note-4

batrick · 2022-09-07T15:36:07Z

@kotreshhr @batrick I'm not against the approach taken, however, I would like to check if the point made here(*) is worth considering?

(*): https://tracker.ceph.com/issues/51177#note-4

I think the issue with that approach is you'd still need to somehow queue other events like mdsmap/osdmap notifications or even subsequent CLI commands. How many async commands should run at once? Leave it to the modules to decide? It gets messy quickly, especially with ordering guarantees.

This approach keeps the sequential nature of commands/events but prevents one module from blocking others from operating. I think it's a nice compromise.

vshankar · 2022-09-08T12:23:11Z

@kotreshhr @batrick I'm not against the approach taken, however, I would like to check if the point made here() is worth considering?
(): https://tracker.ceph.com/issues/51177#note-4

I think the issue with that approach is you'd still need to somehow queue other events like mdsmap/osdmap notifications or even subsequent CLI commands. How many async commands should run at once? Leave it to the modules to decide? It gets messy quickly, especially with ordering guarantees.

This approach keeps the sequential nature of commands/events but prevents one module from blocking others from operating. I think it's a nice compromise.

I'm a tiny bit worried about bloating up ceph-mgr's memory usage with this approach. It solves the issue in hand, however, we are adding up a finite memory (queue, locks, etc..) for each mgr module. IIRC, there was some discussion (somewhere) about trimming ceph-mgr's memory footprint.

@badone ^^

src/mgr/Mgr.cc

badone · 2022-09-08T23:49:36Z

I'm a tiny bit worried about bloating up ceph-mgr's memory usage with this approach. It solves the issue in hand, however, we are adding up a finite memory (queue, locks, etc..) for each mgr module. IIRC, there was some discussion (somewhere) about trimming ceph-mgr's memory footprint.

@badone ^^

I don't recall. Are you sure it was me involved (did you mean Boris maybe)?

vshankar · 2022-09-09T01:53:39Z

I'm a tiny bit worried about bloating up ceph-mgr's memory usage with this approach. It solves the issue in hand, however, we are adding up a finite memory (queue, locks, etc..) for each mgr module. IIRC, there was some discussion (somewhere) about trimming ceph-mgr's memory footprint.
@badone ^^

I don't recall. Are you sure it was me involved (did you mean Boris maybe)?

Maybe, I'm not sure. It was a while back...

Anyway, we need more eyes on this change.

badone · 2022-09-09T02:11:34Z

@vshankar Do you see it adding significantly to the memory usage of the manager? Seems to me the additional overhead of having a finisher for each module plus a generic one shouldn't be massively different to having just a generic one since the content of the combined finisher queues should be about the same size as the queue of the finisher in the one finisher approach right?

vshankar · 2022-09-09T04:14:02Z

@vshankar Do you see it adding significantly to the memory usage of the manager? Seems to me the additional overhead of having a finisher for each module plus a generic one shouldn't be massively different to having just a generic one since the content of the combined finisher queues should be about the same size as the queue of the finisher in the one finisher approach right?

There would be one thread per mgr plugin. That's different that having one thread which is the case now. And then, there is a (finisher) thread even for modules that are not enabled (which can be worked around). But still...

How many plugins do we have now in ceph-mgr?

badone · 2022-09-09T04:27:23Z

In the mid-to-high thirties if my quick count is correct but they are not all enabled/active of course.

kotreshhr · 2022-09-09T07:25:57Z

@vshankar Do you see it adding significantly to the memory usage of the manager? Seems to me the additional overhead of having a finisher for each module plus a generic one shouldn't be massively different to having just a generic one since the content of the combined finisher queues should be about the same size as the queue of the finisher in the one finisher approach right?

Yes, there is nothing changed w.r.t queue size. With this approach, the queue size is distributed among multiple threads which was with the single thread before. The added memory usage is the new Finisher object per new thread. I am not very sure yet how much that would add up to resource consumption.

kotreshhr · 2022-09-12T07:47:09Z

Ran fs and rados:mgr suite.

http://pulpito.front.sepia.ceph.com/khiremat-2022-09-02_09:35:13-fs-wip-khiremat-47893-test-mgr-finisher-thread-0-distro-default-smithi/
http://pulpito.front.sepia.ceph.com/khiremat-2022-09-06_11:22:19-rados:mgr-wip-khiremat-47893-test-mgr-finisher-thread-0-distro-default-smithi/

kotreshhr · 2022-09-12T12:44:31Z

Entire rados suite scheduled:
http://pulpito.front.sepia.ceph.com/khiremat-2022-09-12_10:34:08-rados-wip-khiremat-47893-test-mgr-finisher-thread-0-distro-default-smithi/

vshankar · 2022-09-16T10:26:34Z

I guess this change is fine since noone has shown objection on the approach taken here.

@badone - @kotreshhr has run additionally this through rados:mgr. Would that be enough as a source of validation?

vshankar · 2022-09-21T13:57:15Z

ping?

vshankar · 2022-10-10T07:02:19Z

I guess this change is fine since noone has shown objection on the approach taken here.

@badone - @kotreshhr has run additionally this through rados:mgr. Would that be enough as a source of validation?

ping?

badone · 2022-10-12T21:32:50Z

I guess this change is fine since noone has shown objection on the approach taken here.
@badone - @kotreshhr has run additionally this through rados:mgr. Would that be enough as a source of validation?

ping?

Have Patrick's concerns been addressed? It's still showing a requested change.

kotreshhr · 2022-10-13T06:41:44Z

I guess this change is fine since noone has shown objection on the approach taken here.
@badone - @kotreshhr has run additionally this through rados:mgr. Would that be enough as a source of validation?

ping?

Have Patrick's concerns been addressed? It's still showing a requested change.

@badone I had missed noticing Patrick comment. I am working on it. I will refresh the PR once done. Are there any additional testing that you think which needs to be run ?

badone · 2022-10-17T03:24:25Z

@kotreshhr I think you can just run the same suites again on our updated PR and that should be sufficient.

kotreshhr · 2022-10-17T12:33:06Z

jenkins test windows

kotreshhr · 2022-10-18T04:38:01Z

Scheduled fs and rados suite:

http://pulpito.front.sepia.ceph.com/khiremat-2022-10-18_04:28:21-fs-wip-khiremat-47893-test-mgr-finisher-thread-1-distro-default-smithi/

http://pulpito.front.sepia.ceph.com/khiremat-2022-10-18_04:14:28-rados-wip-khiremat-47893-test-mgr-finisher-thread-1-distro-default-smithi/

kotreshhr · 2023-02-28T06:59:54Z

jenkins test api

kotreshhr · 2023-02-28T07:03:36Z

Can we simply put the new task in rados/mgr/tasks instead of defining an entire new suite?

Yes, Done. Thank you the review. It was added as a separate suite initially as it required a single mgr cluster. I fixed it down the line but the new suite remained.

kotreshhr · 2023-02-28T08:24:18Z

@kotreshhr pls add needs-qa when fixed

Sure, the changes are in place but I couldn't do sanity test since teuthology is down. I will do it once it's back and add needs-qa. Thank you.

kotreshhr · 2023-03-02T19:45:49Z

@yuriw marked needs-qa. The PR is ready for testing.

kotreshhr · 2023-03-02T19:58:55Z

jenkins test windows

kotreshhr · 2023-03-03T06:33:54Z

jenkins test windows

kotreshhr · 2023-03-03T06:34:11Z

jenkins test api

kotreshhr · 2023-03-03T11:25:57Z

jenkins test windows

kotreshhr · 2023-03-06T07:06:41Z

jenkins test windows

kotreshhr · 2023-03-07T07:14:07Z

jenkins test windows

kotreshhr · 2023-03-07T13:32:05Z

@athanatos PTAL. Addressed your comments

rzarzynski · 2023-03-27T18:51:55Z

Repinging @athanatos.

athanatos

The suite changes look reasonable to me now -- as long as tests pass LGTM

lxbsz · 2023-03-29T01:38:08Z

qa/tasks/check_counter.py

-                        if val >= minval:
+                        if isinstance(minval, int) and val >= minval:
+                            seen.add(name)
+                        elif isinstance(expected_val, int) and val == expected_val:


@kotreshhr This caused the following errors:

2023-03-28T10:34:36.302 ERROR:teuthology.run_tasks:Manager failed: check-counter Traceback (most recent call last): File "/home/teuthworker/src/git.ceph.com_teuthology_main/teuthology/run_tasks.py", line 192, in run_tasks suppress = manager.__exit__(*exc_info) File "/home/teuthworker/src/git.ceph.com_teuthology_main/teuthology/task/__init__.py", line 132, in __exit__ self.end() File "/home/teuthworker/src/github.com_ceph_ceph-c_9d12d2cb27d4a45c58218390bfbb56820b010b77/qa/tasks/check_counter.py", line 117, in end elif isinstance(expected_val, int) and val == expected_val: UnboundLocalError: local variable 'expected_val' referenced before assignment

More detail please see https://pulpito.ceph.com/yuriw-2023-03-28_00:15:34-fs-wip-yuri5-testing-2023-03-27-1315-distro-default-smithi/.

Could you have a look ? Thanks.

Thanks Xiubo. I will take a look.

Fixed it. Ran the failed one with the fix. It works. Hopefully this is the last failure.
http://pulpito.front.sepia.ceph.com/khiremat-2023-03-29_06:17:28-fs-wip-yuri5-testing-2023-03-27-1315-distro-default-smithi/

@yuriw This needs a qa re-run after this fix.

Fixes: https://tracker.ceph.com/issues/51177 Signed-off-by: Kotresh HR <khiremat@redhat.com>

kotreshhr · 2023-03-29T17:38:53Z

jenkins test make check arm64

kotreshhr · 2023-03-30T01:07:57Z

jenkins test make check arm64

lxbsz

LGTM.

vshankar

LGTM. Nice work.

kotreshhr requested a review from a team as a code owner September 1, 2022 06:37

kotreshhr marked this pull request as draft September 1, 2022 06:38

github-actions bot added core mgr labels Sep 1, 2022

kotreshhr requested a review from batrick September 1, 2022 12:45

vshankar requested review from badone, jdurgin and tchaikov September 7, 2022 04:40

batrick requested changes Sep 8, 2022

View reviewed changes

src/mgr/Mgr.cc Outdated Show resolved Hide resolved

vshankar marked this pull request as ready for review October 6, 2022 11:22

kotreshhr force-pushed the ceph-mgr-finisher-block branch from 2467508 to 48f5e65 Compare October 17, 2022 11:54

kotreshhr force-pushed the ceph-mgr-finisher-block branch 3 times, most recently from af37e52 to 929eb07 Compare March 2, 2023 18:38

kotreshhr added the needs-qa label Mar 2, 2023

kotreshhr removed the TESTED label Mar 12, 2023

yuriw added the wip-yuri5-testing label Mar 27, 2023

athanatos approved these changes Mar 28, 2023

View reviewed changes

lxbsz reviewed Mar 29, 2023

View reviewed changes

qa: Add test for per-module finisher thread

2c2ef6d

Fixes: https://tracker.ceph.com/issues/51177 Signed-off-by: Kotresh HR <khiremat@redhat.com>

kotreshhr force-pushed the ceph-mgr-finisher-block branch from 929eb07 to 2c2ef6d Compare March 29, 2023 06:04

lxbsz approved these changes Mar 31, 2023

View reviewed changes

vshankar approved these changes Mar 31, 2023

View reviewed changes

yuriw merged commit 23a958d into ceph:main Apr 6, 2023
11 checks passed

This was referenced Apr 12, 2023

reef: mgr: Add one finisher thread per module #51042

Merged

quincy: mgr: Add one finisher thread per module #51044

Merged

pacific: mgr: Add one finisher thread per module #51045

Merged

mgr: Add one finisher thread per module #47893

mgr: Add one finisher thread per module #47893

Conversation

kotreshhr commented Sep 1, 2022 • edited

Contribution Guidelines

Checklist

vshankar commented Sep 7, 2022

batrick commented Sep 7, 2022

vshankar commented Sep 8, 2022

badone commented Sep 8, 2022

vshankar commented Sep 9, 2022

badone commented Sep 9, 2022

vshankar commented Sep 9, 2022

badone commented Sep 9, 2022

kotreshhr commented Sep 9, 2022

kotreshhr commented Sep 12, 2022

kotreshhr commented Sep 12, 2022

vshankar commented Sep 16, 2022

vshankar commented Sep 21, 2022

vshankar commented Oct 10, 2022

badone commented Oct 12, 2022

kotreshhr commented Oct 13, 2022

badone commented Oct 17, 2022

kotreshhr commented Oct 17, 2022

kotreshhr commented Oct 18, 2022

kotreshhr commented Feb 28, 2023

kotreshhr commented Feb 28, 2023

kotreshhr commented Feb 28, 2023

kotreshhr commented Mar 2, 2023

kotreshhr commented Mar 2, 2023

kotreshhr commented Mar 3, 2023

kotreshhr commented Mar 3, 2023

kotreshhr commented Mar 3, 2023

kotreshhr commented Mar 6, 2023

kotreshhr commented Mar 7, 2023

kotreshhr commented Mar 7, 2023

rzarzynski commented Mar 27, 2023

athanatos left a comment

Choose a reason for hiding this comment

lxbsz Mar 29, 2023

Choose a reason for hiding this comment

kotreshhr Mar 29, 2023

Choose a reason for hiding this comment

kotreshhr Mar 29, 2023

Choose a reason for hiding this comment

kotreshhr Apr 3, 2023

Choose a reason for hiding this comment

kotreshhr commented Mar 29, 2023

kotreshhr commented Mar 30, 2023

lxbsz left a comment

Choose a reason for hiding this comment

vshankar left a comment

Choose a reason for hiding this comment

kotreshhr commented Sep 1, 2022 •

edited