| Message ID | 1468533540-66622-1-git-send-email-dlu998@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
Thanks Darrell, this looks good to me, but I'm not familiar with every aspect of ovs-vtep. I'm mostly interested in this patch because it fixes an intermittent failure of the testcase "ovn-controller-vtep - vtep-macs 1". Russell (since you commented on v1), does this look good to you? Thanks, Daniele 2016-07-14 14:59 GMT-07:00 Darrell Ball <dlu998@gmail.com>: > Presently, ovs-vtep expects the datapath tunnel key to be available > in the VTEP DB at startup. This may not be the case which is also > observed as interrmittent unit test failures. This patch allows > for the tunnel key to later appear in the VTEP database. > > Signed-off-by: Darrell Ball <dlu998@gmail.com> > --- > > v1->v2: Cleanup the existing code with broken error handling. > > vtep/ovs-vtep | 28 ++++++++++++++-------------- > 1 file changed, 14 insertions(+), 14 deletions(-) > > diff --git a/vtep/ovs-vtep b/vtep/ovs-vtep > index e52c66f..b32a82a 100644 > --- a/vtep/ovs-vtep > +++ b/vtep/ovs-vtep > @@ -91,7 +91,6 @@ class Logical_Switch(object): > self.local_macs = set() > self.remote_macs = {} > self.unknown_dsts = set() > - self.tunnel_key = 0 > self.setup_ls() > self.replication_mode = "service_node" > > @@ -99,16 +98,6 @@ class Logical_Switch(object): > vlog.info("destroying lswitch %s" % self.name) > > def setup_ls(self): > - column = vtep_ctl("--columns=tunnel_key find logical_switch " > - "name=%s" % self.name) > - tunnel_key = column.partition(":")[2].strip() > - if tunnel_key and isinstance(eval(tunnel_key), six.integer_types): > - self.tunnel_key = tunnel_key > - vlog.info("using tunnel key %s in %s" > - % (self.tunnel_key, self.name)) > - else: > - self.tunnel_key = 0 > - vlog.warn("invalid tunnel key for %s, using 0" % self.name) > > if ps_type: > ovs_vsctl("--may-exist add-br %s -- set Bridge %s > datapath_type=%s" > @@ -175,7 +164,7 @@ class Logical_Switch(object): > del self.ports[lbinding] > self.update_flood() > > - def add_tunnel(self, tunnel): > + def add_tunnel(self, tunnel, tunnel_key): > global tun_id > vlog.info("adding tunnel %s" % tunnel) > encap, ip = tunnel.split("/") > @@ -189,7 +178,7 @@ class Logical_Switch(object): > > ovs_vsctl("add-port %s %s -- set Interface %s type=vxlan " > "options:key=%s options:remote_ip=%s" > - % (self.short_name, tun_name, tun_name, > self.tunnel_key, ip)) > + % (self.short_name, tun_name, tun_name, tunnel_key, ip)) > > for i in range(10): > port_no = ovs_vsctl("get Interface %s ofport" % tun_name) > @@ -259,6 +248,17 @@ class Logical_Switch(object): > tunnels = set() > parse_ucast = True > > + column = vtep_ctl("--columns=tunnel_key find logical_switch " > + "name=%s" % self.name) > + tunnel_key = column.partition(":")[2].strip() > + if tunnel_key and isinstance(eval(tunnel_key), six.integer_types): > + vlog.info("update_remote_macs: using tunnel key %s in %s" > + % (tunnel_key, self.name)) > + else: > + vlog.info("Invalid tunnel key %s in %s post VTEP DB requery" > + % (tunnel_key, self.name)) > + return > + > mac_list = vtep_ctl("list-remote-macs %s" % self.name > ).splitlines() > for line in mac_list: > if (line.find("mcast-mac-remote") != -1): > @@ -282,7 +282,7 @@ class Logical_Switch(object): > old_tunnels = set(self.tunnels.keys()) > > for tunnel in tunnels.difference(old_tunnels): > - self.add_tunnel(tunnel) > + self.add_tunnel(tunnel, tunnel_key) > > for tunnel in old_tunnels.difference(tunnels): > self.del_tunnel(tunnel) > -- > 1.9.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev >
On Thu, Jul 14, 2016 at 7:26 PM, Daniele Di Proietto <diproiettod@ovn.org> wrote: > Thanks Darrell, this looks good to me, but I'm not familiar with every > aspect of ovs-vtep. > > I'm mostly interested in this patch because it fixes an intermittent > failure of the testcase "ovn-controller-vtep - vtep-macs 1". > > Russell (since you commented on v1), does this look good to you? > > I'm not super familiar with ovs-vtep, either. This patch looks straight forward enough to me, though. Acked-by: Russell Bryant <russell@ovn.org> > Thanks, > > Daniele > > 2016-07-14 14:59 GMT-07:00 Darrell Ball <dlu998@gmail.com>: > >> Presently, ovs-vtep expects the datapath tunnel key to be available >> in the VTEP DB at startup. This may not be the case which is also >> observed as interrmittent unit test failures. This patch allows >> for the tunnel key to later appear in the VTEP database. >> >> Signed-off-by: Darrell Ball <dlu998@gmail.com> >> --- >> >> v1->v2: Cleanup the existing code with broken error handling. >> >> vtep/ovs-vtep | 28 ++++++++++++++-------------- >> 1 file changed, 14 insertions(+), 14 deletions(-) >> >> diff --git a/vtep/ovs-vtep b/vtep/ovs-vtep >> index e52c66f..b32a82a 100644 >> --- a/vtep/ovs-vtep >> +++ b/vtep/ovs-vtep >> @@ -91,7 +91,6 @@ class Logical_Switch(object): >> self.local_macs = set() >> self.remote_macs = {} >> self.unknown_dsts = set() >> - self.tunnel_key = 0 >> self.setup_ls() >> self.replication_mode = "service_node" >> >> @@ -99,16 +98,6 @@ class Logical_Switch(object): >> vlog.info("destroying lswitch %s" % self.name) >> >> def setup_ls(self): >> - column = vtep_ctl("--columns=tunnel_key find logical_switch " >> - "name=%s" % self.name) >> - tunnel_key = column.partition(":")[2].strip() >> - if tunnel_key and isinstance(eval(tunnel_key), >> six.integer_types): >> - self.tunnel_key = tunnel_key >> - vlog.info("using tunnel key %s in %s" >> - % (self.tunnel_key, self.name)) >> - else: >> - self.tunnel_key = 0 >> - vlog.warn("invalid tunnel key for %s, using 0" % self.name) >> >> if ps_type: >> ovs_vsctl("--may-exist add-br %s -- set Bridge %s >> datapath_type=%s" >> @@ -175,7 +164,7 @@ class Logical_Switch(object): >> del self.ports[lbinding] >> self.update_flood() >> >> - def add_tunnel(self, tunnel): >> + def add_tunnel(self, tunnel, tunnel_key): >> global tun_id >> vlog.info("adding tunnel %s" % tunnel) >> encap, ip = tunnel.split("/") >> @@ -189,7 +178,7 @@ class Logical_Switch(object): >> >> ovs_vsctl("add-port %s %s -- set Interface %s type=vxlan " >> "options:key=%s options:remote_ip=%s" >> - % (self.short_name, tun_name, tun_name, >> self.tunnel_key, ip)) >> + % (self.short_name, tun_name, tun_name, tunnel_key, >> ip)) >> >> for i in range(10): >> port_no = ovs_vsctl("get Interface %s ofport" % tun_name) >> @@ -259,6 +248,17 @@ class Logical_Switch(object): >> tunnels = set() >> parse_ucast = True >> >> + column = vtep_ctl("--columns=tunnel_key find logical_switch " >> + "name=%s" % self.name) >> + tunnel_key = column.partition(":")[2].strip() >> + if tunnel_key and isinstance(eval(tunnel_key), >> six.integer_types): >> + vlog.info("update_remote_macs: using tunnel key %s in %s" >> + % (tunnel_key, self.name)) >> + else: >> + vlog.info("Invalid tunnel key %s in %s post VTEP DB requery" >> + % (tunnel_key, self.name)) >> + return >> + >> mac_list = vtep_ctl("list-remote-macs %s" % self.name >> ).splitlines() >> for line in mac_list: >> if (line.find("mcast-mac-remote") != -1): >> @@ -282,7 +282,7 @@ class Logical_Switch(object): >> old_tunnels = set(self.tunnels.keys()) >> >> for tunnel in tunnels.difference(old_tunnels): >> - self.add_tunnel(tunnel) >> + self.add_tunnel(tunnel, tunnel_key) >> >> for tunnel in old_tunnels.difference(tunnels): >> self.del_tunnel(tunnel) >> -- >> 1.9.1 >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> http://openvswitch.org/mailman/listinfo/dev >> > >
2016-07-15 5:44 GMT-07:00 Russell Bryant <russell@ovn.org>: > > > On Thu, Jul 14, 2016 at 7:26 PM, Daniele Di Proietto <diproiettod@ovn.org> > wrote: > >> Thanks Darrell, this looks good to me, but I'm not familiar with every >> aspect of ovs-vtep. >> >> I'm mostly interested in this patch because it fixes an intermittent >> failure of the testcase "ovn-controller-vtep - vtep-macs 1". >> >> Russell (since you commented on v1), does this look good to you? >> >> > I'm not super familiar with ovs-vtep, either. This patch looks straight > forward enough to me, though. > > Acked-by: Russell Bryant <russell@ovn.org> > Thanks Russell for the review and Darrell for the patch. I added my ack and pushed this to master. > > >> Thanks, >> >> Daniele >> >> 2016-07-14 14:59 GMT-07:00 Darrell Ball <dlu998@gmail.com>: >> >>> Presently, ovs-vtep expects the datapath tunnel key to be available >>> in the VTEP DB at startup. This may not be the case which is also >>> observed as interrmittent unit test failures. This patch allows >>> for the tunnel key to later appear in the VTEP database. >>> >>> Signed-off-by: Darrell Ball <dlu998@gmail.com> >>> --- >>> >>> v1->v2: Cleanup the existing code with broken error handling. >>> >>> vtep/ovs-vtep | 28 ++++++++++++++-------------- >>> 1 file changed, 14 insertions(+), 14 deletions(-) >>> >>> diff --git a/vtep/ovs-vtep b/vtep/ovs-vtep >>> index e52c66f..b32a82a 100644 >>> --- a/vtep/ovs-vtep >>> +++ b/vtep/ovs-vtep >>> @@ -91,7 +91,6 @@ class Logical_Switch(object): >>> self.local_macs = set() >>> self.remote_macs = {} >>> self.unknown_dsts = set() >>> - self.tunnel_key = 0 >>> self.setup_ls() >>> self.replication_mode = "service_node" >>> >>> @@ -99,16 +98,6 @@ class Logical_Switch(object): >>> vlog.info("destroying lswitch %s" % self.name) >>> >>> def setup_ls(self): >>> - column = vtep_ctl("--columns=tunnel_key find logical_switch " >>> - "name=%s" % self.name) >>> - tunnel_key = column.partition(":")[2].strip() >>> - if tunnel_key and isinstance(eval(tunnel_key), >>> six.integer_types): >>> - self.tunnel_key = tunnel_key >>> - vlog.info("using tunnel key %s in %s" >>> - % (self.tunnel_key, self.name)) >>> - else: >>> - self.tunnel_key = 0 >>> - vlog.warn("invalid tunnel key for %s, using 0" % self.name) >>> >>> if ps_type: >>> ovs_vsctl("--may-exist add-br %s -- set Bridge %s >>> datapath_type=%s" >>> @@ -175,7 +164,7 @@ class Logical_Switch(object): >>> del self.ports[lbinding] >>> self.update_flood() >>> >>> - def add_tunnel(self, tunnel): >>> + def add_tunnel(self, tunnel, tunnel_key): >>> global tun_id >>> vlog.info("adding tunnel %s" % tunnel) >>> encap, ip = tunnel.split("/") >>> @@ -189,7 +178,7 @@ class Logical_Switch(object): >>> >>> ovs_vsctl("add-port %s %s -- set Interface %s type=vxlan " >>> "options:key=%s options:remote_ip=%s" >>> - % (self.short_name, tun_name, tun_name, >>> self.tunnel_key, ip)) >>> + % (self.short_name, tun_name, tun_name, tunnel_key, >>> ip)) >>> >>> for i in range(10): >>> port_no = ovs_vsctl("get Interface %s ofport" % tun_name) >>> @@ -259,6 +248,17 @@ class Logical_Switch(object): >>> tunnels = set() >>> parse_ucast = True >>> >>> + column = vtep_ctl("--columns=tunnel_key find logical_switch " >>> + "name=%s" % self.name) >>> + tunnel_key = column.partition(":")[2].strip() >>> + if tunnel_key and isinstance(eval(tunnel_key), >>> six.integer_types): >>> + vlog.info("update_remote_macs: using tunnel key %s in %s" >>> + % (tunnel_key, self.name)) >>> + else: >>> + vlog.info("Invalid tunnel key %s in %s post VTEP DB >>> requery" >>> + % (tunnel_key, self.name)) >>> + return >>> + >>> mac_list = vtep_ctl("list-remote-macs %s" % self.name >>> ).splitlines() >>> for line in mac_list: >>> if (line.find("mcast-mac-remote") != -1): >>> @@ -282,7 +282,7 @@ class Logical_Switch(object): >>> old_tunnels = set(self.tunnels.keys()) >>> >>> for tunnel in tunnels.difference(old_tunnels): >>> - self.add_tunnel(tunnel) >>> + self.add_tunnel(tunnel, tunnel_key) >>> >>> for tunnel in old_tunnels.difference(tunnels): >>> self.del_tunnel(tunnel) >>> -- >>> 1.9.1 >>> >>> _______________________________________________ >>> dev mailing list >>> dev@openvswitch.org >>> http://openvswitch.org/mailman/listinfo/dev >>> >> >> > > > -- > Russell Bryant >
diff --git a/vtep/ovs-vtep b/vtep/ovs-vtep index e52c66f..b32a82a 100644 --- a/vtep/ovs-vtep +++ b/vtep/ovs-vtep @@ -91,7 +91,6 @@ class Logical_Switch(object): self.local_macs = set() self.remote_macs = {} self.unknown_dsts = set() - self.tunnel_key = 0 self.setup_ls() self.replication_mode = "service_node" @@ -99,16 +98,6 @@ class Logical_Switch(object): vlog.info("destroying lswitch %s" % self.name) def setup_ls(self): - column = vtep_ctl("--columns=tunnel_key find logical_switch " - "name=%s" % self.name) - tunnel_key = column.partition(":")[2].strip() - if tunnel_key and isinstance(eval(tunnel_key), six.integer_types): - self.tunnel_key = tunnel_key - vlog.info("using tunnel key %s in %s" - % (self.tunnel_key, self.name)) - else: - self.tunnel_key = 0 - vlog.warn("invalid tunnel key for %s, using 0" % self.name) if ps_type: ovs_vsctl("--may-exist add-br %s -- set Bridge %s datapath_type=%s" @@ -175,7 +164,7 @@ class Logical_Switch(object): del self.ports[lbinding] self.update_flood() - def add_tunnel(self, tunnel): + def add_tunnel(self, tunnel, tunnel_key): global tun_id vlog.info("adding tunnel %s" % tunnel) encap, ip = tunnel.split("/") @@ -189,7 +178,7 @@ class Logical_Switch(object): ovs_vsctl("add-port %s %s -- set Interface %s type=vxlan " "options:key=%s options:remote_ip=%s" - % (self.short_name, tun_name, tun_name, self.tunnel_key, ip)) + % (self.short_name, tun_name, tun_name, tunnel_key, ip)) for i in range(10): port_no = ovs_vsctl("get Interface %s ofport" % tun_name) @@ -259,6 +248,17 @@ class Logical_Switch(object): tunnels = set() parse_ucast = True + column = vtep_ctl("--columns=tunnel_key find logical_switch " + "name=%s" % self.name) + tunnel_key = column.partition(":")[2].strip() + if tunnel_key and isinstance(eval(tunnel_key), six.integer_types): + vlog.info("update_remote_macs: using tunnel key %s in %s" + % (tunnel_key, self.name)) + else: + vlog.info("Invalid tunnel key %s in %s post VTEP DB requery" + % (tunnel_key, self.name)) + return + mac_list = vtep_ctl("list-remote-macs %s" % self.name).splitlines() for line in mac_list: if (line.find("mcast-mac-remote") != -1): @@ -282,7 +282,7 @@ class Logical_Switch(object): old_tunnels = set(self.tunnels.keys()) for tunnel in tunnels.difference(old_tunnels): - self.add_tunnel(tunnel) + self.add_tunnel(tunnel, tunnel_key) for tunnel in old_tunnels.difference(tunnels): self.del_tunnel(tunnel)
Presently, ovs-vtep expects the datapath tunnel key to be available in the VTEP DB at startup. This may not be the case which is also observed as interrmittent unit test failures. This patch allows for the tunnel key to later appear in the VTEP database. Signed-off-by: Darrell Ball <dlu998@gmail.com> --- v1->v2: Cleanup the existing code with broken error handling. vtep/ovs-vtep | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-)