| Message ID | 20241114135756.1082588-2-xsimonar@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Compare I+P flows with with recompute ones. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | warning | apply and check: warning |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
Bleep bloop. Greetings Xavier Simonart, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. checkpatch: WARNING: Unexpected sign-offs from developers who are not authors or co-authors or committers: Numan Siddique <numans@ovn.org> Lines checked: 135, Warnings: 1, Errors: 0 Please check this out. If you feel there has been an error, please email aconole@redhat.com Thanks, 0-day Robot
On Thu, Nov 14, 2024 at 8:58 AM Xavier Simonart <xsimonar@redhat.com> wrote: > > Delete flows on localnet port deletion, and add localnet > related flows when peer ports are added. This was properly done when > recomputing, but not when doing IP. > > When peer ports are added, some flows such as chassis_mac flows > must be added. > > Fixes: edc064b4c589 ("controller: Properly handle localnet flows in I+P.") > Fixes: d276728a8ead ("Revert "controller: Properly handle localnet flows in I+P.".") > > Signed-off-by: Xavier Simonart <xsimonar@redhat.com> > Acked-by: Ales Musil <amusil@redhat.com> > Signed-off-by: Numan Siddique <numans@ovn.org> > Signed-off-by: Xavier Simonart <xsimonar@redhat.com> Thanks. I applied this patch to main and backported to 24.09 and 24.03 branches. Numan > --- > controller/physical.c | 10 +++++--- > tests/ovn.at | 57 +++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 63 insertions(+), 4 deletions(-) > > diff --git a/controller/physical.c b/controller/physical.c > index 3ca4e0783..8d5065098 100644 > --- a/controller/physical.c > +++ b/controller/physical.c > @@ -699,7 +699,7 @@ put_replace_chassis_mac_flows(const struct shash *ct_zones, > put_resubmit(OFTABLE_LOG_INGRESS_PIPELINE, ofpacts_p); > ofctrl_add_flow(flow_table, OFTABLE_PHY_TO_LOG, 180, > rport_binding->header_.uuid.parts[0], > - &match, ofpacts_p, hc_uuid); > + &match, ofpacts_p, &localnet_port->header_.uuid); > > /* Provide second search criteria, i.e localnet port's > * vlan ID for conjunction flow */ > @@ -707,6 +707,7 @@ put_replace_chassis_mac_flows(const struct shash *ct_zones, > ofpbuf_clear(ofpacts_p); > match_init_catchall(&match); > > + match_set_in_port(&match, ofport); > if (tag) { > match_set_dl_vlan(&match, htons(tag), 0); > } else { > @@ -719,7 +720,7 @@ put_replace_chassis_mac_flows(const struct shash *ct_zones, > conj->clause = 1; > ofctrl_add_flow(flow_table, OFTABLE_PHY_TO_LOG, 180, > rport_binding->header_.uuid.parts[0], > - &match, ofpacts_p, hc_uuid); > + &match, ofpacts_p, &localnet_port->header_.uuid); > } > } > > @@ -2393,8 +2394,9 @@ physical_handle_flows_for_lport(const struct sbrec_port_binding *pb, > struct local_datapath *ldp = > get_local_datapath(p_ctx->local_datapaths, > pb->datapath->tunnel_key); > - if (!strcmp(pb->type, "external")) { > - /* External lports have a dependency on the localnet port. > + if (!strcmp(pb->type, "external") || > + !strcmp(pb->type, "patch") || !strcmp(pb->type, "l3gateway")) { > + /* Those lports have a dependency on the localnet port. > * We need to remove the flows of the localnet port as well > * and re-consider adding the flows for it. > */ > diff --git a/tests/ovn.at b/tests/ovn.at > index 92e1ffadc..40b8e59f8 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -39686,3 +39686,60 @@ OVN_CLEANUP([hv1]) > > AT_CLEANUP > ]) > + > +OVN_FOR_EACH_NORTHD([ > +AT_SETUP([localnet port flows after deletion]) > +ovn_start > +net_add n1 > + > +check ovn-nbctl ls-add sw0 > + > +for i in 1 2; do > + check ovn-nbctl lsp-add sw0 sw0-p${i} -- lsp-set-addresses sw0-p${i} "00:00:10:01:02:0${i} 10.0.0.${i}" > + sim_add hv${i} > + as hv${i} > + ovs-vsctl add-br br-phys > + ovn_attach n1 br-phys 192.168.0.${i} > + ovs-vsctl set open . external_ids:ovn-bridge-mappings=physnet1:br-phys > + ovs-vsctl add-port br-int vif${i} -- \ > + set Interface vif${i} external-ids:iface-id=sw0-p${i} \ > + options:tx_pcap=hv${i}/vif${i}-tx.pcap \ > + options:rxq_pcap=hv${i}/vif${i}-rx.pcap > +done > + > +check ovn-nbctl lr-add lr0 > +check ovn-nbctl lrp-add lr0 lr0-sw0 00:00:00:00:ff:01 10.0.0.254/24 > +check ovn-nbctl lsp-add sw0 sw0-lr0 > +check ovn-nbctl lsp-set-type sw0-lr0 router > +check ovn-nbctl lsp-set-addresses sw0-lr0 router > +check ovn-nbctl lsp-set-options sw0-lr0 router-port=lr0-sw0 > + > +check ovn-nbctl --wait=hv sync > +wait_for_ports_up > + > +# We should not have any flows in table OFTABLE_PHY_TO_LOG from in_port different from vif1 and ovn-hv2-0 > +OVN_WAIT_REMOTE_INPUT_FLOWS(["hv1"],["hv2"]) > +of1=$(as hv1 ovs-vsctl --bare --columns ofport find Interface name=vif1) > +of2=$(as hv1 ovs-vsctl --bare --columns ofport find Interface name=ovn-hv2-0) > +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=OFTABLE_PHY_TO_LOG | grep -v NXST_FLOW | grep "in_port=" | grep -v "in_port=$of1" | grep -v "in_port=$of2" | wc -l], [0], [dnl > +0 > +]) > + > +# Add localnet port to sw0 > +check ovn-nbctl lsp-add sw0 ln-sw0 -- lsp-set-addresses ln-sw0 unknown -- lsp-set-type ln-sw0 localnet > +check ovn-nbctl --wait=hv lsp-set-options ln-sw0 network_name=physnet1 -- set logical_switch_port ln-sw0 tag_request=100 > + > +OVN_WAIT_PATCH_PORT_FLOWS(["ln-sw0"], ["hv1"]) > +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=OFTABLE_PHY_TO_LOG | grep -v NXST_FLOW | grep "in_port=" | grep -v "in_port=$of1" | grep -v "in_port=$of2" | wc -l], [0], [dnl > +3 > +]) > + > +# Remove localnet port from sw0. Peer-ports flows should be deleted. > +check ovn-nbctl --wait=hv lsp-del ln-sw0 > +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=OFTABLE_PHY_TO_LOG | grep -v NXST_FLOW | grep "in_port=" | grep -v "in_port=$of1" | grep -v "in_port=$of2" | wc -l], [0], [dnl > +0 > +]) > + > +OVN_CLEANUP([hv1],[hv2]) > +AT_CLEANUP > +]) > -- > 2.31.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff --git a/controller/physical.c b/controller/physical.c index 3ca4e0783..8d5065098 100644 --- a/controller/physical.c +++ b/controller/physical.c @@ -699,7 +699,7 @@ put_replace_chassis_mac_flows(const struct shash *ct_zones, put_resubmit(OFTABLE_LOG_INGRESS_PIPELINE, ofpacts_p); ofctrl_add_flow(flow_table, OFTABLE_PHY_TO_LOG, 180, rport_binding->header_.uuid.parts[0], - &match, ofpacts_p, hc_uuid); + &match, ofpacts_p, &localnet_port->header_.uuid); /* Provide second search criteria, i.e localnet port's * vlan ID for conjunction flow */ @@ -707,6 +707,7 @@ put_replace_chassis_mac_flows(const struct shash *ct_zones, ofpbuf_clear(ofpacts_p); match_init_catchall(&match); + match_set_in_port(&match, ofport); if (tag) { match_set_dl_vlan(&match, htons(tag), 0); } else { @@ -719,7 +720,7 @@ put_replace_chassis_mac_flows(const struct shash *ct_zones, conj->clause = 1; ofctrl_add_flow(flow_table, OFTABLE_PHY_TO_LOG, 180, rport_binding->header_.uuid.parts[0], - &match, ofpacts_p, hc_uuid); + &match, ofpacts_p, &localnet_port->header_.uuid); } } @@ -2393,8 +2394,9 @@ physical_handle_flows_for_lport(const struct sbrec_port_binding *pb, struct local_datapath *ldp = get_local_datapath(p_ctx->local_datapaths, pb->datapath->tunnel_key); - if (!strcmp(pb->type, "external")) { - /* External lports have a dependency on the localnet port. + if (!strcmp(pb->type, "external") || + !strcmp(pb->type, "patch") || !strcmp(pb->type, "l3gateway")) { + /* Those lports have a dependency on the localnet port. * We need to remove the flows of the localnet port as well * and re-consider adding the flows for it. */ diff --git a/tests/ovn.at b/tests/ovn.at index 92e1ffadc..40b8e59f8 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -39686,3 +39686,60 @@ OVN_CLEANUP([hv1]) AT_CLEANUP ]) + +OVN_FOR_EACH_NORTHD([ +AT_SETUP([localnet port flows after deletion]) +ovn_start +net_add n1 + +check ovn-nbctl ls-add sw0 + +for i in 1 2; do + check ovn-nbctl lsp-add sw0 sw0-p${i} -- lsp-set-addresses sw0-p${i} "00:00:10:01:02:0${i} 10.0.0.${i}" + sim_add hv${i} + as hv${i} + ovs-vsctl add-br br-phys + ovn_attach n1 br-phys 192.168.0.${i} + ovs-vsctl set open . external_ids:ovn-bridge-mappings=physnet1:br-phys + ovs-vsctl add-port br-int vif${i} -- \ + set Interface vif${i} external-ids:iface-id=sw0-p${i} \ + options:tx_pcap=hv${i}/vif${i}-tx.pcap \ + options:rxq_pcap=hv${i}/vif${i}-rx.pcap +done + +check ovn-nbctl lr-add lr0 +check ovn-nbctl lrp-add lr0 lr0-sw0 00:00:00:00:ff:01 10.0.0.254/24 +check ovn-nbctl lsp-add sw0 sw0-lr0 +check ovn-nbctl lsp-set-type sw0-lr0 router +check ovn-nbctl lsp-set-addresses sw0-lr0 router +check ovn-nbctl lsp-set-options sw0-lr0 router-port=lr0-sw0 + +check ovn-nbctl --wait=hv sync +wait_for_ports_up + +# We should not have any flows in table OFTABLE_PHY_TO_LOG from in_port different from vif1 and ovn-hv2-0 +OVN_WAIT_REMOTE_INPUT_FLOWS(["hv1"],["hv2"]) +of1=$(as hv1 ovs-vsctl --bare --columns ofport find Interface name=vif1) +of2=$(as hv1 ovs-vsctl --bare --columns ofport find Interface name=ovn-hv2-0) +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=OFTABLE_PHY_TO_LOG | grep -v NXST_FLOW | grep "in_port=" | grep -v "in_port=$of1" | grep -v "in_port=$of2" | wc -l], [0], [dnl +0 +]) + +# Add localnet port to sw0 +check ovn-nbctl lsp-add sw0 ln-sw0 -- lsp-set-addresses ln-sw0 unknown -- lsp-set-type ln-sw0 localnet +check ovn-nbctl --wait=hv lsp-set-options ln-sw0 network_name=physnet1 -- set logical_switch_port ln-sw0 tag_request=100 + +OVN_WAIT_PATCH_PORT_FLOWS(["ln-sw0"], ["hv1"]) +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=OFTABLE_PHY_TO_LOG | grep -v NXST_FLOW | grep "in_port=" | grep -v "in_port=$of1" | grep -v "in_port=$of2" | wc -l], [0], [dnl +3 +]) + +# Remove localnet port from sw0. Peer-ports flows should be deleted. +check ovn-nbctl --wait=hv lsp-del ln-sw0 +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=OFTABLE_PHY_TO_LOG | grep -v NXST_FLOW | grep "in_port=" | grep -v "in_port=$of1" | grep -v "in_port=$of2" | wc -l], [0], [dnl +0 +]) + +OVN_CLEANUP([hv1],[hv2]) +AT_CLEANUP +])
Delete flows on localnet port deletion, and add localnet related flows when peer ports are added. This was properly done when recomputing, but not when doing IP. When peer ports are added, some flows such as chassis_mac flows must be added. Fixes: edc064b4c589 ("controller: Properly handle localnet flows in I+P.") Fixes: d276728a8ead ("Revert "controller: Properly handle localnet flows in I+P.".") Signed-off-by: Xavier Simonart <xsimonar@redhat.com> Acked-by: Ales Musil <amusil@redhat.com> Signed-off-by: Numan Siddique <numans@ovn.org> Signed-off-by: Xavier Simonart <xsimonar@redhat.com> --- controller/physical.c | 10 +++++--- tests/ovn.at | 57 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+), 4 deletions(-)