Re: [ovs-dev] [PATCH ovn 1/2] pinctrl: Fix missing MAC_Bindings.
Hi
Thanks Ales for the feedback.
There is also a small change needed to make userspace tests to work as well
(as pointed out by ovs_robot).
I'll send a v2.
Thanks
Xavier
On Wed, Mar 20, 2024 at 12:11 PM Ales Musil wrote:
>
>
> On Wed, Mar 20, 2024 at 8:12 AM Xavier Simonart
> wrote:
>
>> Pinctrl is responsible of creating MAC_Bindings on peer router datapaths.
>> However, when sb was read-only, this did not happen.
>> This caused the test "neighbor update on same HV" to fail in a flaky way.
>>
>> Signed-off-by: Xavier Simonart
>> ---
>>
>
> Hi Xavier,
>
> thank you for the patch. I have one comment down below.
>
> controller/pinctrl.c | 2 +-
>> tests/ovn-macros.at | 10 +++-
>> tests/system-ovn.at | 127 +++
>> 3 files changed, 137 insertions(+), 2 deletions(-)
>>
>> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
>> index 2d3595cd2..f75b04696 100644
>> --- a/controller/pinctrl.c
>> +++ b/controller/pinctrl.c
>> @@ -4711,7 +4711,7 @@ send_garp_rarp_update(struct ovsdb_idl_txn
>> *ovnsb_idl_txn,
>> garp_rarp->announce_time = time_msec() + 1000;
>> garp_rarp->backoff = 1000; /* msec. */
>> }
>> -} else {
>> +} else if (ovnsb_idl_txn) {
>> add_garp_rarp(name, laddrs->ea,
>>laddrs->ipv4_addrs[i].addr,
>>binding_rec->datapath->tunnel_key,
>> diff --git a/tests/ovn-macros.at b/tests/ovn-macros.at
>> index ed93764d3..aaa8824cb 100644
>> --- a/tests/ovn-macros.at
>> +++ b/tests/ovn-macros.at
>> @@ -220,12 +220,14 @@ ovn_start_northd() {
>> # options are accepted to adjust that:
>> # --backup-northd Start a backup northd.
>> # --backup-northd=paused Start the backup northd in the paused state.
>> +# --use-tcp-to-sb Use tcp to connect to sb.
>> ovn_start () {
>> local backup_northd=false
>> local backup_northd_options=
>> case $1 in
>> --backup-northd) backup_northd=true; shift ;;
>> --backup-northd=paused) backup_northd=true;
>> backup_northd_options=--paused; shift ;;
>> +--use-tcp-to-sb) use_tcp=true; shift ;;
>> esac
>> local AZ=$1
>> local msg_prefix=${AZ:+$AZ: }
>> @@ -246,7 +248,13 @@ ovn_start () {
>> ovn_start_northd $backup_northd_options backup $AZ
>> fi
>>
>> -if test X$HAVE_OPENSSL = Xyes; then
>> +if test $use_tcp; then
>> +# Create the SB DB ptcp connection.
>> +ovn-sbctl \
>> +-- --id=@c create connection \
>> +target=\"ptcp:0:127.0.0.1\" \
>> +-- add SB_Global . connections @c
>> +elif test X$HAVE_OPENSSL = Xyes; then
>> # Create the SB DB pssl+RBAC connection.
>> ovn-sbctl \
>> -- --id=@c create connection \
>> diff --git a/tests/system-ovn.at b/tests/system-ovn.at
>> index 54d913c0b..20ddb487f 100644
>> --- a/tests/system-ovn.at
>> +++ b/tests/system-ovn.at
>> @@ -12208,3 +12208,130 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query
>> port patch-.*/d
>> /connection dropped.*/d"])
>> AT_CLEANUP
>> ])
>> +
>> +OVN_FOR_EACH_NORTHD([
>> +AT_SETUP([MAC_Bindings updates on read-only sb])
>> +ovn_start --use-tcp-to-sb
>> +OVS_TRAFFIC_VSWITCHD_START()
>> +ADD_BR([br-int])
>> +
>> +PARSE_LISTENING_PORT([$ovs_base/ovn-sb/ovsdb-server.log], [TCP_PORT])
>> +
>> +# Use tcp to connect to sb
>> +ovs-vsctl \
>> +-- set Open_vSwitch . external-ids:system-id=hv1 \
>> +-- set Open_vSwitch .
>> external-ids:ovn-remote=tcp:127.0.0.1:$TCP_PORT
>> \
>> +-- 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
>> +
>> +# Logical network:
>> +# A public switch (pub) with a localnet port connected to two LRs (lr0
>> and lr1)
>> +# each with a distributed gateway port.
>> +# Two VMs: lp0 on sw0 connected to lr0
>> +# lp1 on sw1 connected to lr1
>> +#
>> +# This test adds a floating IP on one VM and checks the MAC_Binding
>> entries to be updated properly.
>> +
>> +# By stopping temporarily updates from controller to sb, we are making
>> sb read-only.
>> +# We can't just pause sb to make it read-only, as we expect sb to still
>> handle northd changes.
>> +stop_ovsdb_controller_updates() {
>> + TCP_PORT=$1
>> + echo Stopping updates from ovn-controller to ovsdb using port $TCP_PORT
>> + on_exit 'iptables -C INPUT -p tcp --destination-port $TCP_PORT -j DROP
>> 2>/dev/null && iptables -D INPUT -p tcp --destination-port $TCP_PORT -j
>> DROP'
>> + iptables -A INPUT -p tcp --destination-port $TCP_PORT -j DROP
>>
>
> iptables are not available by default on Fedora (not sure about Ubuntu),
> we should consider using nftables
Re: [ovs-dev] [PATCH ovn 1/2] pinctrl: Fix missing MAC_Bindings.
On Wed, Mar 20, 2024 at 8:12 AM Xavier Simonart wrote:
> Pinctrl is responsible of creating MAC_Bindings on peer router datapaths.
> However, when sb was read-only, this did not happen.
> This caused the test "neighbor update on same HV" to fail in a flaky way.
>
> Signed-off-by: Xavier Simonart
> ---
>
Hi Xavier,
thank you for the patch. I have one comment down below.
controller/pinctrl.c | 2 +-
> tests/ovn-macros.at | 10 +++-
> tests/system-ovn.at | 127 +++
> 3 files changed, 137 insertions(+), 2 deletions(-)
>
> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> index 2d3595cd2..f75b04696 100644
> --- a/controller/pinctrl.c
> +++ b/controller/pinctrl.c
> @@ -4711,7 +4711,7 @@ send_garp_rarp_update(struct ovsdb_idl_txn
> *ovnsb_idl_txn,
> garp_rarp->announce_time = time_msec() + 1000;
> garp_rarp->backoff = 1000; /* msec. */
> }
> -} else {
> +} else if (ovnsb_idl_txn) {
> add_garp_rarp(name, laddrs->ea,
>laddrs->ipv4_addrs[i].addr,
>binding_rec->datapath->tunnel_key,
> diff --git a/tests/ovn-macros.at b/tests/ovn-macros.at
> index ed93764d3..aaa8824cb 100644
> --- a/tests/ovn-macros.at
> +++ b/tests/ovn-macros.at
> @@ -220,12 +220,14 @@ ovn_start_northd() {
> # options are accepted to adjust that:
> # --backup-northd Start a backup northd.
> # --backup-northd=paused Start the backup northd in the paused state.
> +# --use-tcp-to-sb Use tcp to connect to sb.
> ovn_start () {
> local backup_northd=false
> local backup_northd_options=
> case $1 in
> --backup-northd) backup_northd=true; shift ;;
> --backup-northd=paused) backup_northd=true;
> backup_northd_options=--paused; shift ;;
> +--use-tcp-to-sb) use_tcp=true; shift ;;
> esac
> local AZ=$1
> local msg_prefix=${AZ:+$AZ: }
> @@ -246,7 +248,13 @@ ovn_start () {
> ovn_start_northd $backup_northd_options backup $AZ
> fi
>
> -if test X$HAVE_OPENSSL = Xyes; then
> +if test $use_tcp; then
> +# Create the SB DB ptcp connection.
> +ovn-sbctl \
> +-- --id=@c create connection \
> +target=\"ptcp:0:127.0.0.1\" \
> +-- add SB_Global . connections @c
> +elif test X$HAVE_OPENSSL = Xyes; then
> # Create the SB DB pssl+RBAC connection.
> ovn-sbctl \
> -- --id=@c create connection \
> diff --git a/tests/system-ovn.at b/tests/system-ovn.at
> index 54d913c0b..20ddb487f 100644
> --- a/tests/system-ovn.at
> +++ b/tests/system-ovn.at
> @@ -12208,3 +12208,130 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query
> port patch-.*/d
> /connection dropped.*/d"])
> AT_CLEANUP
> ])
> +
> +OVN_FOR_EACH_NORTHD([
> +AT_SETUP([MAC_Bindings updates on read-only sb])
> +ovn_start --use-tcp-to-sb
> +OVS_TRAFFIC_VSWITCHD_START()
> +ADD_BR([br-int])
> +
> +PARSE_LISTENING_PORT([$ovs_base/ovn-sb/ovsdb-server.log], [TCP_PORT])
> +
> +# Use tcp to connect to sb
> +ovs-vsctl \
> +-- set Open_vSwitch . external-ids:system-id=hv1 \
> +-- set Open_vSwitch . external-ids:ovn-remote=tcp:127.0.0.1:$TCP_PORT
> \
> +-- 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
> +
> +# Logical network:
> +# A public switch (pub) with a localnet port connected to two LRs (lr0
> and lr1)
> +# each with a distributed gateway port.
> +# Two VMs: lp0 on sw0 connected to lr0
> +# lp1 on sw1 connected to lr1
> +#
> +# This test adds a floating IP on one VM and checks the MAC_Binding
> entries to be updated properly.
> +
> +# By stopping temporarily updates from controller to sb, we are making sb
> read-only.
> +# We can't just pause sb to make it read-only, as we expect sb to still
> handle northd changes.
> +stop_ovsdb_controller_updates() {
> + TCP_PORT=$1
> + echo Stopping updates from ovn-controller to ovsdb using port $TCP_PORT
> + on_exit 'iptables -C INPUT -p tcp --destination-port $TCP_PORT -j DROP
> 2>/dev/null && iptables -D INPUT -p tcp --destination-port $TCP_PORT -j
> DROP'
> + iptables -A INPUT -p tcp --destination-port $TCP_PORT -j DROP
>
iptables are not available by default on Fedora (not sure about Ubuntu), we
should consider using nftables instead.
(/workspace/ovn/tests/system-kmod-testsuite.dir/at-groups/162/test-source:
line 214: iptables: command not found)
> +}
> +restart_ovsdb_controller_updates() {
> + TCP_PORT=$1
> + echo Restarting updates from ovn-controller to ovsdb
> + iptables -D INPUT -p tcp --destination-port $TCP_PORT -j DROP
> +}
> +
> +# Create logical switches
> +check
[ovs-dev] [PATCH ovn 1/2] pinctrl: Fix missing MAC_Bindings.
Pinctrl is responsible of creating MAC_Bindings on peer router datapaths.
However, when sb was read-only, this did not happen.
This caused the test "neighbor update on same HV" to fail in a flaky way.
Signed-off-by: Xavier Simonart
---
controller/pinctrl.c | 2 +-
tests/ovn-macros.at | 10 +++-
tests/system-ovn.at | 127 +++
3 files changed, 137 insertions(+), 2 deletions(-)
diff --git a/controller/pinctrl.c b/controller/pinctrl.c
index 2d3595cd2..f75b04696 100644
--- a/controller/pinctrl.c
+++ b/controller/pinctrl.c
@@ -4711,7 +4711,7 @@ send_garp_rarp_update(struct ovsdb_idl_txn *ovnsb_idl_txn,
garp_rarp->announce_time = time_msec() + 1000;
garp_rarp->backoff = 1000; /* msec. */
}
-} else {
+} else if (ovnsb_idl_txn) {
add_garp_rarp(name, laddrs->ea,
laddrs->ipv4_addrs[i].addr,
binding_rec->datapath->tunnel_key,
diff --git a/tests/ovn-macros.at b/tests/ovn-macros.at
index ed93764d3..aaa8824cb 100644
--- a/tests/ovn-macros.at
+++ b/tests/ovn-macros.at
@@ -220,12 +220,14 @@ ovn_start_northd() {
# options are accepted to adjust that:
# --backup-northd Start a backup northd.
# --backup-northd=paused Start the backup northd in the paused state.
+# --use-tcp-to-sb Use tcp to connect to sb.
ovn_start () {
local backup_northd=false
local backup_northd_options=
case $1 in
--backup-northd) backup_northd=true; shift ;;
--backup-northd=paused) backup_northd=true;
backup_northd_options=--paused; shift ;;
+--use-tcp-to-sb) use_tcp=true; shift ;;
esac
local AZ=$1
local msg_prefix=${AZ:+$AZ: }
@@ -246,7 +248,13 @@ ovn_start () {
ovn_start_northd $backup_northd_options backup $AZ
fi
-if test X$HAVE_OPENSSL = Xyes; then
+if test $use_tcp; then
+# Create the SB DB ptcp connection.
+ovn-sbctl \
+-- --id=@c create connection \
+target=\"ptcp:0:127.0.0.1\" \
+-- add SB_Global . connections @c
+elif test X$HAVE_OPENSSL = Xyes; then
# Create the SB DB pssl+RBAC connection.
ovn-sbctl \
-- --id=@c create connection \
diff --git a/tests/system-ovn.at b/tests/system-ovn.at
index 54d913c0b..20ddb487f 100644
--- a/tests/system-ovn.at
+++ b/tests/system-ovn.at
@@ -12208,3 +12208,130 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port
patch-.*/d
/connection dropped.*/d"])
AT_CLEANUP
])
+
+OVN_FOR_EACH_NORTHD([
+AT_SETUP([MAC_Bindings updates on read-only sb])
+ovn_start --use-tcp-to-sb
+OVS_TRAFFIC_VSWITCHD_START()
+ADD_BR([br-int])
+
+PARSE_LISTENING_PORT([$ovs_base/ovn-sb/ovsdb-server.log], [TCP_PORT])
+
+# Use tcp to connect to sb
+ovs-vsctl \
+-- set Open_vSwitch . external-ids:system-id=hv1 \
+-- set Open_vSwitch . external-ids:ovn-remote=tcp:127.0.0.1:$TCP_PORT \
+-- 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
+
+# Logical network:
+# A public switch (pub) with a localnet port connected to two LRs (lr0 and lr1)
+# each with a distributed gateway port.
+# Two VMs: lp0 on sw0 connected to lr0
+# lp1 on sw1 connected to lr1
+#
+# This test adds a floating IP on one VM and checks the MAC_Binding entries to
be updated properly.
+
+# By stopping temporarily updates from controller to sb, we are making sb
read-only.
+# We can't just pause sb to make it read-only, as we expect sb to still handle
northd changes.
+stop_ovsdb_controller_updates() {
+ TCP_PORT=$1
+ echo Stopping updates from ovn-controller to ovsdb using port $TCP_PORT
+ on_exit 'iptables -C INPUT -p tcp --destination-port $TCP_PORT -j DROP
2>/dev/null && iptables -D INPUT -p tcp --destination-port $TCP_PORT -j DROP'
+ iptables -A INPUT -p tcp --destination-port $TCP_PORT -j DROP
+}
+restart_ovsdb_controller_updates() {
+ TCP_PORT=$1
+ echo Restarting updates from ovn-controller to ovsdb
+ iptables -D INPUT -p tcp --destination-port $TCP_PORT -j DROP
+}
+
+# Create logical switches
+check ovn-nbctl ls-add sw0
+check ovn-nbctl ls-add sw1
+check ovn-nbctl ls-add pub
+
+# Created localnet port on public switch
+check ovn-nbctl lsp-add pub ln-pub
+check ovn-nbctl lsp-set-type ln-pub localnet
+check ovn-nbctl lsp-set-addresses ln-pub unknown
+check ovn-nbctl lsp-set-options ln-pub network_name=phys
+
+# Create logical routers and connect them to public switch
+AT_CHECK([(ovn-nbctl create Logical_Router name=lr0;
+ ovn-nbctl create Logical_Router name=lr1) | uuidfilt], [0], [<0>
+<1>
+])
+check ovn-nbctl lrp-add lr0 lr0-pub f0:00:00:00:00:01 172.24.4.220/24
+check ovn-nbctl
