[ovs-dev] ovn-controller: Don't bind non-existent interfaces.
diff mbox

Message ID 1488957537-12991-1-git-send-email-guru@ovn.org
State Rejected
Headers show

Commit Message

Guru Shetty March 8, 2017, 7:18 a.m. UTC
There are multiple reasons why a interface can exist
in the Open vSwitch database but not exist in the system.
For e.g, a restart of a host after a system crash. Ideally,
whoever added the interface in the Open vSwitch database
should remove those interfaces. But that usually does not
happen in practise. Based on experience, I have observerd
that on any long lasting OVS installation there are always
a couple of stale interfaces.

When a stale interface remains in the Open vSwitch database
and the container/VM initially backing that stale interface
is moved to a different machine, the two ovn-controllers
start over-writing the OVN-SB's port_binding table in a loop.

This situation can be avoided, if ovn-controller only binds
the interfaces that actually have a valid 'ofport'.

Signed-off-by: Gurucharan Shetty <guru@ovn.org>
---
 ovn/controller/binding.c        | 3 ++-
 ovn/controller/ovn-controller.c | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

Comments

Russell Bryant March 8, 2017, 5:42 p.m. UTC | #1
On Wed, Mar 8, 2017 at 2:18 AM, Gurucharan Shetty <guru@ovn.org> wrote:
> There are multiple reasons why a interface can exist
> in the Open vSwitch database but not exist in the system.
> For e.g, a restart of a host after a system crash. Ideally,
> whoever added the interface in the Open vSwitch database
> should remove those interfaces. But that usually does not
> happen in practise. Based on experience, I have observerd

practice and observed

> that on any long lasting OVS installation there are always
> a couple of stale interfaces.
>
> When a stale interface remains in the Open vSwitch database
> and the container/VM initially backing that stale interface
> is moved to a different machine, the two ovn-controllers
> start over-writing the OVN-SB's port_binding table in a loop.
>
> This situation can be avoided, if ovn-controller only binds
> the interfaces that actually have a valid 'ofport'.
>
> Signed-off-by: Gurucharan Shetty <guru@ovn.org>

This sounds reasonable to me.

Acked-by: Russell Bryant <russell@ovn.org>
Guru Shetty March 10, 2017, 6:33 p.m. UTC | #2
On 8 March 2017 at 09:42, Russell Bryant <russell@ovn.org> wrote:

> On Wed, Mar 8, 2017 at 2:18 AM, Gurucharan Shetty <guru@ovn.org> wrote:
> > There are multiple reasons why a interface can exist
> > in the Open vSwitch database but not exist in the system.
> > For e.g, a restart of a host after a system crash. Ideally,
> > whoever added the interface in the Open vSwitch database
> > should remove those interfaces. But that usually does not
> > happen in practise. Based on experience, I have observerd
>
> practice and observed
>
> > that on any long lasting OVS installation there are always
> > a couple of stale interfaces.
> >
> > When a stale interface remains in the Open vSwitch database
> > and the container/VM initially backing that stale interface
> > is moved to a different machine, the two ovn-controllers
> > start over-writing the OVN-SB's port_binding table in a loop.
> >
> > This situation can be avoided, if ovn-controller only binds
> > the interfaces that actually have a valid 'ofport'.
> >
> > Signed-off-by: Gurucharan Shetty <guru@ovn.org>
>
> This sounds reasonable to me.
>
> Acked-by: Russell Bryant <russell@ovn.org>
>
Thanks. I applied this to master and 2.7
Guru Shetty March 10, 2017, 6:37 p.m. UTC | #3
On 8 March 2017 at 09:42, Russell Bryant <russell@ovn.org> wrote:

> On Wed, Mar 8, 2017 at 2:18 AM, Gurucharan Shetty <guru@ovn.org> wrote:
> > There are multiple reasons why a interface can exist
> > in the Open vSwitch database but not exist in the system.
> > For e.g, a restart of a host after a system crash. Ideally,
> > whoever added the interface in the Open vSwitch database
> > should remove those interfaces. But that usually does not
> > happen in practise. Based on experience, I have observerd
>
> practice and observed
>
Ugh. Though I fixed this locally, I forgot to regenerate the patch to
apply. So the commit message still has the typo. Sorry about that.

>
> > that on any long lasting OVS installation there are always
> > a couple of stale interfaces.
> >
> > When a stale interface remains in the Open vSwitch database
> > and the container/VM initially backing that stale interface
> > is moved to a different machine, the two ovn-controllers
> > start over-writing the OVN-SB's port_binding table in a loop.
> >
> > This situation can be avoided, if ovn-controller only binds
> > the interfaces that actually have a valid 'ofport'.
> >
> > Signed-off-by: Gurucharan Shetty <guru@ovn.org>
>
> This sounds reasonable to me.
>
> Acked-by: Russell Bryant <russell@ovn.org>
>

Patch
diff mbox

diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c
index c90cb65..95e9deb 100644
--- a/ovn/controller/binding.c
+++ b/ovn/controller/binding.c
@@ -86,8 +86,9 @@  get_local_iface_ids(const struct ovsrec_bridge *br_int,
 
             iface_rec = port_rec->interfaces[j];
             iface_id = smap_get(&iface_rec->external_ids, "iface-id");
+            int64_t ofport = iface_rec->n_ofport ? *iface_rec->ofport : 0;
 
-            if (iface_id) {
+            if (iface_id && ofport > 0) {
                 shash_add(lport_to_iface, iface_id, iface_rec);
                 sset_add(local_lports, iface_id);
             }
diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c
index ea299da..a36973a 100644
--- a/ovn/controller/ovn-controller.c
+++ b/ovn/controller/ovn-controller.c
@@ -518,6 +518,7 @@  main(int argc, char *argv[])
     ovsdb_idl_add_column(ovs_idl_loop.idl, &ovsrec_interface_col_name);
     ovsdb_idl_add_column(ovs_idl_loop.idl, &ovsrec_interface_col_type);
     ovsdb_idl_add_column(ovs_idl_loop.idl, &ovsrec_interface_col_options);
+    ovsdb_idl_add_column(ovs_idl_loop.idl, &ovsrec_interface_col_ofport);
     ovsdb_idl_add_table(ovs_idl_loop.idl, &ovsrec_table_port);
     ovsdb_idl_add_column(ovs_idl_loop.idl, &ovsrec_port_col_name);
     ovsdb_idl_add_column(ovs_idl_loop.idl, &ovsrec_port_col_interfaces);