On 9/9/25 1:14 PM, Eelco Chaudron via dev wrote: > > > On 9 Sep 2025, at 12:04, Ales Musil wrote: > >> On Tue, Sep 9, 2025 at 11:20 AM Dumitru Ceara <[email protected]> wrote: >> >>> On 9/9/25 11:11 AM, Eelco Chaudron wrote: >>>> >>>> >>>> On 9 Sep 2025, at 10:45, Eelco Chaudron wrote: >>>> >>>>> On 5 Sep 2025, at 11:07, Ales Musil via dev wrote: >>>>> >>>>>> Add a way to set IDL txn as read only, that is useful in cases when >>>>>> we want to ensure that the program doesn't write anything into the >>>>>> txn. This is done via assert because this is considered a bug which >>>>>> should be fixed in the IDL caller. >>>>>> >>>>>> Signed-off-by: Ales Musil <[email protected]> >>>>>> --- >>>>>> More context on why, this would be used by OVN to ensure >>>>>> that certain I-P nodes do not write into the database when >>>>>> they shouldn't >>>>> >>> >>>>> Hi Ales, >>>>> >>>>> I haven’t done a full code review yet, but I do want to raise an >>>>> objection regarding the assert() approach used here. >>>>> >>>>> To me, this doesn’t make much sense, why should the whole dbserver >>>>> crash just because a client is misbehaving? It feels like this >>>>> should be handled more gracefully, maybe with an error return or >>>>> logging instead of taking down the entire process. >>>>> >>>>> What do you think? >>>> >>>> Someone pointed out to me that this is only for the client side, >>>> which I guess might be fine to crash in some cases (is it?). >>>> >>>> That said, it still feels odd that ovsdb-client would crash rather >>>> than return a proper error. From a user perspective, I’d expect it >>>> to fail gracefully with an error message instead of aborting. >>>> >>> >>> Hi Eelco, >>> >>> There are quite a few situations in which the ovsdb-idl library crashes >>> the client (assert()) when the IDL API is used incorrectly. Some examples: >>> >>> - ovsdb_idl_index_destroy_row() called on a non-index row >>> - ovsdb_idl_txn_write__() called by all <dbrec>_set_<column>() helpers >>> if the column we try to write to is synthetic or we're trying to write >>> to columns that are not monitored e.g. >>> - ovsdb_idl_txn_insert_persist_uuid() called without explicitly >>> providing a UUID. >>> >>> In my opinion that's fine. >>> >>> Regards, >>> Dumitru >>> >>>> //Eelco >>> >>> >> Hi Eelco and Dumitru, >> >> yes there are cases when we crash the client. Also this is why it's an RFE, >> for our use case it is completely fine to crash. We only expect to detect >> the erronouns behavior once. If the API would return some error instead >> we would assert on that error anyway in OVN resulting in a crash. Which >> again for this use case is desired. >> >> Also there is a slight issue how to handle gracefully some places where >> the assertion is? There isn't any way to propagate that error without >> significant changes to the idl layer itself which probably isn't desirable. >> >> I'm open to suggestions, at the same time I'm trying to find a working >> solution that can be used in OVN. > > +Mike and Ilya, who have more experience in the OVSDB area.
FWIW, I suggested to use assertions for this functionality in earlier discussion with Ales off-list. The idea of this change is to catch coding errors in OVN if they ever happen. Incremental processing nodes that are not supposed to write into database must never attempt to do so, and so the assert is justified. It will show the place where the failure happened and the stack trace, so the code in OVN can be fixed. May be better to re-name the function and the patch, e.g. s/set/assert/ to make it more clear for the users that the code will assert. May also rename the structure field to avoid false impression that it is something that can be used casually. The comment for the function should mention that building with NDEBUG will turn this check into no-op. Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
