diff mbox series

[ovs-dev] Fix ovn-controller crash when a lport of type 'virtual' is deleted.

Message ID 20200826114143.2696189-1-numans@ovn.org
State Accepted
Headers show
Series [ovs-dev] Fix ovn-controller crash when a lport of type 'virtual' is deleted. | expand

Commit Message

Numan Siddique Aug. 26, 2020, 11:41 a.m. UTC 
From: Numan Siddique <numans@ovn.org>

The below bt is seen when a lport of type 'virtual' is deleted.

(gdb) bt
0x00001470c0708655 in __strlen_avx2 () from /lib64/libc.so.6
0x0000563340037449 in hash_string (basis=0, s=s@entry=0x0) at lib/hash.h:342
hash_name (name=name@entry=0x0) at lib/shash.c:28
0x0000563340037a76 in shash_find (sh=0x5633407bb260, name=0x0) at lib/shash.c:231
0x0000563340037b7d in shash_find_data (sh=<optimized out>, name=<optimized out>) at lib/shash.c:245
0x000056333ff71151 in local_binding_find (name=<optimized out>, local_bindings=<optimized out>) at controller/binding.h:108
get_lbinding_for_lport (b_ctx_out=0x7fff616745b0, lport_type=<optimized out>, pb=0x56334314d630) at controller/binding.c:1960
handle_deleted_vif_lport (b_ctx_in=0x7fff61674600, b_ctx_in=0x7fff61674600, b_ctx_out=0x7fff616745b0, lport_type=<optimized out>, pb=0x56334314d630) at controller/binding.c:1979
binding_handle_port_binding_changes (b_ctx_in=b_ctx_in@entry=0x7fff61674600, b_ctx_out=b_ctx_out@entry=0x7fff616745b0) at controller/binding.c:2087
0x000056333ff8e208 in runtime_data_sb_port_binding_handler (node=0x7fff616759f0, data=0x5633407bb240) at controller/ovn-controller.c:1325
0x000056333ffa6de3 in engine_compute (recompute_allowed=<optimized out>, node=<optimized out>) at lib/inc-proc-eng.c:306
...
...

Fixes: 354bdba51ab("ovn-controller: I-P for SB port binding and OVS interface in runtime_data.")
Signed-off-by: Numan Siddique <numans@ovn.org>
---
 controller/binding.c | 12 ++++++++----
 tests/ovn.at         | 11 +++++++++++
 2 files changed, 19 insertions(+), 4 deletions(-)

Comments

0-day Robot Aug. 26, 2020, 11:59 a.m. UTC | #1 
Bleep bloop.  Greetings Numan Siddique, 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.


git-am:
error: sha1 information is lacking or useless (controller/binding.c).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch' to see the failed patch
Patch failed at 0001 Fix ovn-controller crash when a lport of type 'virtual' is deleted.
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".


Please check this out.  If you feel there has been an error, please email aconole@redhat.com

Thanks,
0-day Robot
Mark Michelson Aug. 26, 2020, 12:57 p.m. UTC | #2 
Acked-by: Mark Michelson <mmichels@redhat.com>

Is this bug severe enough that a 20.06.3 release is necessary?

On 8/26/20 7:41 AM, numans@ovn.org wrote:
> From: Numan Siddique <numans@ovn.org>
> 
> The below bt is seen when a lport of type 'virtual' is deleted.
> 
> (gdb) bt
> 0x00001470c0708655 in __strlen_avx2 () from /lib64/libc.so.6
> 0x0000563340037449 in hash_string (basis=0, s=s@entry=0x0) at lib/hash.h:342
> hash_name (name=name@entry=0x0) at lib/shash.c:28
> 0x0000563340037a76 in shash_find (sh=0x5633407bb260, name=0x0) at lib/shash.c:231
> 0x0000563340037b7d in shash_find_data (sh=<optimized out>, name=<optimized out>) at lib/shash.c:245
> 0x000056333ff71151 in local_binding_find (name=<optimized out>, local_bindings=<optimized out>) at controller/binding.h:108
> get_lbinding_for_lport (b_ctx_out=0x7fff616745b0, lport_type=<optimized out>, pb=0x56334314d630) at controller/binding.c:1960
> handle_deleted_vif_lport (b_ctx_in=0x7fff61674600, b_ctx_in=0x7fff61674600, b_ctx_out=0x7fff616745b0, lport_type=<optimized out>, pb=0x56334314d630) at controller/binding.c:1979
> binding_handle_port_binding_changes (b_ctx_in=b_ctx_in@entry=0x7fff61674600, b_ctx_out=b_ctx_out@entry=0x7fff616745b0) at controller/binding.c:2087
> 0x000056333ff8e208 in runtime_data_sb_port_binding_handler (node=0x7fff616759f0, data=0x5633407bb240) at controller/ovn-controller.c:1325
> 0x000056333ffa6de3 in engine_compute (recompute_allowed=<optimized out>, node=<optimized out>) at lib/inc-proc-eng.c:306
> ...
> ...
> 
> Fixes: 354bdba51ab("ovn-controller: I-P for SB port binding and OVS interface in runtime_data.")
> Signed-off-by: Numan Siddique <numans@ovn.org>
> ---
>   controller/binding.c | 12 ++++++++----
>   tests/ovn.at         | 11 +++++++++++
>   2 files changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/controller/binding.c b/controller/binding.c
> index 880fbb13b..3c102dc7f 100644
> --- a/controller/binding.c
> +++ b/controller/binding.c
> @@ -1957,11 +1957,15 @@ get_lbinding_for_lport(const struct sbrec_port_binding *pb,
>       struct local_binding *parent_lbinding = NULL;
>   
>       if (lport_type == LP_VIRTUAL) {
> -        parent_lbinding = local_binding_find(b_ctx_out->local_bindings,
> -                                             pb->virtual_parent);
> +        if (pb->virtual_parent) {
> +            parent_lbinding = local_binding_find(b_ctx_out->local_bindings,
> +                                                 pb->virtual_parent);
> +        }
>       } else {
> -        parent_lbinding = local_binding_find(b_ctx_out->local_bindings,
> -                                             pb->parent_port);
> +        if (pb->parent_port) {
> +            parent_lbinding = local_binding_find(b_ctx_out->local_bindings,
> +                                                 pb->parent_port);
> +        }
>       }
>   
>       return parent_lbinding
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 8aabdf307..c4edbd9e1 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -16125,6 +16125,17 @@ ovn-nbctl lsp-set-addresses sw1-lr0 00:00:00:00:ff:02
>   ovn-nbctl lsp-set-options sw1-lr0 router-port=lr0-sw1
>   
>   OVN_POPULATE_ARP
> +
> +# Delete sw0-vir and add again.
> +ovn-nbctl lsp-del sw0-vir
> +
> +ovn-nbctl lsp-add sw0 sw0-vir
> +ovn-nbctl lsp-set-addresses sw0-vir "50:54:00:00:00:10 10.0.0.10"
> +ovn-nbctl lsp-set-port-security sw0-vir "50:54:00:00:00:10 10.0.0.10"
> +ovn-nbctl lsp-set-type sw0-vir virtual
> +ovn-nbctl set logical_switch_port sw0-vir options:virtual-ip=10.0.0.10
> +ovn-nbctl set logical_switch_port sw0-vir options:virtual-parents=sw0-p1,sw0-p2,sw0-p3
> +
>   ovn-nbctl --wait=hv sync
>   
>   # Check that logical flows are added for sw0-vir in lsp_in_arp_rsp pipeline
>
Numan Siddique Aug. 26, 2020, 1:59 p.m. UTC | #3 
On Wed, Aug 26, 2020 at 6:28 PM Mark Michelson <mmichels@redhat.com> wrote:

> Acked-by: Mark Michelson <mmichels@redhat.com>
>
> Is this bug severe enough that a 20.06.3 release is necessary?
>

Thanks Mark. I applied this patch to master and branch-20.06.

I think this is definitely a severe bug. The issue is seen when virtual
ports are used.
Currently openstack neutron uses it. But I think it's not too urgent that
we should immediately
release 20.06.3. I would suggest to wait for a week or two before releasing
20.06.3.

Thanks
Numan


>
> On 8/26/20 7:41 AM, numans@ovn.org wrote:
> > From: Numan Siddique <numans@ovn.org>
> >
> > The below bt is seen when a lport of type 'virtual' is deleted.
> >
> > (gdb) bt
> > 0x00001470c0708655 in __strlen_avx2 () from /lib64/libc.so.6
> > 0x0000563340037449 in hash_string (basis=0, s=s@entry=0x0) at
> lib/hash.h:342
> > hash_name (name=name@entry=0x0) at lib/shash.c:28
> > 0x0000563340037a76 in shash_find (sh=0x5633407bb260, name=0x0) at
> lib/shash.c:231
> > 0x0000563340037b7d in shash_find_data (sh=<optimized out>,
> name=<optimized out>) at lib/shash.c:245
> > 0x000056333ff71151 in local_binding_find (name=<optimized out>,
> local_bindings=<optimized out>) at controller/binding.h:108
> > get_lbinding_for_lport (b_ctx_out=0x7fff616745b0, lport_type=<optimized
> out>, pb=0x56334314d630) at controller/binding.c:1960
> > handle_deleted_vif_lport (b_ctx_in=0x7fff61674600,
> b_ctx_in=0x7fff61674600, b_ctx_out=0x7fff616745b0, lport_type=<optimized
> out>, pb=0x56334314d630) at controller/binding.c:1979
> > binding_handle_port_binding_changes (b_ctx_in=b_ctx_in@entry=0x7fff61674600,
> b_ctx_out=b_ctx_out@entry=0x7fff616745b0) at controller/binding.c:2087
> > 0x000056333ff8e208 in runtime_data_sb_port_binding_handler
> (node=0x7fff616759f0, data=0x5633407bb240) at
> controller/ovn-controller.c:1325
> > 0x000056333ffa6de3 in engine_compute (recompute_allowed=<optimized out>,
> node=<optimized out>) at lib/inc-proc-eng.c:306
> > ...
> > ...
> >
> > Fixes: 354bdba51ab("ovn-controller: I-P for SB port binding and OVS
> interface in runtime_data.")
> > Signed-off-by: Numan Siddique <numans@ovn.org>
> > ---
> >   controller/binding.c | 12 ++++++++----
> >   tests/ovn.at         | 11 +++++++++++
> >   2 files changed, 19 insertions(+), 4 deletions(-)
> >
> > diff --git a/controller/binding.c b/controller/binding.c
> > index 880fbb13b..3c102dc7f 100644
> > --- a/controller/binding.c
> > +++ b/controller/binding.c
> > @@ -1957,11 +1957,15 @@ get_lbinding_for_lport(const struct
> sbrec_port_binding *pb,
> >       struct local_binding *parent_lbinding = NULL;
> >
> >       if (lport_type == LP_VIRTUAL) {
> > -        parent_lbinding = local_binding_find(b_ctx_out->local_bindings,
> > -                                             pb->virtual_parent);
> > +        if (pb->virtual_parent) {
> > +            parent_lbinding =
> local_binding_find(b_ctx_out->local_bindings,
> > +                                                 pb->virtual_parent);
> > +        }
> >       } else {
> > -        parent_lbinding = local_binding_find(b_ctx_out->local_bindings,
> > -                                             pb->parent_port);
> > +        if (pb->parent_port) {
> > +            parent_lbinding =
> local_binding_find(b_ctx_out->local_bindings,
> > +                                                 pb->parent_port);
> > +        }
> >       }
> >
> >       return parent_lbinding
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index 8aabdf307..c4edbd9e1 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -16125,6 +16125,17 @@ ovn-nbctl lsp-set-addresses sw1-lr0
> 00:00:00:00:ff:02
> >   ovn-nbctl lsp-set-options sw1-lr0 router-port=lr0-sw1
> >
> >   OVN_POPULATE_ARP
> > +
> > +# Delete sw0-vir and add again.
> > +ovn-nbctl lsp-del sw0-vir
> > +
> > +ovn-nbctl lsp-add sw0 sw0-vir
> > +ovn-nbctl lsp-set-addresses sw0-vir "50:54:00:00:00:10 10.0.0.10"
> > +ovn-nbctl lsp-set-port-security sw0-vir "50:54:00:00:00:10 10.0.0.10"
> > +ovn-nbctl lsp-set-type sw0-vir virtual
> > +ovn-nbctl set logical_switch_port sw0-vir options:virtual-ip=10.0.0.10
> > +ovn-nbctl set logical_switch_port sw0-vir
> options:virtual-parents=sw0-p1,sw0-p2,sw0-p3
> > +
> >   ovn-nbctl --wait=hv sync
> >
> >   # Check that logical flows are added for sw0-vir in lsp_in_arp_rsp
> pipeline
> >
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
diff mbox series

Patch

diff --git a/controller/binding.c b/controller/binding.c
index 880fbb13b..3c102dc7f 100644
--- a/controller/binding.c
+++ b/controller/binding.c
@@ -1957,11 +1957,15 @@  get_lbinding_for_lport(const struct sbrec_port_binding *pb,
     struct local_binding *parent_lbinding = NULL;
 
     if (lport_type == LP_VIRTUAL) {
-        parent_lbinding = local_binding_find(b_ctx_out->local_bindings,
-                                             pb->virtual_parent);
+        if (pb->virtual_parent) {
+            parent_lbinding = local_binding_find(b_ctx_out->local_bindings,
+                                                 pb->virtual_parent);
+        }
     } else {
-        parent_lbinding = local_binding_find(b_ctx_out->local_bindings,
-                                             pb->parent_port);
+        if (pb->parent_port) {
+            parent_lbinding = local_binding_find(b_ctx_out->local_bindings,
+                                                 pb->parent_port);
+        }
     }
 
     return parent_lbinding
diff --git a/tests/ovn.at b/tests/ovn.at
index 8aabdf307..c4edbd9e1 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -16125,6 +16125,17 @@  ovn-nbctl lsp-set-addresses sw1-lr0 00:00:00:00:ff:02
 ovn-nbctl lsp-set-options sw1-lr0 router-port=lr0-sw1
 
 OVN_POPULATE_ARP
+
+# Delete sw0-vir and add again.
+ovn-nbctl lsp-del sw0-vir
+
+ovn-nbctl lsp-add sw0 sw0-vir
+ovn-nbctl lsp-set-addresses sw0-vir "50:54:00:00:00:10 10.0.0.10"
+ovn-nbctl lsp-set-port-security sw0-vir "50:54:00:00:00:10 10.0.0.10"
+ovn-nbctl lsp-set-type sw0-vir virtual
+ovn-nbctl set logical_switch_port sw0-vir options:virtual-ip=10.0.0.10
+ovn-nbctl set logical_switch_port sw0-vir options:virtual-parents=sw0-p1,sw0-p2,sw0-p3
+
 ovn-nbctl --wait=hv sync
 
 # Check that logical flows are added for sw0-vir in lsp_in_arp_rsp pipeline