Re: [RFC ndctl PATCH 0/3] Rename monitor.conf to ndctl.conf as a ndctl global config file

Re: [RFC ndctl PATCH 0/3] Rename monitor.conf to ndctl.conf as a ndctl global config file

[Verma, Vishal L]

[switching to the new mailing list]

On Mon, 2021-05-17 at 08:14 +0900, QI Fuli wrote:
> From: QI Fuli <qi.fuli@fujitsu.com>
> 
> This patch set is to rename monitor.conf to ndctl.conf, and make it a
> global ndctl configuration file that all ndctl commands can refer to.
> 
> As this patch set has been pending until now, I would like to know if
> current idea works or not. If yes, I will finish the documents and test.
> 
> Signed-off-by: QI Fuli <qi.fuli@fujitsu.com>

Hi Qi,

Thanks for picking up on this! The approach generally looks good to me,
I think we can definitely move forward with this direction.

One thing that stands out is - I don't think we can simply rename the
existing monitor.conf. We have to keep supporting the 'legacy'
monitor.conf so that we don't break any deployments. I'd suggest
keeping the old monitor.conf as is, and continuing to parse it as is,
but also adding a new ndctl.conf as you have done.

We can indicate that 'monitor.conf' is legacy, and any new features
will only get added to the new global config to encourage migration to
the new config. Perhaps we can even provide a helper script to migrate
the old config to new - but I think it needs to be a user triggered
action.

This is timely as I also need to go add some config related
functionality to daxctl, and basing it on this would be perfect, so I'd
love to get this series merged in soon.

Thanks again!
-Vishal

> 
> QI Fuli (3):
>   ndctl, ccan: import ciniparser
>   ndctl, util: add parse-configs helper
>   ndctl, rename monitor.conf to ndctl.conf
> 
>  Makefile.am                        |   8 +-
>  ccan/ciniparser/ciniparser.c       | 480 +++++++++++++++++++++++++++++
>  ccan/ciniparser/ciniparser.h       | 262 ++++++++++++++++
>  ccan/ciniparser/dictionary.c       | 266 ++++++++++++++++
>  ccan/ciniparser/dictionary.h       | 166 ++++++++++
>  configure.ac                       |   8 +-
>  ndctl/Makefile.am                  |   9 +-
>  ndctl/monitor.c                    | 127 ++------
>  ndctl/{monitor.conf => ndctl.conf} |  16 +-
>  util/parse-configs.c               |  47 +++
>  util/parse-configs.h               |  26 ++
>  11 files changed, 1294 insertions(+), 121 deletions(-)
>  create mode 100644 ccan/ciniparser/ciniparser.c
>  create mode 100644 ccan/ciniparser/ciniparser.h
>  create mode 100644 ccan/ciniparser/dictionary.c
>  create mode 100644 ccan/ciniparser/dictionary.h
>  rename ndctl/{monitor.conf => ndctl.conf} (82%)
>  create mode 100644 util/parse-configs.c
>  create mode 100644 util/parse-configs.h
> 

Re: [RFC ndctl PATCH 0/3] Rename monitor.conf to ndctl.conf as a ndctl global config file

[Dan Williams]

On Tue, Jun 1, 2021 at 10:31 PM Verma, Vishal L
<vishal.l.verma@intel.com> wrote:
>
> [switching to the new mailing list]
>
> On Mon, 2021-05-17 at 08:14 +0900, QI Fuli wrote:
> > From: QI Fuli <qi.fuli@fujitsu.com>
> >
> > This patch set is to rename monitor.conf to ndctl.conf, and make it a
> > global ndctl configuration file that all ndctl commands can refer to.
> >
> > As this patch set has been pending until now, I would like to know if
> > current idea works or not. If yes, I will finish the documents and test.
> >
> > Signed-off-by: QI Fuli <qi.fuli@fujitsu.com>
>
> Hi Qi,
>
> Thanks for picking up on this! The approach generally looks good to me,
> I think we can definitely move forward with this direction.
>
> One thing that stands out is - I don't think we can simply rename the
> existing monitor.conf. We have to keep supporting the 'legacy'
> monitor.conf so that we don't break any deployments. I'd suggest
> keeping the old monitor.conf as is, and continuing to parse it as is,
> but also adding a new ndctl.conf as you have done.
>
> We can indicate that 'monitor.conf' is legacy, and any new features
> will only get added to the new global config to encourage migration to
> the new config. Perhaps we can even provide a helper script to migrate
> the old config to new - but I think it needs to be a user triggered
> action.
>
> This is timely as I also need to go add some config related
> functionality to daxctl, and basing it on this would be perfect, so I'd
> love to get this series merged in soon.

I wonder if ndctl should treat /etc/ndctl like a conf.d directory of
which all files with the .conf suffix are concatenated into one
combined configuration file. I.e. something that maintains legacy, but
also allows for config fragments to be deployed individually.

Re: [RFC ndctl PATCH 0/3] Rename monitor.conf to ndctl.conf as a ndctl global config file

[Verma, Vishal L]

On Wed, 2021-06-02 at 09:47 -0700, Dan Williams wrote:
> On Tue, Jun 1, 2021 at 10:31 PM Verma, Vishal L
> <vishal.l.verma@intel.com> wrote:
> > 
> > [switching to the new mailing list]
> > 
> > On Mon, 2021-05-17 at 08:14 +0900, QI Fuli wrote:
> > > From: QI Fuli <qi.fuli@fujitsu.com>
> > > 
> > > This patch set is to rename monitor.conf to ndctl.conf, and make it a
> > > global ndctl configuration file that all ndctl commands can refer to.
> > > 
> > > As this patch set has been pending until now, I would like to know if
> > > current idea works or not. If yes, I will finish the documents and test.
> > > 
> > > Signed-off-by: QI Fuli <qi.fuli@fujitsu.com>
> > 
> > Hi Qi,
> > 
> > Thanks for picking up on this! The approach generally looks good to me,
> > I think we can definitely move forward with this direction.
> > 
> > One thing that stands out is - I don't think we can simply rename the
> > existing monitor.conf. We have to keep supporting the 'legacy'
> > monitor.conf so that we don't break any deployments. I'd suggest
> > keeping the old monitor.conf as is, and continuing to parse it as is,
> > but also adding a new ndctl.conf as you have done.
> > 
> > We can indicate that 'monitor.conf' is legacy, and any new features
> > will only get added to the new global config to encourage migration to
> > the new config. Perhaps we can even provide a helper script to migrate
> > the old config to new - but I think it needs to be a user triggered
> > action.
> > 
> > This is timely as I also need to go add some config related
> > functionality to daxctl, and basing it on this would be perfect, so I'd
> > love to get this series merged in soon.
> 
> I wonder if ndctl should treat /etc/ndctl like a conf.d directory of
> which all files with the .conf suffix are concatenated into one
> combined configuration file. I.e. something that maintains legacy, but
> also allows for config fragments to be deployed individually.

Agreed, this would be the most flexible. ciniparser doesn't seem to
support multiple files directly, but perhaps ndctl can, early on, load
up multiple files/dictionaries, and stash them in ndctl_ctx. Then there
can be accessor functions to retrieve specific conf strings from that
as needed by different commands.

RE: [RFC ndctl PATCH 0/3] Rename monitor.conf to ndctl.conf as a ndctl global config file

[qi.fuli]

> On Wed, 2021-06-02 at 09:47 -0700, Dan Williams wrote:
> > On Tue, Jun 1, 2021 at 10:31 PM Verma, Vishal L
> > <vishal.l.verma@intel.com> wrote:
> > >
> > > [switching to the new mailing list]
> > >
> > > On Mon, 2021-05-17 at 08:14 +0900, QI Fuli wrote:
> > > > From: QI Fuli <qi.fuli@fujitsu.com>
> > > >
> > > > This patch set is to rename monitor.conf to ndctl.conf, and make
> > > > it a global ndctl configuration file that all ndctl commands can refer to.
> > > >
> > > > As this patch set has been pending until now, I would like to know
> > > > if current idea works or not. If yes, I will finish the documents and test.
> > > >
> > > > Signed-off-by: QI Fuli <qi.fuli@fujitsu.com>
> > >
> > > Hi Qi,
> > >
> > > Thanks for picking up on this! The approach generally looks good to
> > > me, I think we can definitely move forward with this direction.
> > >
> > > One thing that stands out is - I don't think we can simply rename
> > > the existing monitor.conf. We have to keep supporting the 'legacy'
> > > monitor.conf so that we don't break any deployments. I'd suggest
> > > keeping the old monitor.conf as is, and continuing to parse it as
> > > is, but also adding a new ndctl.conf as you have done.
> > >
> > > We can indicate that 'monitor.conf' is legacy, and any new features
> > > will only get added to the new global config to encourage migration
> > > to the new config. Perhaps we can even provide a helper script to
> > > migrate the old config to new - but I think it needs to be a user
> > > triggered action.
> > >
> > > This is timely as I also need to go add some config related
> > > functionality to daxctl, and basing it on this would be perfect, so
> > > I'd love to get this series merged in soon.
> >
> > I wonder if ndctl should treat /etc/ndctl like a conf.d directory of
> > which all files with the .conf suffix are concatenated into one
> > combined configuration file. I.e. something that maintains legacy, but
> > also allows for config fragments to be deployed individually.
> 
> Agreed, this would be the most flexible. ciniparser doesn't seem to support
> multiple files directly, but perhaps ndctl can, early on, load up multiple
> files/dictionaries, and stash them in ndctl_ctx. Then there can be accessor
> functions to retrieve specific conf strings from that as needed by different
> commands.

Thank you very much for the comments.

I also agree, and I am working on the v2 patch set now.
I found that the style of ndctl.conf is different from monitor.conf, since there is no section name in monitor.conf.
Do I need to keep the legacy style in monitor.conf, i.e. Can I add the section name [monitor] to monitor.conf?

Best regards,
QI

Re: [RFC ndctl PATCH 0/3] Rename monitor.conf to ndctl.conf as a ndctl global config file

[Verma, Vishal L]

On Thu, 2021-06-17 at 00:25 +0000, qi.fuli@fujitsu.com wrote:
> > On Wed, 2021-06-02 at 09:47 -0700, Dan Williams wrote:
> > > On Tue, Jun 1, 2021 at 10:31 PM Verma, Vishal L
> > > <vishal.l.verma@intel.com> wrote:
> > > > 
> > > > [switching to the new mailing list]
> > > > 
> > > > On Mon, 2021-05-17 at 08:14 +0900, QI Fuli wrote:
> > > > > From: QI Fuli <qi.fuli@fujitsu.com>
> > > > > 
> > > > > This patch set is to rename monitor.conf to ndctl.conf, and make
> > > > > it a global ndctl configuration file that all ndctl commands can refer to.
> > > > > 
> > > > > As this patch set has been pending until now, I would like to know
> > > > > if current idea works or not. If yes, I will finish the documents and test.
> > > > > 
> > > > > Signed-off-by: QI Fuli <qi.fuli@fujitsu.com>
> > > > 
> > > > Hi Qi,
> > > > 
> > > > Thanks for picking up on this! The approach generally looks good to
> > > > me, I think we can definitely move forward with this direction.
> > > > 
> > > > One thing that stands out is - I don't think we can simply rename
> > > > the existing monitor.conf. We have to keep supporting the 'legacy'
> > > > monitor.conf so that we don't break any deployments. I'd suggest
> > > > keeping the old monitor.conf as is, and continuing to parse it as
> > > > is, but also adding a new ndctl.conf as you have done.
> > > > 
> > > > We can indicate that 'monitor.conf' is legacy, and any new features
> > > > will only get added to the new global config to encourage migration
> > > > to the new config. Perhaps we can even provide a helper script to
> > > > migrate the old config to new - but I think it needs to be a user
> > > > triggered action.
> > > > 
> > > > This is timely as I also need to go add some config related
> > > > functionality to daxctl, and basing it on this would be perfect, so
> > > > I'd love to get this series merged in soon.
> > > 
> > > I wonder if ndctl should treat /etc/ndctl like a conf.d directory of
> > > which all files with the .conf suffix are concatenated into one
> > > combined configuration file. I.e. something that maintains legacy, but
> > > also allows for config fragments to be deployed individually.
> > 
> > Agreed, this would be the most flexible. ciniparser doesn't seem to support
> > multiple files directly, but perhaps ndctl can, early on, load up multiple
> > files/dictionaries, and stash them in ndctl_ctx. Then there can be accessor
> > functions to retrieve specific conf strings from that as needed by different
> > commands.
> 
> Thank you very much for the comments.
> 
> I also agree, and I am working on the v2 patch set now.
> I found that the style of ndctl.conf is different from monitor.conf,
> since there is no section name in monitor.conf.
> Do I need to keep the legacy style in monitor.conf, i.e. Can I add
> the section name [monitor] to monitor.conf?

Sorry about the delayed reply, I missed this thread. I think adding it
to the sample monitor.conf is harmless, but I'm not sure that gets us
anything.. The goal is to continue to support any monitor.confs of the
old style that have already been deployed. Those wouldn't have a
[monitor] section header.

I think we'd have to treat the specific file /etc/ndctl/monitor.conf as
a special case, and just parse it the old way. We can make it so that a
new style [monitor] section is incompatible with the old style
monitor.conf (error out and fail if this is detected), and we won't
ever add any new functionality to the old monitor.conf to encourage
people to switch. We can additionally print a warning to stderr if the
old style file is being used.

Re: [RFC ndctl PATCH 3/3] ndctl, rename monitor.conf to ndctl.conf

[Verma, Vishal L]

On Mon, 2021-05-17 at 08:14 +0900, QI Fuli wrote:
> From: QI Fuli <qi.fuli@fujitsu.com>
> 
> Rename monitor.conf to ndctl.conf, and make it a ndclt global
> configuration file that all commands can refer to.
> Refactor monitor to make it work with ndctl.conf.
> 
> Signed-off-by: QI Fuli <qi.fuli@fujitsu.com>
> ---
>  configure.ac                       |   8 +-
>  ndctl/Makefile.am                  |   9 +-
>  ndctl/monitor.c                    | 127 +++++------------------------
>  ndctl/{monitor.conf => ndctl.conf} |  16 +++-
>  4 files changed, 40 insertions(+), 120 deletions(-)
>  rename ndctl/{monitor.conf => ndctl.conf} (82%)
> 

[snip]

> @@ -601,6 +496,19 @@ int cmd_monitor(int argc, const char **argv, struct ndctl_ctx *ctx)
>  		"ndctl monitor [<options>]",
>  		NULL
>  	};
> +	const struct config configs[] = {
> +		CONF_STR("core:bus", &param.bus, NULL),
> +		CONF_STR("core:region", &param.region, NULL),
> +		CONF_STR("core:dimm", &param.dimm, NULL),
> +		CONF_STR("core:namespace", &param.namespace, NULL),
> +		CONF_STR("monitor:bus", &param.bus, NULL),
> +		CONF_STR("monitor:region", &param.region, NULL),
> +		CONF_STR("monitor:dimm", &param.dimm, NULL),
> +		CONF_STR("monitor:namespace", &param.namespace, NULL),
> +		CONF_STR("monitor:dimm-event", &monitor.dimm_event, NULL),
> +		//CONF_FILE("monitor:log", &monitor.log, NULL),
> +		CONF_END(),
> +	};
>  	const char *prefix = "./";
>  	struct util_filter_ctx fctx = { 0 };
>  	struct monitor_filter_arg mfa = { 0 };
> @@ -621,7 +529,10 @@ int cmd_monitor(int argc, const char **argv, struct ndctl_ctx *ctx)
>  	else
>  		monitor.ctx.log_priority = LOG_INFO;
>  
> -	rc = read_config_file(ctx, &monitor, &param);
> +	if (monitor.config_file)
> +		rc = parse_configs(monitor.config_file, configs);
> +	else
> +		rc = parse_configs(NDCTL_CONF_FILE, configs);
>  	if (rc)
>  		goto out;

If I'm reading this right, it looks like this is set up so that params
from the config file will override any params from the command line,
which I think can be surprising behavior. The command line should
always override anything in the config file.

I suspect we'll need to collect CLI params in a 'param_cli' structure,
config params in a 'param_config' structure, and then consolidate them
to create the final 'param' structure that gets passed to
util_filter_walk etc.