| Message ID | 20220527073040.519706-1-amusil@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | None | expand |
On Fri, May 27, 2022 at 3:38 AM Ales Musil <amusil@redhat.com> wrote: > > When localport is removed from NB, and it is the last port > remaining on the host, it is not part of local datapath > anymore. Which can cause troubles when there is recompute > happening in between the removal from NB and the removal > of interface from host. The localport would stay in lport_ids > set, so any new localport that happens to have the same unique > lport key wouldn't be initiliazed properly thorugh I-P. > > Remove the localport port binding from local datapath > if it was part of the that said datapath before. > > Reported-at: https://bugzilla.redhat.com/2076604 > Signed-off-by: Ales Musil <amusil@redhat.com> Thanks for fixing these issues. I applied both the patches to the main branch and backported upto branch-21.12 Numan > --- > v2: Add a test case for the scenario and rebase on newer main > v3: Fix flakiness of the new test > --- > controller/binding.c | 9 +++++++ > tests/ovn-controller.at | 56 +++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 65 insertions(+) > > diff --git a/controller/binding.c b/controller/binding.c > index 92baebd29..a900c9a50 100644 > --- a/controller/binding.c > +++ b/controller/binding.c > @@ -2131,6 +2131,15 @@ handle_deleted_lport(const struct sbrec_port_binding *pb, > return; > } > > + /* > + * Remove localport that was part of local datapath that is not > + * considered to be local anymore. > + */ > + if (!ld && !strcmp(pb->type, "localport") && > + sset_find(&b_ctx_out->related_lports->lport_names, pb->logical_port)) { > + remove_related_lport(pb, b_ctx_out); > + } > + > /* If the binding is not local, if 'pb' is a L3 gateway port, we should > * remove its peer, if that one is local. > */ > diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at > index 3ca59f7e0..71463187b 100644 > --- a/tests/ovn-controller.at > +++ b/tests/ovn-controller.at > @@ -2186,3 +2186,59 @@ AT_CHECK([grep "Parsing of ovn-chassis-mac-mappings failed" hv1/ovn-controller.l > > OVN_CLEANUP([hv1]) > AT_CLEANUP > + > +AT_SETUP([ovn-controller - localport can be recreated]) > + > +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 > + > +port_binding_cookie() { > + name=$1 > + ovn-sbctl --bare --columns _uuid find port_binding logical_port=$name |\ > + cut -d '-' -f 1 | tr -d '\n' | sed 's/^0\{0,8\}//' > +} > + > +create_localport() { > + AT_CHECK([ovn-nbctl lsp-add ls0 metadata]) > + AT_CHECK([ovn-nbctl lsp-set-type metadata localport]) > + AT_CHECK([ovn-nbctl lsp-set-addresses metadata "00:00:00:00:10:25 192.168.10.25"]) > +} > + > +bind_ports() { > + AT_CHECK([ovs-vsctl add-port br-int vm0 -- set interface vm0 type=internal external_ids:iface-id=vm0]) > + AT_CHECK([ovs-vsctl add-port br-int metadata -- set interface metadata type=internal external_ids:iface-id=metadata]) > +} > + > +# Create one VIF and localport and bind it to chassis > +AT_CHECK([ovn-nbctl ls-add ls0]) > +AT_CHECK([ovn-nbctl lsp-add ls0 vm0]) > +AT_CHECK([ovn-nbctl lsp-set-addresses vm0 "00:00:00:00:10:10 192.168.10.10"]) > +create_localport > +bind_ports > + > +# Check that localport has all physical flows defined > +OVS_WAIT_UNTIL([test 6 = $(as hv1 ovs-ofctl dump-flows br-int | grep -c $(port_binding_cookie metadata))]) > + > +# Remove ls0 from local datapaths > +AT_CHECK([ovs-vsctl del-port br-int vm0]) > +AT_CHECK([ovn-appctl inc-engine/recompute]) > + > +# Check that localports physical flows are removed > +OVS_WAIT_UNTIL([test 0 = $(as hv1 ovs-ofctl dump-flows br-int | grep -c $(port_binding_cookie metadata))]) > + > +# The order is impotant, if the port is removed first, the bug wouldn't be triggered > +AT_CHECK([ovn-nbctl lsp-del metadata]) > +AT_CHECK([ovs-vsctl del-port br-int metadata]) > +create_localport > +bind_ports > + > +# Check that localport has all physical flows re-defined > +OVS_WAIT_UNTIL([test 6 = $(as hv1 ovs-ofctl dump-flows br-int | grep -c $(port_binding_cookie metadata))]) > + > +OVN_CLEANUP([hv1]) > +AT_CLEANUP > -- > 2.35.3 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
diff --git a/controller/binding.c b/controller/binding.c index 92baebd29..a900c9a50 100644 --- a/controller/binding.c +++ b/controller/binding.c @@ -2131,6 +2131,15 @@ handle_deleted_lport(const struct sbrec_port_binding *pb, return; } + /* + * Remove localport that was part of local datapath that is not + * considered to be local anymore. + */ + if (!ld && !strcmp(pb->type, "localport") && + sset_find(&b_ctx_out->related_lports->lport_names, pb->logical_port)) { + remove_related_lport(pb, b_ctx_out); + } + /* If the binding is not local, if 'pb' is a L3 gateway port, we should * remove its peer, if that one is local. */ diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at index 3ca59f7e0..71463187b 100644 --- a/tests/ovn-controller.at +++ b/tests/ovn-controller.at @@ -2186,3 +2186,59 @@ AT_CHECK([grep "Parsing of ovn-chassis-mac-mappings failed" hv1/ovn-controller.l OVN_CLEANUP([hv1]) AT_CLEANUP + +AT_SETUP([ovn-controller - localport can be recreated]) + +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 + +port_binding_cookie() { + name=$1 + ovn-sbctl --bare --columns _uuid find port_binding logical_port=$name |\ + cut -d '-' -f 1 | tr -d '\n' | sed 's/^0\{0,8\}//' +} + +create_localport() { + AT_CHECK([ovn-nbctl lsp-add ls0 metadata]) + AT_CHECK([ovn-nbctl lsp-set-type metadata localport]) + AT_CHECK([ovn-nbctl lsp-set-addresses metadata "00:00:00:00:10:25 192.168.10.25"]) +} + +bind_ports() { + AT_CHECK([ovs-vsctl add-port br-int vm0 -- set interface vm0 type=internal external_ids:iface-id=vm0]) + AT_CHECK([ovs-vsctl add-port br-int metadata -- set interface metadata type=internal external_ids:iface-id=metadata]) +} + +# Create one VIF and localport and bind it to chassis +AT_CHECK([ovn-nbctl ls-add ls0]) +AT_CHECK([ovn-nbctl lsp-add ls0 vm0]) +AT_CHECK([ovn-nbctl lsp-set-addresses vm0 "00:00:00:00:10:10 192.168.10.10"]) +create_localport +bind_ports + +# Check that localport has all physical flows defined +OVS_WAIT_UNTIL([test 6 = $(as hv1 ovs-ofctl dump-flows br-int | grep -c $(port_binding_cookie metadata))]) + +# Remove ls0 from local datapaths +AT_CHECK([ovs-vsctl del-port br-int vm0]) +AT_CHECK([ovn-appctl inc-engine/recompute]) + +# Check that localports physical flows are removed +OVS_WAIT_UNTIL([test 0 = $(as hv1 ovs-ofctl dump-flows br-int | grep -c $(port_binding_cookie metadata))]) + +# The order is impotant, if the port is removed first, the bug wouldn't be triggered +AT_CHECK([ovn-nbctl lsp-del metadata]) +AT_CHECK([ovs-vsctl del-port br-int metadata]) +create_localport +bind_ports + +# Check that localport has all physical flows re-defined +OVS_WAIT_UNTIL([test 6 = $(as hv1 ovs-ofctl dump-flows br-int | grep -c $(port_binding_cookie metadata))]) + +OVN_CLEANUP([hv1]) +AT_CLEANUP
When localport is removed from NB, and it is the last port remaining on the host, it is not part of local datapath anymore. Which can cause troubles when there is recompute happening in between the removal from NB and the removal of interface from host. The localport would stay in lport_ids set, so any new localport that happens to have the same unique lport key wouldn't be initiliazed properly thorugh I-P. Remove the localport port binding from local datapath if it was part of the that said datapath before. Reported-at: https://bugzilla.redhat.com/2076604 Signed-off-by: Ales Musil <amusil@redhat.com> --- v2: Add a test case for the scenario and rebase on newer main v3: Fix flakiness of the new test --- controller/binding.c | 9 +++++++ tests/ovn-controller.at | 56 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+)