Skip to content
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

mgr: make python notifications more efficient #44162

Merged
merged 4 commits into from Dec 8, 2021

Conversation

liewegas
Copy link
Member

@liewegas liewegas commented Dec 1, 2021

  • remove unused notifications
  • only delivery a notification if the module wants one

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)
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

Signed-off-by: Sage Weil <sage@newdream.net>
Note that we don't annotate the dashboard NotificationQueue because it is
used internally by the dashboard with other events.

Signed-off-by: Sage Weil <sage@newdream.net>
Signed-off-by: Sage Weil <sage@newdream.net>
Signed-off-by: Sage Weil <sage@newdream.net>
Dashboard automation moved this from In progress to Reviewer approved Dec 3, 2021
@liewegas
Copy link
Member Author

liewegas commented Dec 4, 2021

jenkins test dashboard cephadm

@liewegas
Copy link
Member Author

liewegas commented Dec 4, 2021

jenkins retest this please

@liewegas liewegas merged commit 1c741b4 into ceph:master Dec 8, 2021
9 of 10 checks passed
Dashboard automation moved this from Reviewer approved to Done Dec 8, 2021
@ThomasLamprecht
Copy link
Contributor

Now, if a module defines no NOTIFY_TYPES, as quite some do, we get a rather ugly error on any manager (re)start, e.g. on quincy:

ceph-mgr[19176]: 2022-06-01T15:35:03.658+0200 7fd4a402fe80 -1 mgr[py] Module telemetry has missing NOTIFY_TYPES member
ceph-mgr[19176]: 2022-06-01T15:35:03.926+0200 7fd4a402fe80 -1 mgr[py] Module volumes has missing NOTIFY_TYPES member
ceph-mgr[19176]: 2022-06-01T15:35:04.078+0200 7fd4a402fe80 -1 mgr[py] Module rbd_support has missing NOTIFY_TYPES member
ceph-mgr[19176]: 2022-06-01T15:35:04.186+0200 7fd4a402fe80 -1 mgr[py] Module devicehealth has missing NOTIFY_TYPES member
ceph-mgr[19176]: 2022-06-01T15:35:04.302+0200 7fd4a402fe80 -1 mgr[py] Module crash has missing NOTIFY_TYPES member
ceph-mgr[19176]: 2022-06-01T15:35:04.386+0200 7fd4a402fe80 -1 mgr[py] Module iostat has missing NOTIFY_TYPES member
ceph-mgr[19176]: 2022-06-01T15:35:04.482+0200 7fd4a402fe80 -1 mgr[py] Module influx has missing NOTIFY_TYPES member
ceph-mgr[19176]: 2022-06-01T15:35:04.574+0200 7fd4a402fe80 -1 mgr[py] Module selftest has missing NOTIFY_TYPES member 

It's not that bad, but QA complained on internal testing, and I'd figure some users of ours would get scared too.
Without being too much into the code base I can think of the following options to improve this from top of my head:

  1. defuse the error to a less visible one
  2. add an empty NOTIFY_TYPES array to all other modules too
  3. modules without any such entry get always notified instead (possibly behavior changing)

The first and second options seem relatively non-invasive to me, for the third option I'd need to know what the notify mechanisms triggers exactly (could look it up just fine but figured I post here first as y'all probably now much better already). FWIW, out of the 37 modules I'm counting here in src/pybind/mgr 10 have a NOTIFY_TYPES array specified.

@epuertat
Copy link
Member

epuertat commented Jun 2, 2022

Hi @ThomasLamprecht !

Good idea, I also find those errors slightly annoying and not really informative.

I filed this issue to track your suggestion and further discussions.

Thanks!

@dparmar18
Copy link
Contributor

Now, if a module defines no NOTIFY_TYPES, as quite some do, we get a rather ugly error on any manager (re)start, e.g. on quincy:

ceph-mgr[19176]: 2022-06-01T15:35:03.658+0200 7fd4a402fe80 -1 mgr[py] Module telemetry has missing NOTIFY_TYPES member
ceph-mgr[19176]: 2022-06-01T15:35:03.926+0200 7fd4a402fe80 -1 mgr[py] Module volumes has missing NOTIFY_TYPES member
ceph-mgr[19176]: 2022-06-01T15:35:04.078+0200 7fd4a402fe80 -1 mgr[py] Module rbd_support has missing NOTIFY_TYPES member
ceph-mgr[19176]: 2022-06-01T15:35:04.186+0200 7fd4a402fe80 -1 mgr[py] Module devicehealth has missing NOTIFY_TYPES member
ceph-mgr[19176]: 2022-06-01T15:35:04.302+0200 7fd4a402fe80 -1 mgr[py] Module crash has missing NOTIFY_TYPES member
ceph-mgr[19176]: 2022-06-01T15:35:04.386+0200 7fd4a402fe80 -1 mgr[py] Module iostat has missing NOTIFY_TYPES member
ceph-mgr[19176]: 2022-06-01T15:35:04.482+0200 7fd4a402fe80 -1 mgr[py] Module influx has missing NOTIFY_TYPES member
ceph-mgr[19176]: 2022-06-01T15:35:04.574+0200 7fd4a402fe80 -1 mgr[py] Module selftest has missing NOTIFY_TYPES member 

It's not that bad, but QA complained on internal testing, and I'd figure some users of ours would get scared too. Without being too much into the code base I can think of the following options to improve this from top of my head:

  1. defuse the error to a less visible one
  2. add an empty NOTIFY_TYPES array to all other modules too
  3. modules without any such entry get always notified instead (possibly behavior changing)

The first and second options seem relatively non-invasive to me, for the third option I'd need to know what the notify mechanisms triggers exactly (could look it up just fine but figured I post here first as y'all probably now much better already). FWIW, out of the 37 modules I'm counting here in src/pybind/mgr 10 have a NOTIFY_TYPES array specified.

I did see this while investigating a failure log, I've noted in the tracker @epuertat created(https://tracker.ceph.com/issues/55835#note-1), thought to add it to this PR as well that maybe we can switch to using dout(10) or dout(20) instead of derr at https://github.com/ceph/ceph/blob/main/src/mgr/PyModule.cc#L518, this would keep the logs clean and debug levels can be ramped up as needed to have these lines on logs. What do you think?

@epuertat
Copy link
Member

epuertat commented Mar 1, 2023

The only reason I get for using derr there is that not having NOTIFY_TYPES is undesirable (from a performance standpoint), and hence modules should define what notify types they'll be handling (commonly: NOTIFY_TYPES = [NotifyType.command] since most modules handle Ceph CLI commands).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Dashboard
  
Done
5 participants