| Message ID | 20240724125330.1235599-1-amusil@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,v2] 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 |
Thanks Ales, especially for the explanation for why the type check is still maintained. Acked-by: Mark Michelson <mmichels@redhat.com> On 7/24/24 08:53, Ales Musil 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. The type check remains for the purpose > of not regressing when iface id is set in different transaction > than the actual interface creation for the internal and empty type. > > 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> > --- > v2: Rebase on top of current main. > Address comment from Xavier: > - Make sure we are not causing recompute if iface-id is set in separate transaction. > --- > controller/binding.c | 14 +++++----- > tests/ovn-controller.at | 57 +++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 64 insertions(+), 7 deletions(-) > > diff --git a/controller/binding.c b/controller/binding.c > index b7e7f4874..f4f5f9407 100644 > --- a/controller/binding.c > +++ b/controller/binding.c > @@ -2630,17 +2630,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 && !is_iface_vif(iface_rec)) { > + /* 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 9cb099e68..208645504 100644 > --- a/tests/ovn-controller.at > +++ b/tests/ovn-controller.at > @@ -3127,3 +3127,60 @@ 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 > + > +# Make sure that setting iface in two different transaction doesn't > +# cause recompute. > +ovs-vsctl add-port br-int vif > +ovs-vsctl 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 > + > +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
I pushed this to main. On 7/30/24 15:05, Mark Michelson wrote: > Thanks Ales, especially for the explanation for why the type check is > still maintained. > > Acked-by: Mark Michelson <mmichels@redhat.com> > > On 7/24/24 08:53, Ales Musil 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. The type check remains for the purpose >> of not regressing when iface id is set in different transaction >> than the actual interface creation for the internal and empty type. >> >> 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> >> --- >> v2: Rebase on top of current main. >> Address comment from Xavier: >> - Make sure we are not causing recompute if iface-id is set in >> separate transaction. >> --- >> controller/binding.c | 14 +++++----- >> tests/ovn-controller.at | 57 +++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 64 insertions(+), 7 deletions(-) >> >> diff --git a/controller/binding.c b/controller/binding.c >> index b7e7f4874..f4f5f9407 100644 >> --- a/controller/binding.c >> +++ b/controller/binding.c >> @@ -2630,17 +2630,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 && !is_iface_vif(iface_rec)) { >> + /* 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 9cb099e68..208645504 100644 >> --- a/tests/ovn-controller.at >> +++ b/tests/ovn-controller.at >> @@ -3127,3 +3127,60 @@ 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 >> + >> +# Make sure that setting iface in two different transaction doesn't >> +# cause recompute. >> +ovs-vsctl add-port br-int vif >> +ovs-vsctl 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 >> + >> +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 >
diff --git a/controller/binding.c b/controller/binding.c index b7e7f4874..f4f5f9407 100644 --- a/controller/binding.c +++ b/controller/binding.c @@ -2630,17 +2630,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 && !is_iface_vif(iface_rec)) { + /* 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 9cb099e68..208645504 100644 --- a/tests/ovn-controller.at +++ b/tests/ovn-controller.at @@ -3127,3 +3127,60 @@ 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 + +# Make sure that setting iface in two different transaction doesn't +# cause recompute. +ovs-vsctl add-port br-int vif +ovs-vsctl 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 + +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. The type check remains for the purpose of not regressing when iface id is set in different transaction than the actual interface creation for the internal and empty type. 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> --- v2: Rebase on top of current main. Address comment from Xavier: - Make sure we are not causing recompute if iface-id is set in separate transaction. --- controller/binding.c | 14 +++++----- tests/ovn-controller.at | 57 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+), 7 deletions(-)