mgr: initialize PyModuleRegistry sooner #20321
Conversation
|
Seems to have conflict From https://github.com/jcsp/ceph
|
|
Sorry, I had a total brainfart and opened this PR instead of the one I meant to open as a backport -- this is not even a backport at all! |
jcsp
changed the title
jcsp
changed the title
|
OK, finally came back to this and fixed it up, should be good to go. Passed locally with tasks.mgr.test_module_selftest |
src/mgr/PyModuleRegistry.h
Outdated
| // or active_start | ||
| for (const auto &i : modules) { | ||
| const auto &module = i.second; | ||
| const bool enabled = (mgr_map.modules.count(module_name) > 0); |
There was a problem hiding this comment.
/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/13.0.1-2432-g7c5dfdf/rpm/el7/BUILD/ceph-13.0.1-2432-g7c5dfdf/src/mgr/PyModuleRegistry.h:97:53: error: 'module_name' was not declared in this scope
const bool enabled = (mgr_map.modules.count(module_name) > 0);
^~~~~~~~~~~
There was a problem hiding this comment.
could also put following, as we have structured binding in C++17 now:
for (auto& [module_name, module] : modules) {
const bool enabled = (mgr_map.modules.count(module_name) > 0);
module.set_enabled(enabled);
}also, i'd suggest move the implementation of method into .cc file.
please note PyModule::set_enabled() is not a const method. so we cannot use const auto& [module_name, module] here.
There was a problem hiding this comment.
Thanks for suggestions, this is my first time using the new structured binding bits and I like it :-)
The const auto is permitted (if a bit misleading) because the module part is a shared_ptr rather than the actual PyModule
I've pushed an update.
jcsp
force-pushed
the
wip-22918
branch
2 times, most recently
from
23a487e
to
61d94d1
Compare
This was waiting on MgrMap to init, so that it would know whether modules should be enabled. However, you don't really need to know that until someone calls standby_start or active_start, so we can initialize during MgrStandby::init, and fill out the enabled flags later. This prevents the mgr from prematurely emitting a MMgrBeacon that claims no modules are found. http://tracker.ceph.com/issues/22918 Signed-off-by: John Spray <john.spray@redhat.com>
This was returning an int that was only ever zero. Signed-off-by: John Spray <john.spray@redhat.com>
|
@tchaikov could you do one last review pass when you have a minute? |
|
|
@jcsp i think this fix also applies to luminous, so i am adding "backport=luminous" in its tracker ticket accordingly. please correct me if i am wrong. |
This was waiting on MgrMap to init, so that it would know whether
modules should be enabled. However, you don't really need to know
that until someone calls standby_start or active_start, so we
can initialize during MgrStandby::init, and fill out the enabled
flags later.
This prevents the mgr from prematurely emitting a MMgrBeacon
that claims no modules are found.
http://tracker.ceph.com/issues/22918
Signed-off-by: John Spray john.spray@redhat.com