Hi Ales Thanks for the patch. Some embedded comments.
On Tue, Jul 16, 2024 at 1:52 PM Ales Musil <[email protected]> wrote: > The handler for OvS interface in runtime data was checking interface > type before proceeding with the I-P processing. The type is not > necessary because the main thing that is interesting for OVN is > the iface-id, if the interface doesn't have any it won't be processed > regardless of the type. The processing would happen anyway, however > it would be more costly because it would lead to full recompute > of the whole runtime data node. > > Reported-at: https://github.com/ovn-org/ovn/issues/174 > Reported-at: https://issues.redhat.com/browse/FDP-255 > Signed-off-by: Ales Musil <[email protected]> > --- > controller/binding.c | 25 ++++++--------------- > tests/ovn-controller.at | 48 +++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 55 insertions(+), 18 deletions(-) > > diff --git a/controller/binding.c b/controller/binding.c > index b7e7f4874..da1f8afcf 100644 > --- a/controller/binding.c > +++ b/controller/binding.c > @@ -2505,17 +2505,6 @@ consider_iface_release(const struct > ovsrec_interface *iface_rec, > return true; > } > > -static bool > -is_iface_vif(const struct ovsrec_interface *iface_rec) > -{ > - if (iface_rec->type && iface_rec->type[0] && > - strcmp(iface_rec->type, "internal")) { > - return false; > - } > - > - return true; > -} > - > static bool > is_iface_in_int_bridge(const struct ovsrec_interface *iface, > const struct ovsrec_bridge *br_int) > @@ -2630,17 +2619,17 @@ binding_handle_ovs_interface_changes(struct > binding_ctx_in *b_ctx_in, > const struct ovsrec_interface *iface_rec; > OVSREC_INTERFACE_TABLE_FOR_EACH_TRACKED (iface_rec, > b_ctx_in->iface_table) { > - if (!is_iface_vif(iface_rec)) { > - /* Right now we are not handling ovs_interface changes of > - * other types. This can be enhanced to handle of > - * types - patch and tunnel. */ > + const char *iface_id = smap_get(&iface_rec->external_ids, > "iface-id"); > + const char *old_iface_id = smap_get(b_ctx_out->local_iface_ids, > + iface_rec->name); > + if (!iface_id && !old_iface_id) { > + /* Right now we are not handling ovs_interface changes if the > + * interface doesn't have iface-id or didn't have it > + * previously. */ > handled = false; > Will this not cause recomputes when adding an interface, and setting iface-id in two steps e.g. ovs-vsctl add-port br-int vif ovs-vsctl set Interface vif external-ids:iface-id=vif While there was no recompute for this c > break; > } > > - const char *iface_id = smap_get(&iface_rec->external_ids, > "iface-id"); > - const char *old_iface_id = smap_get(b_ctx_out->local_iface_ids, > - iface_rec->name); > const char *cleared_iface_id = NULL; > if (!ovsrec_interface_is_deleted(iface_rec)) { > int64_t ofport = iface_rec->n_ofport ? *iface_rec->ofport : 0; > diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at > index 2cb86dc98..be913676b 100644 > --- a/tests/ovn-controller.at > +++ b/tests/ovn-controller.at > @@ -3127,3 +3127,51 @@ OVS_WAIT_UNTIL([grep -q 'tcp:127.0.0.1:1235: > connected' hv1/ovn-controller.log]) > > OVN_CLEANUP([hv1]) > AT_CLEANUP > + > +AT_SETUP([ovn-controller - I-P different port types]) > +AT_KEYWORDS([ovn]) > +ovn_start > + > +net_add n1 > +sim_add hv1 > +ovs-vsctl add-br br-phys > +ovn_attach n1 br-phys 192.168.0.20 > + > +check ovn-nbctl ls-add ls0 > +check ovn-nbctl lsp-add ls0 vif > + > +ovn-appctl inc-engine/clear-stats > + > +ovs-vsctl -- add-port br-int vif -- \ > + set Interface vif external-ids:iface-id=vif > +wait_row_count Port_Binding 1 logical_port="vif" up=true > + > +ovs-vsctl del-port br-int vif > +wait_row_count Port_Binding 1 logical_port="vif" up=false > + > +ovs-vsctl add-port br-int vif -- \ > + set Interface vif type=dummy -- \ > + set Interface vif external-ids:iface-id=vif > +wait_row_count Port_Binding 1 logical_port="vif" up=true > + > +ovs-vsctl del-port br-int vif > +wait_row_count Port_Binding 1 logical_port="vif" up=false > + > +ovs-vsctl add-port br-int vif -- \ > + set Interface vif type=geneve -- \ > + set Interface vif options:remote_ip=1.1.1.1 external-ids:iface-id=vif > +wait_row_count Port_Binding 1 logical_port="vif" up=true > + > +ovs-vsctl del-port br-int vif > +wait_row_count Port_Binding 1 logical_port="vif" up=false > + > +AT_CHECK([ovn-appctl inc-engine/show-stats runtime_data |\ > + sed "s/- compute:\s\+[[0-9]]\+/- compute: ??/"], [0], [dnl > +Node: runtime_data > +- recompute: 0 > +- compute: ?? > +- cancel: 0 > +]) > + > +OVN_CLEANUP([hv1]) > +AT_CLEANUP > -- > 2.45.2 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
