On 4/3/26 6:24 PM, Lorenzo Bianconi wrote: > On Apr 01, Ilya Maximets wrote: >> On 3/12/26 4:01 PM, Lorenzo Bianconi wrote: >>> Store the user monitor condition request in pre-json format in >>> ovsdb_idl_table in order to use it in ovsdb_idl_compose_monitor_request >>> routine and create a monitor condition request based on the >>> tables/columns available in db-schema. >>> >>> Reported-at: https://issues.redhat.com/browse/FDP-3114 >>> Signed-off-by: Lorenzo Bianconi <[email protected]> >>> --- >>> lib/ovsdb-cs.c | 4 +++ >>> lib/ovsdb-idl-provider.h | 3 ++ >>> lib/ovsdb-idl.c | 64 ++++++++++++++++++++++++++++++++++++++-- >>> 3 files changed, 68 insertions(+), 3 deletions(-) >>> >> >> Hi, Lorenzo. Thanks for working on this. This patch seems incomplete >> though, as it only avoids setting the conditions for missing tables during >> the initial monitor request. On the very next iteration, ovn-controller >> will call the set_conditions again and those will be passed directly to >> the ovsdb_cs_set_condition() and that cond_change request will fail. >> >> You can see that by applying your OVN RFC patch and then run: >> >> make -j8 sandbox SANDBOXFLAGS="--no-ovn-rbac --n-ic 0" >> [tutorial]$ cd sandbox/ >> [sandbox]$ pkill -f OVN_IC_ >> [sandbox]$ pkill -f OVN_Southbound >> [sandbox]$ pkill -f ovn-controller >> [sandbox]$ rm sb* >> [sandbox]$ ovsdb-tool create sb1.db ../ovn-sb-degraded.ovsschema >> [sandbox]$ ovsdb-server --detach --no-chdir --pidfile=sb1.pid \ >> -vconsole:off --log-file=sb1.log -vsyslog:off \ >> --remote=db:OVN_Southbound,SB_Global,connections \ >> --private-key=db:OVN_Southbound,SSL,private_key \ >> --certificate=db:OVN_Southbound,SSL,certificate \ >> --ca-cert=db:OVN_Southbound,SSL,ca_cert \ >> --ssl-protocols=db:OVN_Southbound,SSL,ssl_protocols \ >> --ssl-ciphers=db:OVN_Southbound,SSL,ssl_ciphers \ >> --ssl-ciphersuites=db:OVN_Southbound,SSL,ssl_ciphersuites \ >> --unixctl=sb1 --remote=punix:sb1.ovsdb sb1.db >> [sandbox]$ ovn-sbctl set-ssl $(pwd)/ovnsb-privkey.pem \ >> $(pwd)/ovnsb-cert.pem $(pwd)/pki/switchca/cacert.pem >> [sandbox]$ ovn-sbctl set-connection pssl:6642 >> [i.maximets@im-t490s sandbox]$ ovn-controller -p >> $(pwd)/chassis-1-privkey.pem \ >> -c $(pwd)/chassis-1-cert.pem -C $(pwd)/pki/switchca/cacert.pem \ >> --detach --no-chdir -vsyslog:off --log-file=ovn-controller.log \ >> --pidfile=ovn-controller.pid -vconsole:off -vjsonrpc:file:dbg >> [sandbox]$ less ovn-controller.log >> >> Where the ovn-sb-degraded.ovsschema is a schema with the Chassis_Template_Var >> table removed. Since jsonrpc debug logging is enabled, you'll see: >> >> |jsonrpc|DBG|ssl:127.0.0.1:6642: send request, method="monitor_cond", >> params=["OVN_Southbound",["monid","OVN_Southbound"], >> >> {...,"Chassis_Template_Var":[{"columns":[],"where":[false]}],...], id=10 >> |jsonrpc|DBG|ssl:127.0.0.1:6642: received error, >> error={"details":"no table named Chassis_Template_Var","error":"syntax >> error"}, id=10 > > Hi Ilya, > > thx for pointing out the issue, I will fix it in v2. > >> >> This will still be true after applying the second change as well: >> >> https://patchwork.ozlabs.org/project/openvswitch/patch/b754631fb9acb9b985927692e7d64eaff01129ab.1773349026.git.lorenzo.bianc...@redhat.com/ >> >> What we need is to: >> >> 1. Combine both OVS patches. > > ack, I will fix it in v2. > >> >> 2. Filter the conditions inside ovsdb_idl_set_condition__ or >> ovsdb_idl_condition_to_json, so the direct set_conditions >> calls from the application do not set conditions for tables >> and columns that do not exist on a server. > > ack, I will fix it in v2. > >> >> 3. We may need to make sure that we're not sending the last id >> for monitor requests in case we're dropping previously acked >> conditions, so we'll get a full content with the 'clear' >> flag for the new conditions and avoid having stale data. >> Not sure if this is necessary since the database index should >> protect us here, but may be good to do anyway. > > What is the right segno ovsdb_idl_set_condition() should return if all clauses > are removed from incoming ovsdb_idl_condition condition?
Hmm. Let's say table column A exists on the server and B doesn't. Application first sets condition for both A and B, B gets stripped out from the request and A is set as new_cond and then sent out and acknowledged by the server. Now the user calls set condition with just column B. This means that they are removing conditions from column A and potentially modifying conditions from column B. In this case even though the resulted condition is empty after we strip the condition for column B, we still need to call ovsdb_cs_set_condition() with the default "false" condition for the table, so it gets updated on the server. In which case the seqno to return is the sequence number that ovsdb_cs_db_set_condition() returns. Conditions clauses are disjunctive, i.e. A || B, dropping some of them is fine, but dropping all of them means "false". If the entire table doesn't exist though, then just drop the entire condition and don't send anything. And it is not possible that we already monitored that table, so the current seqno should be the correct one to return. Dumitru, what do you think? Am I missing something? Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
