diff mbox series

[ovs-dev] Allow to create tunnel ports with the same config

Message ID 20201119033501.798597-1-ihrachys@redhat.com
State New
Headers show
Series [ovs-dev] Allow to create tunnel ports with the same config | expand

Commit Message

Ihar Hrachyshka Nov. 19, 2020, 3:35 a.m. UTC
It's a legal setup where tunnel ports with the same config are created
on different bridges served by Open vSwitch. Specifically, multiple
OVN controllers may emulate multiple chassis running on the same
physical host, in which case they may need to create separate tunnel
ports to connect to the same remote chassis on their respective
bridges.

Signed-off-by: Ihar Hrachyshka <ihrachys@redhat.com>
---
 ofproto/tunnel.c | 19 ++-----------------
 1 file changed, 2 insertions(+), 17 deletions(-)

Comments

Ben Pfaff Nov. 19, 2020, 5:12 a.m. UTC | #1
On Wed, Nov 18, 2020 at 10:35:01PM -0500, Ihar Hrachyshka wrote:
> It's a legal setup where tunnel ports with the same config are created
> on different bridges served by Open vSwitch. Specifically, multiple
> OVN controllers may emulate multiple chassis running on the same
> physical host, in which case they may need to create separate tunnel
> ports to connect to the same remote chassis on their respective
> bridges.
> 
> Signed-off-by: Ihar Hrachyshka <ihrachys@redhat.com>

That makes sense for sending packets, but which one is supposed to
receive a packet when one arrives for that tunnel?
Ihar Hrachyshka Nov. 19, 2020, 1:31 p.m. UTC | #2
On Thu, Nov 19, 2020 at 12:51 AM Ben Pfaff <blp@ovn.org> wrote:
>
> On Wed, Nov 18, 2020 at 10:35:01PM -0500, Ihar Hrachyshka wrote:
> > It's a legal setup where tunnel ports with the same config are created
> > on different bridges served by Open vSwitch. Specifically, multiple
> > OVN controllers may emulate multiple chassis running on the same
> > physical host, in which case they may need to create separate tunnel
> > ports to connect to the same remote chassis on their respective
> > bridges.
> >
> > Signed-off-by: Ihar Hrachyshka <ihrachys@redhat.com>
>
> That makes sense for sending packets, but which one is supposed to
> receive a packet when one arrives for that tunnel?
>

If we are talking in OVN context, each virtual ovn-controller chassis
on the same host has to have a different IP / port to distinguish
between chassis. I don't think it's a useful configuration to have two
two incoming tunnel ports with the same config for the same IP that
are not served by separate DST IP addresses.

Ihar
Ben Pfaff Nov. 19, 2020, 5:19 p.m. UTC | #3
On Thu, Nov 19, 2020 at 08:31:44AM -0500, Ihar Hrachyshka wrote:
> On Thu, Nov 19, 2020 at 12:51 AM Ben Pfaff <blp@ovn.org> wrote:
> >
> > On Wed, Nov 18, 2020 at 10:35:01PM -0500, Ihar Hrachyshka wrote:
> > > It's a legal setup where tunnel ports with the same config are created
> > > on different bridges served by Open vSwitch. Specifically, multiple
> > > OVN controllers may emulate multiple chassis running on the same
> > > physical host, in which case they may need to create separate tunnel
> > > ports to connect to the same remote chassis on their respective
> > > bridges.
> > >
> > > Signed-off-by: Ihar Hrachyshka <ihrachys@redhat.com>
> >
> > That makes sense for sending packets, but which one is supposed to
> > receive a packet when one arrives for that tunnel?
> >
> 
> If we are talking in OVN context, each virtual ovn-controller chassis
> on the same host has to have a different IP / port to distinguish
> between chassis. I don't think it's a useful configuration to have two
> two incoming tunnel ports with the same config for the same IP that
> are not served by separate DST IP addresses.

I think that this code rejects tunnel ports with exactly the same
configuration, though.  If the two ports were configured with different
local IP addresses, then this code would not flag a conflict.  Do I
misunderstand?
Ihar Hrachyshka Nov. 20, 2020, 5:44 p.m. UTC | #4
On Thu, Nov 19, 2020 at 12:19 PM Ben Pfaff <blp@ovn.org> wrote:
>
> On Thu, Nov 19, 2020 at 08:31:44AM -0500, Ihar Hrachyshka wrote:
> > On Thu, Nov 19, 2020 at 12:51 AM Ben Pfaff <blp@ovn.org> wrote:
> > >
> > > On Wed, Nov 18, 2020 at 10:35:01PM -0500, Ihar Hrachyshka wrote:
> > > > It's a legal setup where tunnel ports with the same config are created
> > > > on different bridges served by Open vSwitch. Specifically, multiple
> > > > OVN controllers may emulate multiple chassis running on the same
> > > > physical host, in which case they may need to create separate tunnel
> > > > ports to connect to the same remote chassis on their respective
> > > > bridges.
> > > >
> > > > Signed-off-by: Ihar Hrachyshka <ihrachys@redhat.com>
> > >
> > > That makes sense for sending packets, but which one is supposed to
> > > receive a packet when one arrives for that tunnel?
> > >
> >
> > If we are talking in OVN context, each virtual ovn-controller chassis
> > on the same host has to have a different IP / port to distinguish
> > between chassis. I don't think it's a useful configuration to have two
> > two incoming tunnel ports with the same config for the same IP that
> > are not served by separate DST IP addresses.
>
> I think that this code rejects tunnel ports with exactly the same
> configuration, though.  If the two ports were configured with different
> local IP addresses, then this code would not flag a conflict.  Do I
> misunderstand?
>

Local IP addresses are not part of tunnel interface options map. Only
remote_ip and dst_port are. Does it address your concern?

Ihar
Ben Pfaff Nov. 20, 2020, 6:16 p.m. UTC | #5
On Fri, Nov 20, 2020 at 12:44:30PM -0500, Ihar Hrachyshka wrote:
> On Thu, Nov 19, 2020 at 12:19 PM Ben Pfaff <blp@ovn.org> wrote:
> >
> > On Thu, Nov 19, 2020 at 08:31:44AM -0500, Ihar Hrachyshka wrote:
> > > On Thu, Nov 19, 2020 at 12:51 AM Ben Pfaff <blp@ovn.org> wrote:
> > > >
> > > > On Wed, Nov 18, 2020 at 10:35:01PM -0500, Ihar Hrachyshka wrote:
> > > > > It's a legal setup where tunnel ports with the same config are created
> > > > > on different bridges served by Open vSwitch. Specifically, multiple
> > > > > OVN controllers may emulate multiple chassis running on the same
> > > > > physical host, in which case they may need to create separate tunnel
> > > > > ports to connect to the same remote chassis on their respective
> > > > > bridges.
> > > > >
> > > > > Signed-off-by: Ihar Hrachyshka <ihrachys@redhat.com>
> > > >
> > > > That makes sense for sending packets, but which one is supposed to
> > > > receive a packet when one arrives for that tunnel?
> > > >
> > >
> > > If we are talking in OVN context, each virtual ovn-controller chassis
> > > on the same host has to have a different IP / port to distinguish
> > > between chassis. I don't think it's a useful configuration to have two
> > > two incoming tunnel ports with the same config for the same IP that
> > > are not served by separate DST IP addresses.
> >
> > I think that this code rejects tunnel ports with exactly the same
> > configuration, though.  If the two ports were configured with different
> > local IP addresses, then this code would not flag a conflict.  Do I
> > misunderstand?
> >
> 
> Local IP addresses are not part of tunnel interface options map. Only
> remote_ip and dst_port are. Does it address your concern?

I don't see where you're getting that.  Tunnel configuration allows both
local and remote IPs to be specified.  The match structure has slots for
both IP addresses, and both of them may be specified.

If the remote (or local) IP addresses are different, then this warning
won't come up.  And you said that the remote IP addresses are different:
"I don't think it's a useful configuration to have two two incoming
tunnel ports with the same config for the same IP that are not served by
separate DST IP addresses."

tnl_port_receive() looks up the tunnel that should receive a packet.  It
calls into tnl_find().  This calls tnl_find_exact() for each of several
lookup tables.  In the case where a warning would be given here, we know
that tnl_find_exact() would find two matching elements in the map,
because that's the function that we use to know to issue the warning.
If we disable the warning and insert the second match anyway, there will
be two elements in the map with exactly the same key, tnl_find_exact()
will only ever return one of them, thus the other will never receive any
packets.

What am I missing?
Ihar Hrachyshka Nov. 20, 2020, 7:23 p.m. UTC | #6
On Fri, Nov 20, 2020 at 1:16 PM Ben Pfaff <blp@ovn.org> wrote:
>
> On Fri, Nov 20, 2020 at 12:44:30PM -0500, Ihar Hrachyshka wrote:
> > On Thu, Nov 19, 2020 at 12:19 PM Ben Pfaff <blp@ovn.org> wrote:
> > >
> > > On Thu, Nov 19, 2020 at 08:31:44AM -0500, Ihar Hrachyshka wrote:
> > > > On Thu, Nov 19, 2020 at 12:51 AM Ben Pfaff <blp@ovn.org> wrote:
> > > > >
> > > > > On Wed, Nov 18, 2020 at 10:35:01PM -0500, Ihar Hrachyshka wrote:
> > > > > > It's a legal setup where tunnel ports with the same config are created
> > > > > > on different bridges served by Open vSwitch. Specifically, multiple
> > > > > > OVN controllers may emulate multiple chassis running on the same
> > > > > > physical host, in which case they may need to create separate tunnel
> > > > > > ports to connect to the same remote chassis on their respective
> > > > > > bridges.
> > > > > >
> > > > > > Signed-off-by: Ihar Hrachyshka <ihrachys@redhat.com>
> > > > >
> > > > > That makes sense for sending packets, but which one is supposed to
> > > > > receive a packet when one arrives for that tunnel?
> > > > >
> > > >
> > > > If we are talking in OVN context, each virtual ovn-controller chassis
> > > > on the same host has to have a different IP / port to distinguish
> > > > between chassis. I don't think it's a useful configuration to have two
> > > > two incoming tunnel ports with the same config for the same IP that
> > > > are not served by separate DST IP addresses.
> > >
> > > I think that this code rejects tunnel ports with exactly the same
> > > configuration, though.  If the two ports were configured with different
> > > local IP addresses, then this code would not flag a conflict.  Do I
> > > misunderstand?
> > >
> >
> > Local IP addresses are not part of tunnel interface options map. Only
> > remote_ip and dst_port are. Does it address your concern?
>
> I don't see where you're getting that.  Tunnel configuration allows both
> local and remote IPs to be specified.  The match structure has slots for
> both IP addresses, and both of them may be specified.
>
> If the remote (or local) IP addresses are different, then this warning
> won't come up.  And you said that the remote IP addresses are different:
> "I don't think it's a useful configuration to have two two incoming
> tunnel ports with the same config for the same IP that are not served by
> separate DST IP addresses."
>
> tnl_port_receive() looks up the tunnel that should receive a packet.  It
> calls into tnl_find().  This calls tnl_find_exact() for each of several
> lookup tables.  In the case where a warning would be given here, we know
> that tnl_find_exact() would find two matching elements in the map,
> because that's the function that we use to know to issue the warning.
> If we disable the warning and insert the second match anyway, there will
> be two elements in the map with exactly the same key, tnl_find_exact()
> will only ever return one of them, thus the other will never receive any
> packets.
>
> What am I missing?
>

Oh I missed that local_ip *may* be configured for a tunnel endpoint,
it's just not configured by OVN controller *right now*. We can deal
with it in OVN controller instead of OVS then. Sorry for the noise,
this patch can be dropped.

Ihar
Ben Pfaff Nov. 20, 2020, 7:32 p.m. UTC | #7
On Fri, Nov 20, 2020 at 02:23:04PM -0500, Ihar Hrachyshka wrote:
> On Fri, Nov 20, 2020 at 1:16 PM Ben Pfaff <blp@ovn.org> wrote:
> >
> > On Fri, Nov 20, 2020 at 12:44:30PM -0500, Ihar Hrachyshka wrote:
> > > On Thu, Nov 19, 2020 at 12:19 PM Ben Pfaff <blp@ovn.org> wrote:
> > > >
> > > > On Thu, Nov 19, 2020 at 08:31:44AM -0500, Ihar Hrachyshka wrote:
> > > > > On Thu, Nov 19, 2020 at 12:51 AM Ben Pfaff <blp@ovn.org> wrote:
> > > > > >
> > > > > > On Wed, Nov 18, 2020 at 10:35:01PM -0500, Ihar Hrachyshka wrote:
> > > > > > > It's a legal setup where tunnel ports with the same config are created
> > > > > > > on different bridges served by Open vSwitch. Specifically, multiple
> > > > > > > OVN controllers may emulate multiple chassis running on the same
> > > > > > > physical host, in which case they may need to create separate tunnel
> > > > > > > ports to connect to the same remote chassis on their respective
> > > > > > > bridges.
> > > > > > >
> > > > > > > Signed-off-by: Ihar Hrachyshka <ihrachys@redhat.com>
> > > > > >
> > > > > > That makes sense for sending packets, but which one is supposed to
> > > > > > receive a packet when one arrives for that tunnel?
> > > > > >
> > > > >
> > > > > If we are talking in OVN context, each virtual ovn-controller chassis
> > > > > on the same host has to have a different IP / port to distinguish
> > > > > between chassis. I don't think it's a useful configuration to have two
> > > > > two incoming tunnel ports with the same config for the same IP that
> > > > > are not served by separate DST IP addresses.
> > > >
> > > > I think that this code rejects tunnel ports with exactly the same
> > > > configuration, though.  If the two ports were configured with different
> > > > local IP addresses, then this code would not flag a conflict.  Do I
> > > > misunderstand?
> > > >
> > >
> > > Local IP addresses are not part of tunnel interface options map. Only
> > > remote_ip and dst_port are. Does it address your concern?
> >
> > I don't see where you're getting that.  Tunnel configuration allows both
> > local and remote IPs to be specified.  The match structure has slots for
> > both IP addresses, and both of them may be specified.
> >
> > If the remote (or local) IP addresses are different, then this warning
> > won't come up.  And you said that the remote IP addresses are different:
> > "I don't think it's a useful configuration to have two two incoming
> > tunnel ports with the same config for the same IP that are not served by
> > separate DST IP addresses."
> >
> > tnl_port_receive() looks up the tunnel that should receive a packet.  It
> > calls into tnl_find().  This calls tnl_find_exact() for each of several
> > lookup tables.  In the case where a warning would be given here, we know
> > that tnl_find_exact() would find two matching elements in the map,
> > because that's the function that we use to know to issue the warning.
> > If we disable the warning and insert the second match anyway, there will
> > be two elements in the map with exactly the same key, tnl_find_exact()
> > will only ever return one of them, thus the other will never receive any
> > packets.
> >
> > What am I missing?
> >
> 
> Oh I missed that local_ip *may* be configured for a tunnel endpoint,
> it's just not configured by OVN controller *right now*. We can deal
> with it in OVN controller instead of OVS then. Sorry for the noise,
> this patch can be dropped.

Oh, wonderful, we understand each other now, and there's nothing
mysterious going on in the code that I didn't know about.  Cool!
diff mbox series

Patch

diff --git a/ofproto/tunnel.c b/ofproto/tunnel.c
index 3455ed233..fc45d40f9 100644
--- a/ofproto/tunnel.c
+++ b/ofproto/tunnel.c
@@ -148,11 +148,11 @@  ofproto_tunnel_init(void)
 
 static bool
 tnl_port_add__(const struct ofport_dpif *ofport, const struct netdev *netdev,
-               odp_port_t odp_port, bool warn, bool native_tnl, const char name[])
+               odp_port_t odp_port, bool warn OVS_UNUSED, bool native_tnl,
+               const char name[])
     OVS_REQ_WRLOCK(rwlock)
 {
     const struct netdev_tunnel_config *cfg;
-    struct tnl_port *existing_port;
     struct tnl_port *tnl_port;
     struct hmap **map;
 
@@ -174,21 +174,6 @@  tnl_port_add__(const struct ofport_dpif *ofport, const struct netdev *netdev,
     tnl_port->match.pt_mode = netdev_get_pt_mode(netdev);
 
     map = tnl_match_map(&tnl_port->match);
-    existing_port = tnl_find_exact(&tnl_port->match, *map);
-    if (existing_port) {
-        if (warn) {
-            struct ds ds = DS_EMPTY_INITIALIZER;
-            tnl_match_fmt(&tnl_port->match, &ds);
-            VLOG_WARN("%s: attempting to add tunnel port with same config as "
-                      "port '%s' (%s)", tnl_port_get_name(tnl_port),
-                      tnl_port_get_name(existing_port), ds_cstr(&ds));
-            ds_destroy(&ds);
-        }
-        netdev_close(tnl_port->netdev);
-        free(tnl_port);
-        return false;
-    }
-
     hmap_insert(ofport_map, &tnl_port->ofport_node, hash_pointer(ofport, 0));
 
     if (!*map) {