Hi Ales On Wed, Jul 24, 2024 at 7:24 AM Ales Musil <amu...@redhat.com> wrote:
> > > On Tue, Jul 23, 2024 at 3:41 PM Xavier Simonart <xsimo...@redhat.com> > wrote: > >> Hi Ales >> >> Thanks for the patch. >> Some embedded comments. >> >> > Hi Xavier, > > thank you for the review. > > >> On Tue, Jul 16, 2024 at 1:52 PM Ales Musil <amu...@redhat.com> 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 <amu...@redhat.com> >>> --- >>> 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 >> > > You are right, however I'm not sure how to better approach this. I mean > the "workaround" > should be to use a single transaction which is doable from a CMS > perspective. > > I'm open to suggestions on how to avoid this problem though. > I agree that probably CMS would add the iface and set the iface-id in one command, but we would fix an issue related to recomputes w/ dpdk ports, and potentially add more recomputes in some (rare?) cases. As discussed separately, can we (also) check the i/f types as before - to avoid at least recompute in the cases where we had no recompute before ? So something like if (!iface_id && !old_iface_id && !is_iface_vif(iface_rec)) ... This would cover different iface types when iface-id is added within the same transaction as iface, and keep current behavior for vif/internal ports (i.e. no recompute when added in two idl transactions) WDYT? > > >> 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 >>> d...@openvswitch.org >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>> >>> > Thanks, > Ales > Thanks Xavier > -- > > Ales Musil > > Senior Software Engineer - OVN Core > > Red Hat EMEA <https://www.redhat.com> > > amu...@redhat.com > <https://red.ht/sig> > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
