| Message ID | 20240716115224.606737-1-amusil@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] controller: Remove OvS iface type check in I-P processing. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
Hi Ales Thanks for the patch. Some embedded comments. On Tue, Jul 16, 2024 at 1:52 PM Ales Musil <amusil@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 <amusil@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 > 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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
On Tue, Jul 23, 2024 at 3:41 PM Xavier Simonart <xsimonar@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 <amusil@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 <amusil@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. > 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 >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >> Thanks, Ales
Hi Ales On Wed, Jul 24, 2024 at 7:24 AM Ales Musil <amusil@redhat.com> wrote: > > > On Tue, Jul 23, 2024 at 3:41 PM Xavier Simonart <xsimonar@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 <amusil@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 <amusil@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 >>> dev@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> > > amusil@redhat.com > <https://red.ht/sig> >
On Wed, Jul 24, 2024 at 1:23 PM Xavier Simonart <xsimonar@redhat.com> wrote: > Hi Ales > > On Wed, Jul 24, 2024 at 7:24 AM Ales Musil <amusil@redhat.com> wrote: > >> >> >> On Tue, Jul 23, 2024 at 3:41 PM Xavier Simonart <xsimonar@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 <amusil@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 <amusil@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 >>>> dev@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> >> >> amusil@redhat.com >> <https://red.ht/sig> >> > Thanks, Ales
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; 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
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 <amusil@redhat.com> --- controller/binding.c | 25 ++++++--------------- tests/ovn-controller.at | 48 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 18 deletions(-)