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 <i.maxim...@ovn.org> 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 d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev