On Thu, 6 Sep 2018, John Spray wrote:
> > contributing. This module does life expectancy prediction based on health
> > metrics either via a built-in model/predictor, or via an external
> > saas service. One of my goals with this is to make the user experience
> > well-integrated, so it adds commands like
> >
> > device get-predicted-status <dev_id> Get physical device predicted result
> > device predict-life-expectancy <dev_id> Predict life expectancy with local
> > predictor
> > device set-cloud-prediction-config Configure Disk Prediction service
> > <server> <user> <password> <certfile>
> > {<port>}
> > device set-prediction-mode <mode> config disk prediction mode ["cloud"|"
> > local"]
> > device show-prediction-config Prints diskprediction configuration
> >
> > There are commands here to set most of the relevant configuration options
> > and a separate command to view them (show-prediction-config), but these
> > translate to options that also appear when you do 'ceph config dump' or
> > 'ceph config get mgr.x', but as
> > "mgr/diskprediction/diskprediction_config_mode" (which could be a bit less
> > redundant, but still includes the mgr/module/ prefix). For someone
> > looking at the 'ceph config ...' output, these options strike me as
> > weirdly named and confusing.
>
> Imagining the slightly cleaner version
> "mgr/diskprediction/config_mode", I'd say that the confusing part is
> really the "mgr/" prefix, right? Because people aren't thinking about
> ceph-mgr when they think about disk failure prediction, they're
> thinking about OSDs, or Ceph in general.
>
> The "mgr/" prefix is currently necessary to let the monitor
> conditional skip validation when doing a "config set", but we could
> get away from that if we had the mgr daemons transmit their config
> option schema to the monitor (iirc we discussed doing that as a follow
> up to moving the mgr config options into the main config store).
>
> Then, if we wanted to, we could move to a more uniform convention,
> replacing "/" with "_" between the module name and the setting, and
> the entry in the configuration map would just be called
> "diskprediction_config_mode".
I think this is the right path forward: giving the mgr options a fully
specified schema.
> > We could make this a bit less weird by being careful about how the modules
> > are named. For example, currently the diskpredition module includes both
> > the saas mode code and the local predictor. We could separate the local
> > predictor into a 'localpredictor' module so that any options are
> > 'mgr/localpredictor/foo' and the saas mode code options are
> > 'mgr/saasprediction/bar' (or whatever). Usually this will work out well
> > enough (e.g., the 'rook' orchestrator module is called 'rook' so the
> > options will be 'mgr/rook/something').
> >
> > There is an implementation detail that a module foo can't read or write an
> > option defined by another module, so for example multiple modules
> > (localpredictor, saaspredition) couldn't both consume an option
> > controlling which module implements a feature (say,
> > mgr/devicehealth/prediction_mode). (I expect this issue already makes the
> > 'mgr/orchestrator_cli/orchestrator' option a bit awkward?)
>
> I think it's a good thing to have an option clearly "owned" by a
> particular module -- if we have things which are truly global then
> they probably belong in options.cc, even if they're only consumed by
> (multiple) mgr modules.
I think is sufficient given what we've seen so far. My only real nit to
pick here is the orchestrator_cli naming... AFAICS it has that _cli thing
at the end because the module would otherwise collide with the
abstract interface Orchestrator class? Maybe we can clean that part
up somehow? :)
> For one module to peek at another's configuration, they can already do
> that if the config-owning module exposes a method and the interested
> party calls MgrModule.remote() to get at it.
>
> > Alternatively, we could allow arbitrary option names. All of our options
> > are already kind of weird in that they tend to have mon_ or osd_ or mds_
> > prefixes but don't necessarily only apply to that daemon (and we can map
> > them to different daemon types and specific daemons). The 'mgr/' prefix
> > adds a new variation of weird to it in that it has a / instead of _. We
> > could instead make the mgr modules allow us to define arbitrary new option
> > names (without constraints).
>
> Perhaps if we can get to the case I described above
> (diskprediction_config_mode), then maybe we don't need to go this far,
> as the module options will already look pretty normal?
FWIW I don't think it's more or less work to namespace the module
options... it's just a question of whether we define the full object name
in the OPTIONS dict or whather a prefix is silently added inside the mgr
code somewhere. TBH I'd lean toward not namespacing for
simplicity/explicitness, but either way will work well enough. I think
the module names won't conflict with existing options.cc prefixes
(osd_, filestore_, etc.) so it should be okay regardless.
sage
