On Tue, Sep 9, 2025 at 1:39 PM Ilya Maximets <[email protected]> wrote:
> 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. > I think that's reasonable, should I post it as regular patch after those changes or still as RFC? > > Best regards, Ilya Maximets. > > Thanks, Ales _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
