On 12/14/23 13:40, Ilya Maximets wrote:
> 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 agree with Frode on this, it seems a bit confusing.
>
> 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?
>
To me this sounds like a reasonable alternative.
Overall the series looks in good shape to me. I didn't do a thorough
review yet though, I only did a first read of the patches. However, I
didn't spot any major issue.
I do plan to spend more time in the near future, next week but likely
also in the first part of January, reviewing this patch set more closely.
I hope we get this feature in v3.3.0!
Regards,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
