| Message ID | 20241014101046.3309426-6-xsimonar@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Peer ports. | 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 | fail | github build: failed |
On Mon, Oct 14, 2024 at 12:10 PM Xavier Simonart <xsimonar@redhat.com> wrote: > Before this patch, peer ports might be missing if, at the time of > adding the datapath to local the port is not yet fully claimed > (i.e. pb->chassis still empty) > > Signed-off-by: Xavier Simonart <xsimonar@redhat.com> > > --- > v2: - Rebased on main > - Addressed Ales' comment: > - Updated tests > - Use OVN_WAIT_PATCH_PORT_FLOWS macro). > - Use check for ovn-nbctl. > - Removed unused switches & leftovers. > > Signed-off-by: Xavier Simonart <xsimonar@redhat.com> > --- > controller/binding.c | 48 ++++++++++++---- > controller/local_data.c | 6 +- > tests/system-ovn.at | 119 ++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 162 insertions(+), 11 deletions(-) > > diff --git a/controller/binding.c b/controller/binding.c > index 492aef530..5b33468ab 100644 > --- a/controller/binding.c > +++ b/controller/binding.c > @@ -1871,14 +1871,35 @@ consider_nonvif_lport_(const struct > sbrec_port_binding *pb, > struct binding_ctx_in *b_ctx_in, > struct binding_ctx_out *b_ctx_out) > { > + struct local_datapath *ld = > + get_local_datapath(b_ctx_out->local_datapaths, > + pb->datapath->tunnel_key); > + > if (our_chassis) { > update_local_lports(pb->logical_port, b_ctx_out); > - add_local_datapath(b_ctx_in->sbrec_datapath_binding_by_key, > - b_ctx_in->sbrec_port_binding_by_datapath, > - b_ctx_in->sbrec_port_binding_by_name, > - pb->datapath, b_ctx_in->chassis_rec, > - b_ctx_out->local_datapaths, > - b_ctx_out->tracked_dp_bindings); > + if (!ld) { > + add_local_datapath(b_ctx_in->sbrec_datapath_binding_by_key, > + b_ctx_in->sbrec_port_binding_by_datapath, > + b_ctx_in->sbrec_port_binding_by_name, > + pb->datapath, b_ctx_in->chassis_rec, > + b_ctx_out->local_datapaths, > + b_ctx_out->tracked_dp_bindings); > + } else { > + /* Add the peer datapath to the local datapaths if it's > + * not present yet. > + */ > + if (need_add_peer_to_local( > + b_ctx_in->sbrec_port_binding_by_name, pb, > + b_ctx_in->chassis_rec)) { > > nit: It could be combined into "else if" to avoid the second nesting. > + add_local_datapath_peer_port( > + pb, b_ctx_in->chassis_rec, > + b_ctx_in->sbrec_datapath_binding_by_key, > + b_ctx_in->sbrec_port_binding_by_datapath, > + b_ctx_in->sbrec_port_binding_by_name, > + ld, b_ctx_out->local_datapaths, > + b_ctx_out->tracked_dp_bindings); > + } > + } > > update_related_lport(pb, b_ctx_out); > return claim_lport(pb, NULL, b_ctx_in->chassis_rec, NULL, > @@ -1895,10 +1916,17 @@ consider_nonvif_lport_(const struct > sbrec_port_binding *pb, > || is_additional_chassis(pb, b_ctx_in->chassis_rec) > || if_status_is_port_claimed(b_ctx_out->if_mgr, > pb->logical_port)) { > - return release_lport(pb, b_ctx_in->chassis_rec, > - !b_ctx_in->ovnsb_idl_txn, > - b_ctx_out->tracked_dp_bindings, > - b_ctx_out->if_mgr); > + if (!release_lport(pb, b_ctx_in->chassis_rec, > + !b_ctx_in->ovnsb_idl_txn, > + b_ctx_out->tracked_dp_bindings, > + b_ctx_out->if_mgr)) { > + return false; > + } > + > + if (ld) { > + remove_local_datapath_peer_port(pb, ld, > + b_ctx_out->local_datapaths); > + } > } > > return true; > diff --git a/controller/local_data.c b/controller/local_data.c > index f1a7ce00e..e12523d61 100644 > --- a/controller/local_data.c > +++ b/controller/local_data.c > @@ -208,7 +208,11 @@ add_local_datapath_peer_port( > struct hmap *tracked_datapaths) > { > const struct sbrec_port_binding *peer; > - peer = lport_get_peer(pb, sbrec_port_binding_by_name); > + if (!strcmp(pb->type, "l3gateway")) { > nit: Extra space > + peer = lport_get_l3gw_peer(pb, sbrec_port_binding_by_name); > + } else { > + peer = lport_get_peer(pb, sbrec_port_binding_by_name); > + } > > if (!peer) { > return; > diff --git a/tests/system-ovn.at b/tests/system-ovn.at > index 861b1cb99..a23471bf8 100644 > --- a/tests/system-ovn.at > +++ b/tests/system-ovn.at > @@ -13929,3 +13929,122 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/.*error > receiving.*/d > /.*terminating with signal 15.*/d"]) > AT_CLEANUP > ]) > + > +OVN_FOR_EACH_NORTHD([ > +AT_SETUP([IPv6 prefix delegation - l3gateway ports creation]) > +AT_SKIP_IF([test $HAVE_DHCPD = no]) > +AT_SKIP_IF([test $HAVE_TCPDUMP = no]) > +AT_KEYWORDS([ovn-ipv6-prefix_d]) > +ovn_start > +OVS_TRAFFIC_VSWITCHD_START() > + > +ADD_BR([br-int]) > +ADD_BR([br-ext]) > + > +ovs-vsctl set-fail-mode br-ext standalone > +# Set external-ids in br-int needed for ovn-controller > +ovs-vsctl \ > + -- set Open_vSwitch . external-ids:system-id=hv1 \ > + -- set Open_vSwitch . > external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \ > + -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \ > + -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \ > + -- set bridge br-int fail-mode=secure > other-config:disable-in-band=true > + > +# Start ovn-controller > +start_daemon ovn-controller > + > +ADD_NAMESPACES(server) > +ADD_VETH(s1, server, br-ext, "2001:1db8:3333::2/64", "f0:00:00:01:02:05", > \ > + "2001:1db8:3333::1", "nodad") > + > +ovn-nbctl create Logical_Router name=R1 options:chassis=hv1 > + > +check ovn-nbctl ls-add sw0 > +check ovn-nbctl ls-add public > + > +# Add localnet port on public and router ports between R1 and public. > +# This should create the patch ports. > +# This is done early in the test as this used to cause some recomputes > (due > +# to runtime data handler for ovs_interface). > +# Hence, we avoid such recompute later in the test, so we only rely on > I+P. > +AT_CHECK([ovs-vsctl set Open_vSwitch . > external-ids:ovn-bridge-mappings=phynet:br-ext]) > +check ovn-nbctl lsp-add public ln1 \ > + -- lsp-set-addresses ln1 unknown \ > + -- lsp-set-type ln1 localnet \ > + -- lsp-set-options ln1 network_name=phynet > + > +check ovn-nbctl lrp-add R1 rp-public 00:00:02:01:02:03 172.16.1.1/24 > +check ovn-nbctl lsp-add public public-rp -- set Logical_Switch_Port > public-rp \ > + type=router options:router-port=rp-public \ > + -- lsp-set-addresses public-rp router > + > +# Wait for a flow outputing to patch port, so we know ovn-controller > handled the patch port creation in OVS. > +OVN_WAIT_PATCH_PORT_FLOWS(["ln1"], ["./"]) > + > +check ovn-nbctl lrp-add R1 rp-sw0 00:00:01:01:02:03 192.168.1.1/24 > + > +check ovn-nbctl lsp-add sw0 sw0-rp -- set Logical_Switch_Port sw0-rp \ > + type=router options:router-port=rp-sw0 \ > + -- lsp-set-addresses sw0-rp router > +check ovn-nbctl lsp-add sw0 sw01 \ > + -- lsp-set-addresses sw01 "f0:00:00:01:02:03 192.168.1.2" > + > +check ovn-nbctl set logical_router_port rp-public > options:prefix_delegation=true > +check ovn-nbctl set logical_router_port rp-public options:prefix=true > +check ovn-nbctl set logical_router_port rp-sw0 options:prefix=true > + > +OVN_POPULATE_ARP > + > +check ovn-nbctl --wait=hv sync > + > +cat > /etc/dhcp/dhcpd.conf <<EOF > +option dhcp-rebinding-time 10; > +option dhcp-renewal-time 5; > +subnet6 2001:1db8:3333::/64 { > + prefix6 2001:1db8:3333:100:: 2001:1db8:3333:111:: /96; > +} > +EOF > +rm -f /var/lib/dhcp/dhcpd6.leases > +touch /var/lib/dhcp/dhcpd6.leases > +chown root:dhcpd /var/lib/dhcp /var/lib/dhcp/dhcpd6.leases > +chmod 775 /var/lib/dhcp > +chmod 664 /var/lib/dhcp/dhcpd6.leases > + > +NETNS_START_TCPDUMP([server], [-nni s1], [server]) > +NETNS_DAEMONIZE([server], [dhcpd -6 -f s1 > dhcpd.log 2>&1], [dhcpd.pid]) > +ovn-nbctl --wait=hv sync > + > +AT_CHECK([ovn-appctl debug/dump-peer-ports | sort], [0], [dnl > +dp R1 : local = rp-public, remote = public-rp > +dp R1 : local = rp-sw0, remote = sw0-rp > +dp public : local = public-rp, remote = rp-public > +dp sw0 : local = sw0-rp, remote = rp-sw0 > +]) > + > +OVS_WAIT_WHILE([test "$(ovn-nbctl get logical_router_port rp-public > ipv6_prefix | cut -c4-15)" = ""]) > +OVS_WAIT_WHILE([test "$(ovn-nbctl get logical_router_port rp-sw0 > ipv6_prefix | cut -c4-15)" = ""]) > + > +AT_CHECK([ovn-nbctl get logical_router_port rp-public ipv6_prefix | cut > -c3-16], [0], [dnl > +[2001:1db8:3333] > +]) > +AT_CHECK([ovn-nbctl get logical_router_port rp-sw0 ipv6_prefix | cut > -c3-16], [0], [dnl > +[2001:1db8:3333] > +]) > + > +OVS_APP_EXIT_AND_WAIT([ovn-controller]) > + > +as ovn-sb > +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > + > +as ovn-nb > +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > + > +as northd > +OVS_APP_EXIT_AND_WAIT([ovn-northd]) > + > +as > +OVS_TRAFFIC_VSWITCHD_STOP(["/.*error receiving.*/d > +/failed to query port patch-.*/d > +/.*terminating with signal 15.*/d"]) > +AT_CLEANUP > +]) > -- > 2.31.1 > > Other than that it looks good. Acked-by: Ales Musil <amusil@redhat.com>
diff --git a/controller/binding.c b/controller/binding.c index 492aef530..5b33468ab 100644 --- a/controller/binding.c +++ b/controller/binding.c @@ -1871,14 +1871,35 @@ consider_nonvif_lport_(const struct sbrec_port_binding *pb, struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out) { + struct local_datapath *ld = + get_local_datapath(b_ctx_out->local_datapaths, + pb->datapath->tunnel_key); + if (our_chassis) { update_local_lports(pb->logical_port, b_ctx_out); - add_local_datapath(b_ctx_in->sbrec_datapath_binding_by_key, - b_ctx_in->sbrec_port_binding_by_datapath, - b_ctx_in->sbrec_port_binding_by_name, - pb->datapath, b_ctx_in->chassis_rec, - b_ctx_out->local_datapaths, - b_ctx_out->tracked_dp_bindings); + if (!ld) { + add_local_datapath(b_ctx_in->sbrec_datapath_binding_by_key, + b_ctx_in->sbrec_port_binding_by_datapath, + b_ctx_in->sbrec_port_binding_by_name, + pb->datapath, b_ctx_in->chassis_rec, + b_ctx_out->local_datapaths, + b_ctx_out->tracked_dp_bindings); + } else { + /* Add the peer datapath to the local datapaths if it's + * not present yet. + */ + if (need_add_peer_to_local( + b_ctx_in->sbrec_port_binding_by_name, pb, + b_ctx_in->chassis_rec)) { + add_local_datapath_peer_port( + pb, b_ctx_in->chassis_rec, + b_ctx_in->sbrec_datapath_binding_by_key, + b_ctx_in->sbrec_port_binding_by_datapath, + b_ctx_in->sbrec_port_binding_by_name, + ld, b_ctx_out->local_datapaths, + b_ctx_out->tracked_dp_bindings); + } + } update_related_lport(pb, b_ctx_out); return claim_lport(pb, NULL, b_ctx_in->chassis_rec, NULL, @@ -1895,10 +1916,17 @@ consider_nonvif_lport_(const struct sbrec_port_binding *pb, || is_additional_chassis(pb, b_ctx_in->chassis_rec) || if_status_is_port_claimed(b_ctx_out->if_mgr, pb->logical_port)) { - return release_lport(pb, b_ctx_in->chassis_rec, - !b_ctx_in->ovnsb_idl_txn, - b_ctx_out->tracked_dp_bindings, - b_ctx_out->if_mgr); + if (!release_lport(pb, b_ctx_in->chassis_rec, + !b_ctx_in->ovnsb_idl_txn, + b_ctx_out->tracked_dp_bindings, + b_ctx_out->if_mgr)) { + return false; + } + + if (ld) { + remove_local_datapath_peer_port(pb, ld, + b_ctx_out->local_datapaths); + } } return true; diff --git a/controller/local_data.c b/controller/local_data.c index f1a7ce00e..e12523d61 100644 --- a/controller/local_data.c +++ b/controller/local_data.c @@ -208,7 +208,11 @@ add_local_datapath_peer_port( struct hmap *tracked_datapaths) { const struct sbrec_port_binding *peer; - peer = lport_get_peer(pb, sbrec_port_binding_by_name); + if (!strcmp(pb->type, "l3gateway")) { + peer = lport_get_l3gw_peer(pb, sbrec_port_binding_by_name); + } else { + peer = lport_get_peer(pb, sbrec_port_binding_by_name); + } if (!peer) { return; diff --git a/tests/system-ovn.at b/tests/system-ovn.at index 861b1cb99..a23471bf8 100644 --- a/tests/system-ovn.at +++ b/tests/system-ovn.at @@ -13929,3 +13929,122 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/.*error receiving.*/d /.*terminating with signal 15.*/d"]) AT_CLEANUP ]) + +OVN_FOR_EACH_NORTHD([ +AT_SETUP([IPv6 prefix delegation - l3gateway ports creation]) +AT_SKIP_IF([test $HAVE_DHCPD = no]) +AT_SKIP_IF([test $HAVE_TCPDUMP = no]) +AT_KEYWORDS([ovn-ipv6-prefix_d]) +ovn_start +OVS_TRAFFIC_VSWITCHD_START() + +ADD_BR([br-int]) +ADD_BR([br-ext]) + +ovs-vsctl set-fail-mode br-ext standalone +# Set external-ids in br-int needed for ovn-controller +ovs-vsctl \ + -- set Open_vSwitch . external-ids:system-id=hv1 \ + -- set Open_vSwitch . external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \ + -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \ + -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \ + -- set bridge br-int fail-mode=secure other-config:disable-in-band=true + +# Start ovn-controller +start_daemon ovn-controller + +ADD_NAMESPACES(server) +ADD_VETH(s1, server, br-ext, "2001:1db8:3333::2/64", "f0:00:00:01:02:05", \ + "2001:1db8:3333::1", "nodad") + +ovn-nbctl create Logical_Router name=R1 options:chassis=hv1 + +check ovn-nbctl ls-add sw0 +check ovn-nbctl ls-add public + +# Add localnet port on public and router ports between R1 and public. +# This should create the patch ports. +# This is done early in the test as this used to cause some recomputes (due +# to runtime data handler for ovs_interface). +# Hence, we avoid such recompute later in the test, so we only rely on I+P. +AT_CHECK([ovs-vsctl set Open_vSwitch . external-ids:ovn-bridge-mappings=phynet:br-ext]) +check ovn-nbctl lsp-add public ln1 \ + -- lsp-set-addresses ln1 unknown \ + -- lsp-set-type ln1 localnet \ + -- lsp-set-options ln1 network_name=phynet + +check ovn-nbctl lrp-add R1 rp-public 00:00:02:01:02:03 172.16.1.1/24 +check ovn-nbctl lsp-add public public-rp -- set Logical_Switch_Port public-rp \ + type=router options:router-port=rp-public \ + -- lsp-set-addresses public-rp router + +# Wait for a flow outputing to patch port, so we know ovn-controller handled the patch port creation in OVS. +OVN_WAIT_PATCH_PORT_FLOWS(["ln1"], ["./"]) + +check ovn-nbctl lrp-add R1 rp-sw0 00:00:01:01:02:03 192.168.1.1/24 + +check ovn-nbctl lsp-add sw0 sw0-rp -- set Logical_Switch_Port sw0-rp \ + type=router options:router-port=rp-sw0 \ + -- lsp-set-addresses sw0-rp router +check ovn-nbctl lsp-add sw0 sw01 \ + -- lsp-set-addresses sw01 "f0:00:00:01:02:03 192.168.1.2" + +check ovn-nbctl set logical_router_port rp-public options:prefix_delegation=true +check ovn-nbctl set logical_router_port rp-public options:prefix=true +check ovn-nbctl set logical_router_port rp-sw0 options:prefix=true + +OVN_POPULATE_ARP + +check ovn-nbctl --wait=hv sync + +cat > /etc/dhcp/dhcpd.conf <<EOF +option dhcp-rebinding-time 10; +option dhcp-renewal-time 5; +subnet6 2001:1db8:3333::/64 { + prefix6 2001:1db8:3333:100:: 2001:1db8:3333:111:: /96; +} +EOF +rm -f /var/lib/dhcp/dhcpd6.leases +touch /var/lib/dhcp/dhcpd6.leases +chown root:dhcpd /var/lib/dhcp /var/lib/dhcp/dhcpd6.leases +chmod 775 /var/lib/dhcp +chmod 664 /var/lib/dhcp/dhcpd6.leases + +NETNS_START_TCPDUMP([server], [-nni s1], [server]) +NETNS_DAEMONIZE([server], [dhcpd -6 -f s1 > dhcpd.log 2>&1], [dhcpd.pid]) +ovn-nbctl --wait=hv sync + +AT_CHECK([ovn-appctl debug/dump-peer-ports | sort], [0], [dnl +dp R1 : local = rp-public, remote = public-rp +dp R1 : local = rp-sw0, remote = sw0-rp +dp public : local = public-rp, remote = rp-public +dp sw0 : local = sw0-rp, remote = rp-sw0 +]) + +OVS_WAIT_WHILE([test "$(ovn-nbctl get logical_router_port rp-public ipv6_prefix | cut -c4-15)" = ""]) +OVS_WAIT_WHILE([test "$(ovn-nbctl get logical_router_port rp-sw0 ipv6_prefix | cut -c4-15)" = ""]) + +AT_CHECK([ovn-nbctl get logical_router_port rp-public ipv6_prefix | cut -c3-16], [0], [dnl +[2001:1db8:3333] +]) +AT_CHECK([ovn-nbctl get logical_router_port rp-sw0 ipv6_prefix | cut -c3-16], [0], [dnl +[2001:1db8:3333] +]) + +OVS_APP_EXIT_AND_WAIT([ovn-controller]) + +as ovn-sb +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) + +as ovn-nb +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) + +as northd +OVS_APP_EXIT_AND_WAIT([ovn-northd]) + +as +OVS_TRAFFIC_VSWITCHD_STOP(["/.*error receiving.*/d +/failed to query port patch-.*/d +/.*terminating with signal 15.*/d"]) +AT_CLEANUP +])