[ovs-dev] OVN: Add port addresses to IPAM after all ports are joined.

Message ID 20190306143304.16155-1-mmichels@redhat.com
State New
Headers show
Series
  • [ovs-dev] OVN: Add port addresses to IPAM after all ports are joined.
Related show

Commit Message

Mark Michelson March 6, 2019, 2:33 p.m.
Joining ports involves setting the peer field on ovn_ports. If a switch
port is visited, and it is connected to a router port, then the switch
port's peer is set to the router port and the router port's peer is set
to the switch port.

A router port's addresses are added to IPAM if it is peered with a
switch that has dynamic addressing enabled.

When visiting ports, if a router port is visited before its connected
switch port, then the router port's peer is not set yet. Therefore the
router's port addresses cannot be added to IPAM. The result is that
duplicate addresses can be assigned by a logical switch.

The fix for this is to wait until all ports have been joined and then
add port addresses to IPAM. This way, we guarantee that all peer
assignments have been set, and no duplicate IP addresses may be assigned
by a switch.

Reported-by: James Page <james.page@canonical.com>
Signed-off-by: Mark Michelson <mmichels@redhat.com>
---
 ovn/northd/ovn-northd.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Mark Michelson March 6, 2019, 2:34 p.m. | #1
This will require backporting to 2.11

On 3/6/19 9:33 AM, Mark Michelson wrote:
> Joining ports involves setting the peer field on ovn_ports. If a switch
> port is visited, and it is connected to a router port, then the switch
> port's peer is set to the router port and the router port's peer is set
> to the switch port.
> 
> A router port's addresses are added to IPAM if it is peered with a
> switch that has dynamic addressing enabled.
> 
> When visiting ports, if a router port is visited before its connected
> switch port, then the router port's peer is not set yet. Therefore the
> router's port addresses cannot be added to IPAM. The result is that
> duplicate addresses can be assigned by a logical switch.
> 
> The fix for this is to wait until all ports have been joined and then
> add port addresses to IPAM. This way, we guarantee that all peer
> assignments have been set, and no duplicate IP addresses may be assigned
> by a switch.
> 
> Reported-by: James Page <james.page@canonical.com>
> Signed-off-by: Mark Michelson <mmichels@redhat.com>
> ---
>   ovn/northd/ovn-northd.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> index 3569ea2be..a9de366fb 100644
> --- a/ovn/northd/ovn-northd.c
> +++ b/ovn/northd/ovn-northd.c
> @@ -1829,7 +1829,12 @@ join_logical_ports(struct northd_context *ctx,
>                   }
>               }
>           }
> +    }
>   
> +    /* Wait until all ports have been connected to add to IPAM since
> +     * it relies on proper peers to be set
> +     */
> +    HMAP_FOR_EACH (op, key_node, ports) {
>           ipam_add_port_addresses(op->od, op);
>       }
>   }
>
Ben Pfaff March 6, 2019, 10:29 p.m. | #2
Thanks, applied and backported.

On Wed, Mar 06, 2019 at 09:34:14AM -0500, Mark Michelson wrote:
> This will require backporting to 2.11
> 
> On 3/6/19 9:33 AM, Mark Michelson wrote:
> > Joining ports involves setting the peer field on ovn_ports. If a switch
> > port is visited, and it is connected to a router port, then the switch
> > port's peer is set to the router port and the router port's peer is set
> > to the switch port.
> > 
> > A router port's addresses are added to IPAM if it is peered with a
> > switch that has dynamic addressing enabled.
> > 
> > When visiting ports, if a router port is visited before its connected
> > switch port, then the router port's peer is not set yet. Therefore the
> > router's port addresses cannot be added to IPAM. The result is that
> > duplicate addresses can be assigned by a logical switch.
> > 
> > The fix for this is to wait until all ports have been joined and then
> > add port addresses to IPAM. This way, we guarantee that all peer
> > assignments have been set, and no duplicate IP addresses may be assigned
> > by a switch.
> > 
> > Reported-by: James Page <james.page@canonical.com>
> > Signed-off-by: Mark Michelson <mmichels@redhat.com>
> > ---
> >   ovn/northd/ovn-northd.c | 5 +++++
> >   1 file changed, 5 insertions(+)
> > 
> > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> > index 3569ea2be..a9de366fb 100644
> > --- a/ovn/northd/ovn-northd.c
> > +++ b/ovn/northd/ovn-northd.c
> > @@ -1829,7 +1829,12 @@ join_logical_ports(struct northd_context *ctx,
> >                   }
> >               }
> >           }
> > +    }
> > +    /* Wait until all ports have been connected to add to IPAM since
> > +     * it relies on proper peers to be set
> > +     */
> > +    HMAP_FOR_EACH (op, key_node, ports) {
> >           ipam_add_port_addresses(op->od, op);
> >       }
> >   }
> > 
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Patch

diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index 3569ea2be..a9de366fb 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -1829,7 +1829,12 @@  join_logical_ports(struct northd_context *ctx,
                 }
             }
         }
+    }
 
+    /* Wait until all ports have been connected to add to IPAM since
+     * it relies on proper peers to be set
+     */
+    HMAP_FOR_EACH (op, key_node, ports) {
         ipam_add_port_addresses(op->od, op);
     }
 }