| Message ID | 7e807296ff4af8ba0fbe3beaac2b4461a39c36c7.1542367772.git.lorenzo.bianconi@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] OVN: add selected mac address to MACAM in update_dynamic_addresses | expand |
Acked-by: Mark Michelson <mmichels@redhat.com> On 11/16/2018 06:31 AM, Lorenzo Bianconi wrote: > Add selected dynamic mac address to MACAM in update_dynamic_addresses > and not just in in ipam_add_port_addresses/ipam_insert_lsp_addresses > since the second approach can produce a duplicated L2 address in a > IPv6-only network if ipv6_prefix is provided after logical port creation. > The issue can be triggered with the following reproducer: > > $ovn-nbctl ls-add sw0 > $ovn-nbctl lsp-add sw0 sw0-port1 > $ovn-nbctl lsp-set-addresses sw0-port1 "dynamic" > $ovn-nbctl lsp-add sw0 sw0-port2 > $ovn-nbctl lsp-set-addresses sw0-port2 "dynamic" > $ovs-vsctl add-port br-int p1 -- \ > set Interface p1 external_ids:iface-id=sw0-port1 > $ovs-vsctl add-port br-int p2 -- \ > set Interface p2 external_ids:iface-id=sw0-port2 > [..] > $ovn-nbctl --wait=sb set Logical-switch sw0 \ > other_config:ipv6_prefix="aef0::" > > $ovn-nbctl list logical_switch_port > _uuid : 1e0e2ed8-20c6-48dc-bfa8-d823e48c9f45 > addresses : [dynamic] > dhcpv4_options : [] > dhcpv6_options : [] > dynamic_addresses : "0a:00:00:00:00:01 aef0::800:ff:fe00:1" > enabled : [] > external_ids : {} > name : "sw0-port1" > options : {} > parent_name : [] > port_security : [] > tag : [] > tag_request : [] > type : "" > up : true > > _uuid : cfeab7fb-e20b-41f1-974c-f99e0b5293d7 > addresses : [dynamic] > dhcpv4_options : [] > dhcpv6_options : [] > dynamic_addresses : "0a:00:00:00:00:01 aef0::800:ff:fe00:1" > enabled : [] > external_ids : {} > name : "sw0-port2" > options : {} > parent_name : [] > port_security : [] > tag : [] > tag_request : [] > type : "" > up : true > > Fixes: c814545b43ac ("OVN: configure L2 address according to the used IP > address") > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > --- > ovn/northd/ovn-northd.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c > index 58bef7de5..4580cd705 100644 > --- a/ovn/northd/ovn-northd.c > +++ b/ovn/northd/ovn-northd.c > @@ -1341,6 +1341,8 @@ update_dynamic_addresses(struct dynamic_address_update *update) > > struct ds new_addr = DS_EMPTY_INITIALIZER; > ds_put_format(&new_addr, ETH_ADDR_FMT, ETH_ADDR_ARGS(mac)); > + ipam_insert_mac(&mac, true); > + > if (ip4) { > ipam_insert_ip(update->od, ntohl(ip4)); > ds_put_format(&new_addr, " "IP_FMT, IP_ARGS(ip4)); >
On Sat, Nov 17, 2018 at 2:10 AM Mark Michelson <mmichels@redhat.com> wrote: > Acked-by: Mark Michelson <mmichels@redhat.com> > > On 11/16/2018 06:31 AM, Lorenzo Bianconi wrote: > > Add selected dynamic mac address to MACAM in update_dynamic_addresses > > and not just in in ipam_add_port_addresses/ipam_insert_lsp_addresses > > since the second approach can produce a duplicated L2 address in a > > IPv6-only network if ipv6_prefix is provided after logical port creation. > > The issue can be triggered with the following reproducer: > > > > $ovn-nbctl ls-add sw0 > > $ovn-nbctl lsp-add sw0 sw0-port1 > > $ovn-nbctl lsp-set-addresses sw0-port1 "dynamic" > > $ovn-nbctl lsp-add sw0 sw0-port2 > > $ovn-nbctl lsp-set-addresses sw0-port2 "dynamic" > > $ovs-vsctl add-port br-int p1 -- \ > > set Interface p1 external_ids:iface-id=sw0-port1 > > $ovs-vsctl add-port br-int p2 -- \ > > set Interface p2 external_ids:iface-id=sw0-port2 > > [..] > > $ovn-nbctl --wait=sb set Logical-switch sw0 \ > > other_config:ipv6_prefix="aef0::" > > > > $ovn-nbctl list logical_switch_port > > _uuid : 1e0e2ed8-20c6-48dc-bfa8-d823e48c9f45 > > addresses : [dynamic] > > dhcpv4_options : [] > > dhcpv6_options : [] > > dynamic_addresses : "0a:00:00:00:00:01 aef0::800:ff:fe00:1" > > enabled : [] > > external_ids : {} > > name : "sw0-port1" > > options : {} > > parent_name : [] > > port_security : [] > > tag : [] > > tag_request : [] > > type : "" > > up : true > > > > _uuid : cfeab7fb-e20b-41f1-974c-f99e0b5293d7 > > addresses : [dynamic] > > dhcpv4_options : [] > > dhcpv6_options : [] > > dynamic_addresses : "0a:00:00:00:00:01 aef0::800:ff:fe00:1" > > enabled : [] > > external_ids : {} > > name : "sw0-port2" > > options : {} > > parent_name : [] > > port_security : [] > > tag : [] > > tag_request : [] > > type : "" > > up : true > > > > Fixes: c814545b43ac ("OVN: configure L2 address according to the used IP > > address") > > > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > Acked-by: Numan Siddique <nusiddiq@redhat.com> Since you have a reproducer code in the commit message, would it make sense to add them as a test case so that we don't regress later ? Thanks Numan > --- > > ovn/northd/ovn-northd.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c > > index 58bef7de5..4580cd705 100644 > > --- a/ovn/northd/ovn-northd.c > > +++ b/ovn/northd/ovn-northd.c > > @@ -1341,6 +1341,8 @@ update_dynamic_addresses(struct > dynamic_address_update *update) > > > > struct ds new_addr = DS_EMPTY_INITIALIZER; > > ds_put_format(&new_addr, ETH_ADDR_FMT, ETH_ADDR_ARGS(mac)); > > + ipam_insert_mac(&mac, true); > > + > > if (ip4) { > > ipam_insert_ip(update->od, ntohl(ip4)); > > ds_put_format(&new_addr, " "IP_FMT, IP_ARGS(ip4)); > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
> On Sat, Nov 17, 2018 at 2:10 AM Mark Michelson <mmichels@redhat.com> wrote: > > > Acked-by: Mark Michelson <mmichels@redhat.com> > > > > On 11/16/2018 06:31 AM, Lorenzo Bianconi wrote: [...] > > > > > > Fixes: c814545b43ac ("OVN: configure L2 address according to the used IP > > > address") > > > > > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > > > > Acked-by: Numan Siddique <nusiddiq@redhat.com> > > Since you have a reproducer code in the commit message, would it make sense > to > add them as a test case so that we don't regress later ? Sure, will do in v2. Thanks for the review Regards, Lorenzo > > Thanks > Numan >
diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c index 58bef7de5..4580cd705 100644 --- a/ovn/northd/ovn-northd.c +++ b/ovn/northd/ovn-northd.c @@ -1341,6 +1341,8 @@ update_dynamic_addresses(struct dynamic_address_update *update) struct ds new_addr = DS_EMPTY_INITIALIZER; ds_put_format(&new_addr, ETH_ADDR_FMT, ETH_ADDR_ARGS(mac)); + ipam_insert_mac(&mac, true); + if (ip4) { ipam_insert_ip(update->od, ntohl(ip4)); ds_put_format(&new_addr, " "IP_FMT, IP_ARGS(ip4));
Add selected dynamic mac address to MACAM in update_dynamic_addresses and not just in in ipam_add_port_addresses/ipam_insert_lsp_addresses since the second approach can produce a duplicated L2 address in a IPv6-only network if ipv6_prefix is provided after logical port creation. The issue can be triggered with the following reproducer: $ovn-nbctl ls-add sw0 $ovn-nbctl lsp-add sw0 sw0-port1 $ovn-nbctl lsp-set-addresses sw0-port1 "dynamic" $ovn-nbctl lsp-add sw0 sw0-port2 $ovn-nbctl lsp-set-addresses sw0-port2 "dynamic" $ovs-vsctl add-port br-int p1 -- \ set Interface p1 external_ids:iface-id=sw0-port1 $ovs-vsctl add-port br-int p2 -- \ set Interface p2 external_ids:iface-id=sw0-port2 [..] $ovn-nbctl --wait=sb set Logical-switch sw0 \ other_config:ipv6_prefix="aef0::" $ovn-nbctl list logical_switch_port _uuid : 1e0e2ed8-20c6-48dc-bfa8-d823e48c9f45 addresses : [dynamic] dhcpv4_options : [] dhcpv6_options : [] dynamic_addresses : "0a:00:00:00:00:01 aef0::800:ff:fe00:1" enabled : [] external_ids : {} name : "sw0-port1" options : {} parent_name : [] port_security : [] tag : [] tag_request : [] type : "" up : true _uuid : cfeab7fb-e20b-41f1-974c-f99e0b5293d7 addresses : [dynamic] dhcpv4_options : [] dhcpv6_options : [] dynamic_addresses : "0a:00:00:00:00:01 aef0::800:ff:fe00:1" enabled : [] external_ids : {} name : "sw0-port2" options : {} parent_name : [] port_security : [] tag : [] tag_request : [] type : "" up : true Fixes: c814545b43ac ("OVN: configure L2 address according to the used IP address") Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> --- ovn/northd/ovn-northd.c | 2 ++ 1 file changed, 2 insertions(+)