[ovs-dev,v2.12] ovn: Prevent erroneous duplicate IP address messages.

This is a backport to OVS 2.12 of OVN master commit 21c29d5b0c.

When using dynamic address assignment for logical switches, OVN reserves
the first address in the subnet for the attached router port to use.

In commit 488d153ee87841c042af05bc0eb8b5481aaa98cf, the IPAM code was
modified to add assigned router port addresses to IPAM. The use case for
this was when a switch was joined to multiple routers, and all router
addresses were dynamically assigned.

However, that commit also made it so that when a router rightly claimed
the first address in the subnet, ovn-northd would issue a warning about
a duplicate IP address being set. This change fixes the issue by adding
a special case so that we don't add the router's IP address to IPAM if
it is the first address in the subnet. This prevents the warning message
from appearing.

Signed-off-by: Mark Michelson <mmichels@redhat.com>
Acked-by: Numan Siddique <nusiddiq@redhat.com>
Acked-by: Han ZHou <hzhou8@ebay.com>
---
 ovn/northd/ovn-northd.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

In addition to applying this to 2.12, I am requesting a backport to 2.11 
as well. Thank you.

On 11/12/19 11:59 AM, Mark Michelson wrote:
> This is a backport to OVS 2.12 of OVN master commit 21c29d5b0c.
> 
> When using dynamic address assignment for logical switches, OVN reserves
> the first address in the subnet for the attached router port to use.
> 
> In commit 488d153ee87841c042af05bc0eb8b5481aaa98cf, the IPAM code was
> modified to add assigned router port addresses to IPAM. The use case for
> this was when a switch was joined to multiple routers, and all router
> addresses were dynamically assigned.
> 
> However, that commit also made it so that when a router rightly claimed
> the first address in the subnet, ovn-northd would issue a warning about
> a duplicate IP address being set. This change fixes the issue by adding
> a special case so that we don't add the router's IP address to IPAM if
> it is the first address in the subnet. This prevents the warning message
> from appearing.
> 
> Signed-off-by: Mark Michelson <mmichels@redhat.com>
> Acked-by: Numan Siddique <nusiddiq@redhat.com>
> Acked-by: Han ZHou <hzhou8@ebay.com>
> ---
>   ovn/northd/ovn-northd.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> index 6c6de2afd..1c9164924 100644
> --- a/ovn/northd/ovn-northd.c
> +++ b/ovn/northd/ovn-northd.c
> @@ -1194,7 +1194,14 @@ ipam_add_port_addresses(struct ovn_datapath *od, struct ovn_port *op)
>   
>           for (size_t i = 0; i < lrp_networks.n_ipv4_addrs; i++) {
>               uint32_t ip = ntohl(lrp_networks.ipv4_addrs[i].addr);
> -            ipam_insert_ip(op->peer->od, ip);
> +            /* If the router has the first IP address of the subnet, don't add
> +             * it to IPAM. We already added this when we initialized IPAM for
> +             * the datapath. This will just result in an erroneous message
> +             * about a duplicate IP address.
> +             */
> +            if (ip != op->peer->od->ipam_info.start_ipv4) {
> +                ipam_insert_ip(op->peer->od, ip);
> +            }
>           }
>   
>           destroy_lport_addresses(&lrp_networks);
>

Bleep bloop.  Greetings Mark Michelson, 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:
Failed to merge in the changes.
Patch failed at 0001 ovn: Prevent erroneous duplicate IP address messages.
The copy of the patch that failed is found in:
   /var/lib/jenkins/jobs/upstream_build_from_pw/workspace/.git/rebase-apply/patch
When you have resolved this problem, run "git am --resolved".
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

I applied it to both branches.  Thank you for asking for backports.

On Tue, Nov 12, 2019 at 12:01:42PM -0500, Mark Michelson wrote:
> In addition to applying this to 2.12, I am requesting a backport to 2.11 as
> well. Thank you.
> 
> On 11/12/19 11:59 AM, Mark Michelson wrote:
> > This is a backport to OVS 2.12 of OVN master commit 21c29d5b0c.
> > 
> > When using dynamic address assignment for logical switches, OVN reserves
> > the first address in the subnet for the attached router port to use.
> > 
> > In commit 488d153ee87841c042af05bc0eb8b5481aaa98cf, the IPAM code was
> > modified to add assigned router port addresses to IPAM. The use case for
> > this was when a switch was joined to multiple routers, and all router
> > addresses were dynamically assigned.
> > 
> > However, that commit also made it so that when a router rightly claimed
> > the first address in the subnet, ovn-northd would issue a warning about
> > a duplicate IP address being set. This change fixes the issue by adding
> > a special case so that we don't add the router's IP address to IPAM if
> > it is the first address in the subnet. This prevents the warning message
> > from appearing.
> > 
> > Signed-off-by: Mark Michelson <mmichels@redhat.com>
> > Acked-by: Numan Siddique <nusiddiq@redhat.com>
> > Acked-by: Han ZHou <hzhou8@ebay.com>
> > ---
> >   ovn/northd/ovn-northd.c | 9 ++++++++-
> >   1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> > index 6c6de2afd..1c9164924 100644
> > --- a/ovn/northd/ovn-northd.c
> > +++ b/ovn/northd/ovn-northd.c
> > @@ -1194,7 +1194,14 @@ ipam_add_port_addresses(struct ovn_datapath *od, struct ovn_port *op)
> >           for (size_t i = 0; i < lrp_networks.n_ipv4_addrs; i++) {
> >               uint32_t ip = ntohl(lrp_networks.ipv4_addrs[i].addr);
> > -            ipam_insert_ip(op->peer->od, ip);
> > +            /* If the router has the first IP address of the subnet, don't add
> > +             * it to IPAM. We already added this when we initialized IPAM for
> > +             * the datapath. This will just result in an erroneous message
> > +             * about a duplicate IP address.
> > +             */
> > +            if (ip != op->peer->od->ipam_info.start_ipv4) {
> > +                ipam_insert_ip(op->peer->od, ip);
> > +            }
> >           }
> >           destroy_lport_addresses(&lrp_networks);
> > 
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

It appears this flew under the radar and was never reviewed/merged. 
Would it be possible for an OVS maintainer to have a quick look at this 
backport and get it merged into OVS 2.12 and 2.11 if possible?

On 11/12/19 11:59 AM, Mark Michelson wrote:
> This is a backport to OVS 2.12 of OVN master commit 21c29d5b0c.
> 
> When using dynamic address assignment for logical switches, OVN reserves
> the first address in the subnet for the attached router port to use.
> 
> In commit 488d153ee87841c042af05bc0eb8b5481aaa98cf, the IPAM code was
> modified to add assigned router port addresses to IPAM. The use case for
> this was when a switch was joined to multiple routers, and all router
> addresses were dynamically assigned.
> 
> However, that commit also made it so that when a router rightly claimed
> the first address in the subnet, ovn-northd would issue a warning about
> a duplicate IP address being set. This change fixes the issue by adding
> a special case so that we don't add the router's IP address to IPAM if
> it is the first address in the subnet. This prevents the warning message
> from appearing.
> 
> Signed-off-by: Mark Michelson <mmichels@redhat.com>
> Acked-by: Numan Siddique <nusiddiq@redhat.com>
> Acked-by: Han ZHou <hzhou8@ebay.com>
> ---
>   ovn/northd/ovn-northd.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> index 6c6de2afd..1c9164924 100644
> --- a/ovn/northd/ovn-northd.c
> +++ b/ovn/northd/ovn-northd.c
> @@ -1194,7 +1194,14 @@ ipam_add_port_addresses(struct ovn_datapath *od, struct ovn_port *op)
>   
>           for (size_t i = 0; i < lrp_networks.n_ipv4_addrs; i++) {
>               uint32_t ip = ntohl(lrp_networks.ipv4_addrs[i].addr);
> -            ipam_insert_ip(op->peer->od, ip);
> +            /* If the router has the first IP address of the subnet, don't add
> +             * it to IPAM. We already added this when we initialized IPAM for
> +             * the datapath. This will just result in an erroneous message
> +             * about a duplicate IP address.
> +             */
> +            if (ip != op->peer->od->ipam_info.start_ipv4) {
> +                ipam_insert_ip(op->peer->od, ip);
> +            }
>           }
>   
>           destroy_lport_addresses(&lrp_networks);
>

Are you sure?  I see it on branches for both 2.12 and 2.11.

On Fri, Jan 31, 2020 at 08:24:48AM -0500, Mark Michelson wrote:
> It appears this flew under the radar and was never reviewed/merged. Would it
> be possible for an OVS maintainer to have a quick look at this backport and
> get it merged into OVS 2.12 and 2.11 if possible?
> 
> On 11/12/19 11:59 AM, Mark Michelson wrote:
> > This is a backport to OVS 2.12 of OVN master commit 21c29d5b0c.
> > 
> > When using dynamic address assignment for logical switches, OVN reserves
> > the first address in the subnet for the attached router port to use.
> > 
> > In commit 488d153ee87841c042af05bc0eb8b5481aaa98cf, the IPAM code was
> > modified to add assigned router port addresses to IPAM. The use case for
> > this was when a switch was joined to multiple routers, and all router
> > addresses were dynamically assigned.
> > 
> > However, that commit also made it so that when a router rightly claimed
> > the first address in the subnet, ovn-northd would issue a warning about
> > a duplicate IP address being set. This change fixes the issue by adding
> > a special case so that we don't add the router's IP address to IPAM if
> > it is the first address in the subnet. This prevents the warning message
> > from appearing.
> > 
> > Signed-off-by: Mark Michelson <mmichels@redhat.com>
> > Acked-by: Numan Siddique <nusiddiq@redhat.com>
> > Acked-by: Han ZHou <hzhou8@ebay.com>
> > ---
> >   ovn/northd/ovn-northd.c | 9 ++++++++-
> >   1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> > index 6c6de2afd..1c9164924 100644
> > --- a/ovn/northd/ovn-northd.c
> > +++ b/ovn/northd/ovn-northd.c
> > @@ -1194,7 +1194,14 @@ ipam_add_port_addresses(struct ovn_datapath *od, struct ovn_port *op)
> >           for (size_t i = 0; i < lrp_networks.n_ipv4_addrs; i++) {
> >               uint32_t ip = ntohl(lrp_networks.ipv4_addrs[i].addr);
> > -            ipam_insert_ip(op->peer->od, ip);
> > +            /* If the router has the first IP address of the subnet, don't add
> > +             * it to IPAM. We already added this when we initialized IPAM for
> > +             * the datapath. This will just result in an erroneous message
> > +             * about a duplicate IP address.
> > +             */
> > +            if (ip != op->peer->od->ipam_info.start_ipv4) {
> > +                ipam_insert_ip(op->peer->od, ip);
> > +            }
> >           }
> >           destroy_lport_addresses(&lrp_networks);
> > 
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev