Re: [ovs-dev] [PATCH ovn] northd: set svc_mon status to offline if port_binding released

[Vladislav Odintsov]

Thanks Numan.

Regards,
Vladislav Odintsov

> On 7 Jul 2022, at 19:57, Numan Siddique  wrote:
> 
> On Tue, Jul 5, 2022 at 1:54 PM Vladislav Odintsov  > wrote:
>> 
>> This patch fixes situation where a Load Balancer
>> has corresponding Load_Balancer_Health_Check and
>> current checks are successful.  This means
>> Service_Monitor record in SBDB has status == "online".
>> 
>> In case VM (or container) stops and its Port_Binding
>> is released (for instance, by removing interface from
>> OVS, or removing iface-id from interface's external_ids),
>> prior to this patch state remained "online" and affected
>> traffic directing it to dead backend.
>> 
>> Corresponding testcase was updated with described behaviour.
>> 
>> Reported-at: 
>> https://mail.openvswitch.org/pipermail/ovs-dev/2022-July/395504.html
>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2103740
>> Signed-off-by: Vladislav Odintsov 
> 
> Thanks for fixing this issue.  I applied this patch to the main branch
> and backported to branch-22.06, 22.03 and 21.12.
> 
> Numan
> 
>> ---
>> northd/northd.c |  7 +++
>> tests/ovn-northd.at | 33 -
>> tests/system-ovn.at | 11 ++-
>> 3 files changed, 37 insertions(+), 14 deletions(-)
>> 
>> diff --git a/northd/northd.c b/northd/northd.c
>> index 27cc8775c..63b6a73c8 100644
>> --- a/northd/northd.c
>> +++ b/northd/northd.c
>> @@ -3884,6 +3884,13 @@ ovn_lb_svc_create(struct ovsdb_idl_txn *ovnsb_txn, 
>> struct ovn_northd_lb *lb,
>> backend_nb->svc_mon_src_ip);
>> }
>> 
>> +if ((!op->sb->n_up || !op->sb->up[0])
>> +&& mon_info->sbrec_mon->status
>> +&& !strcmp(mon_info->sbrec_mon->status, "online")) {
>> +sbrec_service_monitor_set_status(mon_info->sbrec_mon,
>> + "offline");
>> +}
>> +
>> backend_nb->sbrec_monitor = mon_info->sbrec_mon;
>> mon_info->required = true;
>> }
>> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
>> index fe97bedad..812c60b53 100644
>> --- a/tests/ovn-northd.at
>> +++ b/tests/ovn-northd.at
>> @@ -1207,6 +1207,13 @@ ovn-nbctl ls-add sw1
>> ovn-nbctl --wait=sb lsp-add sw1 sw1-p1 -- lsp-set-addresses sw1-p1 \
>> "02:00:00:00:00:03 20.0.0.3"
>> 
>> +# service_monitors state online requires corresponding port_binding to be 
>> "up"
>> +ovn-sbctl chassis-add hv1 geneve 127.0.0.1
>> +ovn-sbctl lsp-bind sw0-p1 hv1
>> +ovn-sbctl lsp-bind sw1-p1 hv1
>> +wait_row_count nb:Logical_Switch_Port 1 name=sw0-p1 'up=true'
>> +wait_row_count nb:Logical_Switch_Port 1 name=sw1-p1 'up=true'
>> +
>> wait_row_count Service_Monitor 0
>> 
>> ovn-nbctl --wait=sb set load_balancer . 
>> ip_port_mappings:10.0.0.3=sw0-p1:10.0.0.2
>> @@ -1220,7 +1227,7 @@ check ovn-nbctl --wait=sb ls-lb-add sw0 lb1
>> AT_CAPTURE_FILE([sbflows])
>> OVS_WAIT_FOR_OUTPUT(
>>   [ovn-sbctl dump-flows sw0 | tee sbflows | grep 'priority=120.*backends' | 
>> sed 's/table=..//'], 0, [dnl
>> -  (ls_in_lb   ), priority=120  , match=(ct.new && ip4.dst == 
>> 10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; reg1 = 10.0.0.10; 
>> reg2[[0..15]] = 80; ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80);)
>> +  (ls_in_lb   ), priority=120  , match=(ct.new && ip4.dst == 
>> 10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; reg1 = 10.0.0.10; 
>> reg2[[0..15]] = 80; ct_lb(backends=10.0.0.3:80,20.0.0.3:80);)
>> ])
>> 
>> AS_BOX([Delete the Load_Balancer_Health_Check])
>> @@ -1230,7 +1237,7 @@ wait_row_count Service_Monitor 0
>> AT_CAPTURE_FILE([sbflows2])
>> OVS_WAIT_FOR_OUTPUT(
>>   [ovn-sbctl dump-flows sw0 | tee sbflows2 | grep 'priority=120.*backends' | 
>> sed 's/table=..//'], [0],
>> -[  (ls_in_lb   ), priority=120  , match=(ct.new && ip4.dst == 
>> 10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; reg1 = 10.0.0.10; 
>> reg2[[0..15]] = 80; ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80);)
>> +[  (ls_in_lb   ), priority=120  , match=(ct.new && ip4.dst == 
>> 10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; reg1 = 10.0.0.10; 
>> reg2[[0..15]] = 80; ct_lb(backends=10.0.0.3:80,20.0.0.3:80);)
>> ])
>> 
>> AS_BOX([Create the Load_Balancer_Health_Check again.])
>> @@ -1242,7 +1249,7 @@ check ovn-nbctl --wait=sb sync
>> 
>> ovn-sbctl dump-flows sw0 | grep backends | grep priority=120 > lflows.txt
>> AT_CHECK([cat lflows.txt | sed 's/table=..//'], [0], [dnl
>> -  (ls_in_lb   ), priority=120  , match=(ct.new && ip4.dst == 
>> 10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; reg1 = 10.0.0.10; 
>> reg2[[0..15]] = 80; ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80);)
>> +  (ls_in_lb   ), priority=120  , match=(ct.new && ip4.dst == 
>> 10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; reg1 = 10.0.0.10; 
>> reg2[[0..15]] = 80; ct_lb(backends=10.0.0.3:80,20.0.0.3:80);)
>> ])
>> 
>> AS_BOX([Get the uuid of both the service_monitor])
>> @@ -1252,7 

Re: [ovs-dev] [PATCH ovn] northd: set svc_mon status to offline if port_binding released

[Numan Siddique]

On Tue, Jul 5, 2022 at 1:54 PM Vladislav Odintsov  wrote:
>
> This patch fixes situation where a Load Balancer
> has corresponding Load_Balancer_Health_Check and
> current checks are successful.  This means
> Service_Monitor record in SBDB has status == "online".
>
> In case VM (or container) stops and its Port_Binding
> is released (for instance, by removing interface from
> OVS, or removing iface-id from interface's external_ids),
> prior to this patch state remained "online" and affected
> traffic directing it to dead backend.
>
> Corresponding testcase was updated with described behaviour.
>
> Reported-at: 
> https://mail.openvswitch.org/pipermail/ovs-dev/2022-July/395504.html
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2103740
> Signed-off-by: Vladislav Odintsov 

Thanks for fixing this issue.  I applied this patch to the main branch
and backported to branch-22.06, 22.03 and 21.12.

Numan

> ---
>  northd/northd.c |  7 +++
>  tests/ovn-northd.at | 33 -
>  tests/system-ovn.at | 11 ++-
>  3 files changed, 37 insertions(+), 14 deletions(-)
>
> diff --git a/northd/northd.c b/northd/northd.c
> index 27cc8775c..63b6a73c8 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -3884,6 +3884,13 @@ ovn_lb_svc_create(struct ovsdb_idl_txn *ovnsb_txn, 
> struct ovn_northd_lb *lb,
>  backend_nb->svc_mon_src_ip);
>  }
>
> +if ((!op->sb->n_up || !op->sb->up[0])
> +&& mon_info->sbrec_mon->status
> +&& !strcmp(mon_info->sbrec_mon->status, "online")) {
> +sbrec_service_monitor_set_status(mon_info->sbrec_mon,
> + "offline");
> +}
> +
>  backend_nb->sbrec_monitor = mon_info->sbrec_mon;
>  mon_info->required = true;
>  }
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index fe97bedad..812c60b53 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -1207,6 +1207,13 @@ ovn-nbctl ls-add sw1
>  ovn-nbctl --wait=sb lsp-add sw1 sw1-p1 -- lsp-set-addresses sw1-p1 \
>  "02:00:00:00:00:03 20.0.0.3"
>
> +# service_monitors state online requires corresponding port_binding to be 
> "up"
> +ovn-sbctl chassis-add hv1 geneve 127.0.0.1
> +ovn-sbctl lsp-bind sw0-p1 hv1
> +ovn-sbctl lsp-bind sw1-p1 hv1
> +wait_row_count nb:Logical_Switch_Port 1 name=sw0-p1 'up=true'
> +wait_row_count nb:Logical_Switch_Port 1 name=sw1-p1 'up=true'
> +
>  wait_row_count Service_Monitor 0
>
>  ovn-nbctl --wait=sb set load_balancer . 
> ip_port_mappings:10.0.0.3=sw0-p1:10.0.0.2
> @@ -1220,7 +1227,7 @@ check ovn-nbctl --wait=sb ls-lb-add sw0 lb1
>  AT_CAPTURE_FILE([sbflows])
>  OVS_WAIT_FOR_OUTPUT(
>[ovn-sbctl dump-flows sw0 | tee sbflows | grep 'priority=120.*backends' | 
> sed 's/table=..//'], 0, [dnl
> -  (ls_in_lb   ), priority=120  , match=(ct.new && ip4.dst == 
> 10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; reg1 = 10.0.0.10; 
> reg2[[0..15]] = 80; ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80);)
> +  (ls_in_lb   ), priority=120  , match=(ct.new && ip4.dst == 
> 10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; reg1 = 10.0.0.10; 
> reg2[[0..15]] = 80; ct_lb(backends=10.0.0.3:80,20.0.0.3:80);)
>  ])
>
>  AS_BOX([Delete the Load_Balancer_Health_Check])
> @@ -1230,7 +1237,7 @@ wait_row_count Service_Monitor 0
>  AT_CAPTURE_FILE([sbflows2])
>  OVS_WAIT_FOR_OUTPUT(
>[ovn-sbctl dump-flows sw0 | tee sbflows2 | grep 'priority=120.*backends' | 
> sed 's/table=..//'], [0],
> -[  (ls_in_lb   ), priority=120  , match=(ct.new && ip4.dst == 
> 10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; reg1 = 10.0.0.10; 
> reg2[[0..15]] = 80; ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80);)
> +[  (ls_in_lb   ), priority=120  , match=(ct.new && ip4.dst == 
> 10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; reg1 = 10.0.0.10; 
> reg2[[0..15]] = 80; ct_lb(backends=10.0.0.3:80,20.0.0.3:80);)
>  ])
>
>  AS_BOX([Create the Load_Balancer_Health_Check again.])
> @@ -1242,7 +1249,7 @@ check ovn-nbctl --wait=sb sync
>
>  ovn-sbctl dump-flows sw0 | grep backends | grep priority=120 > lflows.txt
>  AT_CHECK([cat lflows.txt | sed 's/table=..//'], [0], [dnl
> -  (ls_in_lb   ), priority=120  , match=(ct.new && ip4.dst == 
> 10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; reg1 = 10.0.0.10; 
> reg2[[0..15]] = 80; ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80);)
> +  (ls_in_lb   ), priority=120  , match=(ct.new && ip4.dst == 
> 10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; reg1 = 10.0.0.10; 
> reg2[[0..15]] = 80; ct_lb(backends=10.0.0.3:80,20.0.0.3:80);)
>  ])
>
>  AS_BOX([Get the uuid of both the service_monitor])
> @@ -1252,7 +1259,7 @@ sm_sw1_p1=$(fetch_column Service_Monitor _uuid 
> logical_port=sw1-p1)
>  AT_CAPTURE_FILE([sbflows3])
>  OVS_WAIT_FOR_OUTPUT(
>[ovn-sbctl dump-flows sw0 | tee sbflows 3 | grep 'priority=120.*backends' 
> | sed 

[ovs-dev] [PATCH ovn] northd: set svc_mon status to offline if port_binding released

[Vladislav Odintsov]

This patch fixes situation where a Load Balancer
has corresponding Load_Balancer_Health_Check and
current checks are successful.  This means
Service_Monitor record in SBDB has status == "online".

In case VM (or container) stops and its Port_Binding
is released (for instance, by removing interface from
OVS, or removing iface-id from interface's external_ids),
prior to this patch state remained "online" and affected
traffic directing it to dead backend.

Corresponding testcase was updated with described behaviour.

Reported-at: 
https://mail.openvswitch.org/pipermail/ovs-dev/2022-July/395504.html
Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2103740
Signed-off-by: Vladislav Odintsov 
---
 northd/northd.c |  7 +++
 tests/ovn-northd.at | 33 -
 tests/system-ovn.at | 11 ++-
 3 files changed, 37 insertions(+), 14 deletions(-)

diff --git a/northd/northd.c b/northd/northd.c
index 27cc8775c..63b6a73c8 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -3884,6 +3884,13 @@ ovn_lb_svc_create(struct ovsdb_idl_txn *ovnsb_txn, 
struct ovn_northd_lb *lb,
 backend_nb->svc_mon_src_ip);
 }
 
+if ((!op->sb->n_up || !op->sb->up[0])
+&& mon_info->sbrec_mon->status
+&& !strcmp(mon_info->sbrec_mon->status, "online")) {
+sbrec_service_monitor_set_status(mon_info->sbrec_mon,
+ "offline");
+}
+
 backend_nb->sbrec_monitor = mon_info->sbrec_mon;
 mon_info->required = true;
 }
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index fe97bedad..812c60b53 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -1207,6 +1207,13 @@ ovn-nbctl ls-add sw1
 ovn-nbctl --wait=sb lsp-add sw1 sw1-p1 -- lsp-set-addresses sw1-p1 \
 "02:00:00:00:00:03 20.0.0.3"
 
+# service_monitors state online requires corresponding port_binding to be "up"
+ovn-sbctl chassis-add hv1 geneve 127.0.0.1
+ovn-sbctl lsp-bind sw0-p1 hv1
+ovn-sbctl lsp-bind sw1-p1 hv1
+wait_row_count nb:Logical_Switch_Port 1 name=sw0-p1 'up=true'
+wait_row_count nb:Logical_Switch_Port 1 name=sw1-p1 'up=true'
+
 wait_row_count Service_Monitor 0
 
 ovn-nbctl --wait=sb set load_balancer . 
ip_port_mappings:10.0.0.3=sw0-p1:10.0.0.2
@@ -1220,7 +1227,7 @@ check ovn-nbctl --wait=sb ls-lb-add sw0 lb1
 AT_CAPTURE_FILE([sbflows])
 OVS_WAIT_FOR_OUTPUT(
   [ovn-sbctl dump-flows sw0 | tee sbflows | grep 'priority=120.*backends' | 
sed 's/table=..//'], 0, [dnl
-  (ls_in_lb   ), priority=120  , match=(ct.new && ip4.dst == 10.0.0.10 
&& tcp.dst == 80), action=(reg0[[1]] = 0; reg1 = 10.0.0.10; reg2[[0..15]] = 80; 
ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80);)
+  (ls_in_lb   ), priority=120  , match=(ct.new && ip4.dst == 10.0.0.10 
&& tcp.dst == 80), action=(reg0[[1]] = 0; reg1 = 10.0.0.10; reg2[[0..15]] = 80; 
ct_lb(backends=10.0.0.3:80,20.0.0.3:80);)
 ])
 
 AS_BOX([Delete the Load_Balancer_Health_Check])
@@ -1230,7 +1237,7 @@ wait_row_count Service_Monitor 0
 AT_CAPTURE_FILE([sbflows2])
 OVS_WAIT_FOR_OUTPUT(
   [ovn-sbctl dump-flows sw0 | tee sbflows2 | grep 'priority=120.*backends' | 
sed 's/table=..//'], [0],
-[  (ls_in_lb   ), priority=120  , match=(ct.new && ip4.dst == 
10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; reg1 = 10.0.0.10; 
reg2[[0..15]] = 80; ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80);)
+[  (ls_in_lb   ), priority=120  , match=(ct.new && ip4.dst == 
10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; reg1 = 10.0.0.10; 
reg2[[0..15]] = 80; ct_lb(backends=10.0.0.3:80,20.0.0.3:80);)
 ])
 
 AS_BOX([Create the Load_Balancer_Health_Check again.])
@@ -1242,7 +1249,7 @@ check ovn-nbctl --wait=sb sync
 
 ovn-sbctl dump-flows sw0 | grep backends | grep priority=120 > lflows.txt
 AT_CHECK([cat lflows.txt | sed 's/table=..//'], [0], [dnl
-  (ls_in_lb   ), priority=120  , match=(ct.new && ip4.dst == 10.0.0.10 
&& tcp.dst == 80), action=(reg0[[1]] = 0; reg1 = 10.0.0.10; reg2[[0..15]] = 80; 
ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80);)
+  (ls_in_lb   ), priority=120  , match=(ct.new && ip4.dst == 10.0.0.10 
&& tcp.dst == 80), action=(reg0[[1]] = 0; reg1 = 10.0.0.10; reg2[[0..15]] = 80; 
ct_lb(backends=10.0.0.3:80,20.0.0.3:80);)
 ])
 
 AS_BOX([Get the uuid of both the service_monitor])
@@ -1252,7 +1259,7 @@ sm_sw1_p1=$(fetch_column Service_Monitor _uuid 
logical_port=sw1-p1)
 AT_CAPTURE_FILE([sbflows3])
 OVS_WAIT_FOR_OUTPUT(
   [ovn-sbctl dump-flows sw0 | tee sbflows 3 | grep 'priority=120.*backends' | 
sed 's/table=..//'], [0],
-[  (ls_in_lb   ), priority=120  , match=(ct.new && ip4.dst == 
10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; reg1 = 10.0.0.10; 
reg2[[0..15]] = 80; ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80);)
+[  (ls_in_lb   ), priority=120  , match=(ct.new && ip4.dst == 
10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; reg1 = 10.0.0.10; 
reg2[[0..15]] = 80;