Re: [ovs-dev] [PATCH 20/22] ovsdb-server: Allow user-provided config files.
On Mon, Jan 8, 2024 at 8:17 AM Ilya Maximets wrote: > > On 1/5/24 21:26, Terry Wilson wrote: > > On Wed, Dec 13, 2023 at 7:05 PM Ilya Maximets wrote: > > > >> -/* Clears and replaces 'remotes' and 'dbnames' by a configuration read > >> from > >> - * 'config_file', which must have been previously written by > >> save_config(). */ > >> -static void > >> +/* Clears and replaces 'remotes' and 'db_conf' by a configuration read > >> from > >> + * 'config_file', which must have been previously written by save_config() > >> + * or provided by the user with --config-file. > >> + * Returns 'true', if parsing was successful, 'false' otherwise. */ > >> +static bool > >> load_config(FILE *config_file, struct shash *remotes, > >> struct shash *db_conf, char **sync_from, > >> char **sync_exclude, bool *is_backup) > >> @@ -2890,17 +3117,34 @@ load_config(FILE *config_file, struct shash > >> *remotes, > >> struct json *json; > > > > I'm wondering if having an argument for everything we parse out of a > > config file might get a little unwieldy down the road. Say we add > > configuration of SSL stuff, etc. Maybe it's something we could modify > > as it becomes an issue, but it might be nice to have something for > > config options that is similar to what we have for registering unixctl > > commands or struct ctl_command_syntax. Documentation could be added as > > part of the registration/definition of the config option, there could > > be a .get() that parses the values out of the json, and a .run() that > > gets called after all of the parsing is shown to succeed. > > > > Terry > > > > Hi, Terry. Yes, I agree that the current interface is far from being > ideal, and I actually tried to rework it multiple times while working > on this patch set. That's one of the reasons it took so long. :) > Unfortunately it ended up with a huge amount of refactoring every time. > The main reason for that, I think, is the fact that the same function > is used for loading the legacy internal configuration file and the new > user-provided file. And the code around legacy internal configuration > is not easy to adapt. > > I think, it would be much easier to do this kind of refactoring once > we deprecate and remove all the appctl commands that can change the > server configuration. For now I decided to keep the interface as it is > without this patch set to avoid making the patch set even bigger. > I hope that makes sense. > > The idea of registering the options with a common parsing logic sounds > interesting though and definitely worth exploring in the future. > > Best regards, Ilya Maximets. Yeah, I'm all for merging as-is and prettying up as needed. In case you have entirely too much free time on your hands (ha!), here's an ini-style config loading/reloading framework I wrote many many years ago (Warning: I'd only been programming professionally for ~3 years or so at that point). Asterisk was very modular and had over 100 different config files, and many of them used ini files in completely different ways so it's definitely more generic and overkill for what we are needing, but there may be some useful ideas [1-5] if you ignore all of the macro/varargs magic. ;) The general idea was a per-module `aco_info` structure that defined the information that was obtained from configs, `aco_type` structures that defined what parts of the config file matched up to what fields of the `aco_info` struc, and aco_option_register() which would define the specific options set under each [category], how they were matched, what fields in a struct they set, etc. aco_option_register_custom() just used custom callback functions to handle setting things up. aco_process_config() would be called on load/reload and would parse the config file and create an internal 'pending' representation of the user-defined aco_info and then if parsing succeeded would then atomically swap the pending and live aco_info pointers. There were also pre_apply and post_apply callbacks so modules could do special things if needed during reloads, etc. Prior to the patch modules would often just modify their internal structures as the config file was being processed, so if there was a parse error things would just be left in a state where configs were partially applied. Anyway, it may or may not be interesting and certainly isn't something we need to worry about right now. Just food for thought. Terry [1] https://github.com/asterisk/asterisk/blob/master/include/asterisk/config_options.h [2] https://github.com/asterisk/asterisk/blob/master/main/config_options.c [3] https://github.com/asterisk/asterisk/blob/master/configs/samples/app_skel.conf.sample [4] https://github.com/asterisk/asterisk/blob/master/apps/app_skel.c [5] https://github.com/asterisk/asterisk/commit/d54717c39e62f4cc8b290ac4836c4d4469d87c24 ___ dev mailing list d...@openvswitch.org
Re: [ovs-dev] [PATCH 20/22] ovsdb-server: Allow user-provided config files.
On 1/5/24 21:26, Terry Wilson wrote: > On Wed, Dec 13, 2023 at 7:05 PM Ilya Maximets wrote: > >> -/* Clears and replaces 'remotes' and 'dbnames' by a configuration read from >> - * 'config_file', which must have been previously written by save_config(). >> */ >> -static void >> +/* Clears and replaces 'remotes' and 'db_conf' by a configuration read from >> + * 'config_file', which must have been previously written by save_config() >> + * or provided by the user with --config-file. >> + * Returns 'true', if parsing was successful, 'false' otherwise. */ >> +static bool >> load_config(FILE *config_file, struct shash *remotes, >> struct shash *db_conf, char **sync_from, >> char **sync_exclude, bool *is_backup) >> @@ -2890,17 +3117,34 @@ load_config(FILE *config_file, struct shash *remotes, >> struct json *json; > > I'm wondering if having an argument for everything we parse out of a > config file might get a little unwieldy down the road. Say we add > configuration of SSL stuff, etc. Maybe it's something we could modify > as it becomes an issue, but it might be nice to have something for > config options that is similar to what we have for registering unixctl > commands or struct ctl_command_syntax. Documentation could be added as > part of the registration/definition of the config option, there could > be a .get() that parses the values out of the json, and a .run() that > gets called after all of the parsing is shown to succeed. > > Terry > Hi, Terry. Yes, I agree that the current interface is far from being ideal, and I actually tried to rework it multiple times while working on this patch set. That's one of the reasons it took so long. :) Unfortunately it ended up with a huge amount of refactoring every time. The main reason for that, I think, is the fact that the same function is used for loading the legacy internal configuration file and the new user-provided file. And the code around legacy internal configuration is not easy to adapt. I think, it would be much easier to do this kind of refactoring once we deprecate and remove all the appctl commands that can change the server configuration. For now I decided to keep the interface as it is without this patch set to avoid making the patch set even bigger. I hope that makes sense. The idea of registering the options with a common parsing logic sounds interesting though and definitely worth exploring in the future. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 20/22] ovsdb-server: Allow user-provided config files.
On Wed, Dec 13, 2023 at 7:05 PM Ilya Maximets wrote: > -/* Clears and replaces 'remotes' and 'dbnames' by a configuration read from > - * 'config_file', which must have been previously written by save_config(). > */ > -static void > +/* Clears and replaces 'remotes' and 'db_conf' by a configuration read from > + * 'config_file', which must have been previously written by save_config() > + * or provided by the user with --config-file. > + * Returns 'true', if parsing was successful, 'false' otherwise. */ > +static bool > load_config(FILE *config_file, struct shash *remotes, > struct shash *db_conf, char **sync_from, > char **sync_exclude, bool *is_backup) > @@ -2890,17 +3117,34 @@ load_config(FILE *config_file, struct shash *remotes, > struct json *json; I'm wondering if having an argument for everything we parse out of a config file might get a little unwieldy down the road. Say we add configuration of SSL stuff, etc. Maybe it's something we could modify as it becomes an issue, but it might be nice to have something for config options that is similar to what we have for registering unixctl commands or struct ctl_command_syntax. Documentation could be added as part of the registration/definition of the config option, there could be a .get() that parses the values out of the json, and a .run() that gets called after all of the parsing is shown to succeed. Terry ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 20/22] ovsdb-server: Allow user-provided config files.
On Thu, Dec 14, 2023 at 2:05 AM Ilya Maximets wrote:
>
> OVSDB server maintains a temporary file with the current database
> configuration for the case it is restarted by a monitor process
> after a crash. On startup the configuration from command line
> arguments is stored there in a JSON format, also whenever user
> changes the configuration with different UnixCtl commands, those
> changes are getting added to the file. When restarted from the
> crash it reads the configuration from the file and continues
> with all the necessary remotes and databases.
>
> This change allows it to be an external user-provided file that
> OVSDB server will read the configuration from. The file can be
> specified with a --config-file command line argument and it is
> mutually exclusive with most other command line arguments that
> set up remotes or databases, it is also mutually exclusive with
> use of appctl commands that modify same configurations, e.g.
> add/remove-db or add/remove-remote.
>
> If the user wants to change the configuration of a running server,
> they may change the file and call ovsdb-server/reload appctl.
> OVSDB server will open a file, read and parse it, compare the
> new configuration with the current one and adjust the running
> configuration as needed. OVSDB server will try to keep existing
> databases and connections intact, if the change can be applied
> without disrupting the normal operation.
>
> User-provided files are not trustworthy, so extra checks were
> added to ensure a correct file format. If the file cannot be
> correctly parsed, e.g. contains invalid JSON, no changes will
> be applied and the server will keep using the previous
> configuration until the next reload.
>
> If config-file is provided for active-backup databases, permanent
> disconnection of one of the backup databases no longer leads to
> switching all other databases to 'active'. Only the disconnected
> one will transition, since all of them have their own records in
> the configuration file.
>
> With this change, users can run all types of databases within
> the same ovsdb-server process at the same time.
>
> Simple configuration may look like this:
>
> {
> "remotes": {
> "punix:db.sock": {},
> "pssl:6641": {
> "inactivity-probe": 16000,
> "read-only": false,
> "role": "ovn-controller"
> }
> },
> "databases": {
> "conf.db": {},
> "sb.db": {
> "service-model": "clustered"
As noted earlier in the series, I think we should omit 'standalone'
and 'clustered' in the vocabulary so it is clear to the user that this
is determined from the on-disk database file and avoid duplication of
effort in keeping the two in sync.
--
Frode Nordahl
> },
> "OVN_Northbound": {
> "service-model": "relay",
> "source": {
> "ssl:[fe:::1]:6642,ssl:[fe:::2]:6642": {
> "max-backoff": 8000,
> "inactivity-probe": 1
> }
> }
> }
> }
> }
>
> Signed-off-by: Ilya Maximets
> ---
> Documentation/ref/ovsdb.7.rst| 86 +-
> Documentation/topics/ovsdb-relay.rst | 19 ++
> NEWS | 4 +
> ovsdb/ovsdb-server.1.in | 96 ++-
> ovsdb/ovsdb-server.c | 384 ++-
> 5 files changed, 513 insertions(+), 76 deletions(-)
>
> diff --git a/Documentation/ref/ovsdb.7.rst b/Documentation/ref/ovsdb.7.rst
> index 84b153d24..42e5f4089 100644
> --- a/Documentation/ref/ovsdb.7.rst
> +++ b/Documentation/ref/ovsdb.7.rst
> @@ -155,6 +155,22 @@ standalone database, configure the server to listen on a
> "connection method"
> that the client can reach, then point the client to that connection method.
> See `Connection Methods`_ below for information about connection methods.
>
> +Open vSwitch 3.3 introduced support for configuration files via
> +``--config-file`` command line option. The configuration file for a server
> +with a **standalone** database may look like this::
> +
> + {
> + "remotes": { "": {} },
> + "databases": {
> + "": {
> + "service-model": "standalone"
> + }
> + }
> + }
> +
> +The ``"service-model"`` key can be omitted. In this case ``ovsdb-server``
> +will infer the service model from the database file itself.
> +
> Active-Backup Database Service Model
>
>
> @@ -177,10 +193,36 @@ database file from the active server. Then use
> connects to the active server. At that point, the backup server will fetch a
> copy of the active database and keep it up-to-date until it is killed.
>
> +Open vSwitch 3.3 introduced support for configuration files via
> +``--config-file`` command line option. The configuration file for a backup
> +server in this case may look like this::
> +
> + {
> + "remotes": { "": {} },
> +
[ovs-dev] [PATCH 20/22] ovsdb-server: Allow user-provided config files.
OVSDB server maintains a temporary file with the current database
configuration for the case it is restarted by a monitor process
after a crash. On startup the configuration from command line
arguments is stored there in a JSON format, also whenever user
changes the configuration with different UnixCtl commands, those
changes are getting added to the file. When restarted from the
crash it reads the configuration from the file and continues
with all the necessary remotes and databases.
This change allows it to be an external user-provided file that
OVSDB server will read the configuration from. The file can be
specified with a --config-file command line argument and it is
mutually exclusive with most other command line arguments that
set up remotes or databases, it is also mutually exclusive with
use of appctl commands that modify same configurations, e.g.
add/remove-db or add/remove-remote.
If the user wants to change the configuration of a running server,
they may change the file and call ovsdb-server/reload appctl.
OVSDB server will open a file, read and parse it, compare the
new configuration with the current one and adjust the running
configuration as needed. OVSDB server will try to keep existing
databases and connections intact, if the change can be applied
without disrupting the normal operation.
User-provided files are not trustworthy, so extra checks were
added to ensure a correct file format. If the file cannot be
correctly parsed, e.g. contains invalid JSON, no changes will
be applied and the server will keep using the previous
configuration until the next reload.
If config-file is provided for active-backup databases, permanent
disconnection of one of the backup databases no longer leads to
switching all other databases to 'active'. Only the disconnected
one will transition, since all of them have their own records in
the configuration file.
With this change, users can run all types of databases within
the same ovsdb-server process at the same time.
Simple configuration may look like this:
{
"remotes": {
"punix:db.sock": {},
"pssl:6641": {
"inactivity-probe": 16000,
"read-only": false,
"role": "ovn-controller"
}
},
"databases": {
"conf.db": {},
"sb.db": {
"service-model": "clustered"
},
"OVN_Northbound": {
"service-model": "relay",
"source": {
"ssl:[fe:::1]:6642,ssl:[fe:::2]:6642": {
"max-backoff": 8000,
"inactivity-probe": 1
}
}
}
}
}
Signed-off-by: Ilya Maximets
---
Documentation/ref/ovsdb.7.rst| 86 +-
Documentation/topics/ovsdb-relay.rst | 19 ++
NEWS | 4 +
ovsdb/ovsdb-server.1.in | 96 ++-
ovsdb/ovsdb-server.c | 384 ++-
5 files changed, 513 insertions(+), 76 deletions(-)
diff --git a/Documentation/ref/ovsdb.7.rst b/Documentation/ref/ovsdb.7.rst
index 84b153d24..42e5f4089 100644
--- a/Documentation/ref/ovsdb.7.rst
+++ b/Documentation/ref/ovsdb.7.rst
@@ -155,6 +155,22 @@ standalone database, configure the server to listen on a
"connection method"
that the client can reach, then point the client to that connection method.
See `Connection Methods`_ below for information about connection methods.
+Open vSwitch 3.3 introduced support for configuration files via
+``--config-file`` command line option. The configuration file for a server
+with a **standalone** database may look like this::
+
+ {
+ "remotes": { "": {} },
+ "databases": {
+ "": {
+ "service-model": "standalone"
+ }
+ }
+ }
+
+The ``"service-model"`` key can be omitted. In this case ``ovsdb-server``
+will infer the service model from the database file itself.
+
Active-Backup Database Service Model
@@ -177,10 +193,36 @@ database file from the active server. Then use
connects to the active server. At that point, the backup server will fetch a
copy of the active database and keep it up-to-date until it is killed.
+Open vSwitch 3.3 introduced support for configuration files via
+``--config-file`` command line option. The configuration file for a backup
+server in this case may look like this::
+
+ {
+ "remotes": { "": {} },
+ "databases": {
+ "": {
+ "service-model": "active-backup",
+ "backup": true,
+ "source": {
+ "": {
+ "inactivity-probe": ,
+ "max-backoff":
+ }
+ }
+ }
+ }
+ }
+
+All the fields in the ``""`` description above are required.
+Options for the ``""`` connection method (``"inactivity-probe"``, etc.)
+can be omitted.
+
When the active server in an active-backup server pair fails, an administrator
