| Message ID | 20180730143749.8173-4-jkbs@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Port Group fixes | expand |
Hi Jakub, On Mon, Jul 30, 2018 at 04:37:49PM +0200, Jakub Sitnicki wrote: > If a logical switch port belongs to a port group and has dynamic > addresses assigned, propagate the addresses to the auto-generated > address sets for the port group. > > Signed-off-by: Jakub Sitnicki <jkbs@redhat.com> > Acked-by: Han Zhou <hzhou8@ebay.com> > --- > ovn/northd/ovn-northd.c | 34 ++++++++++++++++++++-------- > tests/ovn.at | 59 +++++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 84 insertions(+), 9 deletions(-) The test added by this patch, which is now present in the master branch, seems to fail. https://travis-ci.org/openvswitch/ovs/jobs/410550769 ... > diff --git a/tests/ovn.at b/tests/ovn.at > index e7bdfe250..32e6c1738 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -10267,6 +10267,65 @@ AT_CHECK([ovn-sbctl get Address_Set pg1_ip6 addresses], > > AT_CLEANUP > > +AT_SETUP([ovn -- Address Set generation from Port Groups (dynamic addressing)]) > +ovn_start > + > +ovn-nbctl ls-add ls1 > +ovn-nbctl ls-add ls2 > +ovn-nbctl ls-add ls3 > + > +ovn-nbctl set Logical_Switch ls1 \ > + other_config:subnet=10.1.0.0/24 other_config:ipv6_prefix="2001:db8:1::" > +ovn-nbctl set Logical_Switch ls2 \ > + other_config:subnet=10.2.0.0/24 other_config:ipv6_prefix="2001:db8:2::" > +ovn-nbctl set Logical_Switch ls3 \ > + other_config:subnet=10.3.0.0/24 other_config:ipv6_prefix="2001:db8:3::" > + > +ovn-nbctl lsp-add ls1 lp1 > +ovn-nbctl lsp-add ls2 lp2 > +ovn-nbctl lsp-add ls3 lp3 > + > +ovn-nbctl lsp-set-addresses lp1 "02:00:00:00:00:01 dynamic" > +ovn-nbctl lsp-set-addresses lp2 "02:00:00:00:00:02 dynamic" > +ovn-nbctl lsp-set-addresses lp3 "02:00:00:00:00:03 dynamic" > + > +ovn-nbctl create Port_Group name=pg1 > +ovn-nbctl create Port_Group name=pg2 > + > +ovn-nbctl --id=@p get Logical_Switch_Port lp1 -- add Port_Group pg1 ports @p > +ovn-nbctl --id=@p get Logical_Switch_Port lp2 -- add Port_Group pg1 ports @p > +ovn-nbctl --id=@p get Logical_Switch_Port lp2 -- add Port_Group pg2 ports @p > +ovn-nbctl --id=@p get Logical_Switch_Port lp3 -- add Port_Group pg2 ports @p > + > +ovn-nbctl --wait=sb sync > + > +dnl Check if port group address sets were populated with ports' addresses > +AT_CHECK([ovn-sbctl get Address_Set pg1_ip4 addresses], > + [0], [[["10.1.0.2", "10.2.0.2"]] > +]) > +AT_CHECK([ovn-sbctl get Address_Set pg2_ip4 addresses], > + [0], [[["10.2.0.2", "10.3.0.2"]] > +]) > +AT_CHECK([ovn-sbctl get Address_Set pg1_ip6 addresses], > + [0], [[["2001:db8:1::ff:fe00:1", "2001:db8:2::ff:fe00:2"]] > +]) > +AT_CHECK([ovn-sbctl get Address_Set pg2_ip6 addresses], > + [0], [[["2001:db8:2::ff:fe00:2", "2001:db8:3::ff:fe00:3"]] > +]) > + > +ovn-nbctl set Logical_Switch ls1 \ > + other_config:subnet=10.11.0.0/24 other_config:ipv6_prefix="2001:db8:11::" > + Empirically the following allows the test to pass in my local environment: > +dnl Check if updated address got propagated to the port group address sets > +AT_CHECK([ovn-sbctl get Address_Set pg1_ip4 addresses], > + [0], [[["10.11.0.2", "10.2.0.2"]] - s/10.11.0.2/10.1.0.2/ > +]) > +AT_CHECK([ovn-sbctl get Address_Set pg1_ip6 addresses], > + [0], [[["2001:db8:11::ff:fe00:1", "2001:db8:2::ff:fe00:2"]] - s/2001:db8:11::ff:fe00:1/2001:db8:1::ff:fe00:1/ > +]) > + > +AT_CLEANUP > + > AT_SETUP([ovn -- ACL conjunction]) > ovn_start > > -- > 2.14.4 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
Hi Simon, On Wed, 1 Aug 2018 11:23:30 +0200 Simon Horman <simon.horman@netronome.com> wrote: > Hi Jakub, > > On Mon, Jul 30, 2018 at 04:37:49PM +0200, Jakub Sitnicki wrote: > > If a logical switch port belongs to a port group and has dynamic > > addresses assigned, propagate the addresses to the auto-generated > > address sets for the port group. > > > > Signed-off-by: Jakub Sitnicki <jkbs@redhat.com> > > Acked-by: Han Zhou <hzhou8@ebay.com> > > --- > > ovn/northd/ovn-northd.c | 34 ++++++++++++++++++++-------- > > tests/ovn.at | 59 +++++++++++++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 84 insertions(+), 9 deletions(-) > > The test added by this patch, which is now present in the master branch, > seems to fail. > > https://travis-ci.org/openvswitch/ovs/jobs/410550769 Thanks for the heads-up. These patches depend on another patch that is still under review. Hence the failure. I've already notified Ben, who he has proposed a fix-up similar to yours: https://mail.openvswitch.org/pipermail/ovs-dev/2018-August/350390.html -Jakub
On Wed, Aug 01, 2018 at 11:44:58AM +0200, Jakub Sitnicki wrote: > Hi Simon, > > On Wed, 1 Aug 2018 11:23:30 +0200 > Simon Horman <simon.horman@netronome.com> wrote: > > > Hi Jakub, > > > > On Mon, Jul 30, 2018 at 04:37:49PM +0200, Jakub Sitnicki wrote: > > > If a logical switch port belongs to a port group and has dynamic > > > addresses assigned, propagate the addresses to the auto-generated > > > address sets for the port group. > > > > > > Signed-off-by: Jakub Sitnicki <jkbs@redhat.com> > > > Acked-by: Han Zhou <hzhou8@ebay.com> > > > --- > > > ovn/northd/ovn-northd.c | 34 ++++++++++++++++++++-------- > > > tests/ovn.at | 59 +++++++++++++++++++++++++++++++++++++++++++++++++ > > > 2 files changed, 84 insertions(+), 9 deletions(-) > > > > The test added by this patch, which is now present in the master branch, > > seems to fail. > > > > https://travis-ci.org/openvswitch/ovs/jobs/410550769 > > Thanks for the heads-up. These patches depend on another patch that is > still under review. Hence the failure. I've already notified Ben, who > he has proposed a fix-up similar to yours: > > https://mail.openvswitch.org/pipermail/ovs-dev/2018-August/350390.html Thanks Jakub, I see that now. The fix looks good :)
diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c index 52f47c5ed..d744836d3 100644 --- a/ovn/northd/ovn-northd.c +++ b/ovn/northd/ovn-northd.c @@ -6653,6 +6653,24 @@ sync_address_set(struct northd_context *ctx, const char *name, addrs, n_addrs); } +/* Go through 'addresses' and add found IPv4 addresses to 'ipv4_addrs' and IPv6 + * addresses to 'ipv6_addrs'. + */ +static void +split_addresses(const char *addresses, struct svec *ipv4_addrs, + struct svec *ipv6_addrs) +{ + struct lport_addresses laddrs; + extract_lsp_addresses(addresses, &laddrs); + for (size_t k = 0; k < laddrs.n_ipv4_addrs; k++) { + svec_add(ipv4_addrs, laddrs.ipv4_addrs[k].addr_s); + } + for (size_t k = 0; k < laddrs.n_ipv6_addrs; k++) { + svec_add(ipv6_addrs, laddrs.ipv6_addrs[k].addr_s); + } + destroy_lport_addresses(&laddrs); +} + /* OVN_Southbound Address_Set table contains same records as in north * bound, plus the records generated from Port_Group table in north bound. * @@ -6680,16 +6698,14 @@ sync_address_sets(struct northd_context *ctx) struct svec ipv6_addrs = SVEC_EMPTY_INITIALIZER; for (size_t i = 0; i < nb_port_group->n_ports; i++) { for (size_t j = 0; j < nb_port_group->ports[i]->n_addresses; j++) { - struct lport_addresses laddrs; - extract_lsp_addresses(nb_port_group->ports[i]->addresses[j], - &laddrs); - for (size_t k = 0; k < laddrs.n_ipv4_addrs; k++) { - svec_add(&ipv4_addrs, laddrs.ipv4_addrs[k].addr_s); - } - for (size_t k = 0; k < laddrs.n_ipv6_addrs; k++) { - svec_add(&ipv6_addrs, laddrs.ipv6_addrs[k].addr_s); + const char *addrs = nb_port_group->ports[i]->addresses[j]; + if (!is_dynamic_lsp_address(addrs)) { + split_addresses(addrs, &ipv4_addrs, &ipv6_addrs); } - destroy_lport_addresses(&laddrs); + } + if (nb_port_group->ports[i]->dynamic_addresses) { + split_addresses(nb_port_group->ports[i]->dynamic_addresses, + &ipv4_addrs, &ipv6_addrs); } } char *ipv4_addrs_name = xasprintf("%s_ip4", nb_port_group->name); diff --git a/tests/ovn.at b/tests/ovn.at index e7bdfe250..32e6c1738 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -10267,6 +10267,65 @@ AT_CHECK([ovn-sbctl get Address_Set pg1_ip6 addresses], AT_CLEANUP +AT_SETUP([ovn -- Address Set generation from Port Groups (dynamic addressing)]) +ovn_start + +ovn-nbctl ls-add ls1 +ovn-nbctl ls-add ls2 +ovn-nbctl ls-add ls3 + +ovn-nbctl set Logical_Switch ls1 \ + other_config:subnet=10.1.0.0/24 other_config:ipv6_prefix="2001:db8:1::" +ovn-nbctl set Logical_Switch ls2 \ + other_config:subnet=10.2.0.0/24 other_config:ipv6_prefix="2001:db8:2::" +ovn-nbctl set Logical_Switch ls3 \ + other_config:subnet=10.3.0.0/24 other_config:ipv6_prefix="2001:db8:3::" + +ovn-nbctl lsp-add ls1 lp1 +ovn-nbctl lsp-add ls2 lp2 +ovn-nbctl lsp-add ls3 lp3 + +ovn-nbctl lsp-set-addresses lp1 "02:00:00:00:00:01 dynamic" +ovn-nbctl lsp-set-addresses lp2 "02:00:00:00:00:02 dynamic" +ovn-nbctl lsp-set-addresses lp3 "02:00:00:00:00:03 dynamic" + +ovn-nbctl create Port_Group name=pg1 +ovn-nbctl create Port_Group name=pg2 + +ovn-nbctl --id=@p get Logical_Switch_Port lp1 -- add Port_Group pg1 ports @p +ovn-nbctl --id=@p get Logical_Switch_Port lp2 -- add Port_Group pg1 ports @p +ovn-nbctl --id=@p get Logical_Switch_Port lp2 -- add Port_Group pg2 ports @p +ovn-nbctl --id=@p get Logical_Switch_Port lp3 -- add Port_Group pg2 ports @p + +ovn-nbctl --wait=sb sync + +dnl Check if port group address sets were populated with ports' addresses +AT_CHECK([ovn-sbctl get Address_Set pg1_ip4 addresses], + [0], [[["10.1.0.2", "10.2.0.2"]] +]) +AT_CHECK([ovn-sbctl get Address_Set pg2_ip4 addresses], + [0], [[["10.2.0.2", "10.3.0.2"]] +]) +AT_CHECK([ovn-sbctl get Address_Set pg1_ip6 addresses], + [0], [[["2001:db8:1::ff:fe00:1", "2001:db8:2::ff:fe00:2"]] +]) +AT_CHECK([ovn-sbctl get Address_Set pg2_ip6 addresses], + [0], [[["2001:db8:2::ff:fe00:2", "2001:db8:3::ff:fe00:3"]] +]) + +ovn-nbctl set Logical_Switch ls1 \ + other_config:subnet=10.11.0.0/24 other_config:ipv6_prefix="2001:db8:11::" + +dnl Check if updated address got propagated to the port group address sets +AT_CHECK([ovn-sbctl get Address_Set pg1_ip4 addresses], + [0], [[["10.11.0.2", "10.2.0.2"]] +]) +AT_CHECK([ovn-sbctl get Address_Set pg1_ip6 addresses], + [0], [[["2001:db8:11::ff:fe00:1", "2001:db8:2::ff:fe00:2"]] +]) + +AT_CLEANUP + AT_SETUP([ovn -- ACL conjunction]) ovn_start