Message ID | 20191112165931.23936-1-mmichels@redhat.com |
---|---|
State | Accepted |
Commit | 30227d3b6f82381c3bf0ffa6e988a0799c0682f4 |
Headers | show |
Series | [ovs-dev,v2.12] ovn: Prevent erroneous duplicate IP address messages. | expand |
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
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);