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: map injector #41260

Merged
merged 7 commits into from Jul 7, 2021
Merged

Conversation

pereman2
Copy link
Contributor

@pereman2 pereman2 commented May 10, 2021

Well, I've used the OSDMap methods to set the max of osds and check them as existent in order to show them.

Signed-off-by: Pere Diaz Bou pdiazbou@redhat.com

Checklist

  • References tracker ticket
  • Updates documentation if necessary
  • Includes tests for new functionality or reproducer for bug

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 api
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox

@pereman2 pereman2 added this to In progress in Dashboard via automation May 10, 2021
Copy link
Member

@jdurgin jdurgin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another question is how do we enable/parameterize this?

One way is dev-level ceph options - at least one to enable/disable fake map injection. We could use more to determine e.g. number of osds, or other things we might want to vary.

src/mgr/Injector.cc Outdated Show resolved Hide resolved
src/mgr/ActivePyModules.cc Outdated Show resolved Hide resolved
@pereman2 pereman2 changed the title mgr: inject osdmap PyObject* get_python mgr: [wip] mgr map injector May 12, 2021
@pereman2 pereman2 changed the title mgr: [wip] mgr map injector mgr: [wip] map injector May 12, 2021
@pereman2 pereman2 changed the title mgr: [wip] map injector [WIP] mgr: map injector May 12, 2021
src/common/options/global.yaml.in Outdated Show resolved Hide resolved
src/common/options/global.yaml.in Outdated Show resolved Hide resolved
src/mgr/Injector.cc Show resolved Hide resolved
@tchaikov
Copy link
Contributor

if we want to inject osdmaps into mgr. i'd suggest just monkey patch the mgr module under test using #41591. the change in this PR is able to inject maps in some predefined ways but it's still not flexible enough for testing.

@epuertat
Copy link
Member

epuertat commented Jun 2, 2021

if we want to inject osdmaps into mgr. i'd suggest just monkey patch the mgr module under test using #41591. the change in this PR is able to inject maps in some predefined ways but it's still not flexible enough for testing.

@tchaikov we need to inject them at C++ side so the performance overhead of the PyFormatter is counted in. Otherwise, we cannot properly estimate the caching benefits unless we deploy a cluster with 1000's OSDs.

@pereman2
Copy link
Contributor Author

pereman2 commented Jun 2, 2021

if we want to inject osdmaps into mgr. i'd suggest just monkey patch the mgr module under test using #41591. the change in this PR is able to inject maps in some predefined ways but it's still not flexible enough for testing.

@tchaikov we need to inject them at C++ side so the performance overhead of the PyFormatter is counted in. Otherwise, we cannot properly estimate the caching benefits unless we deploy a cluster with 1000's OSDs.

@epuertat if you look at the PyOSDMap.cc we have this:

static PyObject *osdmap_dump(BasePyOSDMap* self, PyObject *obj)
{
  PyFormatter f;
  self->osdmap->dump(&f);
  return f.get();
}

which transaltes to BasePyOSDMap._dump(self) in the ceph_module.pyi. So what kefu wants is find a way to override the osdmap from mgr._get_osdmap so we can dump with the function above and then this would include the PyFormatter overhead that we need. All of this being done in the python side.

@github-actions
Copy link

github-actions bot commented Jun 5, 2021

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

@epuertat
Copy link
Member

@pereman2 @tchaikov understood! Thanks for pointing to that. I wasn't aware that we had a CPython wrapper for the OSDMap class. If we plan to have more of those, it might be useful to rely on some improved C++ Python binding, like pybind11, in order to reduce boilerplate code, improve maintainability and get safer code.

That said, I still don't see clearly an alternative way to inject a fake number of OSDs together with the real data. @pereman2 if you know how to continue, no problem from my end, but please let's do something about this one: there're no approvals nor change requests.

If we don't have this merged, there's no easy way to validate the TTL Cache improvements (unless you have a few hundred nodes under your desk).

Signed-off-by: Pere Diaz Bou <pdiazbou@redhat.com>
Signed-off-by: Pere Diaz Bou <pdiazbou@redhat.com>
Signed-off-by: Pere Diaz Bou <pdiazbou@redhat.com>
Signed-off-by: Pere Diaz Bou <pdiazbou@redhat.com>
Signed-off-by: Pere Diaz Bou <pdiazbou@redhat.com>
Signed-off-by: Pere Diaz Bou <pdiazbou@redhat.com>
Signed-off-by: Pere Diaz Bou <pdiazbou@redhat.com>
@pereman2 pereman2 changed the base branch from master to feature-48388-cache July 5, 2021 10:14
@pereman2 pereman2 marked this pull request as ready for review July 6, 2021 06:39
Copy link
Member

@epuertat epuertat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Let's merge this in the feature branch for testing purposes, and drop it when merging this back to master.

Dashboard automation moved this from In progress to Reviewer approved Jul 6, 2021
@pereman2 pereman2 changed the title [WIP] mgr: map injector mgr: map injector Jul 7, 2021
@pereman2 pereman2 removed the DNM label Jul 7, 2021
@alfonsomthd alfonsomthd moved this from Reviewer approved to Ready-to-merge in Dashboard Jul 7, 2021
@alfonsomthd alfonsomthd merged commit bf8b703 into ceph:feature-48388-cache Jul 7, 2021
Dashboard automation moved this from Ready-to-merge to Done Jul 7, 2021
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