On Sat, Dec 16, 2023 at 12:42:01AM +0100, Ilya Maximets wrote: > On 12/4/23 12:23, Felix Huettner via dev wrote: > > This exposes the old constants regarding min backoff, max backoff and > > probe interval using environment variables. In case previously users > > wanted to tune the probe interval for all connections this required > > setting this setting in multiple locations. E.g. to configure the probe > > interval from a relay to the ovsdb server you need to call an appctl > > command after the relay has started up. > > The new environment variables make it easy to set them for all new > > connections. The existing configuration options for these settings stay > > in place and take precedence over the environment variables. > > In case the environment variables are not specified/invalid formatted we > > default to the previous defaults. > > > > Signed-off-by: Felix Huettner <felix.huettner@mail.schwarz> > > --- > > v3->v4: fix conflict in NEWS file > > v2->v3: add minimal values and defaults as defines > > v1->v2: fixed wrong function definitions > > > > NEWS | 7 +++++ > > lib/jsonrpc.c | 4 +-- > > lib/reconnect.c | 70 +++++++++++++++++++++++++++++++++++++---- > > lib/reconnect.h | 7 ++--- > > ovsdb/jsonrpc-server.c | 4 +-- > > ovsdb/ovsdb-server.c | 2 +- > > ovsdb/relay.h | 2 -- > > python/ovs/reconnect.py | 45 ++++++++++++++++++++++++-- > > 8 files changed, 121 insertions(+), 20 deletions(-) > > > > diff --git a/NEWS b/NEWS > > index 490e275da..6b580edf3 100644 > > --- a/NEWS > > +++ b/NEWS > > @@ -19,6 +19,13 @@ Post-v3.2.0 > > * Added support for Generic Segmentation Offloading for the cases > > where > > TSO is enabled but not supported by an egress interface (except for > > tunnel interfaces). > > + - The environment variable OVS_RECONNECT_MIN_BACKOFF, > > + OVS_RECONNECT_MAX_BACKOFF and OVS_RECONNECT_PROBE_INTERVAL, if set, > > allow > > + users to tune the default reconnect configuration. E.g. adapting > > + OVS_RECONNECT_PROBE_INTERVAL modifies the default probe interval, for > > all > > + servers and clients. These variables are valid for all ovs code as > > well as > > + the python client. Values set for individual connections (e.g. using > > the > > + `inactivity_probe` column in the `Manager` tables) will take > > precedence. > > Hi, Felix. Thanks for the patch! Though I'm not sure if introduction > of yet another way of setting the same thing is a good approach for > this problem. If anything, we should probably try to reduce the number > of configuration methods. I tried to come up with a way to solve the > configuration for ovsdb-server for a long time now, and the config file > approach that I've been working for a past few months seems promising: > > https://patchwork.ozlabs.org/project/openvswitch/patch/20231214010431.1664005-21-i.maximets%40ovn.org/ > At least it should reduce the configuration method variety while the > config file is in use. And it should solve the problem of configuring > relays that you mentioned in the commit message. >
Hi Ilya, thanks for the feedback. I honestly like your approach significantly more since it fixes a bunch of other issues as well. > Having a configurable global default may also be confusing for the end > users. Primarily because it doesn't actually apply to everything. > For example, active-backup replication has its own default probe interval > that will not be affected, and RAFT is calculating probe interval based > on election timer. So, it may be not clear why the variable is set, > nothing is explicitly configured by the user, but some connections are > not using the value. > Another potential source of confusion is that OpenFlow connections use > the same terminology (probe_interval), but a different implementation > that is not going to be affected by this change as well. > We noticed that as well. I guess the config file allows for more expansions later on more easily (e.g. to specify a target RAFT election timer and then let ovsdb change it in multiple steps as needed). > Is there a scenario that we still need to cover that will not be covered > by the configuration file? If there are some, we could try to come > up with solutions for these. I know of a few, but these are just bugs > that should be fixed and this patch will not help with them anyway. > The only downside i see there, is that it will not allow us to configure the northd and ovn-controller settings as well (which the environment variables would allow). Maybe for their future they can also get config files instead of relying on values set in NB_Global or the local ovsdbs. However they are currently not our main pain point, so i don't see them as too critical. So i guess we can then ignore this patch. Thanks Felix > Best regards, Ilya Maximets. Diese E Mail enthält möglicherweise vertrauliche Inhalte und ist nur für die Verwertung durch den vorgesehenen Empfänger bestimmt. Sollten Sie nicht der vorgesehene Empfänger sein, setzen Sie den Absender bitte unverzüglich in Kenntnis und löschen diese E Mail. Hinweise zum Datenschutz finden Sie hier<https://www.datenschutz.schwarz>. This e-mail may contain confidential content and is intended only for the specified recipient/s. If you are not the intended recipient, please inform the sender immediately and delete this e-mail. Information on data protection can be found here<https://www.datenschutz.schwarz>. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
