On Wed, Sep 5, 2018 at 11:27 PM Sage Weil <sweil@xxxxxxxxxx> wrote:
>
> Right now we have a baked-in assumption that config option names for
> manager modules are "mgr/$modulename/$optionname". This works well enough
> for some modules that are clearly optional and map 1:1 to the type
> of functionality they provide. For example, the prometheus module
> options are "mgr/prometheus/foo" and so on.
>
> In other cases, this is somewhat awkward. For example, we have built-in
> modules like 'devicehealth' (that could perhaps be renamed to device?)
> that are "always on" and currently handle scraping of metrics from
> devices. There are several options here:
>
> 'enable_monitoring': True,
> 'scrape_frequency': str(86400),
> 'retention_period': str(86400*14),
> 'pool_name': 'device_health_metrics',
>
> which translate to options like "mgr/devicehealth/enablemonitoring". That
> is not too bad, although it bugs me that the "mgr/devicehealth" is there
> and fixed but this is really "built-in" functionality (that just happens
> to be implemented by the mgr daemon).
The original motivation was to sandbox the modules' configuration, and
the "mgr/" prefix was to let module authors name their options
succinctly without worrying about collisions (i.e. just call your port
option "port" instead of "my_module_port"). At that point, the
"mgr/foo/bar" things were meant to be hidden (config-key keys) and
modules were meant to implement their own CLIs, because updates to
config-key data wouldn't take effect until restart anyway.
Now we have the nice "config set" commands, users are naturally more
inclined to just use those rather than going via a module specific
command, so the long-form option names are much more visible. The
namespacing is also a bit of a questionable motivation, given that we
have no problem letting modules populate globally visible CLI
operations.
Of course, it is already possible for modules to use globally defined
config options, by just adding them in options.cc and then reading
them out of MgrModule.get("config"). Not the most elegant, but
potentially something we would do for settings that don't naturally
belong to a particular module, such as the configuration of which
orchestrator module to use.
> Things get weirder with the diskprediction module that prophetstor is
> 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".
> 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.
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?
John
> Thoughts?
> sage
