New configuration / command line handling

New configuration / command line handling

[Roald van Loon]

Hi list, ceph devs,

I've been doing some toying with Ceph lately in my spare time and one
of the things I'm doing is trying to refactor some of the
configuration handling / argument parsing. I had some trouble finding
specific runtime options and configuration settings, so I thought this
was a good way to get to know the code somewhat better :-) Because
boost was already used in some places, and some tests also used
boost::program_options, I've created a new config framework based on
this. This framework allows for more advanced configuration files in
the future, beter command line handling, and reduces the amount of
code in argument parsing somewhat. I also created a different observer
system, where different parts of the code base can subscribe to
changes in specific parameters and get notified on changes.

To allow somewhat of a smoother transition and backwards
compatibility, I've made the new configuration class inherit from the
one one (md_config_t) and switched all references (like g_conf) to the
new class. There were also some small changes in files with references
directly to md_config_t, like pidfile_write. Global_init and
common_init were duplicated such that it 'injects' the new
configuration class in the ceph_context object. Most config entries
are read directly from the md_config_t class (e.g. g_conf->pid_file),
so it will take some more changes to allow my configuration framework
to function properly.

Of course it's an extremely long long long way from ready, but maybe
you're interested. Maybe you think this is not needed at all... I'd
love to have some feedback on it either way.

Anyway, let me know if you want to check it out and I'll push my
branch to github for you to pull.


- Roald

Re: New configuration / command line handling

[Roald van Loon]

Hi,

I've pushed a "working" (read: compiling) branch to github with the
basics of the new configuration handling I mentioned earlier;

http://github.com/roaldvanloon/ceph.git

branch = wip-config

As an example, I've copied the ceph_osd to ceph_osd2 and altered the
initialization in main() such that it uses the new config.

A make ceph-osd2 && ./ceph_osd2 --help should work.

- Roald


On Mon, Dec 3, 2012 at 10:57 AM, Roald van Loon <roaldvanloon@gmail.com> wrote:
> Hi list, ceph devs,
>
> I've been doing some toying with Ceph lately in my spare time and one
> of the things I'm doing is trying to refactor some of the
> configuration handling / argument parsing. I had some trouble finding
> specific runtime options and configuration settings, so I thought this
> was a good way to get to know the code somewhat better :-) Because
> boost was already used in some places, and some tests also used
> boost::program_options, I've created a new config framework based on
> this. This framework allows for more advanced configuration files in
> the future, beter command line handling, and reduces the amount of
> code in argument parsing somewhat. I also created a different observer
> system, where different parts of the code base can subscribe to
> changes in specific parameters and get notified on changes.
>
> To allow somewhat of a smoother transition and backwards
> compatibility, I've made the new configuration class inherit from the
> one one (md_config_t) and switched all references (like g_conf) to the
> new class. There were also some small changes in files with references
> directly to md_config_t, like pidfile_write. Global_init and
> common_init were duplicated such that it 'injects' the new
> configuration class in the ceph_context object. Most config entries
> are read directly from the md_config_t class (e.g. g_conf->pid_file),
> so it will take some more changes to allow my configuration framework
> to function properly.
>
> Of course it's an extremely long long long way from ready, but maybe
> you're interested. Maybe you think this is not needed at all... I'd
> love to have some feedback on it either way.
>
> Anyway, let me know if you want to check it out and I'll push my
> branch to github for you to pull.
>
>
> - Roald

Re: New configuration / command line handling

[Josh Durgin]

On 12/03/2012 01:57 AM, Roald van Loon wrote:
> Hi list, ceph devs,
>
> I've been doing some toying with Ceph lately in my spare time and one
> of the things I'm doing is trying to refactor some of the
> configuration handling / argument parsing. I had some trouble finding
> specific runtime options and configuration settings, so I thought this
> was a good way to get to know the code somewhat better :-) Because
> boost was already used in some places, and some tests also used
> boost::program_options, I've created a new config framework based on
> this. This framework allows for more advanced configuration files in
> the future, beter command line handling, and reduces the amount of
> code in argument parsing somewhat. I also created a different observer
> system, where different parts of the code base can subscribe to
> changes in specific parameters and get notified on changes.

Awesome! We've needed better command line parsing and usage for a while.
It looks like boost::program_options does a lot of nice things there.

> To allow somewhat of a smoother transition and backwards
> compatibility, I've made the new configuration class inherit from the
> one one (md_config_t) and switched all references (like g_conf) to the
> new class. There were also some small changes in files with references
> directly to md_config_t, like pidfile_write. Global_init and
> common_init were duplicated such that it 'injects' the new
> configuration class in the ceph_context object. Most config entries
> are read directly from the md_config_t class (e.g. g_conf->pid_file),
> so it will take some more changes to allow my configuration framework
> to function properly.

This seems like a nice way to allow it to be used gradually by more
and more tools.

> Of course it's an extremely long long long way from ready, but maybe
> you're interested. Maybe you think this is not needed at all... I'd
> love to have some feedback on it either way.

It looks like you've got a good start on integrating it into the main
ceph infrastructure. Skimming over it, the general structure makes sense
to me. One part that might be a bit tricky is making the new parsing
backwards compatible with the old parsing.

1) For config options in general, '-', '_', and ' ' are all equivalent.
2) In ceph's config file format, we treat ';' as a comment in addition 
to '#'.

I'm not sure how difficult those are to handle with the built-in
boost::program_options parsers.

Converting some of the individual command line tools that use
subcommands might be a little cumbersome too, but I think they'll
be better off once it's done.

Josh

Re: New configuration / command line handling

[Roald van Loon]

On Mon, Dec 3, 2012 at 10:51 PM, Josh Durgin <josh.durgin@inktank.com> wrote:
> It looks like you've got a good start on integrating it into the main
> ceph infrastructure. Skimming over it, the general structure makes sense
> to me. One part that might be a bit tricky is making the new parsing
> backwards compatible with the old parsing.
>
> 1) For config options in general, '-', '_', and ' ' are all equivalent.
> 2) In ceph's config file format, we treat ';' as a comment in addition to
> '#'.
>
> I'm not sure how difficult those are to handle with the built-in
> boost::program_options parsers.

Well, there is another (maybe larger) problem; metavalues. Boost::po
can't handle them. That's why I am implementing the raw config file
parsing separately (partly using boost::property_tree actually), load
all options in a tree hierarchy (for example [osd.1] -> [osd] ->
[global]) and then do metavalue lookups top-down with static regexps.
Then, I have a parsed and complete set in the top of the tree (osd.1
in this case) which I can feed to boost::po to be parsed. Still have
to work out some of the details though, but I think this is generally
a nice way of config parsing. I'll see if I can find the time this
week to implement it if you're interested.

> Converting some of the individual command line tools that use
> subcommands might be a little cumbersome too, but I think they'll
> be better off once it's done.

My biggest concern is that most code directly accesses members of
md_config_t by means of g_conf->foo. It's very difficult to port that
to the new class without actually loading all of the available config
options as class members - something I'm personally not a big fan of.
I haven't had the time to think of a nice solution, yet (something
like getter/setter methods maybe, or macros?). If you have any
thoughts about this I'd very much like to hear them.

- Roald

Re: New configuration / command line handling

[Josh Durgin]

On 12/03/2012 02:46 PM, Roald van Loon wrote:
> On Mon, Dec 3, 2012 at 10:51 PM, Josh Durgin <josh.durgin@inktank.com> wrote:
>> It looks like you've got a good start on integrating it into the main
>> ceph infrastructure. Skimming over it, the general structure makes sense
>> to me. One part that might be a bit tricky is making the new parsing
>> backwards compatible with the old parsing.
>>
>> 1) For config options in general, '-', '_', and ' ' are all equivalent.
>> 2) In ceph's config file format, we treat ';' as a comment in addition to
>> '#'.
>>
>> I'm not sure how difficult those are to handle with the built-in
>> boost::program_options parsers.
>
> Well, there is another (maybe larger) problem; metavalues. Boost::po
> can't handle them. That's why I am implementing the raw config file
> parsing separately (partly using boost::property_tree actually), load
> all options in a tree hierarchy (for example [osd.1] -> [osd] ->
> [global]) and then do metavalue lookups top-down with static regexps.
> Then, I have a parsed and complete set in the top of the tree (osd.1
> in this case) which I can feed to boost::po to be parsed. Still have
> to work out some of the details though, but I think this is generally
> a nice way of config parsing. I'll see if I can find the time this
> week to implement it if you're interested.

I think this makes sense. I guess the alternative is expanding the
metavalues after boost::po breaks them down, but I don't see any
big advantages either way.

>> Converting some of the individual command line tools that use
>> subcommands might be a little cumbersome too, but I think they'll
>> be better off once it's done.
>
> My biggest concern is that most code directly accesses members of
> md_config_t by means of g_conf->foo. It's very difficult to port that
> to the new class without actually loading all of the available config
> options as class members - something I'm personally not a big fan of.
> I haven't had the time to think of a nice solution, yet (something
> like getter/setter methods maybe, or macros?). If you have any
> thoughts about this I'd very much like to hear them.

I'm not sure this is the best solution, but we have a small number of
types of config values. We could have a getter for each type (like
md_config_t::get_val(), but nicer to use in C++ and asserting that
the configuration option is valid and of the appropriate type), i.e.:

     uint64_t get_uint64_t(const std::string &name);

This could work for internal users (i.e. anyone using g_conf->foo right
now). The library users (e.g. rados_conf_get()) would still want an
interface like md_config_t::get_val() that returns an error if the
option doesn't exist.

Josh

Re: New configuration / command line handling

[Roald van Loon]

Hi,

Alright, I did some work on this. My time was limited last week, so it
took a while. Nonetheless, I have some changes pushed to github
(github.com/roaldvanloon, ceph, branch wip-config).

Most of the time went into thinking about a nice & clean interface
which A) would make porting the existing code fairly easy and B) would
be easy extensible;

On Wed, Dec 5, 2012 at 4:04 AM, Josh Durgin <josh.durgin@inktank.com> wrote:
>
> I'm not sure this is the best solution, but we have a small number of
> types of config values. We could have a getter for each type (like
> md_config_t::get_val(), but nicer to use in C++ and asserting that
> the configuration option is valid and of the appropriate type), i.e.:
>
>     uint64_t get_uint64_t(const std::string &name);
>
> This could work for internal users (i.e. anyone using g_conf->foo right
> now). The library users (e.g. rados_conf_get()) would still want an
> interface like md_config_t::get_val() that returns an error if the
> option doesn't exist.


My first idea was to make nice templated 'Value' classes, and use a
CRTP pattern or something to make it cloneable and dynamic_castable to
different types at runtime. However, this would make the whole shebang
both complicated and more error prone. So... I dropped that idea.

I think I have found a workable solution, so please check it out in
git :-) Basically it boils down to the following: the configuration
object creates an 'active' set of parameters, eg this is the set of
all config values for any given entity (like mon.alpha or osd.55).
This set is accessible using;

(Configuration*)->active()

This returns a Collection* that contains all Parameter* objects
accessible by parameter key;

(Configuration*)->active()->param("osd data") /* all [\._-] is
translated to a space btw */

Internally, all values are stored as std::strings, and to get it you can do;

(Configuration*)->active()->param("osd data")->get()

If you want in a different type, you can do;

(Configuration*)->active()->param("osd data")->as<T>()

It tries to do lexical casts as good as can be (returning an empty T()
if all fails), but of course this needs some flexibility when you want
an uuid_d for instance. So you need to make small overloads for this
method. For uuid_d these are;

template<>
uuid_d Parameter::as<uuid_d>() const
{
  uuid_d v;
  v.parse(get().c_str());
  return v;
}

template<>
void Parameter::from<uuid_d>(const uuid_d& u)
{
  char b[37];
  uuid_unparse(u.uuid, b);
  this->set(std::string(b));
}

Then the following will work and will give you an uuid_d object loaded
with the right fsid;

uuid_d my_fsid = (Configuration*)->active()->param("fsid")->as<uuid_d>();

For the global config in g_ceph_context, I created some macros for ease of use;

uuid_d my_fsid = CEPH_CFG_UUID("fsid");

(which actually does; CEPH_CFG(uuid_d, "fsid"), see the macro
definitions in config.h)

Default config values are defined in defaults.h. If you want to have
non-std types here, you also have to create a 'validator' for that
type that checks the config input (command line, config file) for
validness. See Validators.{cc,h}.

Still work in progress (I have a backlog filled with weirdness and
bugs, and also env params are not yet working), but I think this is a
nice way to access config values.

Looking forward for your comments,

Cheers,

- Roald


PS; metavalue parsing is now transparent, so also on the command line.
Great for infinite loops;

dev src # ./ceph-osd2 --id \$id
Segmentation fault
dev src #

Like I said, still a lot of bugs to squash :-)