On Wed, Jul 24, 2024 at 1:23 PM Xavier Simonart <xsimo...@redhat.com> wrote:
> 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? > Makes sense, posted v2. >> >>> 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> >> > Thanks, Ales -- 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
