| Message ID | 20230731171816.2932499-1-mheib@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,v2] binding: handle ovs ofport update | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
Thanks for the changes, Mohammad. Acked-by: Mark Michelson <mmichels@redhat.com> On 7/31/23 13:18, Mohammad Heib wrote: > Currently when ovs interface ofport is updated > after setting external_ids:iface_id, the ovn-controller > will see this change but will not do much if it handles > this change incrementally. > > This behavior leads to a mismatch between the ovs openflow > flows in table=0 (inaccurate in_port) and the ofport number > that the packet was received at which will lead to packets > drop in table=0. > > This patch will resolve the above issue by triggering > flows recompute during the I-P processing only if the > affected port are associated with lport and has flows > that need to be updated. > > Reported-at: https://issues.redhat.com/browse/FD-3063 > Signed-off-by: Mohammad Heib <mheib@redhat.com> > --- > controller/binding.c | 15 ++++++++++++++ > tests/ovn-controller.at | 45 +++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 60 insertions(+) > > diff --git a/controller/binding.c b/controller/binding.c > index 9aa3fc6c4..cc4c2b0bb 100644 > --- a/controller/binding.c > +++ b/controller/binding.c > @@ -2360,6 +2360,21 @@ consider_iface_claim(const struct ovsrec_interface *iface_rec, > /* Get the (updated) b_lport again for the lbinding. */ > b_lport = local_binding_get_primary_lport(lbinding); > > + /* > + * Update the tracked_dp_bindings whenever an ofport > + * on a specific ovs port changes. > + * This update will trigger flow recomputation during > + * the incremental processing run which updates the local > + * flows in_port filed. > + */ > + if (b_lport && ovsrec_interface_is_updated(iface_rec, > + OVSREC_INTERFACE_COL_OFPORT)) { > + tracked_datapath_lport_add(b_lport->pb, TRACKED_RESOURCE_UPDATED, > + b_ctx_out->tracked_dp_bindings); > + b_ctx_out->local_lports_changed = true; > + } > + > + > /* Update the child local_binding's iface (if any children) and try to > * claim the container lbindings. */ > LIST_FOR_EACH (b_lport, list_node, &lbinding->binding_lports) { > diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at > index e1b6491b3..f2216d245 100644 > --- a/tests/ovn-controller.at > +++ b/tests/ovn-controller.at > @@ -2606,3 +2606,48 @@ AT_CHECK([ovn-sbctl get chassis $chassis_id other_config:unsupported], [1], [ign > OVN_CLEANUP([hv1]) > AT_CLEANUP > ]) > + > +OVN_FOR_EACH_NORTHD([ > +AT_SETUP([ovn-controller - ovs iface change ofport]) > +AT_KEYWORDS([ovn]) > +ovn_start > + > +net_add n1 > + > +sim_add hv1 > +as hv1 > +ovs-vsctl add-br br-phys > +ovn_attach n1 br-phys 192.168.0.1 > +ovs-vsctl -- add-port br-int hv1-vif1 -- \ > + set interface hv1-vif1 external-ids:iface-id=sw0-p1 \ > + options:tx_pcap=hv1/vif1-tx.pcap \ > + options:rxq_pcap=hv1/vif1-rx.pcap \ > + ofport-request=1 > + > + > +ovn-nbctl ls-add sw0 > + > +check ovn-nbctl lsp-add sw0 sw0-p1 > +check ovn-nbctl lsp-set-addresses sw0-p1 "50:54:00:00:00:03 10.0.0.3 1000::3" > + > +wait_for_ports_up > +ovn-nbctl --wait=hv sync > + > +OVS_WAIT_FOR_OUTPUT([as hv1 ovs-ofctl dump-flows br-int table=0 | grep -c in_port=1], [0],[dnl > +1 > +]) > + > +# update the ovs interface ofport from 1 to 24 > +check as hv1 ovs-vsctl set Interface hv1-vif1 ofport-request=24 > +OVS_WAIT_UNTIL([test x`as hv1 ovs-vsctl get Interface hv1-vif1 ofport` = x24]) > + > +OVS_WAIT_FOR_OUTPUT([as hv1 ovs-ofctl dump-flows br-int table=0 | grep -c in_port=24], [0],[dnl > +1 > +]) > +OVS_WAIT_FOR_OUTPUT([as hv1 ovs-ofctl dump-flows br-int table=0 | grep -c in_port=1], [1],[dnl > +0 > +]) > + > +OVN_CLEANUP([hv1]) > +AT_CLEANUP > +])
On Mon, Jul 31, 2023 at 7:18 PM Mohammad Heib <mheib@redhat.com> wrote: > Currently when ovs interface ofport is updated > after setting external_ids:iface_id, the ovn-controller > will see this change but will not do much if it handles > this change incrementally. > > This behavior leads to a mismatch between the ovs openflow > flows in table=0 (inaccurate in_port) and the ofport number > that the packet was received at which will lead to packets > drop in table=0. > > This patch will resolve the above issue by triggering > flows recompute during the I-P processing only if the > affected port are associated with lport and has flows > that need to be updated. > > Reported-at: https://issues.redhat.com/browse/FD-3063 > Signed-off-by: Mohammad Heib <mheib@redhat.com> > --- > controller/binding.c | 15 ++++++++++++++ > tests/ovn-controller.at | 45 +++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 60 insertions(+) > > diff --git a/controller/binding.c b/controller/binding.c > index 9aa3fc6c4..cc4c2b0bb 100644 > --- a/controller/binding.c > +++ b/controller/binding.c > @@ -2360,6 +2360,21 @@ consider_iface_claim(const struct ovsrec_interface > *iface_rec, > /* Get the (updated) b_lport again for the lbinding. */ > b_lport = local_binding_get_primary_lport(lbinding); > > + /* > + * Update the tracked_dp_bindings whenever an ofport > + * on a specific ovs port changes. > + * This update will trigger flow recomputation during > + * the incremental processing run which updates the local > + * flows in_port filed. > + */ > + if (b_lport && ovsrec_interface_is_updated(iface_rec, > + OVSREC_INTERFACE_COL_OFPORT)) { > + tracked_datapath_lport_add(b_lport->pb, TRACKED_RESOURCE_UPDATED, > + b_ctx_out->tracked_dp_bindings); > + b_ctx_out->local_lports_changed = true; > + } > + > + > /* Update the child local_binding's iface (if any children) and try to > * claim the container lbindings. */ > LIST_FOR_EACH (b_lport, list_node, &lbinding->binding_lports) { > diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at > index e1b6491b3..f2216d245 100644 > --- a/tests/ovn-controller.at > +++ b/tests/ovn-controller.at > @@ -2606,3 +2606,48 @@ AT_CHECK([ovn-sbctl get chassis $chassis_id > other_config:unsupported], [1], [ign > OVN_CLEANUP([hv1]) > AT_CLEANUP > ]) > + > +OVN_FOR_EACH_NORTHD([ > +AT_SETUP([ovn-controller - ovs iface change ofport]) > +AT_KEYWORDS([ovn]) > +ovn_start > + > +net_add n1 > + > +sim_add hv1 > +as hv1 > +ovs-vsctl add-br br-phys > +ovn_attach n1 br-phys 192.168.0.1 > +ovs-vsctl -- add-port br-int hv1-vif1 -- \ > + set interface hv1-vif1 external-ids:iface-id=sw0-p1 \ > + options:tx_pcap=hv1/vif1-tx.pcap \ > + options:rxq_pcap=hv1/vif1-rx.pcap \ > + ofport-request=1 > + > + > +ovn-nbctl ls-add sw0 > + > +check ovn-nbctl lsp-add sw0 sw0-p1 > +check ovn-nbctl lsp-set-addresses sw0-p1 "50:54:00:00:00:03 10.0.0.3 > 1000::3" > + > +wait_for_ports_up > +ovn-nbctl --wait=hv sync > + > +OVS_WAIT_FOR_OUTPUT([as hv1 ovs-ofctl dump-flows br-int table=0 | grep -c > in_port=1], [0],[dnl > +1 > +]) > + > +# update the ovs interface ofport from 1 to 24 > +check as hv1 ovs-vsctl set Interface hv1-vif1 ofport-request=24 > +OVS_WAIT_UNTIL([test x`as hv1 ovs-vsctl get Interface hv1-vif1 ofport` = > x24]) > + > +OVS_WAIT_FOR_OUTPUT([as hv1 ovs-ofctl dump-flows br-int table=0 | grep -c > in_port=24], [0],[dnl > +1 > +]) > +OVS_WAIT_FOR_OUTPUT([as hv1 ovs-ofctl dump-flows br-int table=0 | grep -c > in_port=1], [1],[dnl > +0 > +]) > + > +OVN_CLEANUP([hv1]) > +AT_CLEANUP > +]) > -- > 2.34.3 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Looks good to me, thanks. Acked-by: Ales Musil <amusil@redhat.com>
Thanks Mohammad and Ales. I have pushed the change to main and all branches back to 22.03. On 8/3/23 04:46, Ales Musil wrote: > On Mon, Jul 31, 2023 at 7:18 PM Mohammad Heib <mheib@redhat.com> wrote: > >> Currently when ovs interface ofport is updated >> after setting external_ids:iface_id, the ovn-controller >> will see this change but will not do much if it handles >> this change incrementally. >> >> This behavior leads to a mismatch between the ovs openflow >> flows in table=0 (inaccurate in_port) and the ofport number >> that the packet was received at which will lead to packets >> drop in table=0. >> >> This patch will resolve the above issue by triggering >> flows recompute during the I-P processing only if the >> affected port are associated with lport and has flows >> that need to be updated. >> >> Reported-at: https://issues.redhat.com/browse/FD-3063 >> Signed-off-by: Mohammad Heib <mheib@redhat.com> >> --- >> controller/binding.c | 15 ++++++++++++++ >> tests/ovn-controller.at | 45 +++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 60 insertions(+) >> >> diff --git a/controller/binding.c b/controller/binding.c >> index 9aa3fc6c4..cc4c2b0bb 100644 >> --- a/controller/binding.c >> +++ b/controller/binding.c >> @@ -2360,6 +2360,21 @@ consider_iface_claim(const struct ovsrec_interface >> *iface_rec, >> /* Get the (updated) b_lport again for the lbinding. */ >> b_lport = local_binding_get_primary_lport(lbinding); >> >> + /* >> + * Update the tracked_dp_bindings whenever an ofport >> + * on a specific ovs port changes. >> + * This update will trigger flow recomputation during >> + * the incremental processing run which updates the local >> + * flows in_port filed. >> + */ >> + if (b_lport && ovsrec_interface_is_updated(iface_rec, >> + OVSREC_INTERFACE_COL_OFPORT)) { >> + tracked_datapath_lport_add(b_lport->pb, TRACKED_RESOURCE_UPDATED, >> + b_ctx_out->tracked_dp_bindings); >> + b_ctx_out->local_lports_changed = true; >> + } >> + >> + >> /* Update the child local_binding's iface (if any children) and try to >> * claim the container lbindings. */ >> LIST_FOR_EACH (b_lport, list_node, &lbinding->binding_lports) { >> diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at >> index e1b6491b3..f2216d245 100644 >> --- a/tests/ovn-controller.at >> +++ b/tests/ovn-controller.at >> @@ -2606,3 +2606,48 @@ AT_CHECK([ovn-sbctl get chassis $chassis_id >> other_config:unsupported], [1], [ign >> OVN_CLEANUP([hv1]) >> AT_CLEANUP >> ]) >> + >> +OVN_FOR_EACH_NORTHD([ >> +AT_SETUP([ovn-controller - ovs iface change ofport]) >> +AT_KEYWORDS([ovn]) >> +ovn_start >> + >> +net_add n1 >> + >> +sim_add hv1 >> +as hv1 >> +ovs-vsctl add-br br-phys >> +ovn_attach n1 br-phys 192.168.0.1 >> +ovs-vsctl -- add-port br-int hv1-vif1 -- \ >> + set interface hv1-vif1 external-ids:iface-id=sw0-p1 \ >> + options:tx_pcap=hv1/vif1-tx.pcap \ >> + options:rxq_pcap=hv1/vif1-rx.pcap \ >> + ofport-request=1 >> + >> + >> +ovn-nbctl ls-add sw0 >> + >> +check ovn-nbctl lsp-add sw0 sw0-p1 >> +check ovn-nbctl lsp-set-addresses sw0-p1 "50:54:00:00:00:03 10.0.0.3 >> 1000::3" >> + >> +wait_for_ports_up >> +ovn-nbctl --wait=hv sync >> + >> +OVS_WAIT_FOR_OUTPUT([as hv1 ovs-ofctl dump-flows br-int table=0 | grep -c >> in_port=1], [0],[dnl >> +1 >> +]) >> + >> +# update the ovs interface ofport from 1 to 24 >> +check as hv1 ovs-vsctl set Interface hv1-vif1 ofport-request=24 >> +OVS_WAIT_UNTIL([test x`as hv1 ovs-vsctl get Interface hv1-vif1 ofport` = >> x24]) >> + >> +OVS_WAIT_FOR_OUTPUT([as hv1 ovs-ofctl dump-flows br-int table=0 | grep -c >> in_port=24], [0],[dnl >> +1 >> +]) >> +OVS_WAIT_FOR_OUTPUT([as hv1 ovs-ofctl dump-flows br-int table=0 | grep -c >> in_port=1], [1],[dnl >> +0 >> +]) >> + >> +OVN_CLEANUP([hv1]) >> +AT_CLEANUP >> +]) >> -- >> 2.34.3 >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >> > Looks good to me, thanks. > > Acked-by: Ales Musil <amusil@redhat.com> >
On 8/9/23 20:01, Mark Michelson wrote: > Thanks Mohammad and Ales. I have pushed the change to main and all > branches back to 22.03. > Hi Mark, Mohammad, It seems this change broke 22.03: https://github.com/ovn-org/ovn/actions/runs/5812479357 > On 8/3/23 04:46, Ales Musil wrote: >> On Mon, Jul 31, 2023 at 7:18 PM Mohammad Heib <mheib@redhat.com> wrote: >> >>> Currently when ovs interface ofport is updated >>> after setting external_ids:iface_id, the ovn-controller >>> will see this change but will not do much if it handles >>> this change incrementally. >>> >>> This behavior leads to a mismatch between the ovs openflow >>> flows in table=0 (inaccurate in_port) and the ofport number >>> that the packet was received at which will lead to packets >>> drop in table=0. >>> >>> This patch will resolve the above issue by triggering >>> flows recompute during the I-P processing only if the >>> affected port are associated with lport and has flows >>> that need to be updated. >>> >>> Reported-at: https://issues.redhat.com/browse/FD-3063 >>> Signed-off-by: Mohammad Heib <mheib@redhat.com> >>> --- >>> controller/binding.c | 15 ++++++++++++++ >>> tests/ovn-controller.at | 45 +++++++++++++++++++++++++++++++++++++++++ >>> 2 files changed, 60 insertions(+) >>> >>> diff --git a/controller/binding.c b/controller/binding.c >>> index 9aa3fc6c4..cc4c2b0bb 100644 >>> --- a/controller/binding.c >>> +++ b/controller/binding.c >>> @@ -2360,6 +2360,21 @@ consider_iface_claim(const struct >>> ovsrec_interface >>> *iface_rec, >>> /* Get the (updated) b_lport again for the lbinding. */ >>> b_lport = local_binding_get_primary_lport(lbinding); >>> >>> + /* >>> + * Update the tracked_dp_bindings whenever an ofport >>> + * on a specific ovs port changes. >>> + * This update will trigger flow recomputation during >>> + * the incremental processing run which updates the local >>> + * flows in_port filed. >>> + */ >>> + if (b_lport && ovsrec_interface_is_updated(iface_rec, >>> + OVSREC_INTERFACE_COL_OFPORT)) { >>> + tracked_datapath_lport_add(b_lport->pb, >>> TRACKED_RESOURCE_UPDATED, This part triggers the ct_zones_runtime_data_handler() I-P handler to be called. But on 22.03 this aborts on TRACKED_RESOURCE_UPDATED: https://github.com/ovn-org/ovn/blob/915c556f0c50e81b1dbc05a68d754cf4cb7d4551/controller/ovn-controller.c#L2154 Do you maybe have some time to look into this? Or should we just revert the backport on 22.03? I don't think ofport updates are common. Thanks, Dumitru
On 8/14/23 22:17, Dumitru Ceara wrote: > On 8/9/23 20:01, Mark Michelson wrote: >> Thanks Mohammad and Ales. I have pushed the change to main and all >> branches back to 22.03. >> > > Hi Mark, Mohammad, > > It seems this change broke 22.03: > > https://github.com/ovn-org/ovn/actions/runs/5812479357 > >> On 8/3/23 04:46, Ales Musil wrote: >>> On Mon, Jul 31, 2023 at 7:18 PM Mohammad Heib <mheib@redhat.com> wrote: >>> >>>> Currently when ovs interface ofport is updated >>>> after setting external_ids:iface_id, the ovn-controller >>>> will see this change but will not do much if it handles >>>> this change incrementally. >>>> >>>> This behavior leads to a mismatch between the ovs openflow >>>> flows in table=0 (inaccurate in_port) and the ofport number >>>> that the packet was received at which will lead to packets >>>> drop in table=0. >>>> >>>> This patch will resolve the above issue by triggering >>>> flows recompute during the I-P processing only if the >>>> affected port are associated with lport and has flows >>>> that need to be updated. >>>> >>>> Reported-at: https://issues.redhat.com/browse/FD-3063 >>>> Signed-off-by: Mohammad Heib <mheib@redhat.com> >>>> --- >>>> controller/binding.c | 15 ++++++++++++++ >>>> tests/ovn-controller.at | 45 +++++++++++++++++++++++++++++++++++++++++ >>>> 2 files changed, 60 insertions(+) >>>> >>>> diff --git a/controller/binding.c b/controller/binding.c >>>> index 9aa3fc6c4..cc4c2b0bb 100644 >>>> --- a/controller/binding.c >>>> +++ b/controller/binding.c >>>> @@ -2360,6 +2360,21 @@ consider_iface_claim(const struct >>>> ovsrec_interface >>>> *iface_rec, >>>> /* Get the (updated) b_lport again for the lbinding. */ >>>> b_lport = local_binding_get_primary_lport(lbinding); >>>> >>>> + /* >>>> + * Update the tracked_dp_bindings whenever an ofport >>>> + * on a specific ovs port changes. >>>> + * This update will trigger flow recomputation during >>>> + * the incremental processing run which updates the local >>>> + * flows in_port filed. >>>> + */ >>>> + if (b_lport && ovsrec_interface_is_updated(iface_rec, >>>> + OVSREC_INTERFACE_COL_OFPORT)) { >>>> + tracked_datapath_lport_add(b_lport->pb, >>>> TRACKED_RESOURCE_UPDATED, > > This part triggers the ct_zones_runtime_data_handler() I-P handler to be > called. But on 22.03 this aborts on TRACKED_RESOURCE_UPDATED: > > https://github.com/ovn-org/ovn/blob/915c556f0c50e81b1dbc05a68d754cf4cb7d4551/controller/ovn-controller.c#L2154 > > Do you maybe have some time to look into this? Or should we just revert > the backport on 22.03? I don't think ofport updates are common. > I went ahead and reverted this on branch 22.03 as that's our LTS and I wanted to make sure we have green CI on it. Mohammad, please post a custom backport patch if you want this fixed on 22.03 too. Regards, Dumitru
diff --git a/controller/binding.c b/controller/binding.c index 9aa3fc6c4..cc4c2b0bb 100644 --- a/controller/binding.c +++ b/controller/binding.c @@ -2360,6 +2360,21 @@ consider_iface_claim(const struct ovsrec_interface *iface_rec, /* Get the (updated) b_lport again for the lbinding. */ b_lport = local_binding_get_primary_lport(lbinding); + /* + * Update the tracked_dp_bindings whenever an ofport + * on a specific ovs port changes. + * This update will trigger flow recomputation during + * the incremental processing run which updates the local + * flows in_port filed. + */ + if (b_lport && ovsrec_interface_is_updated(iface_rec, + OVSREC_INTERFACE_COL_OFPORT)) { + tracked_datapath_lport_add(b_lport->pb, TRACKED_RESOURCE_UPDATED, + b_ctx_out->tracked_dp_bindings); + b_ctx_out->local_lports_changed = true; + } + + /* Update the child local_binding's iface (if any children) and try to * claim the container lbindings. */ LIST_FOR_EACH (b_lport, list_node, &lbinding->binding_lports) { diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at index e1b6491b3..f2216d245 100644 --- a/tests/ovn-controller.at +++ b/tests/ovn-controller.at @@ -2606,3 +2606,48 @@ AT_CHECK([ovn-sbctl get chassis $chassis_id other_config:unsupported], [1], [ign OVN_CLEANUP([hv1]) AT_CLEANUP ]) + +OVN_FOR_EACH_NORTHD([ +AT_SETUP([ovn-controller - ovs iface change ofport]) +AT_KEYWORDS([ovn]) +ovn_start + +net_add n1 + +sim_add hv1 +as hv1 +ovs-vsctl add-br br-phys +ovn_attach n1 br-phys 192.168.0.1 +ovs-vsctl -- add-port br-int hv1-vif1 -- \ + set interface hv1-vif1 external-ids:iface-id=sw0-p1 \ + options:tx_pcap=hv1/vif1-tx.pcap \ + options:rxq_pcap=hv1/vif1-rx.pcap \ + ofport-request=1 + + +ovn-nbctl ls-add sw0 + +check ovn-nbctl lsp-add sw0 sw0-p1 +check ovn-nbctl lsp-set-addresses sw0-p1 "50:54:00:00:00:03 10.0.0.3 1000::3" + +wait_for_ports_up +ovn-nbctl --wait=hv sync + +OVS_WAIT_FOR_OUTPUT([as hv1 ovs-ofctl dump-flows br-int table=0 | grep -c in_port=1], [0],[dnl +1 +]) + +# update the ovs interface ofport from 1 to 24 +check as hv1 ovs-vsctl set Interface hv1-vif1 ofport-request=24 +OVS_WAIT_UNTIL([test x`as hv1 ovs-vsctl get Interface hv1-vif1 ofport` = x24]) + +OVS_WAIT_FOR_OUTPUT([as hv1 ovs-ofctl dump-flows br-int table=0 | grep -c in_port=24], [0],[dnl +1 +]) +OVS_WAIT_FOR_OUTPUT([as hv1 ovs-ofctl dump-flows br-int table=0 | grep -c in_port=1], [1],[dnl +0 +]) + +OVN_CLEANUP([hv1]) +AT_CLEANUP +])
Currently when ovs interface ofport is updated after setting external_ids:iface_id, the ovn-controller will see this change but will not do much if it handles this change incrementally. This behavior leads to a mismatch between the ovs openflow flows in table=0 (inaccurate in_port) and the ofport number that the packet was received at which will lead to packets drop in table=0. This patch will resolve the above issue by triggering flows recompute during the I-P processing only if the affected port are associated with lport and has flows that need to be updated. Reported-at: https://issues.redhat.com/browse/FD-3063 Signed-off-by: Mohammad Heib <mheib@redhat.com> --- controller/binding.c | 15 ++++++++++++++ tests/ovn-controller.at | 45 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+)