On 12/14/23 08:12, Frode Nordahl wrote:
> On Thu, Dec 14, 2023 at 2:04 AM Ilya Maximets <[email protected]> wrote:
>>
>> The original problem was summarized on the OVS+OVN Conf'22 last year:
>> https://www.openvswitch.org/support/ovscon2022/#t19
>> Slides:
>> https://www.openvswitch.org/support/ovscon2022/slides/ovsdb-a-database.pdf
>>
>> In short, there are way to many ways to configure remotes and databases
>> but not a single one of them can cover all the situations. And there
>> are cases like max-backoff on active-backup connection that are not
>> configurable at all.
>>
>> The proposed solution for this problem was to unify configurations
>> within a new Local_Config database that will be local to a server
>> and will have support for all we need in the schema.
>>
>> However, there are few issues with this solution:
>>
>> 1. It doesn't reduce the number of ways things can be configured,
>> but increases, because we'll need to add things that are covered
>> by other methods to the database, while those other methods
>> are still in use. And it is actually required to use the
>> command line or appctl in order to use the values stored in the
>> database, i.e. --remote=db:db:table:row.
>>
>> 2. Not particularly user-friendly. Database files are user-readable
>> -ish, but not writable, i.e. require special tools in order to
>> make changes and in most cases require a running ovsdb-server
>> in order to make changes.
>>
>> 3. We need a way to restrict write access for external users,
>> but allow local administrators to change the configuration.
>> Hence, potentially complex access management and potential
>> security issues.
>>
>> Taking all of that into account and spending another year on a
>> solution :) , I believe, introduction of a configuration file can
>> make things easier. This patch set adds support for a new command
>> line argument --config-file that is mutually exclusive with many
>> other command line arguments and appctl commands that can configure
>> the same thing.
>
> Thank you for your work on this! The existing configuration model of
> the OVSDB server is indeed on the list of black magic arts for anyone
> using it for the first time.
>
> I like the way you think wrt. making the new way mutually exclusive
> with the old ways to reduce the end user surface area on these knobs.
>
>> A few key points:
>>
>> 1. Configuration file is a plain text JSON file, so it can be edited
>> with just a text editor without need for any extra tools.
>> (JSON because we have a decent JOSN parser and there is no need
>> for extra dependencies. And no other format is actually superior,
>> all of them have their own issues.)
>>
>> 2. Configuration file is a replacement for many command line options
>> and appctls, because it is mutually exclusive with them.
>> If --config-file is set, configuration that can be done via
>> configuration file can only be done via configuration file.
>> (It still allows to point to the database for remotes, but there
>> is no actual need for this, except in very rare cases, and it is
>> visible from the file that it points to the database.)
>>
>> 3. If configuration change is needed in runtime: change the file
>> and call ovsdb-server/reload. Ihar also suggested to use SIGHUP
>> for this, can be added in the future.
>
> +1
>
>> SSL configuration is not moved to a config-file in this patch set,
>> but can be done as a follow up.
>>
>>
>> Structure of this patch set:
>>
>> - Patches 1-5 are general fixes and enhancements that are not
>> really related to the change, but needed/useful
>> one way or another.
>>
>> - There are only 4 actually large patches that are not tests.
>> And they are mainly refactoring of the existing code without
>> externally visible changes. This is because we have to
>> isolate all the databases from each other so they do not
>> share configuration. But at the same time we have to preserve
>> all the current interfaces for backward compatibility and
>> that takes a lot of code.
>>
>> - Most other patches in the set a small simple changes.
>>
>> More description and examples are in individual patches and the
>> documentation.
>>
>> TL;DR; A small example of a config file from the patch #20:
>>
>> {
>> "remotes": {
>> "punix:db.sock": {},
>> "pssl:6641": {
>> "inactivity-probe": 16000,
>> "read-only": false,
>> "role": "ovn-controller"
>> }
>> },
>> "databases": {
>> "conf.db": {},
>> "sb.db": {
>> "service-model": "clustered"
>
> I'm a bit worried about this item in the configuration file:
Hi. Thanks for your input!
I thought about this as well and let me try to describe my thinking.
I'll also try to answer to the very similar questions from the other
patches here, so we do not have 3 separate threads. :)
> 1) The database file itself is the authoritative source of truth for
> the service model, and the configured value can get out of sync with
> the on-disk reality. Throughout the lifetime of a database, the
> operator may need to momentarily change the service model. Would this
> not just add to the steps required to do this?
It may, but there are few things here:
1. cluster-to/from-standalone conversion doesn't happen in-place.
IIRC, ovsdb-tool requires the source and the result to be different
files on disk. So, the user will need to change the database name
at least while running on a new database, or replace the file, but
that sounds like an already not a very smooth process.
2. In my experience such conversions usually happen on a different
machine after extracting the database file from a production
environment for debugging or something like that. I'm not sure
how likely it is that a user will want to routinely convert
databases back and forth so the extra s/standalone/clustered/
will be a problem. Please, let me know if you have a different
experience or a specific use case where this will be necessary.
The only case I can think of is a disaster recovery when you
convert a broken cluster to standalone and then back, but that
doesn't require running an ovsdb-server process with the intermediate
version of a file.
3. "service-model" object is optional for standalone and clustered
databases, so users may choose to not specify it and avoid the
problem.
> 2) Having this in the configuration file may give the impression that
> OVSDB will manage the on-disk DB files, while the reality is that it
> will not. Is this a potential source of confusion?
I see that point. I tried to document this behavior, but I agree that
expecting people to read the documentation is a futile exercise.
I hope it is clear though that ovsdb-server doesn't create any of these
databases. But there might be an impression that ovsdb-server will
convert one to another. Configuration file doesn't contain enough
information to create a clustered database, that should be clear-ish (?).
But no extra information is needed in order to go from clustered to
standalone. So, that might be the main point.
I'm not sure if it's a big problem though, because ovsdb-server will
fail to start or reload in case the specified model doesn't match the
actual one. So, the users should figure that out after one failure,
I hope. It's spelled out and the process failing to start or reload
should be a loud enough sign of something going wrong.
Question from patch 12/22:
> I see the reason for having the `service-model` config key for 'relay'
> and 'active-backup' models, as they cannot be determined by other
> means. However, the authoritative source of truth for the 'standalone'
> and 'clustered' models is the database file itself.
>
> Why not omit those service models from the configuration file
> vocabulary, this information can get out of sync with reality and be a
> source of confusion and extra manual steps?
Mismatch is a noticeable hard failure, so I hope that even if there
will be some confusion, it will be dissolved quickly.
Remark from patch 20/22:
> 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.
In the code the whole checking part is a single if statement in the
open_db:
if (conf->model != SM_UNDEFINED && conf->model != model) {
ovsdb_storage_close(storage);
return ovsdb_error(NULL, "%s: database is %s and not %s",
filename, service_model_to_string(model),
service_model_to_string(conf->model));
}
So, it's not hard to track in the code. I think, the main reason
I added the ability to specify the service model explicitly is for
the users to be able to harden the configuration of their setups
in case they want to. I think it may be important for an admin to
assert the database type just for the sake of being sure that the
system works exactly as it supposed to.
One potential middle ground I guess is to not advertise this as a
primary method of configuration. For example, we may say that a
way to add a standalone or clustered database is to not specify
anything, i.e. "databases": { "file.db": {} }. And then make a
remark that if users want ovsdb-server to check that the provided
database file corresponds to a particular service-model, then they
can add an option and ovsdb-server will fail in case of a model
mismatch.
Will that be a better solution? What do you think?
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
